From 7038e7074e45f1f816c7fd936118fcbfc7cd6c70 Mon Sep 17 00:00:00 2001
From: Rohit Ramkumar <rramkumar@google.com>
Date: Wed, 30 May 2018 09:26:59 -0700
Subject: [PATCH] Fix bug with ensuring BackendService settings + health checks

---
 pkg/backends/backends.go      | 43 +++++++++++++++++++++--------------
 pkg/backends/backends_test.go | 21 ++++++++++++++---
 2 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/pkg/backends/backends.go b/pkg/backends/backends.go
index 88214927d0..4d4e6f6965 100644
--- a/pkg/backends/backends.go
+++ b/pkg/backends/backends.go
@@ -239,16 +239,17 @@ func (b *Backends) Get(name string, isAlpha bool) (*BackendService, error) {
 	return &BackendService{Ga: beGa, Alpha: beAlpha}, nil
 }
 
-func (b *Backends) ensureHealthCheck(sp utils.ServicePort) (string, error) {
+func (b *Backends) ensureHealthCheck(sp utils.ServicePort, hasLegacyHC bool) (string, error) {
 	hc := b.healthChecker.New(sp)
-	existingLegacyHC, err := b.healthChecker.GetLegacy(sp.NodePort)
-	if err != nil && !utils.IsNotFoundError(err) {
-		return "", err
-	}
-
-	if existingLegacyHC != nil {
-		glog.V(4).Infof("Applying settings of existing health check to newer health check on port %+v", sp)
-		applyLegacyHCToHC(existingLegacyHC, hc)
+	if hasLegacyHC {
+		existingLegacyHC, err := b.healthChecker.GetLegacy(sp.NodePort)
+		if err != nil && !utils.IsNotFoundError(err) {
+			return "", err
+		}
+		if existingLegacyHC != nil {
+			glog.V(4).Infof("Applying settings of existing health check to newer health check on port %+v", sp)
+			applyLegacyHCToHC(existingLegacyHC, hc)
+		}
 	} else if b.prober != nil {
 		probe, err := b.prober.GetProbe(sp)
 		if err != nil {
@@ -321,14 +322,24 @@ func (b *Backends) ensureBackendService(sp utils.ServicePort, igLinks []string)
 		b.snapshotter.Add(beName, be)
 	}()
 
-	// Ensure health check for backend service exists
-	hcLink, err := b.ensureHealthCheck(sp)
+	be, _ = b.Get(beName, sp.IsAlpha())
+	hasLegacyHC := false
+	if be != nil {
+		// If the backend already exists, find out if it is using a legacy health check.
+		existingHCLink := be.GetHealthCheckLink()
+		if strings.Contains(existingHCLink, "/httpHealthChecks/") {
+			hasLegacyHC = true
+		}
+	}
+
+	// Ensure health check for backend service exists. Note that hasLegacyHC
+	// will dictate whether we search for an existing legacy health check.
+	hcLink, err := b.ensureHealthCheck(sp, hasLegacyHC)
 	if err != nil {
 		return err
 	}
 
 	// Verify existance of a backend service for the proper port, but do not specify any backends/igs
-	be, _ = b.Get(beName, sp.IsAlpha())
 	if be == nil {
 		namedPort := &compute.NamedPort{
 			Name: b.namer.NamedPort(sp.NodePort),
@@ -343,8 +354,8 @@ func (b *Backends) ensureBackendService(sp utils.ServicePort, igLinks []string)
 	}
 
 	needUpdate := be.ensureProtocol(sp)
-	needUpdate = needUpdate || be.ensureHealthCheckLink(hcLink)
-	needUpdate = needUpdate || be.ensureDescription(sp.Description())
+	needUpdate = be.ensureHealthCheckLink(hcLink) || needUpdate
+	needUpdate = be.ensureDescription(sp.Description()) || needUpdate
 
 	if needUpdate {
 		if err = b.update(be); err != nil {
@@ -352,10 +363,8 @@ func (b *Backends) ensureBackendService(sp utils.ServicePort, igLinks []string)
 		}
 	}
 
-	existingHCLink := be.GetHealthCheckLink()
-
 	// If previous health check was legacy type, we need to delete it.
-	if existingHCLink != hcLink && strings.Contains(existingHCLink, "/httpHealthChecks/") {
+	if hasLegacyHC {
 		if err = b.healthChecker.DeleteLegacy(sp.NodePort); err != nil {
 			glog.Warning("Failed to delete legacy HttpHealthCheck %v; Will not try again, err: %v", beName, err)
 		}
diff --git a/pkg/backends/backends_test.go b/pkg/backends/backends_test.go
index bd388b1b61..f9a8be2f9c 100644
--- a/pkg/backends/backends_test.go
+++ b/pkg/backends/backends_test.go
@@ -166,23 +166,38 @@ func TestHealthCheckMigration(t *testing.T) {
 	pool, hcp := newTestJig(f, fakeIGs, false)
 
 	p := utils.ServicePort{NodePort: 7000, Protocol: annotations.ProtocolHTTP}
+	beName := p.BackendName(defaultNamer)
 
 	// Create a legacy health check and insert it into the HC provider.
 	legacyHC := &compute.HttpHealthCheck{
-		Name:               p.BackendName(defaultNamer),
+		Name:               beName,
 		RequestPath:        "/my-healthz-path",
 		Host:               "k8s.io",
 		Description:        "My custom HC",
 		UnhealthyThreshold: 30,
 		CheckIntervalSec:   40,
 	}
-	hcp.CreateHttpHealthCheck(legacyHC)
+	if err := hcp.CreateHttpHealthCheck(legacyHC); err != nil {
+		t.Fatalf("unexpected error creating http health check %v", err)
+	}
+
+	// Verify legacy check exists
+	legacyHC, err := hcp.GetHttpHealthCheck(beName)
+	if err != nil {
+		t.Fatalf("unexpected error getting http health check %v", err)
+	}
+
+	// Create backend service with expected name and link to legacy health check
+	f.CreateGlobalBackendService(&compute.BackendService{
+		Name:         beName,
+		HealthChecks: []string{legacyHC.SelfLink},
+	})
 
 	// Add the service port to the backend pool
 	pool.Ensure([]utils.ServicePort{p}, nil)
 
 	// Assert the proper health check was created
-	hc, _ := pool.healthChecker.Get(p.BackendName(defaultNamer), p.IsAlpha())
+	hc, _ := pool.healthChecker.Get(beName, p.IsAlpha())
 	if hc == nil || hc.Protocol() != p.Protocol {
 		t.Fatalf("Expected %s health check, received %v: ", p.Protocol, hc)
 	}