NE-956: Configurable LB Source Ranges - Functional Implementation

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/go-logr/zapr v1.2.3
 	github.com/google/go-cmp v0.5.8
 	github.com/kevinburke/go-bindata v3.11.0+incompatible
-	github.com/openshift/api v0.0.0-20220907152121-48d78630feb3
+	github.com/openshift/api v0.0.0-20221003191342-e2c96618934f
 	github.com/openshift/library-go v0.0.0-20220920133651-093893cf326b
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.12.2

go.sum

@@ -539,8 +539,8 @@ github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7J
 github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo=
 github.com/onsi/gomega v1.10.5/go.mod h1:gza4q3jKQJijlu05nKWRCW/GavJumGt8aNRxWg7mt48=
 github.com/onsi/gomega v1.20.1 h1:PA/3qinGoukvymdIDV8pii6tiZgC8kbmJO6Z5+b002Q=
-github.com/openshift/api v0.0.0-20220907152121-48d78630feb3 h1:7y9VOklNufIkJ8pQprlrMq908m16iO11MnP4Ut1rS/E=
-github.com/openshift/api v0.0.0-20220907152121-48d78630feb3/go.mod h1:9JWn+H7X8wEPPc9D63krigXl8r3F1Mt6/lC98brUyhQ=
+github.com/openshift/api v0.0.0-20221003191342-e2c96618934f h1:H8jFwVgDpC0Y23cq+8N6zelYmOkmwm4bdUttjKVDxN8=
+github.com/openshift/api v0.0.0-20221003191342-e2c96618934f/go.mod h1:JRz+ZvTqu9u7t6suhhPTacbFl5K65Y6rJbNM7HjWA3g=
 github.com/openshift/library-go v0.0.0-20220920133651-093893cf326b h1:LWwB7uN91G/JsMnZFd0+q6ZzAXlB4/oUOfpZWA585gw=
 github.com/openshift/library-go v0.0.0-20220920133651-093893cf326b/go.mod h1:KPBAXGaq7pPmA+1wUVtKr5Axg3R68IomWDkzaOxIhxM=
 github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k=

manifests/00-custom-resource-definition.yaml

@@ -218,6 +218,27 @@ spec:
                     description: loadBalancer holds parameters for the load balancer.
                       Present only if type is LoadBalancerService.
                     properties:
