NE-518 GCP: Implement GCP Internal LB Global Access option

go.mod

@@ -18,7 +18,7 @@ require (
 	github.com/imdario/mergo v0.3.10 // indirect
 	github.com/kevinburke/go-bindata v3.11.0+incompatible
 	github.com/onsi/ginkgo v1.14.0 // indirect
-	github.com/openshift/api v0.0.0-20210208192252-670ac3fc997c
+	github.com/openshift/api v0.0.0-20210216211028-bb81baaf35cd
 	github.com/openshift/library-go v0.0.0-20200423123937-d1360419413d
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.7.1

go.sum

@@ -473,8 +473,8 @@ github.com/opencontainers/image-spec v1.0.1/go.mod h1:BtxoFyWECRxE4U/7sNtV5W15zM
 github.com/opencontainers/runc v0.0.0-20191031171055-b133feaeeb2e/go.mod h1:qT5XzbpPznkRYVz/mWwUaVBUv2rmF59PVA73FjuZG0U=
 github.com/openshift/api v0.0.0-20200326152221-912866ddb162/go.mod h1:RKMJ5CBnljLfnej+BJ/xnOWc3kZDvJUaIAEq2oKSPtE=
 github.com/openshift/api v0.0.0-20200326160804-ecb9283fe820/go.mod h1:RKMJ5CBnljLfnej+BJ/xnOWc3kZDvJUaIAEq2oKSPtE=
-github.com/openshift/api v0.0.0-20210208192252-670ac3fc997c h1:l+KbFcYYFsp3sMIGOZxg0pApxRE1ARkS7vTLTaRiTIM=
-github.com/openshift/api v0.0.0-20210208192252-670ac3fc997c/go.mod h1:aqU5Cq+kqKKPbDMqxo9FojgDeSpNJI7iuskjXjtojDg=
+github.com/openshift/api v0.0.0-20210216211028-bb81baaf35cd h1:SzOmbKIulFFEBPoBosc4G4QBVN4QwynEvb3RU9tOdJA=
+github.com/openshift/api v0.0.0-20210216211028-bb81baaf35cd/go.mod h1:aqU5Cq+kqKKPbDMqxo9FojgDeSpNJI7iuskjXjtojDg=
 github.com/openshift/build-machinery-go v0.0.0-20200211121458-5e3d6e570160/go.mod h1:1CkcsT3aVebzRBzVTSbiKSkJMsC/CASqxesfqEMfJEc=
 github.com/openshift/build-machinery-go v0.0.0-20200917070002-f171684f77ab/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=
 github.com/openshift/client-go v0.0.0-20200326155132-2a6cd50aedd0/go.mod h1:uUQ4LClRO+fg5MF/P6QxjMCb1C9f7Oh4RKepftDnEJE=

manifests/00-custom-resource-definition.yaml

@@ -137,6 +137,29 @@ spec:
                             required:
                             - type
                             type: object
+                          gcp:
+                            description: "gcp provides configuration settings that
+                              are specific to GCP load balancers. \n If empty, defaults
+                              will be applied. See specific gcp fields for details
+                              about their defaults."
+                            properties:
+                              clientAccess:
+                                description: "clientAccess describes how client access
+                                  is restricted for internal load balancers. \n Valid
+                                  values are: * \"Global\": Specifying an internal
+                                  load balancer with Global client access   allows
+                                  clients from any region within the VPC to communicate
+                                  with the load   balancer. \n     https://cloud.google.com/kubernetes-engine/docs/how-to/internal-load-balancing#global_access
+                                  \n * \"Local\": Specifying an internal load balancer
+                                  with Local client access   means only clients within
+                                  the same region (and VPC) as the GCP load balancer
+                                  \  can communicate with the load balancer. Note
+                                  that this is the default behavior. \n     https://cloud.google.com/load-balancing/docs/internal#client_access"
+                                enum:
+                                - Global
+                                - Local
+                                type: string
+                            type: object
                           type:
                             description: type is the underlying infrastructure provider
                               for the load balancer. Allowed values are "AWS", "Azure",
@@ -947,6 +970,29 @@ spec:
                             required:
                             - type
                             type: object
+                          gcp:
+                            description: "gcp provides configuration settings that
+                              are specific to GCP load balancers. \n If empty, defaults
+                              will be applied. See specific gcp fields for details
+                              about their defaults."
+                            properties:
+                              clientAccess:
+                                description: "clientAccess describes how client access
+                                  is restricted for internal load balancers. \n Valid
+                                  values are: * \"Global\": Specifying an internal
+                                  load balancer with Global client access   allows
+                                  clients from any region within the VPC to communicate
+                                  with the load   balancer. \n     https://cloud.google.com/kubernetes-engine/docs/how-to/internal-load-balancing#global_access
+                                  \n * \"Local\": Specifying an internal load balancer
+                                  with Local client access   means only clients within
+                                  the same region (and VPC) as the GCP load balancer
+                                  \  can communicate with the load balancer. Note
+                                  that this is the default behavior. \n     https://cloud.google.com/load-balancing/docs/internal#client_access"
+                                enum:
+                                - Global
+                                - Local
+                                type: string
+                            type: object
                           type:
                             description: type is the underlying infrastructure provider
                               for the load balancer. Allowed values are "AWS", "Azure",

pkg/operator/controller/ingress/controller.go

@@ -373,6 +373,16 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig
 		ic.Status.EndpointPublishingStrategy = effectiveStrategy
 		return true
 	}
