From 32c90799eebe4d1edfb51d8333b912ffb70f7e3a Mon Sep 17 00:00:00 2001 From: Bowei Du Date: Mon, 17 Feb 2020 12:45:18 -0800 Subject: [PATCH] finished with first pass on testing --- pkg/healthchecks/healthchecks.go | 20 +-- pkg/healthchecks/healthchecks_test.go | 236 ++++++++++++++++++++++---- 2 files changed, 217 insertions(+), 39 deletions(-) diff --git a/pkg/healthchecks/healthchecks.go b/pkg/healthchecks/healthchecks.go index 32d51bde80..67ffaa08f5 100644 --- a/pkg/healthchecks/healthchecks.go +++ b/pkg/healthchecks/healthchecks.go @@ -271,7 +271,7 @@ func (h *HealthChecks) update(hc *HealthCheck) error { } return h.cloud.UpdateBetaHealthCheck(beta) case meta.VersionGA: - klog.V(2).Infof("Updating health check for port %v with protocol %v", hc.Port, hc.Type) + klog.V(2).Infof("Updating health check %q for port %v with protocol %v", hc.Name, hc.Port, hc.Type) ga, err := hc.ToComputeHealthCheck() if err != nil { return err @@ -294,16 +294,17 @@ func (h *HealthChecks) update(hc *HealthCheck) error { // TODO(bowei): this is very unstable in combination with Probe, we do not // have a clear signal as to where the settings are coming from. Once a // healthcheck is created, it will basically not change. -func mergeUserSettings(old, newHC *HealthCheck) *HealthCheck { +func mergeUserSettings(existing, newHC *HealthCheck) *HealthCheck { hc := *newHC // return a copy - hc.HTTPHealthCheck = old.HTTPHealthCheck - hc.HealthCheck.CheckIntervalSec = old.HealthCheck.CheckIntervalSec - hc.HealthCheck.HealthyThreshold = old.HealthCheck.HealthyThreshold - hc.HealthCheck.TimeoutSec = old.HealthCheck.TimeoutSec - hc.HealthCheck.UnhealthyThreshold = old.HealthCheck.UnhealthyThreshold - if old.HealthCheck.LogConfig != nil { - l := *old.HealthCheck.LogConfig + hc.HTTPHealthCheck = existing.HTTPHealthCheck + hc.HealthCheck.CheckIntervalSec = existing.HealthCheck.CheckIntervalSec + hc.HealthCheck.HealthyThreshold = existing.HealthCheck.HealthyThreshold + hc.HealthCheck.TimeoutSec = existing.HealthCheck.TimeoutSec + hc.HealthCheck.UnhealthyThreshold = existing.HealthCheck.UnhealthyThreshold + + if existing.HealthCheck.LogConfig != nil { + l := *existing.HealthCheck.LogConfig hc.HealthCheck.LogConfig = &l } @@ -315,7 +316,6 @@ func mergeUserSettings(old, newHC *HealthCheck) *HealthCheck { hc.PortSpecification = "" hc.Port = newHC.Port } - return &hc } diff --git a/pkg/healthchecks/healthchecks_test.go b/pkg/healthchecks/healthchecks_test.go index 8658b62f50..081237130d 100644 --- a/pkg/healthchecks/healthchecks_test.go +++ b/pkg/healthchecks/healthchecks_test.go @@ -29,6 +29,7 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock" "github.com/kr/pretty" computealpha "google.golang.org/api/compute/v0.alpha" + computebeta "google.golang.org/api/compute/v0.beta" "google.golang.org/api/compute/v1" api_v1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" @@ -692,10 +693,6 @@ func TestCalculateDiff(t *testing.T) { } } -func filterComputeHC(hc *compute.HealthCheck) { - hc.Description = "" -} - const ( regSelfLink = "https://www.googleapis.com/compute/v1/projects/test-project/global/healthChecks/k8s-be-80--uid1" negSelfLink = "https://www.googleapis.com/compute/beta/projects/test-project/global/healthChecks/k8s1-uid1---0-56ff9a48" @@ -720,9 +717,14 @@ func (*syncSPFixtures) hc() *compute.HealthCheck { } } -func (f *syncSPFixtures) hcs() *compute.HealthCheck { - h := f.hc() - // Swap the structs. +func (f *syncSPFixtures) hcs() *compute.HealthCheck { return f.toS(f.hc()) } +func (f *syncSPFixtures) hc2() *compute.HealthCheck { return f.to2(f.hc()) } +func (f *syncSPFixtures) negs() *compute.HealthCheck { return f.toS(f.neg()) } +func (f *syncSPFixtures) neg2() *compute.HealthCheck { return f.to2(f.neg()) } +func (f *syncSPFixtures) ilbs() *compute.HealthCheck { return f.toS(f.ilb()) } +func (f *syncSPFixtures) ilb2() *compute.HealthCheck { return f.to2(f.ilb()) } + +func (f *syncSPFixtures) toS(h *compute.HealthCheck) *compute.HealthCheck { h.Type = "HTTPS" c := (compute.HTTPSHealthCheck)(*h.HttpHealthCheck) h.HttpsHealthCheck = &c @@ -730,9 +732,7 @@ func (f *syncSPFixtures) hcs() *compute.HealthCheck { return h } -func (f *syncSPFixtures) hc2() *compute.HealthCheck { - h := f.hc() - // Swap the structs. +func (f *syncSPFixtures) to2(h *compute.HealthCheck) *compute.HealthCheck { h.Type = "HTTP2" c := (compute.HTTP2HealthCheck)(*h.HttpHealthCheck) h.Http2HealthCheck = &c @@ -770,6 +770,41 @@ func (f *syncSPFixtures) makeSetup(hc *compute.HealthCheck) func(m *cloud.MockGC } } +func (f *syncSPFixtures) makeRegionalSetup(region string, hc *compute.HealthCheck) func(m *cloud.MockGCE) { + return func(m *cloud.MockGCE) { + key := meta.RegionalKey(hc.Name, region) + m.RegionHealthChecks().Insert(context.Background(), key, hc) + } +} + +func setupMockUpdate(mock *cloud.MockGCE) { + // Mock does not know how to autogenerate update() methods. + mock.MockHealthChecks.UpdateHook = func(ctx context.Context, k *meta.Key, hc *compute.HealthCheck, m *cloud.MockHealthChecks) error { + m.Objects[*k].Obj = hc + return nil + } + mock.MockAlphaHealthChecks.UpdateHook = func(ctx context.Context, k *meta.Key, hc *computealpha.HealthCheck, m *cloud.MockAlphaHealthChecks) error { + m.Objects[*k].Obj = hc + return nil + } + mock.MockBetaHealthChecks.UpdateHook = func(ctx context.Context, k *meta.Key, hc *computebeta.HealthCheck, m *cloud.MockBetaHealthChecks) error { + m.Objects[*k].Obj = hc + return nil + } + mock.MockRegionHealthChecks.UpdateHook = func(ctx context.Context, k *meta.Key, hc *compute.HealthCheck, m *cloud.MockRegionHealthChecks) error { + m.Objects[*k].Obj = hc + return nil + } + mock.MockAlphaRegionHealthChecks.UpdateHook = func(ctx context.Context, k *meta.Key, hc *computealpha.HealthCheck, m *cloud.MockAlphaRegionHealthChecks) error { + m.Objects[*k].Obj = hc + return nil + } + mock.MockBetaRegionHealthChecks.UpdateHook = func(ctx context.Context, k *meta.Key, hc *computebeta.HealthCheck, m *cloud.MockBetaRegionHealthChecks) error { + m.Objects[*k].Obj = hc + return nil + } +} + func TestSyncServicePort(t *testing.T) { type tc struct { desc string @@ -782,25 +817,18 @@ func TestSyncServicePort(t *testing.T) { wantErr bool wantComputeHC *compute.HealthCheck } + fixture := syncSPFixtures{} var cases []*tc - fixture := syncSPFixtures{} cases = append(cases, &tc{desc: "create http", sp: testSPs["HTTP-80-reg-nil"], wantComputeHC: fixture.hc()}) cases = append(cases, &tc{desc: "create https", sp: testSPs["HTTPS-80-reg-nil"], wantComputeHC: fixture.hcs()}) - - // HTTP2 - chc := fixture.hc2() - // TODO(bowei) -- should this be in Beta? - chc.SelfLink = "https://www.googleapis.com/compute/beta/projects/test-project/global/healthChecks/k8s-be-80--uid1" - cases = append(cases, &tc{desc: "create http2", sp: testSPs["HTTP2-80-reg-nil"], wantComputeHC: chc}) - - // NEG, ILB + cases = append(cases, &tc{desc: "create http2", sp: testSPs["HTTP2-80-reg-nil"], wantComputeHC: fixture.hc2()}) cases = append(cases, &tc{desc: "create neg", sp: testSPs["HTTP-80-neg-nil"], wantComputeHC: fixture.neg()}) cases = append(cases, &tc{desc: "create ilb", sp: testSPs["HTTP-80-ilb-nil"], regional: true, wantComputeHC: fixture.ilb()}) // Probe override - chc = fixture.hc() + chc := fixture.hc() chc.HttpHealthCheck.RequestPath = "/foo" chc.HttpHealthCheck.Host = "foo.com" chc.CheckIntervalSec = 61 @@ -912,57 +940,196 @@ func TestSyncServicePort(t *testing.T) { // Update tests cases = append(cases, &tc{ desc: "update http no change", - sp: testSPs["HTTP-80-reg-nil"], setup: fixture.makeSetup(fixture.hc()), + sp: testSPs["HTTP-80-reg-nil"], wantComputeHC: fixture.hc(), }) cases = append(cases, &tc{ desc: "update https no change", - sp: testSPs["HTTPS-80-reg-nil"], setup: fixture.makeSetup(fixture.hcs()), + sp: testSPs["HTTPS-80-reg-nil"], wantComputeHC: fixture.hcs(), }) cases = append(cases, &tc{ desc: "update http2 no change", - sp: testSPs["HTTP2-80-reg-nil"], setup: fixture.makeSetup(fixture.hc2()), + sp: testSPs["HTTP2-80-reg-nil"], wantComputeHC: fixture.hc2(), }) + cases = append(cases, &tc{ + desc: "update neg no change", + setup: fixture.makeSetup(fixture.neg()), + sp: testSPs["HTTP-80-neg-nil"], + wantComputeHC: fixture.neg(), + }) + cases = append(cases, &tc{ + desc: "update ilb no change", + setup: fixture.makeSetup(fixture.ilb()), + sp: testSPs["HTTP-80-ilb-nil"], + regional: true, + wantComputeHC: fixture.ilb(), + }) + // Update protocol cases = append(cases, &tc{ desc: "update http to https", - sp: testSPs["HTTPS-80-reg-nil"], setup: fixture.makeSetup(fixture.hc()), + sp: testSPs["HTTPS-80-reg-nil"], wantComputeHC: fixture.hcs(), }) + cases = append(cases, &tc{ + desc: "update http to http2", + setup: fixture.makeSetup(fixture.hc()), + sp: testSPs["HTTP2-80-reg-nil"], + wantComputeHC: fixture.hc2(), + }) + cases = append(cases, &tc{ + desc: "update https to http", + setup: fixture.makeSetup(fixture.hcs()), + sp: testSPs["HTTP-80-reg-nil"], + wantComputeHC: fixture.hc(), + }) + cases = append(cases, &tc{ + desc: "update https to http2", + setup: fixture.makeSetup(fixture.hcs()), + sp: testSPs["HTTP2-80-reg-nil"], + wantComputeHC: fixture.hc2(), + }) + cases = append(cases, &tc{ + desc: "update neg protocol", + setup: fixture.makeSetup(fixture.neg()), + sp: testSPs["HTTPS-80-neg-nil"], + wantComputeHC: fixture.negs(), + }) + cases = append(cases, &tc{ + desc: "update ilb protocol", + setup: fixture.makeRegionalSetup("us-central1", fixture.ilb()), + sp: testSPs["HTTPS-80-ilb-nil"], + regional: true, + wantComputeHC: fixture.ilbs(), + }) + // Preserve user settings. chc = fixture.hc() chc.HttpHealthCheck.RequestPath = "/user-path" chc.CheckIntervalSec = 1234 cases = append(cases, &tc{ - desc: "update preserve user settings", - sp: testSPs["HTTP-80-reg-nil"], + desc: "update preserve", setup: fixture.makeSetup(chc), + sp: testSPs["HTTP-80-reg-nil"], wantComputeHC: chc, }) + // Preserve user settings while changing the protocol chc = fixture.hc() chc.HttpHealthCheck.RequestPath = "/user-path" chc.CheckIntervalSec = 1234 + wantCHC := fixture.hcs() + wantCHC.HttpsHealthCheck.RequestPath = "/user-path" + wantCHC.CheckIntervalSec = 1234 cases = append(cases, &tc{ - desc: "update preserve user settings", - sp: testSPs["HTTP-80-reg-nil"], + desc: "update preserve across protocol change", + setup: fixture.makeSetup(chc), + sp: testSPs["HTTPS-80-reg-nil"], + wantComputeHC: wantCHC, + }) + + // Preserve user settings while changing the protocol (neg) + chc = fixture.neg() + chc.HttpHealthCheck.RequestPath = "/user-path" + chc.CheckIntervalSec = 1234 + wantCHC = fixture.negs() + wantCHC.HttpsHealthCheck.RequestPath = "/user-path" + wantCHC.CheckIntervalSec = 1234 + cases = append(cases, &tc{ + desc: "update preserve across protocol change neg", setup: fixture.makeSetup(chc), + sp: testSPs["HTTPS-80-neg-nil"], + wantComputeHC: wantCHC, + }) + + // Preserve user settings while changing the protocol (ilb) + chc = fixture.ilb() + chc.HttpHealthCheck.RequestPath = "/user-path" + chc.CheckIntervalSec = 1234 + wantCHC = fixture.ilbs() + wantCHC.HttpsHealthCheck.RequestPath = "/user-path" + wantCHC.CheckIntervalSec = 1234 + cases = append(cases, &tc{ + desc: "update preserve across protocol change ilb", + setup: fixture.makeRegionalSetup("us-central1", chc), + sp: testSPs["HTTPS-80-ilb-nil"], + regional: true, + wantComputeHC: wantCHC, + }) + + // Preserve some settings, but override some in the backend config + chc = fixture.hc() + chc.HttpHealthCheck.RequestPath = "/user-path" + chc.CheckIntervalSec = 1234 + wantCHC = fixture.hc() + wantCHC.HttpHealthCheck.RequestPath = "/foo" // from bc + wantCHC.CheckIntervalSec = 1234 // same + cases = append(cases, &tc{ + desc: "update preserve and backendconfig (path)", + sp: testSPs["HTTP-80-reg-bc"], + setup: fixture.makeSetup(chc), + wantComputeHC: wantCHC, + }) + + // Override all settings from backendconfig. + chc = fixture.hc() + chc.HttpHealthCheck.RequestPath = "/user-path" + chc.CheckIntervalSec = 1234 + wantCHC = fixture.hc() + wantCHC.HttpHealthCheck.RequestPath = "/foo" // from bc + wantCHC.CheckIntervalSec = 1234 + wantCHC.HealthyThreshold = 1234 + wantCHC.UnhealthyThreshold = 1234 + wantCHC.TimeoutSec = 1234 + cases = append(cases, &tc{ + desc: "update preserve backendconfig all", + setup: fixture.makeSetup(chc), + sp: testSPs["HTTP-80-reg-bcall"], + wantComputeHC: wantCHC, + }) + + // BUG: changing probe settings has not effect on the healthcheck + // update. + // TODO(bowei): document this. + chc = fixture.hc() + chc.HttpHealthCheck.RequestPath = "/user-path" + cases = append(cases, &tc{ + desc: "update probe has no effect (bug)", + setup: fixture.makeSetup(chc), + sp: testSPs["HTTP-80-reg-nil"], + probe: &v1.Probe{ + Handler: v1.Handler{ + HTTPGet: &v1.HTTPGetAction{Path: "/foo", Host: "foo.com"}, + }, + PeriodSeconds: 1, + TimeoutSeconds: 1234, + }, wantComputeHC: chc, }) + // Bug: Enable NEG, leaks old healthcheck, does not preserve old + // settings. + + // Bug: Switch to/from ILB, leaks old healthcheck, does not preserve old + // settings. + for _, tc := range cases { t.Run(tc.desc, func(t *testing.T) { fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) + mock := fakeGCE.Compute().(*cloud.MockGCE) + setupMockUpdate(mock) + if tc.setup != nil { tc.setup(mock) } + hcs := NewHealthChecker(fakeGCE, "/", defaultBackendSvc) gotSelfLink, err := hcs.SyncServicePort(tc.sp, tc.probe) @@ -986,12 +1153,20 @@ func TestSyncServicePort(t *testing.T) { } gotHC := computeHCs[0] - filterComputeHC(gotHC) + // Filter out fields that are hard to deal with in the mock and + // test cases. + filter := func(hc *compute.HealthCheck) { + hc.Description = "" + hc.SelfLink = "" + } + filter(gotHC) + 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)) } } + verify() // Check that resync should not have an effect and does not issue @@ -1012,3 +1187,6 @@ func TestSyncServicePort(t *testing.T) { }) } } + +// TODO(bowei): test regional delete +// TODO(bowei): test errors from GCE