From 93b646b6be6e760fedbc59594a79e37799580f9e Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Wed, 5 Aug 2020 03:59:31 -0700 Subject: [PATCH 1/2] Add Custom health check port to firewall rule --- pkg/firewalls/controller.go | 22 +++++++++++++++++- pkg/firewalls/controller_test.go | 38 ++++++++++++++++++++++++++++++++ pkg/firewalls/firewalls_test.go | 2 +- pkg/utils/utils.go | 5 +++++ 4 files changed, 65 insertions(+), 2 deletions(-) diff --git a/pkg/firewalls/controller.go b/pkg/firewalls/controller.go index 0a59bc0b9b..f18e439326 100644 --- a/pkg/firewalls/controller.go +++ b/pkg/firewalls/controller.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "reflect" + "strconv" "time" apiv1 "k8s.io/api/core/v1" @@ -181,8 +182,15 @@ func (fwc *FirewallController) sync(key string) error { } } + var additionalPorts []string + if flags.F.EnableBackendConfigHealthCheck { + hcPorts := fwc.getCustomHealthCheckPorts(gceSvcPorts) + additionalPorts = append(additionalPorts, hcPorts...) + } + additionalPorts = append(additionalPorts, negPorts...) + // Ensure firewall rule for the cluster and pass any NEG endpoint ports. - if err := fwc.firewallPool.Sync(nodeNames, negPorts, additionalRanges); err != nil { + if err := fwc.firewallPool.Sync(nodeNames, additionalPorts, additionalRanges); err != nil { if fwErr, ok := err.(*FirewallXPNError); ok { // XPN: Raise an event on each ingress for _, ing := range gceIngresses { @@ -217,3 +225,15 @@ func (fwc *FirewallController) ilbFirewallSrcRange(gceIngresses []*v1beta1.Ingre return "", ErrNoILBIngress } + +func (fwc *FirewallController) getCustomHealthCheckPorts(svcPorts []utils.ServicePort) []string { + var result []string + + for _, svcPort := range svcPorts { + if svcPort.BackendConfig != nil && svcPort.BackendConfig.Spec.HealthCheck != nil && svcPort.BackendConfig.Spec.HealthCheck.Port != nil { + result = append(result, strconv.FormatInt(*svcPort.BackendConfig.Spec.HealthCheck.Port, 10)) + } + } + + return result +} diff --git a/pkg/firewalls/controller_test.go b/pkg/firewalls/controller_test.go index ccfc1ee1f6..270988567c 100644 --- a/pkg/firewalls/controller_test.go +++ b/pkg/firewalls/controller_test.go @@ -21,10 +21,12 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" api_v1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/fake" + v1 "k8s.io/ingress-gce/pkg/apis/backendconfig/v1" backendconfigclient "k8s.io/ingress-gce/pkg/backendconfig/client/clientset/versioned/fake" test "k8s.io/ingress-gce/pkg/test" "k8s.io/ingress-gce/pkg/utils" @@ -103,3 +105,39 @@ func TestFirewallCreateDelete(t *testing.T) { t.Fatalf("cloud.GetFirewall(%v) = _, %v, want _, 404 error", ruleName, err) } } + +func TestGetCustomHealthCheckPorts(t *testing.T) { + t.Parallel() + + testCases := []struct { + desc string + svcPorts []utils.ServicePort + expect []string + }{ + { + desc: "One service port with custom port", + svcPorts: []utils.ServicePort{utils.ServicePort{BackendConfig: &v1.BackendConfig{Spec: v1.BackendConfigSpec{HealthCheck: &v1.HealthCheckConfig{Port: utils.NewInt64Pointer(8000)}}}}}, + expect: []string{"8000"}, + }, + { + desc: "Two service ports with custom port", + svcPorts: []utils.ServicePort{utils.ServicePort{BackendConfig: &v1.BackendConfig{Spec: v1.BackendConfigSpec{HealthCheck: &v1.HealthCheckConfig{Port: utils.NewInt64Pointer(8000)}}}}, + utils.ServicePort{BackendConfig: &v1.BackendConfig{Spec: v1.BackendConfigSpec{HealthCheck: &v1.HealthCheckConfig{Port: utils.NewInt64Pointer(9000)}}}}}, + expect: []string{"8000", "9000"}, + }, + { + desc: "No service ports", + expect: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + fwc := newFirewallController() + result := fwc.getCustomHealthCheckPorts(tc.svcPorts) + if diff := cmp.Diff(tc.expect, result); diff != "" { + t.Errorf("unexpected diff of ports (-want +got): \n%s", diff) + } + }) + } +} diff --git a/pkg/firewalls/firewalls_test.go b/pkg/firewalls/firewalls_test.go index 158a53af9b..c94b0b92a1 100644 --- a/pkg/firewalls/firewalls_test.go +++ b/pkg/firewalls/firewalls_test.go @@ -17,13 +17,13 @@ limitations under the License. package firewalls import ( - "k8s.io/kubernetes/pkg/util/slice" "strings" "testing" "google.golang.org/api/compute/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/ingress-gce/pkg/utils/namer" + "k8s.io/kubernetes/pkg/util/slice" "k8s.io/legacy-cloud-providers/gce" ) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 9e34b02b89..9cf3d81f74 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -604,3 +604,8 @@ func MakeL4ILBServiceDescription(svcName, ip string, version meta.Version) (stri func NewStringPointer(s string) *string { return &s } + +// NewInt64Pointer returns a pointer to the provided int64 literal +func NewInt64Pointer(i int64) *int64 { + return &i +} From 9043c91e10a5707998309b43c74f9bff995a3c8f Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Wed, 5 Aug 2020 18:08:11 -0700 Subject: [PATCH 2/2] Update e2e tests for custom health check firewall port fix --- cmd/e2e-test/healthcheck_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cmd/e2e-test/healthcheck_test.go b/cmd/e2e-test/healthcheck_test.go index 79aab77721..e7d782f87e 100644 --- a/cmd/e2e-test/healthcheck_test.go +++ b/cmd/e2e-test/healthcheck_test.go @@ -61,7 +61,7 @@ func TestHealthCheck(t *testing.T) { beConfig: fuzz.NewBackendConfigBuilder("", "backendconfig-1").Build(), want: &backendconfig.HealthCheckConfig{ RequestPath: pstring("/my-path"), - Port: pint64(8080), // Matches the targetPort + Port: pint64(9000), // Purposely does not match deployment, we verify it separately }, }, } { @@ -106,9 +106,15 @@ func TestHealthCheck(t *testing.T) { } t.Logf("Ingress created (%s/%s)", s.Namespace, ing.Name) - ing, err = e2e.WaitForIngress(s, ing, nil) - if err != nil { - t.Fatalf("error waiting for Ingress to stabilize: %v", err) + // If health check port does not match deployment, then the deployment will be unreachable + if tc.want.Port != nil { + options := &e2e.WaitForIngressOptions{ExpectUnreachable: true} + ing, _ = e2e.WaitForIngress(s, ing, options) + } else { + ing, err = e2e.WaitForIngress(s, ing, nil) + if err != nil { + t.Fatalf("error waiting for Ingress to stabilize: %v", err) + } } t.Logf("GCLB resources created (%s/%s)", s.Namespace, ing.Name)