From 39af8ca9bf242018db9ac3fafa7f55fbca3f2e10 Mon Sep 17 00:00:00 2001 From: Bowei Du Date: Sun, 16 Feb 2020 15:05:10 -0800 Subject: [PATCH 1/2] Reduce surface area of healthchecks - Remove unused variable isLegacy - Sync is now package private - SyncServicePort encapsulates Sync and Probe override logic - applyProbeSettingsToHC is now package private --- pkg/backends/syncer.go | 30 +++++-------------- pkg/healthchecks/healthchecks.go | 22 ++++++++++---- pkg/healthchecks/healthchecks_test.go | 42 ++++++++++++--------------- pkg/healthchecks/interfaces.go | 11 +++++-- 4 files changed, 50 insertions(+), 55 deletions(-) diff --git a/pkg/backends/syncer.go b/pkg/backends/syncer.go index 56ddb13d5e..47218b7b78 100644 --- a/pkg/backends/syncer.go +++ b/pkg/backends/syncer.go @@ -15,9 +15,9 @@ package backends import ( "fmt" - "strings" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/ingress-gce/pkg/backends/features" "k8s.io/ingress-gce/pkg/composite" @@ -77,17 +77,9 @@ func (s *backendSyncer) ensureBackendService(sp utils.ServicePort) error { scope := features.ScopeFromServicePort(&sp) be, getErr := s.backendPool.Get(beName, version, scope) - hasLegacyHC := false - if be != nil { - // If the backend already exists, find out if it is using a legacy health check. - existingHCLink := getHealthCheckLink(be) - if strings.Contains(existingHCLink, "/httpHealthChecks/") { - hasLegacyHC = true - } - } // Ensure health check for backend service exists. - hcLink, err := s.ensureHealthCheck(sp, hasLegacyHC) + hcLink, err := s.ensureHealthCheck(sp) if err != nil { return fmt.Errorf("error ensuring health check: %v", err) } @@ -234,23 +226,17 @@ func (s *backendSyncer) Shutdown() error { return nil } -func (s *backendSyncer) ensureHealthCheck(sp utils.ServicePort, hasLegacyHC bool) (string, error) { - if hasLegacyHC { - klog.Errorf("Backend %+v has legacy health check", sp.ID) - } - hc := s.healthChecker.New(sp) +func (s *backendSyncer) ensureHealthCheck(sp utils.ServicePort) (string, error) { + var probe *v1.Probe + var err error + if s.prober != nil { - probe, err := s.prober.GetProbe(sp) + probe, err = s.prober.GetProbe(sp) if err != nil { return "", fmt.Errorf("Error getting prober: %v", err) } - if probe != nil { - klog.V(4).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp) - healthchecks.ApplyProbeSettingsToHC(probe, hc) - } } - - return s.healthChecker.Sync(hc) + return s.healthChecker.SyncServicePort(&sp, probe) } // getHealthCheckLink gets the Healthcheck link off the BackendService diff --git a/pkg/healthchecks/healthchecks.go b/pkg/healthchecks/healthchecks.go index a814e29771..315db0afe4 100644 --- a/pkg/healthchecks/healthchecks.go +++ b/pkg/healthchecks/healthchecks.go @@ -87,7 +87,7 @@ type HealthChecks struct { // 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) HealthChecker { +func NewHealthChecker(cloud HealthCheckProvider, healthCheckPath string, defaultBackendSvc types.NamespacedName) *HealthChecks { return &HealthChecks{cloud, healthCheckPath, defaultBackendSvc} } @@ -109,9 +109,19 @@ func (h *HealthChecks) New(sp utils.ServicePort) *HealthCheck { return hc } -// Sync retrieves a health check based on port, checks type and settings and updates/creates if necessary. -// Sync is only called by the backends.Add func - it's not a pool like other resources. -func (h *HealthChecks) Sync(hc *HealthCheck) (string, error) { +// SyncServicePort implements HealthChecker. +func (h *HealthChecks) SyncServicePort(sp *utils.ServicePort, probe *v1.Probe) (string, error) { + hc := h.New(*sp) + if probe != nil { + klog.V(4).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp) + applyProbeSettingsToHC(probe, hc) + } + return h.sync(hc) +} + +// sync retrieves a health check based on port, checks type and settings and updates/creates if necessary. +// sync is only called by the backends.Add func - it's not a pool like other resources. +func (h *HealthChecks) sync(hc *HealthCheck) (string, error) { var scope meta.KeyType // TODO(shance): find a way to remove this if hc.forILB { @@ -642,8 +652,8 @@ func copyViaJSON(dest interface{}, src jsonConvertable) error { return json.Unmarshal(bytes, dest) } -// ApplyProbeSettingsToHC TODO -func ApplyProbeSettingsToHC(p *v1.Probe, hc *HealthCheck) { +// applyProbeSettingsToHC TODO +func applyProbeSettingsToHC(p *v1.Probe, hc *HealthCheck) { healthPath := p.Handler.HTTPGet.Path // GCE requires a leading "/" for health check urls. if !strings.HasPrefix(healthPath, "/") { diff --git a/pkg/healthchecks/healthchecks_test.go b/pkg/healthchecks/healthchecks_test.go index 36f91b712c..d9ffb34cf0 100644 --- a/pkg/healthchecks/healthchecks_test.go +++ b/pkg/healthchecks/healthchecks_test.go @@ -43,9 +43,8 @@ func TestHealthCheckAdd(t *testing.T) { fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) healthChecks := NewHealthChecker(fakeGCE, "/", defaultBackendSvc) - sp := utils.ServicePort{NodePort: 80, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: namer} - hc := healthChecks.New(sp) - _, err := healthChecks.Sync(hc) + sp := &utils.ServicePort{NodePort: 80, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: namer} + _, err := healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -55,9 +54,8 @@ func TestHealthCheckAdd(t *testing.T) { t.Fatalf("expected the health check to exist, err: %v", err) } - sp = utils.ServicePort{NodePort: 443, Protocol: annotations.ProtocolHTTPS, NEGEnabled: false, BackendNamer: namer} - hc = healthChecks.New(sp) - _, err = healthChecks.Sync(hc) + sp = &utils.ServicePort{NodePort: 443, Protocol: annotations.ProtocolHTTPS, NEGEnabled: false, BackendNamer: namer} + _, err = healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -67,9 +65,8 @@ func TestHealthCheckAdd(t *testing.T) { t.Fatalf("expected the health check to exist, err: %v", err) } - sp = utils.ServicePort{NodePort: 3000, Protocol: annotations.ProtocolHTTP2, NEGEnabled: false, BackendNamer: namer} - hc = healthChecks.New(sp) - _, err = healthChecks.Sync(hc) + sp = &utils.ServicePort{NodePort: 3000, Protocol: annotations.ProtocolHTTP2, NEGEnabled: false, BackendNamer: namer} + _, err = healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -95,10 +92,9 @@ func TestHealthCheckAddExisting(t *testing.T) { } fakeGCE.CreateHealthCheck(v1hc) - sp := utils.ServicePort{NodePort: 3000, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: namer} + sp := &utils.ServicePort{NodePort: 3000, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: namer} // Should not fail adding the same type of health check - hc := healthChecks.New(sp) - _, err = healthChecks.Sync(hc) + _, err = healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -119,9 +115,8 @@ func TestHealthCheckAddExisting(t *testing.T) { } fakeGCE.CreateHealthCheck(v1hc) - sp = utils.ServicePort{NodePort: 4000, Protocol: annotations.ProtocolHTTPS, NEGEnabled: false, BackendNamer: namer} - hc = healthChecks.New(sp) - _, err = healthChecks.Sync(hc) + sp = &utils.ServicePort{NodePort: 4000, Protocol: annotations.ProtocolHTTPS, NEGEnabled: false, BackendNamer: namer} + _, err = healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -142,9 +137,8 @@ func TestHealthCheckAddExisting(t *testing.T) { } fakeGCE.CreateHealthCheck(v1hc) - sp = utils.ServicePort{NodePort: 5000, Protocol: annotations.ProtocolHTTPS, NEGEnabled: false, BackendNamer: namer} - hc = healthChecks.New(sp) - _, err = healthChecks.Sync(hc) + sp = &utils.ServicePort{NodePort: 5000, Protocol: annotations.ProtocolHTTPS, NEGEnabled: false, BackendNamer: namer} + _, err = healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -244,7 +238,7 @@ func TestHealthCheckUpdate(t *testing.T) { // Change to HTTPS hc.Type = string(annotations.ProtocolHTTPS) - _, err = healthChecks.Sync(hc) + _, err = healthChecks.sync(hc) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) } @@ -262,7 +256,7 @@ func TestHealthCheckUpdate(t *testing.T) { // Change to HTTP2 hc.Type = string(annotations.ProtocolHTTP2) - _, err = healthChecks.Sync(hc) + _, err = healthChecks.sync(hc) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) } @@ -281,7 +275,7 @@ func TestHealthCheckUpdate(t *testing.T) { // Change to NEG Health Check hc.ForNEG = true hc.PortSpecification = UseServingPortSpecification - _, err = healthChecks.Sync(hc) + _, err = healthChecks.sync(hc) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) @@ -302,7 +296,7 @@ func TestHealthCheckUpdate(t *testing.T) { hc.Port = 3000 hc.PortSpecification = "" - _, err = healthChecks.Sync(hc) + _, err = healthChecks.sync(hc) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) } @@ -327,7 +321,7 @@ func TestAlphaHealthCheck(t *testing.T) { healthChecks := NewHealthChecker(fakeGCE, "/", defaultBackendSvc) sp := utils.ServicePort{NodePort: 8000, Protocol: annotations.ProtocolHTTPS, NEGEnabled: true, BackendNamer: namer} hc := healthChecks.New(sp) - _, err := healthChecks.Sync(hc) + _, err := healthChecks.sync(hc) if err != nil { t.Fatalf("got %v, want nil", err) } @@ -418,7 +412,7 @@ func TestApplyProbeSettingsToHC(t *testing.T) { }, } - ApplyProbeSettingsToHC(probe, hc) + applyProbeSettingsToHC(probe, hc) if hc.Protocol() != annotations.ProtocolHTTPS || hc.Port != 8080 { t.Errorf("Basic HC settings changed") diff --git a/pkg/healthchecks/interfaces.go b/pkg/healthchecks/interfaces.go index 5e4e62d98d..52b833b6cc 100644 --- a/pkg/healthchecks/interfaces.go +++ b/pkg/healthchecks/interfaces.go @@ -20,9 +20,10 @@ import ( computealpha "google.golang.org/api/compute/v0.alpha" computebeta "google.golang.org/api/compute/v0.beta" compute "google.golang.org/api/compute/v1" + v1 "k8s.io/api/core/v1" + "k8s.io/ingress-gce/pkg/utils" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" - "k8s.io/ingress-gce/pkg/utils" ) // HealthCheckProvider is an interface to manage a single GCE health check. @@ -47,8 +48,12 @@ type HealthCheckProvider interface { // HealthChecker is an interface to manage cloud HTTPHealthChecks. type HealthChecker interface { - New(sp utils.ServicePort) *HealthCheck - Sync(hc *HealthCheck) (string, error) + New(utils.ServicePort) *HealthCheck + // SyncServicePort syncs the healthcheck associated with the given + // ServicePort and Pod Probe definition. + // + // `probe` can be nil if no probe exists. + SyncServicePort(sp *utils.ServicePort, probe *v1.Probe) (string, error) Delete(name string, scope meta.KeyType) error Get(name string, version meta.Version, scope meta.KeyType) (*HealthCheck, error) } From 8e29a6b35c73dd0a0b411d805dd0459ae687d27c Mon Sep 17 00:00:00 2001 From: Bowei Du Date: Sun, 16 Feb 2020 15:13:48 -0800 Subject: [PATCH 2/2] Remove `New` from the interface --- pkg/healthchecks/healthchecks.go | 6 +++--- pkg/healthchecks/healthchecks_test.go | 2 +- pkg/healthchecks/interfaces.go | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/healthchecks/healthchecks.go b/pkg/healthchecks/healthchecks.go index 315db0afe4..f8966dd413 100644 --- a/pkg/healthchecks/healthchecks.go +++ b/pkg/healthchecks/healthchecks.go @@ -91,8 +91,8 @@ func NewHealthChecker(cloud HealthCheckProvider, healthCheckPath string, default return &HealthChecks{cloud, healthCheckPath, defaultBackendSvc} } -// New returns a *HealthCheck with default settings and specified port/protocol -func (h *HealthChecks) New(sp utils.ServicePort) *HealthCheck { +// new returns a *HealthCheck with default settings and specified port/protocol +func (h *HealthChecks) new(sp utils.ServicePort) *HealthCheck { var hc *HealthCheck if sp.NEGEnabled && !sp.L7ILBEnabled { hc = DefaultNEGHealthCheck(sp.Protocol) @@ -111,7 +111,7 @@ func (h *HealthChecks) New(sp utils.ServicePort) *HealthCheck { // SyncServicePort implements HealthChecker. func (h *HealthChecks) SyncServicePort(sp *utils.ServicePort, probe *v1.Probe) (string, error) { - hc := h.New(*sp) + hc := h.new(*sp) if probe != nil { klog.V(4).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp) applyProbeSettingsToHC(probe, hc) diff --git a/pkg/healthchecks/healthchecks_test.go b/pkg/healthchecks/healthchecks_test.go index d9ffb34cf0..ab449351f1 100644 --- a/pkg/healthchecks/healthchecks_test.go +++ b/pkg/healthchecks/healthchecks_test.go @@ -320,7 +320,7 @@ func TestAlphaHealthCheck(t *testing.T) { fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) healthChecks := NewHealthChecker(fakeGCE, "/", defaultBackendSvc) sp := utils.ServicePort{NodePort: 8000, Protocol: annotations.ProtocolHTTPS, NEGEnabled: true, BackendNamer: namer} - hc := healthChecks.New(sp) + hc := healthChecks.new(sp) _, err := healthChecks.sync(hc) if err != nil { t.Fatalf("got %v, want nil", err) diff --git a/pkg/healthchecks/interfaces.go b/pkg/healthchecks/interfaces.go index 52b833b6cc..3c60f9c71a 100644 --- a/pkg/healthchecks/interfaces.go +++ b/pkg/healthchecks/interfaces.go @@ -48,7 +48,6 @@ type HealthCheckProvider interface { // HealthChecker is an interface to manage cloud HTTPHealthChecks. type HealthChecker interface { - New(utils.ServicePort) *HealthCheck // SyncServicePort syncs the healthcheck associated with the given // ServicePort and Pod Probe definition. //