+                      allowedSourceRanges:
+                        description: "allowedSourceRanges specifies an allowlist of
+                          IP address ranges to which access to the load balancer should
+                          be restricted.  Each range must be specified using CIDR
+                          notation (e.g. \"10.0.0.0/8\" or \"fd00::/8\"). If no range
+                          is specified, \"0.0.0.0/0\" for IPv4 and \"::/0\" for IPv6
+                          are used by default, which allows all source addresses.
+                          \n To facilitate migration from earlier versions of OpenShift
+                          that did not have the allowedSourceRanges field, you may
+                          set the service.beta.kubernetes.io/load-balancer-source-ranges
+                          annotation on the \"router-<ingresscontroller name>\" service
+                          in the \"openshift-ingress\" namespace, and this annotation
+                          will take effect if allowedSourceRanges is empty on OpenShift
+                          4.12."
+                        items:
+                          description: CIDR is an IP address range in CIDR notation
+                            (for example, "10.0.0.0/8" or "fd00::/8").
+                          pattern: (^(([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/([0-9]|[12][0-9]|3[0-2])$)|(^s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:)))(%.+)?s*(\/(12[0-8]|1[0-1][0-9]|[1-9][0-9]|[0-9]))$)
+                          type: string
+                        nullable: true
+                        type: array
                       dnsManagementPolicy:
                         default: Managed
                         description: 'dnsManagementPolicy indicates if the lifecycle
@@ -1505,6 +1526,27 @@ spec:
                     description: loadBalancer holds parameters for the load balancer.
                       Present only if type is LoadBalancerService.
                     properties:
+                      allowedSourceRanges:
+                        description: "allowedSourceRanges specifies an allowlist of
+                          IP address ranges to which access to the load balancer should
+                          be restricted.  Each range must be specified using CIDR
+                          notation (e.g. \"10.0.0.0/8\" or \"fd00::/8\"). If no range
+                          is specified, \"0.0.0.0/0\" for IPv4 and \"::/0\" for IPv6
+                          are used by default, which allows all source addresses.
+                          \n To facilitate migration from earlier versions of OpenShift
+                          that did not have the allowedSourceRanges field, you may
+                          set the service.beta.kubernetes.io/load-balancer-source-ranges
+                          annotation on the \"router-<ingresscontroller name>\" service
+                          in the \"openshift-ingress\" namespace, and this annotation
+                          will take effect if allowedSourceRanges is empty on OpenShift
+                          4.12."
+                        items:
+                          description: CIDR is an IP address range in CIDR notation
+                            (for example, "10.0.0.0/8" or "fd00::/8").
+                          pattern: (^(([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/([0-9]|[12][0-9]|3[0-2])$)|(^s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:)))(%.+)?s*(\/(12[0-8]|1[0-1][0-9]|[1-9][0-9]|[0-9]))$)
+                          type: string
+                        nullable: true
+                        type: array
                       dnsManagementPolicy:
                         default: Managed
                         description: 'dnsManagementPolicy indicates if the lifecycle

pkg/operator/controller/ingress/controller.go

@@ -56,6 +56,7 @@ const (
 	IngressControllerDeploymentRollingOutConditionType           = "DeploymentRollingOut"
 	IngressControllerLoadBalancerProgressingConditionType        = "LoadBalancerProgressing"
 	IngressControllerCanaryCheckSuccessConditionType             = "CanaryChecksSucceeding"
+	IngressControllerEvaluationConditionsDetectedConditionType   = "EvaluationConditionsDetected"
 
 	routerDefaultHeaderBufferSize           = 32768
 	routerDefaultHeaderBufferMaxRewriteSize = 8192

pkg/operator/controller/ingress/load_balancer_service.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"strconv"
 	"strings"
 	"time"
@@ -20,6 +21,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -436,6 +438,17 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 		}
 	}
 
+	if ci.Spec.EndpointPublishingStrategy != nil {
+		lb := ci.Spec.EndpointPublishingStrategy.LoadBalancer
+		if lb != nil && len(lb.AllowedSourceRanges) > 0 {
+			cidrs := make([]string, len(lb.AllowedSourceRanges))
+			for i, cidr := range lb.AllowedSourceRanges {
+				cidrs[i] = string(cidr)
+			}
+			service.Spec.LoadBalancerSourceRanges = cidrs
+		}
+	}
+
 	service.SetOwnerReferences([]metav1.OwnerReference{deploymentRef})
 	return true, service, nil
 }
@@ -599,7 +612,30 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
 	// avoid problems, make sure the previous release blocks upgrades when
 	// the user has modified an annotation or spec field that the new
 	// release manages.
-	return loadBalancerServiceAnnotationsChanged(current, expected, managedLoadBalancerServiceAnnotations)
+	changed, updated := loadBalancerServiceAnnotationsChanged(current, expected, managedLoadBalancerServiceAnnotations)
+
+	// If spec.loadBalancerSourceRanges is nonempty on the service, that
+	// means that allowedSourceRanges is nonempty on the ingresscontroller,
+	// which means we can clear the annotation if it's set and overwrite the
+	// value in the current service.
+	if len(expected.Spec.LoadBalancerSourceRanges) != 0 {
+		if _, ok := current.Annotations[corev1.AnnotationLoadBalancerSourceRangesKey]; ok {
+			if !changed {
+				changed = true
+				updated = current.DeepCopy()
+			}
+			delete(updated.Annotations, corev1.AnnotationLoadBalancerSourceRangesKey)
+		}
+		if !reflect.DeepEqual(current.Spec.LoadBalancerSourceRanges, expected.Spec.LoadBalancerSourceRanges) {
+			if !changed {
+				changed = true
+				updated = current.DeepCopy()
+			}
+			updated.Spec.LoadBalancerSourceRanges = expected.Spec.LoadBalancerSourceRanges
+		}
+	}
+
+	return changed, updated
 }
 
 // loadBalancerServiceAnnotationsChanged checks if the annotations on the expected Service
@@ -682,3 +718,78 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme
 
 	return nil
 }