+
+	// Detect changes to GCP LB provider parameters, which is something we can safely roll out.
+	statusLB := ic.Status.EndpointPublishingStrategy.LoadBalancer
+	specLB := effectiveStrategy.LoadBalancer
+	if specLB != nil && statusLB != nil && specLB.ProviderParameters != nil && statusLB.ProviderParameters != nil &&
+		specLB.ProviderParameters.GCP != statusLB.ProviderParameters.GCP {
+		ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP = effectiveStrategy.LoadBalancer.ProviderParameters.GCP
+		return true
+	}
+
 	return false
 }
 

pkg/operator/controller/ingress/load_balancer_service.go

@@ -3,6 +3,7 @@ package ingress
 import (
 	"context"
 	"fmt"
+	"strconv"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
@@ -85,6 +86,10 @@ const (
 	// load balancer.
 	gcpLBTypeAnnotation = "cloud.google.com/load-balancer-type"
 
+	// GCPGlobalAccessAnnotation is the annotation used on an internal load balancer service
+	// to enable the GCP Global Access feature.
+	GCPGlobalAccessAnnotation = "networking.gke.io/internal-load-balancer-allow-global-access"
+
 	// openstackInternalLBAnnotation is the annotation used on a service to specify an
 	// OpenStack load balancer as being internal.
 	openstackInternalLBAnnotation = "service.beta.kubernetes.io/openstack-internal-load-balancer"
@@ -226,6 +231,16 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 			for name, value := range annotation {
 				service.Annotations[name] = value
 			}
+
+			// Set the GCP Global Access annotation for internal load balancers on GCP only
+			if platform.Type == configv1.GCPPlatformType {
+				if ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil &&
+					ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.Type == operatorv1.GCPLoadBalancerProvider &&
+					ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP != nil {
+					globalAccessEnabled := ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP.ClientAccess == operatorv1.GCPGlobalAccess
+					service.Annotations[GCPGlobalAccessAnnotation] = strconv.FormatBool(globalAccessEnabled)
+				}
+			}
 		}
 		switch platform.Type {
 		case configv1.AWSPlatformType:
@@ -353,20 +368,30 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service,
 // matches the expected and if not returns an updated one.
 func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
 	updated := current.DeepCopy()
+	changed := false
 
-	// Preserve everything but the AWS LB health check interval annotation
+	// Preserve everything but the AWS LB health check interval annotation &
+	// GCP Global Access internal Load Balancer annotation.
 	// (see <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>).
 	// Updating annotations and spec fields cannot be done unless the
 	// previous release blocks upgrades when the user has modified those
 	// fields (see <https://bugzilla.redhat.com/show_bug.cgi?id=1905490>).
 	if updated.Annotations == nil {
 		updated.Annotations = map[string]string{}
 	}
-	if current.Annotations[awsLBHealthCheckIntervalAnnotation] == expected.Annotations[awsLBHealthCheckIntervalAnnotation] {
-		return false, nil
+	if current.Annotations[awsLBHealthCheckIntervalAnnotation] != expected.Annotations[awsLBHealthCheckIntervalAnnotation] {
+		updated.Annotations[awsLBHealthCheckIntervalAnnotation] = expected.Annotations[awsLBHealthCheckIntervalAnnotation]
+		changed = true
 	}
 
-	updated.Annotations[awsLBHealthCheckIntervalAnnotation] = expected.Annotations[awsLBHealthCheckIntervalAnnotation]
+	if current.Annotations[GCPGlobalAccessAnnotation] != expected.Annotations[GCPGlobalAccessAnnotation] {
+		updated.Annotations[GCPGlobalAccessAnnotation] = expected.Annotations[GCPGlobalAccessAnnotation]
+		changed = true
+	}
+
+	if !changed {
+		return false, nil
+	}
 
 	return true, updated
 }

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -147,6 +147,36 @@ func TestDesiredLoadBalancerService(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description:  "internal load balancer for gcp platform with global ClientAccess",
+			platform:     configv1.GCPPlatformType,
+			strategyType: operatorv1.LoadBalancerServiceStrategyType,
+			lbStrategy: operatorv1.LoadBalancerStrategy{
+				Scope: operatorv1.InternalLoadBalancer,
+				ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
+					Type: operatorv1.GCPLoadBalancerProvider,
+					GCP: &operatorv1.GCPLoadBalancerParameters{
+						ClientAccess: operatorv1.GCPGlobalAccess,
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			description:  "internal load balancer for gcp platform with local ClientAccess",
+			platform:     configv1.GCPPlatformType,
+			strategyType: operatorv1.LoadBalancerServiceStrategyType,
+			lbStrategy: operatorv1.LoadBalancerStrategy{
+				Scope: operatorv1.InternalLoadBalancer,
+				ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
+					Type: operatorv1.GCPLoadBalancerProvider,
+					GCP: &operatorv1.GCPLoadBalancerParameters{
+						ClientAccess: operatorv1.GCPLocalAccess,
+					},
+				},
+			},
+			expect: true,
+		},
 		{
 			description:  "external load balancer for openstack platform",
 			platform:     configv1.OpenStackPlatformType,

test/e2e/operator_test.go

@@ -631,6 +631,96 @@ func TestInternalLoadBalancer(t *testing.T) {
 	}
 }
 
+// TestInternalLoadBalancerGlobalAccessGCP creates an ingresscontroller
+// with internal load balancer on GCP with the GCP Global Access provider
+// parameter set to both "Global" and "local" to verify that the
+// Load Balancer service is created properly.
+func TestInternalLoadBalancerGlobalAccessGCP(t *testing.T) {
+	platform := infraConfig.Status.Platform
+
+	supportedPlatforms := map[configv1.PlatformType]struct{}{
+		configv1.GCPPlatformType: {},
+	}
+	if _, supported := supportedPlatforms[platform]; !supported {
+		t.Skip(fmt.Sprintf("test skipped on platform %q", platform))
+	}
+
+	name := types.NamespacedName{Namespace: operatorNamespace, Name: "test-gcp"}
+	ic := newLoadBalancerController(name, name.Name+"."+dnsConfig.Spec.BaseDomain)
+	ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{
+		Scope: operatorv1.InternalLoadBalancer,
+		ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
+			Type: operatorv1.GCPLoadBalancerProvider,
+			GCP: &operatorv1.GCPLoadBalancerParameters{
+				ClientAccess: operatorv1.GCPGlobalAccess,
+			},
+		},
+	}
+	if err := kclient.Create(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to create ingresscontroller: %v", err)
+	}
+	defer assertIngressControllerDeleted(t, kclient, ic)
+
+	// Wait for the load balancer and DNS to be ready.
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, nondefaultAvailableConditions...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	lbService := &corev1.Service{}
+	if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), lbService); err != nil {
+		t.Fatalf("failed to get LoadBalancer service: %v", err)
+	}
+
+	// Verify load balancer has desired global access annotation
+	annotation := ingresscontroller.GCPGlobalAccessAnnotation
+	// A ClientAccess value of "Global" results in the Global Access Annotation
+	// being set to "true".
+	expected := "true"
+
+	if actual, ok := lbService.Annotations[annotation]; !ok {
+		t.Fatalf("load balancer has no %q annotation: %v", annotation, lbService.Annotations)
+	} else if actual != expected {
+		t.Fatalf("expected %s=%s, found %s=%s", annotation, expected, annotation, actual)
+	}
+
+	// Update ingress controller to use "Local" Global Access
+	if err := kclient.Get(context.TODO(), name, ic); err != nil {
+		t.Fatalf("failed to get ingresscontroller %s: %v", name, err)
+	}
+
+	ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP.ClientAccess = operatorv1.GCPLocalAccess
+
+	if err := kclient.Update(context.TODO(), ic); err != nil {
+		t.Errorf("failed to update ingresscontroller %s: %v", name, err)
+	}
+
+	// A ClientAccess value of "Local" results in the Global Access Annotation
+	// being set to "false".
+	expected = "false"
+
+	// Verify load balancer has desired global access annotation.
+	// Use a polling loop since the operator might not switch out the annotation
+	// immediately.
+	err := wait.PollImmediate(1*time.Second, 3*time.Minute, func() (bool, error) {
+		if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), lbService); err != nil {
+			t.Logf("failed to get LoadBalancer service: %v", err)
+			return false, nil
+		}
+		if actual, ok := lbService.Annotations[annotation]; !ok {
+			t.Logf("load balancer has no %q annotation: %v", annotation, lbService.Annotations)
+			return false, nil
+		} else if actual != expected {
+			t.Logf("expected %s=%s, found %s=%s", annotation, expected, annotation, actual)
+			return false, nil
+		}
+		return true, nil
+	})
+
+	if err != nil {
+		t.Errorf("failed to observe expected annotations on load balancer service %s: %v", controller.LoadBalancerServiceName(ic), err)
+	}
+}
+
 // TestNodePortServiceEndpointPublishingStrategy creates an ingresscontroller
 // with the "NodePortService" endpoint publishing strategy type and verifies
 // that the operator creates a router and that the router becomes available.