Merge pull request #1092 from spencerhance/bc-hc-port

Add support for Port to BackendConfig HealthCheckConfig

cmd/e2e-test/healthcheck_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/retry"
@@ -45,7 +46,7 @@ func TestHealthCheck(t *testing.T) {
 		want     *backendconfig.HealthCheckConfig
 	}{
 		{
-			desc:     "override healthcheck",
+			desc:     "override healthcheck with IG",
 			beConfig: fuzz.NewBackendConfigBuilder("", "backendconfig-1").Build(),
 			want: &backendconfig.HealthCheckConfig{
 				CheckIntervalSec:   pint64(7),
@@ -55,6 +56,14 @@ func TestHealthCheck(t *testing.T) {
 				RequestPath:        pstring("/my-path"),
 			},
 		},
+		{
+			desc:     "override healthcheck and port with NEG",
+			beConfig: fuzz.NewBackendConfigBuilder("", "backendconfig-1").Build(),
+			want: &backendconfig.HealthCheckConfig{
+				RequestPath: pstring("/my-path"),
+				Port:        pint64(8080), // Matches the targetPort
+			},
+		},
 	} {
 		tc := tc // Capture tc as we are running this in parallel.
 		Framework.RunWithSandbox(tc.desc, t, func(t *testing.T, s *e2e.Sandbox) {
@@ -74,12 +83,20 @@ func TestHealthCheck(t *testing.T) {
 			}
 			t.Logf("BackendConfig created (%s/%s) ", s.Namespace, tc.beConfig.Name)
 
-			_, err := e2e.CreateEchoService(s, "service-1", backendConfigAnnotation)
+			svc, err := e2e.CreateEchoService(s, "service-1", backendConfigAnnotation)
 			if err != nil {
 				t.Fatalf("error creating echo service: %v", err)
 			}
 			t.Logf("Echo service created (%s/%s)", s.Namespace, "service-1")
 
+			// Update service for NEG
+			if tc.want.Port != nil {
+				svc.Annotations[annotations.NEGAnnotationKey] = `{"ingress":true}`
+				if _, err := Framework.Clientset.CoreV1().Services(s.Namespace).Update(ctx, svc, v1.UpdateOptions{}); err != nil {
+					t.Fatalf("error updating port on svc: %v", err)
+				}
+			}
+
 			ing := fuzz.NewIngressBuilder(s.Namespace, "ingress-1", "").
 				DefaultBackend("service-1", intstr.FromInt(80)).
 				Build()

pkg/apis/backendconfig/v1/types.go

@@ -154,7 +154,10 @@ type HealthCheckConfig struct {
 	// Type is a health check parameter. See
 	// https://cloud.google.com/compute/docs/reference/rest/v1/healthChecks.
 	Type *string `json:"type,omitempty"`
-	Port *int64  `json:"port,omitempty"`
+	// Port is a health check parameter. See
+	// https://cloud.google.com/compute/docs/reference/rest/v1/healthChecks.
+	// If Port is used, the controller updates portSpecification as well
+	Port *int64 `json:"port,omitempty"`
 	// RequestPath is a health check parameter. See
 	// https://cloud.google.com/compute/docs/reference/rest/v1/healthChecks.
 	RequestPath *string `json:"requestPath,omitempty"`

pkg/healthchecks/healthcheck.go

@@ -183,8 +183,9 @@ func (hc *HealthCheck) ToAlphaComputeHealthCheck() *computealpha.HealthCheck {
 }
 
 func (hc *HealthCheck) merge() {
-	// Cannot specify both portSpecification and port field.
-	if hc.PortSpecification != "" {
+	// Cannot specify both portSpecification and port field unless fixed port is specified.
+	// This can happen if the user overrides the port using backendconfig
+	if hc.PortSpecification != "" && hc.PortSpecification != "USE_FIXED_PORT" {
 		hc.Port = 0
 	}
 
@@ -239,7 +240,9 @@ func (hc *HealthCheck) updateFromBackendConfig(c *backendconfigv1.HealthCheckCon
 		hc.RequestPath = *c.RequestPath
 	}
 	if c.Port != nil {
-		klog.Warningf("Setting Port is not supported (healthcheck %q, backendconfig = %+v)", hc.Name, c)
+		hc.Port = *c.Port
+		// This override is necessary regardless of type
+		hc.PortSpecification = "USE_FIXED_PORT"
 	}
 }
 
@@ -287,6 +290,10 @@ func calculateDiff(old, new *HealthCheck, c *backendconfigv1.HealthCheckConfig)
 	if c.RequestPath != nil && old.RequestPath != new.RequestPath {
 		changes.add("RequestPath", old.RequestPath, new.RequestPath)
 	}
+	if c.Port != nil && old.Port != new.Port {
+		changes.add("Port", strconv.FormatInt(old.Port, 10), strconv.FormatInt(new.Port, 10))
+	}
+
 	// TODO(bowei): Host seems to be missing.
 
 	return &changes

pkg/healthchecks/healthchecks.go

@@ -402,6 +402,7 @@ func (h *HealthChecks) pathFromSvcPort(sp utils.ServicePort) string {
 	return h.path
 }
 
+// formatBackendConfigHC returns a human readable string version of the HealthCheckConfig
 func formatBackendConfigHC(b *backendconfigv1.HealthCheckConfig) string {
 	var ret []string
 
@@ -413,6 +414,7 @@ func formatBackendConfigHC(b *backendconfigv1.HealthCheckConfig) string {
 		{k: "healthyThreshold", v: b.HealthyThreshold},
 		{k: "unhealthyThreshold", v: b.UnhealthyThreshold},
 		{k: "timeoutSec", v: b.TimeoutSec},
+		{k: "port", v: b.Port},
 	} {
 		if e.v != nil {
 			ret = append(ret, fmt.Sprintf("%s=%d", e.k, *e.v))

pkg/healthchecks/healthchecks_test.go

@@ -78,6 +78,7 @@ func init() {
 						TimeoutSec:         &num,
 						HealthyThreshold:   &num,
 						UnhealthyThreshold: &num,
+						Port:               &num,
 					},
 				} {
 					sp := &utils.ServicePort{
@@ -675,6 +676,16 @@ func TestCalculateDiff(t *testing.T) {
 		hasDiff: true,
 	})
 
+	newHC = DefaultHealthCheck(500, annotations.ProtocolHTTP)
+	newHC.Port = 500
+	cases = append(cases, tc{
+		desc:    "Backendconfig Port",
+		old:     DefaultHealthCheck(8080, annotations.ProtocolHTTP),
+		new:     newHC,
+		c:       &backendconfigv1.HealthCheckConfig{Port: i64(500)},
+		hasDiff: true,
+	})
+
 	for _, tc := range cases {
 		t.Run(tc.desc, func(t *testing.T) {
 			diffs := calculateDiff(tc.old, tc.new, tc.c)
@@ -895,8 +906,26 @@ func TestSyncServicePort(t *testing.T) {
 	chc.HealthyThreshold = 1234
 	chc.UnhealthyThreshold = 1234
 	chc.TimeoutSec = 1234
+	chc.HttpHealthCheck.Port = 1234
+	// PortSpecification is set by the controller
+	chc.HttpHealthCheck.PortSpecification = "USE_FIXED_PORT"
 	cases = append(cases, &tc{desc: "create backendconfig all", sp: testSPs["HTTP-80-reg-bcall"], wantComputeHC: chc})
 
+	i64 := func(i int64) *int64 { return &i }
+
+	// BackendConfig port
+	chc = fixture.hc()
+	chc.HttpHealthCheck.Port = 1234
+	// PortSpecification is set by the controller
+	chc.HttpHealthCheck.PortSpecification = "USE_FIXED_PORT"
+	sp := utils.ServicePort{
+		NodePort:      80,
+		Protocol:      annotations.ProtocolHTTP,
+		BackendNamer:  testNamer,
+		BackendConfig: &backendconfigv1.BackendConfig{Spec: backendconfigv1.BackendConfigSpec{HealthCheck: &backendconfigv1.HealthCheckConfig{Port: i64(1234)}}},
+	}
+	cases = append(cases, &tc{desc: "create backendconfig port", sp: &sp, wantComputeHC: chc})
+
 	// BackendConfig neg
 	chc = fixture.neg()
 	chc.HttpHealthCheck.RequestPath = "/foo"
@@ -1085,6 +1114,8 @@ func TestSyncServicePort(t *testing.T) {
 	wantCHC.HealthyThreshold = 1234
 	wantCHC.UnhealthyThreshold = 1234
 	wantCHC.TimeoutSec = 1234
+	wantCHC.HttpHealthCheck.Port = 1234
+	wantCHC.HttpHealthCheck.PortSpecification = "USE_FIXED_PORT"
 	cases = append(cases, &tc{
 		desc:          "update preserve backendconfig all",
 		setup:         fixture.setupExistingHCFunc(chc),