Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSONify healthcheck Description for BackendConfig #2068

Merged
merged 2 commits into from
Apr 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions cmd/glbc/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,12 +328,7 @@ func runControllers(ctx *ingctx.ControllerContext) {
// In NonGCP mode, use the zone specified in gce.conf directly.
// This overrides the zone/fault-domain label on nodes for NEG controller.
if flags.F.EnableNonGCPMode {
zone, err := ctx.Cloud.GetZone(context.Background())
if err != nil {
klog.Errorf("Failed to retrieve zone information from Cloud provider: %v; Please check if local-zone is specified in gce.conf.", err)
} else {
zoneGetter = negtypes.NewSimpleZoneGetter(zone.FailureDomain)
}
zoneGetter = negtypes.NewSimpleZoneGetter(ctx.Cloud.LocalZone())
}

enableAsm := false
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ require (
k8s.io/apimachinery v0.26.2
k8s.io/client-go v0.26.2
k8s.io/cloud-provider v0.26.2
k8s.io/cloud-provider-gcp/providers v0.26.3-0.20230322202223-f7280f55e36f
k8s.io/cloud-provider-gcp/providers v0.26.3-0.20230414153241-658d4df496a9
k8s.io/component-base v0.26.2
k8s.io/klog/v2 v2.80.1
k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -628,8 +628,8 @@ k8s.io/cloud-provider-gcp/crd v0.0.0-20230227183622-3f1543249ff5 h1:xMNJz+8qU0Kk
k8s.io/cloud-provider-gcp/crd v0.0.0-20230227183622-3f1543249ff5/go.mod h1:YDxdayH9Du6f2XUhZiRfM2B0vsJ9b7qbrmI2J2dd89Y=
k8s.io/cloud-provider-gcp/crd v0.0.0-20230411231432-6ab330834e4e h1:MtQR9NbmkvuotfNK3MbZRjF13quIXWZ4624yYvy4biw=
k8s.io/cloud-provider-gcp/crd v0.0.0-20230411231432-6ab330834e4e/go.mod h1:A//VRRwQBcvAI20bbCc5jmxMSegf/DqxDI7XVK9oyYo=
k8s.io/cloud-provider-gcp/providers v0.26.3-0.20230322202223-f7280f55e36f h1:XPbo07vZ+h1xiGFIAN+gmlGm6G8zrZyhf/Xb6jNerTk=
k8s.io/cloud-provider-gcp/providers v0.26.3-0.20230322202223-f7280f55e36f/go.mod h1:dDBqOcDqYsqo7ZM7yuX+6Xg9WiyEPYk1Q9m2oHrnePo=
k8s.io/cloud-provider-gcp/providers v0.26.3-0.20230414153241-658d4df496a9 h1:u6Vv7TkVd34+nO6kB79CLWa0OW/QgBdWEI38og04S/k=
k8s.io/cloud-provider-gcp/providers v0.26.3-0.20230414153241-658d4df496a9/go.mod h1:dDBqOcDqYsqo7ZM7yuX+6Xg9WiyEPYk1Q9m2oHrnePo=
k8s.io/component-base v0.26.2 h1:IfWgCGUDzrD6wLLgXEstJKYZKAFS2kO+rBRi0p3LqcI=
k8s.io/component-base v0.26.2/go.mod h1:DxbuIe9M3IZPRxPIzhch2m1eT7uFrSBJUBuVCQEBivs=
k8s.io/klog/v2 v2.80.1 h1:atnLQ121W371wYYFawwYx1aEY2eUfs4l3J72wtgAwV4=
Expand Down
31 changes: 30 additions & 1 deletion pkg/healthchecks/healthchecks.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"k8s.io/ingress-gce/pkg/loadbalancers/features"
"k8s.io/ingress-gce/pkg/translator"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/healthcheck"
"k8s.io/klog/v2"
)

Expand All @@ -45,13 +46,27 @@ type HealthChecks struct {
defaultBackendSvc types.NamespacedName
recorderGetter RecorderGetter
serviceGetter ServiceGetter
clusterInfo healthcheck.ClusterInfo
}

// NewHealthChecker creates a new health checker.
// cloud: the cloud object implementing SingleHealthCheck.
// defaultHealthCheckPath: is the HTTP path to use for health checks.
func NewHealthChecker(cloud HealthCheckProvider, healthCheckPath string, defaultBackendSvc types.NamespacedName, recorderGetter RecorderGetter, serviceGetter ServiceGetter) *HealthChecks {
return &HealthChecks{cloud, healthCheckPath, defaultBackendSvc, recorderGetter, serviceGetter}
ci := generateClusterInfo(cloud.(*gce.Cloud))
return &HealthChecks{cloud, healthCheckPath, defaultBackendSvc, recorderGetter, serviceGetter, ci}
}

func generateClusterInfo(gceCloud *gce.Cloud) healthcheck.ClusterInfo {
var location string
regionalCluster := gceCloud.Regional()
if regionalCluster {
location = gceCloud.Region()
} else {
location = gceCloud.LocalZone()
}
name := flags.F.GKEClusterName
return healthcheck.ClusterInfo{Name: name, Location: location, Regional: regionalCluster}
}

// new returns a *HealthCheck with default settings and specified port/protocol
Expand All @@ -70,6 +85,7 @@ func (h *HealthChecks) new(sp utils.ServicePort) *translator.HealthCheck {
hc.Port = sp.NodePort
hc.RequestPath = h.pathFromSvcPort(sp)
hc.Service = h.getService(sp)
hc.SetHealthcheckInfo(h.generateHealthcheckInfo(sp, hc.ForILB))
return hc
}

Expand All @@ -94,6 +110,19 @@ func (h *HealthChecks) mainService(sp utils.ServicePort) types.NamespacedName {
return service
}

func (h *HealthChecks) generateHealthcheckInfo(sp utils.ServicePort, iLB bool) healthcheck.HealthcheckInfo {
serviceInfo := healthcheck.ServiceInfo(h.defaultBackendSvc)
if sp.ID.Service.Name != "" {
serviceInfo = healthcheck.ServiceInfo(sp.ID.Service)
}

return healthcheck.HealthcheckInfo{
ClusterInfo: h.clusterInfo,
ServiceInfo: serviceInfo,
HealthcheckConfig: healthcheck.DefaultHC,
}
}

// SyncServicePort implements HealthChecker.
func (h *HealthChecks) SyncServicePort(sp *utils.ServicePort, probe *v1.Probe) (string, error) {
hc := h.new(*sp)
Expand Down
103 changes: 84 additions & 19 deletions pkg/healthchecks/healthchecks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package healthchecks

import (
"context"
"encoding/json"
"fmt"
"net/http"
"reflect"
Expand All @@ -44,6 +45,7 @@ import (
"k8s.io/ingress-gce/pkg/loadbalancers/features"
"k8s.io/ingress-gce/pkg/translator"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/healthcheck"
namer_util "k8s.io/ingress-gce/pkg/utils/namer"
"k8s.io/klog/v2"
)
Expand Down Expand Up @@ -444,18 +446,28 @@ func getSingletonHealthcheck(t *testing.T, c *gce.Cloud) *compute.HealthCheck {

// Test changing the value of the flag EnableUpdateCustomHealthCheckDescription from false to true.
func TestRolloutUpdateCustomHCDescription(t *testing.T) {
// No parallel() because we modify the value of the flags EnableBackendConfigHealthCheck and EnableUpdateCustomHealthCheckDescription.
// No parallel() because we modify the value of the flags:
// - EnableBackendConfigHealthCheck,
// - EnableUpdateCustomHealthCheckDescription,
// - GKEClusterName.

testClusterValues := gce.DefaultTestClusterValues()

oldEnableBC := flags.F.EnableBackendConfigHealthCheck
oldUpdateDescription := flags.F.EnableUpdateCustomHealthCheckDescription
oldGKEClusterName := flags.F.GKEClusterName
flags.F.EnableBackendConfigHealthCheck = true
// Start with EnableUpdateCustomHealthCheckDescription = false.
flags.F.EnableUpdateCustomHealthCheckDescription = false
flags.F.GKEClusterName = testClusterValues.ClusterName
defer func() {
flags.F.EnableBackendConfigHealthCheck = oldEnableBC
flags.F.EnableUpdateCustomHealthCheckDescription = oldUpdateDescription
flags.F.GKEClusterName = oldGKEClusterName
}()

fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
testClusterValues.Regional = true
fakeGCE := gce.NewFakeGCECloud(testClusterValues)

var (
defaultSP *utils.ServicePort = testSPs["HTTP-80-reg-nil"]
Expand Down Expand Up @@ -506,9 +518,20 @@ func TestRolloutUpdateCustomHCDescription(t *testing.T) {

outputBCHCWithFlag := getSingletonHealthcheck(t, fakeGCE)

if outputBCHCWithFlag.Description != translator.DescriptionForHealthChecksFromBackendConfig {
wantDesc := healthcheck.HealthcheckDesc{
K8sCluster: fmt.Sprintf("/locations/%s/clusters/%s", gce.DefaultTestClusterValues().Region, gce.DefaultTestClusterValues().ClusterName),
K8sResource: fmt.Sprintf("/namespaces/%s/services/%s", defaultBackendSvc.Namespace, defaultBackendSvc.Name),
Config: "BackendConfig",
}
bytes, err := json.MarshalIndent(wantDesc, "", " ")
if err != nil {
t.Fatalf("Error while generating the wantDesc JSON.")
}
wantDescription := string(bytes)

if outputBCHCWithFlag.Description != wantDescription {
t.Fatalf("incorrect Description, is: \"%v\", want: \"%v\"",
outputBCHCWithFlag.Description, translator.DescriptionForHealthChecksFromBackendConfig)
outputBCHCWithFlag.Description, wantDescription)
}

// Verify that only the Description is modified after rollout.
Expand Down Expand Up @@ -984,21 +1007,28 @@ func setupMockUpdate(mock *cloud.MockGCE) {
}

func TestSyncServicePort(t *testing.T) {
// No parallel() because we modify the value of the flags EnableBackendConfigHealthCheck and EnableUpdateCustomHealthCheckDescription.
// No parallel() because we modify the value of the flags:
// - EnableBackendConfigHealthCheck,
// - EnableUpdateCustomHealthCheckDescription,
// - GKEClusterName. oldEnableBC := flags.F.EnableBackendConfigHealthCheck
oldEnableBC := flags.F.EnableBackendConfigHealthCheck
oldUpdateDescription := flags.F.EnableUpdateCustomHealthCheckDescription
flags.F.EnableBackendConfigHealthCheck = true
oldUpdateDescription := flags.F.EnableUpdateCustomHealthCheckDescription
oldGKEClusterName := flags.F.GKEClusterName
flags.F.GKEClusterName = gce.DefaultTestClusterValues().ClusterName
defer func() {
flags.F.EnableBackendConfigHealthCheck = oldEnableBC
flags.F.EnableUpdateCustomHealthCheckDescription = oldUpdateDescription
flags.F.GKEClusterName = oldGKEClusterName
}()

type tc struct {
desc string
setup func(*cloud.MockGCE)
sp *utils.ServicePort
probe *v1.Probe
regional bool
desc string
setup func(*cloud.MockGCE)
sp *utils.ServicePort
probe *v1.Probe
regional bool
regionalCluster bool

wantSelfLink string
wantErr bool
Expand Down Expand Up @@ -1132,6 +1162,18 @@ func TestSyncServicePort(t *testing.T) {
wantComputeHC: chc,
})

// BackendConfig ilb, regional cluster
chc = fixture.ilb()
chc.HttpHealthCheck.RequestPath = "/foo"
chc.Description = translator.DescriptionForHealthChecksFromBackendConfig
cases = append(cases, &tc{
desc: "create backendconfig ilb regional cluster",
sp: testSPs["HTTP-80-ilb-bc"],
regional: true,
regionalCluster: true,
wantComputeHC: chc,
})

// Probe and BackendConfig override
chc = fixture.hc()
chc.HttpHealthCheck.RequestPath = "/foo"
Expand Down Expand Up @@ -1339,15 +1381,38 @@ func TestSyncServicePort(t *testing.T) {
// settings.

for _, tc := range cases {
tc := *tc
tc.updateHCDescription = true
tc.desc = tc.desc + " with updateHCDescription"
cases = append(cases, tc)
copyOfWant := *tc.wantComputeHC
if tc.sp.BackendConfig != nil {
var wantLocation string
if tc.regionalCluster {
wantLocation = gce.DefaultTestClusterValues().Region
} else {
wantLocation = gce.DefaultTestClusterValues().ZoneName
}
wantDesc := healthcheck.HealthcheckDesc{
K8sCluster: fmt.Sprintf("/locations/%s/clusters/%s", wantLocation, gce.DefaultTestClusterValues().ClusterName),
K8sResource: fmt.Sprintf("/namespaces/%s/services/%s", defaultBackendSvc.Namespace, defaultBackendSvc.Name),
Config: "BackendConfig",
}
bytes, err := json.MarshalIndent(wantDesc, "", " ")
if err != nil {
t.Fatalf("Error while generating the wantDesc JSON.")
}
copyOfWant.Description = string(bytes)
}
tc.wantComputeHC = &copyOfWant
cases = append(cases, &tc)
}

for _, tc := range cases {
t.Run(tc.desc, func(t *testing.T) {
flags.F.EnableUpdateCustomHealthCheckDescription = tc.updateHCDescription
fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
testClusterValues := gce.DefaultTestClusterValues()
testClusterValues.Regional = tc.regionalCluster
fakeGCE := gce.NewFakeGCECloud(testClusterValues)

mock := fakeGCE.Compute().(*cloud.MockGCE)
setupMockUpdate(mock)
Expand Down Expand Up @@ -1378,20 +1443,20 @@ func TestSyncServicePort(t *testing.T) {
t.Fatalf("Got %d healthchecks, want 1\n%s", len(computeHCs), pretty.Sprint(computeHCs))
}

gotHC := computeHCs[0]
// Filter out SelfLink because it is hard to deal with in the mock and
// test cases.
filter := func(hc *compute.HealthCheck) {
filter := func(hc compute.HealthCheck) compute.HealthCheck {
hc.SelfLink = ""
if !tc.updateHCDescription {
hc.Description = ""
}
return hc
}
filter(gotHC)
filter(tc.wantComputeHC)
gotHC := filter(*computeHCs[0])
wantHC := filter(*tc.wantComputeHC)

if !reflect.DeepEqual(gotHC, tc.wantComputeHC) {
t.Fatalf("Compute healthcheck is:\n%s, want:\n%s", pretty.Sprint(gotHC), pretty.Sprint(tc.wantComputeHC))
if !reflect.DeepEqual(gotHC, wantHC) {
t.Fatalf("Compute healthcheck is:\n%s, want:\n%s", pretty.Sprint(gotHC), pretty.Sprint(wantHC))
}
}

Expand Down
6 changes: 1 addition & 5 deletions pkg/psc/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,7 @@ func NewController(ctx *context.ControllerContext) *Controller {
if controller.regionalCluster {
controller.clusterLoc = controller.cloud.Region()
} else {
zone, err := controller.cloud.GetZone(context2.Background())
if err != nil {
klog.Errorf("Failed to retrieve zone information from cloud provider: %q", err)
}
controller.clusterLoc = zone.FailureDomain
controller.clusterLoc = controller.cloud.LocalZone()
}

ctx.SAInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
Expand Down
7 changes: 2 additions & 5 deletions pkg/psc/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,8 @@ func TestServiceAttachmentCreation(t *testing.T) {
t.Errorf("%s", err)
}

zone, err := fakeCloud.GetZone(context2.TODO())
if err != nil {
t.Errorf("failed to get zone %q", err)
}
desc := sautils.NewServiceAttachmentDesc(testNamespace, saName, ClusterName, zone.FailureDomain, false)
zone := fakeCloud.LocalZone()
desc := sautils.NewServiceAttachmentDesc(testNamespace, saName, ClusterName, zone, false)

expectedSA := &ga.ServiceAttachment{
ConnectionPreference: tc.connectionPreference,
Expand Down
22 changes: 18 additions & 4 deletions pkg/translator/healthchecks.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"k8s.io/ingress-gce/pkg/flags"
"k8s.io/ingress-gce/pkg/loadbalancers/features"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/healthcheck"
"k8s.io/klog/v2"
)

Expand Down Expand Up @@ -91,7 +92,19 @@ type HealthCheck struct {
computealpha.HTTPHealthCheck
computealpha.HealthCheck

Service *v1.Service
Service *v1.Service
healthcheckInfo healthcheck.HealthcheckInfo
}

func (hc *HealthCheck) SetHealthcheckInfo(i healthcheck.HealthcheckInfo) {
hc.healthcheckInfo = i
hc.reconcileHCDescription()
}

func (hc *HealthCheck) reconcileHCDescription() {
if flags.F.EnableUpdateCustomHealthCheckDescription && hc.healthcheckInfo.HealthcheckConfig == healthcheck.BackendConfigHC {
hc.Description = hc.healthcheckInfo.GenerateHealthcheckDescription()
}
}

// NewHealthCheck creates a HealthCheck which abstracts nested structs away
Expand Down Expand Up @@ -226,9 +239,8 @@ func (hc *HealthCheck) UpdateFromBackendConfig(c *backendconfigv1.HealthCheckCon
// This override is necessary regardless of type
hc.PortSpecification = "USE_FIXED_PORT"
}
if flags.F.EnableUpdateCustomHealthCheckDescription {
hc.Description = DescriptionForHealthChecksFromBackendConfig
}
hc.healthcheckInfo.HealthcheckConfig = healthcheck.BackendConfigHC
hc.reconcileHCDescription()
}

// DefaultHealthCheck simply returns the default health check.
Expand Down Expand Up @@ -339,4 +351,6 @@ func ApplyProbeSettingsToHC(p *v1.Probe, hc *HealthCheck) {
}

hc.Description = DescriptionForHealthChecksFromReadinessProbe
hc.healthcheckInfo.HealthcheckConfig = healthcheck.ReadinessProbeHC
hc.reconcileHCDescription()
}
Loading