Skip to content

Commit

Permalink
Merge pull request #1028 from bowei/hc-surface-area
Browse files Browse the repository at this point in the history 
Reduce surface area of healthchecks
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Feb 17, 2020
2 parents 847b282 + 8e29a6b commit a2dc440
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 58 deletions.
30 changes: 8 additions & 22 deletions pkg/backends/syncer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
26 changes: 18 additions & 8 deletions pkg/healthchecks/healthchecks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@ 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}
}

// 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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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, "/") {
Expand Down
44 changes: 19 additions & 25 deletions pkg/healthchecks/healthchecks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
Expand All @@ -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)
}
Expand All @@ -326,8 +320,8 @@ 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)
_, err := healthChecks.Sync(hc)
hc := healthChecks.new(sp)
_, err := healthChecks.sync(hc)
if err != nil {
t.Fatalf("got %v, want nil", err)
}
Expand Down Expand Up @@ -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")
Expand Down
10 changes: 7 additions & 3 deletions pkg/healthchecks/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -47,8 +48,11 @@ type HealthCheckProvider interface {

// HealthChecker is an interface to manage cloud HTTPHealthChecks.
type HealthChecker interface {
New(sp utils.ServicePort) *HealthCheck
Sync(hc *HealthCheck) (string, error)
// 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)
}

0 comments on commit a2dc440

Please sign in to comment.