+
+// loadBalancerServiceIsProgressing returns an error value indicating if the
+// load balancer service is in progressing status.
+func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus) error {
+	var errs []error
+	wantScope := ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope
+	haveScope := operatorv1.ExternalLoadBalancer
+	if IsServiceInternal(service) {
+		haveScope = operatorv1.InternalLoadBalancer
+	}
+	if wantScope != haveScope {
+		err := fmt.Errorf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
+		switch platform.Type {
+		case configv1.AWSPlatformType, configv1.IBMCloudPlatformType:
+			err = fmt.Errorf("%[1]s  To effectuate this change, you must delete the service: `oc -n %[2]s delete svc/%[3]s`; the service load-balancer will then be deprovisioned and a new one created.  This will most likely cause the new load-balancer to have a different host name and IP address from the old one's.  Alternatively, you can revert the change to the IngressController: `oc -n openshift-ingress-operator patch ingresscontrollers/%[4]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[5]s\"}}}}'", err.Error(), service.Namespace, service.Name, ic.Name, haveScope)
+		}
+		errs = append(errs, err)
+	}
+
+	errs = append(errs, loadBalancerSourceRangesAnnotationSet(service))
+	errs = append(errs, loadBalancerSourceRangesMatch(ic, service))
+
+	return kerrors.NewAggregate(errs)
+}
+
+// loadBalancerServiceEvaluationConditionsDetected returns an error value indicating if the
+// load balancer service is in EvaluationConditionsDetected status.
+func loadBalancerServiceEvaluationConditionsDetected(ic *operatorv1.IngressController, service *corev1.Service) error {
+	var errs []error
+	errs = append(errs, loadBalancerSourceRangesAnnotationSet(service))
+	errs = append(errs, loadBalancerSourceRangesMatch(ic, service))
+
+	return kerrors.NewAggregate(errs)
+}
+
+// loadBalancerSourceRangesAnnotationSet returns an error value indicating if the
+// ingresscontroller associated with load balancer service should report the Progressing
+// and EvaluationConditionsDetected status conditions with status True by checking
+// if it has "service.beta.kubernetes.io/load-balancer-source-ranges"
+// annotation set. If it is not set, then the service is not progressing and should not affect
+// evaluation conditions, and the return value is nil.
+// Otherwise, the return value is a non-nil error indicating that the annotation
+// must be unset. The intention is to guide the cluster
+// admin towards using the IngressController API and deprecate use of the service
+// annotation for ingress.
+func loadBalancerSourceRangesAnnotationSet(current *corev1.Service) error {
+	if a, ok := current.Annotations[corev1.AnnotationLoadBalancerSourceRangesKey]; !ok || (ok && len(a) == 0) {
+		return nil
+	}
+
+	return fmt.Errorf("You have manually edited an operator-managed object. You must revert your modifications by removing the %v annotation on service %q. You can use the new AllowedSourceRanges API field on the ingresscontroller object to configure this setting instead.", corev1.AnnotationLoadBalancerSourceRangesKey, current.Name)
+}
+
+// loadBalancerSourceRangesMatch returns an error value indicating if the
+// ingresscontroller associated with the load balancer service should report the Progressing
+// and EvaluationConditionsDetected status conditions with status True.  This function
+// checks if the service's LoadBalancerSourceRanges field is empty when AllowedSourceRanges
+// of the ingresscontroller is empty. If this is the case, then the service is not progressing
+// and has no evaluation conditions, and the return value is nil. Otherwise, if AllowedSourceRanges
+// is empty and LoadBalancerSourceRanges is nonempty, the return value is a non-nil error indicating
+// that the LoadBalancerSourceRanges field must be cleared. The intention is to guide the cluster
+// admin towards using the IngressController API and deprecate use of the LoadBalancerSourceRanges
+// field for ingress.
+func loadBalancerSourceRangesMatch(ic *operatorv1.IngressController, current *corev1.Service) error {
+	if len(current.Spec.LoadBalancerSourceRanges) < 1 {
+		return nil
+	}
+	if ic.Spec.EndpointPublishingStrategy != nil && ic.Spec.EndpointPublishingStrategy.LoadBalancer != nil {
+		if len(ic.Spec.EndpointPublishingStrategy.LoadBalancer.AllowedSourceRanges) > 0 {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("You have manually edited an operator-managed object. You must revert your modifications by removing the Spec.LoadBalancerSourceRanges field of LoadBalancer-typed service %q. You can use the new AllowedSourceRanges API field on the ingresscontroller to configure this setting instead.", current.Name)
+}