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.

Sign up for GitHub

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

Reduce surface area of healthchecks #1028

Merged
merged 2 commits into from
Feb 17, 2020
Merged

Reduce surface area of healthchecks #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39af8ca
Reduce surface area of healthchecks
bowei Feb 16, 2020
8e29a6b
Remove `New` from the interface
bowei Feb 16, 2020
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
30 changes: 8 additions & 22 deletions pkg/backends/syncer.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
}