Skip to content

Commit

Permalink
finished with first pass on testing
Browse files Browse the repository at this point in the history
  • Loading branch information
@bowei
bowei committed Feb 17, 2020
1 parent 8a05f9f commit 32c9079
Show file tree
Hide file tree
Showing 2 changed files with 217 additions and 39 deletions.
20 changes: 10 additions & 10 deletions pkg/healthchecks/healthchecks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}

Expand All @@ -315,7 +316,6 @@ func mergeUserSettings(old, newHC *HealthCheck) *HealthCheck {
hc.PortSpecification = ""
hc.Port = newHC.Port
}

return &hc
}

Expand Down
236 changes: 207 additions & 29 deletions pkg/healthchecks/healthchecks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand All @@ -720,19 +717,22 @@ 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
h.HttpHealthCheck = nil
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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -1012,3 +1187,6 @@ func TestSyncServicePort(t *testing.T) {
})
}
}

// TODO(bowei): test regional delete
// TODO(bowei): test errors from GCE

0 comments on commit 32c9079

Please sign in to comment.