DNM/SPLAT-2113: spike aws/CIO/IngressNLBSecurityGroup

go.mod

@@ -162,3 +162,7 @@ require (
 replace github.com/imdario/mergo => github.com/imdario/mergo v0.3.5
 
 replace k8s.io/client-go => k8s.io/client-go v0.32.1
+
+// WIP SPLAT-2137 / branch: SPLAT-2113
+//replace github.com/openshift/api => /home/mtulio/openshift/OCPSTRAT-1553/api
+replace github.com/openshift/api => github.com/mtulio/openshift-api v0.0.0-20250520212508-507b878818bf

go.sum

@@ -318,6 +318,8 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJ
 github.com/modern-go/reflect2 v1.0.2 h1:xBagoLtFs94CBntxluKeaWgTMpvLxC4ur3nMaC9Gz0M=
 github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk=
 github.com/montanaflynn/stats v0.0.0-20171201202039-1bf9dbcd8cbe/go.mod h1:wL8QJuTMNUDYhXwkmfOly8iTdp5TEcJFWZD2D7SIkUc=
+github.com/mtulio/openshift-api v0.0.0-20250520212508-507b878818bf h1:p6sK5UjglbOkwE5bNmtE3G8wZZIxV1z+MwbDBxVwXvk=
+github.com/mtulio/openshift-api v0.0.0-20250520212508-507b878818bf/go.mod h1:yk60tHAmHhtVpJQo3TwVYq2zpuP70iJIFDCmeKMIzPw=
 github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA=
 github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
 github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f h1:KUppIJq7/+SVif2QVs3tOP0zanoHgBEVAwHxUSIzRqU=
@@ -342,8 +344,6 @@ github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1y
 github.com/onsi/gomega v1.10.5/go.mod h1:gza4q3jKQJijlu05nKWRCW/GavJumGt8aNRxWg7mt48=
 github.com/onsi/gomega v1.36.2 h1:koNYke6TVk6ZmnyHrCXba/T/MoLBXFjeC1PtvYgw0A8=
 github.com/onsi/gomega v1.36.2/go.mod h1:DdwyADRjrc825LhMEkD76cHR5+pUnjhUN8GlHlRPHzY=
-github.com/openshift/api v0.0.0-20250305225826-b8da3bfeaf77 h1:w6F0sEhlUB1K54Ev4EELsLo5w/xur9pFT19VtemlB4Y=
-github.com/openshift/api v0.0.0-20250305225826-b8da3bfeaf77/go.mod h1:yk60tHAmHhtVpJQo3TwVYq2zpuP70iJIFDCmeKMIzPw=
 github.com/openshift/client-go v0.0.0-20240405120947-c67c8325cdd8 h1:HGfbllzRcrJBSiwzNjBCs7sExLUxC5/1evnvlNGB0Cg=
 github.com/openshift/client-go v0.0.0-20240405120947-c67c8325cdd8/go.mod h1:+VvvaMSTUhOt+rBq7NwRLSNxq06hTeRCBqm0j0PQEq8=
 github.com/openshift/library-go v0.0.0-20240419113445-f1541d628746 h1:MyLp0GgPyIgeVd1scI0iUasVgd6Xpy/t04Rk+I23wRE=

manifests/00-custom-resource-definition.yaml

@@ -7,6 +7,7 @@ metadata:
     capability.openshift.io/name: Ingress
     include.release.openshift.io/ibm-cloud-managed: "true"
     include.release.openshift.io/self-managed-high-availability: "true"
+    release.openshift.io/feature-set: Default
   name: ingresscontrollers.operator.openshift.io
 spec:
   group: operator.openshift.io

pkg/operator/controller/ingress/controller.go

@@ -205,12 +205,13 @@ func hasName(name string) func(o client.Object) bool {
 
 // Config holds all the things necessary for the controller to run.
 type Config struct {
-	Namespace                                 string
-	IngressControllerImage                    string
-	RouteExternalCertificateEnabled           bool
-	IngressControllerLBSubnetsAWSEnabled      bool
-	IngressControllerEIPAllocationsAWSEnabled bool
-	IngressControllerDCMEnabled               bool
+	Namespace                                   string
+	IngressControllerImage                      string
+	RouteExternalCertificateEnabled             bool
+	IngressControllerLBSubnetsAWSEnabled        bool
+	IngressControllerEIPAllocationsAWSEnabled   bool
+	IngressControllerDCMEnabled                 bool
+	IngressControllerNLBSecurityGroupAWSEnabled bool
 }
 
 // reconciler handles the actual ingress reconciliation logic in response to

pkg/operator/controller/ingress/load_balancer_service.go

@@ -92,6 +92,9 @@ const (
 	// awsEIPAllocationsAnnotation specifies a list of eips for NLBs.
 	awsEIPAllocationsAnnotation = "service.beta.kubernetes.io/aws-load-balancer-eip-allocations"
 
+	// awsManagedSecurityGroupAnnotation is the flag to enable security group creation for NLBs.
+	awsManagedSecurityGroupAnnotation = "service.beta.kubernetes.io/aws-load-balancer-managed-security-group"
+
 	// iksLBScopeAnnotation is the annotation used on a service to specify an IBM
 	// load balancer IP type.
 	iksLBScopeAnnotation = "service.kubernetes.io/ibm-load-balancer-cloud-provider-ip-type"
@@ -270,7 +273,14 @@ var (
 // Always returns the current LB service if one exists (whether it already
 // existed or was created during the course of the function).
 func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platformStatus *configv1.PlatformStatus) (bool, *corev1.Service, error) {
-	wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled)
+	wantLBS, desiredLBService, err := desiredLoadBalancerService(&ingressServiceConfigInput{
+		ic:                         ci,
+		deploymentRef:              deploymentRef,
+		platform:                   platformStatus,
+		subnetsAWSEnabled:          r.config.IngressControllerLBSubnetsAWSEnabled,
+		eipAllocationsAWSEnabled:   r.config.IngressControllerEIPAllocationsAWSEnabled,
+		nlbSecurityGroupAWSEnabled: r.config.IngressControllerNLBSecurityGroupAWSEnabled,
+	})
 	if err != nil {
 		return false, nil, err
 	}
@@ -336,7 +346,9 @@ func isServiceOwnedByIngressController(service *corev1.Service, ic *operatorv1.I
 // ingresscontroller, or nil if an LB service isn't desired. An LB service is
 // desired if the high availability type is Cloud. An LB service will declare an
 // owner reference to the given deployment.
-func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) (bool, *corev1.Service, error) {
+func desiredLoadBalancerService(in *ingressServiceConfigInput) (bool, *corev1.Service, error) {
+	ci := in.ic
+	platform := in.platform
 	if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType {
 		return false, nil, nil
 	}
@@ -408,28 +420,35 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 						// See <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>.
 						service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalNLB
 
-						if subnetsAWSEnabled {
+						if in.subnetsAWSEnabled {
 							nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ci)
 							if nlbParams != nil && awsSubnetsExist(nlbParams.Subnets) {
 								service.Annotations[awsLBSubnetsAnnotation] = JoinAWSSubnets(nlbParams.Subnets, ",")
 							}
 						}
 
-						if eipAllocationsAWSEnabled {
+						if in.eipAllocationsAWSEnabled {
 							nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ci)
 							if nlbParams != nil && awsEIPAllocationsExist(nlbParams.EIPAllocations) {
 								service.Annotations[awsEIPAllocationsAnnotation] = JoinAWSEIPAllocations(nlbParams.EIPAllocations, ",")
 							}
 						}
 
+						if in.nlbSecurityGroupAWSEnabled {
+							nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ci)
+							if nlbParams != nil && nlbParams.ManagedSecurityGroup {
+								service.Annotations[awsManagedSecurityGroupAnnotation] = "true"
+							}
+						}
+
 					case operatorv1.AWSClassicLoadBalancer:
 						if aws.ClassicLoadBalancerParameters != nil {
 							if v := aws.ClassicLoadBalancerParameters.ConnectionIdleTimeout; v.Duration > 0 {
 								service.Annotations[awsELBConnectionIdleTimeoutAnnotation] = strconv.FormatUint(uint64(v.Round(time.Second).Seconds()), 10)
 							}
 						}
 
-						if subnetsAWSEnabled {
+						if in.subnetsAWSEnabled {
 							clbParams := getAWSClassicLoadBalancerParametersInSpec(ci)
 							if clbParams != nil && awsSubnetsExist(clbParams.Subnets) {
 								service.Annotations[awsLBSubnetsAnnotation] = JoinAWSSubnets(clbParams.Subnets, ",")
@@ -497,7 +516,7 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 		}
 	}
 
-	service.SetOwnerReferences([]metav1.OwnerReference{deploymentRef})
+	service.SetOwnerReferences([]metav1.OwnerReference{in.deploymentRef})
 	return true, service, nil
 }
 
@@ -760,8 +779,8 @@ func IsServiceInternal(service *corev1.Service) bool {
 // return value is nil.  Otherwise, if something or someone else has modified
 // the service, then the return value is a non-nil error indicating that the
 // modification must be reverted before upgrading is allowed.
-func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error {
-	want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled)
+func loadBalancerServiceIsUpgradeable(in *ingressServiceConfigInput) error {
+	want, desired, err := desiredLoadBalancerService(in)
 	if err != nil {
 		return err
 	}
@@ -774,9 +793,9 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme
 	// Since the status logic runs after the controller sets the annotations, it checks for
 	// any discrepancy (in case modified) between the desired annotation values of the controller
 	// and the current annotation values.
-	changed, updated := loadBalancerServiceAnnotationsChanged(current, desired, managedLoadBalancerServiceAnnotations)
+	changed, updated := loadBalancerServiceAnnotationsChanged(in.service, desired, managedLoadBalancerServiceAnnotations)
 	if changed {
-		diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
+		diff := cmp.Diff(in.service, updated, cmpopts.EquateEmpty())
 		return fmt.Errorf("load balancer service has been modified; changes must be reverted before upgrading: %s", diff)
 	}
 

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -142,6 +142,7 @@ func Test_desiredLoadBalancerService(t *testing.T) {
 		platformStatus                  *configv1.PlatformStatus
 		subnetsAWSFeatureEnabled        bool
 		eipAllocationsAWSFeatureEnabled bool
+		securityGroupsAWSFeatureEnabled bool
 		expectedFloatingIP              string
 	}{
 		{
@@ -753,7 +754,14 @@ func Test_desiredLoadBalancerService(t *testing.T) {
 				t.Errorf("expected IsProxyProtocolNeeded to return %v, got %v", tc.proxyNeeded, proxyNeeded)
 			}
 
-			haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, tc.subnetsAWSFeatureEnabled, tc.eipAllocationsAWSFeatureEnabled)
+			haveSvc, svc, err := desiredLoadBalancerService(&ingressServiceConfigInput{
+				ic:                         ic,
+				deploymentRef:              deploymentRef,
+				platform:                   infraConfig.Status.PlatformStatus,
+				subnetsAWSEnabled:          tc.subnetsAWSFeatureEnabled,
+				eipAllocationsAWSEnabled:   tc.eipAllocationsAWSFeatureEnabled,
+				nlbSecurityGroupAWSEnabled: tc.securityGroupsAWSFeatureEnabled,
+			})
 			switch {
 			case err != nil:
 				t.Error(err)
@@ -938,7 +946,14 @@ func TestDesiredLoadBalancerServiceAWSIdleTimeout(t *testing.T) {
 					},
 				},
 			}
-			haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true)
+
+			haveSvc, svc, err := desiredLoadBalancerService(&ingressServiceConfigInput{
+				ic:                         ic,
+				deploymentRef:              deploymentRef,
+				platform:                   infraConfig.Status.PlatformStatus,
+				subnetsAWSEnabled:          true,
+				eipAllocationsAWSEnabled:   true,
+				nlbSecurityGroupAWSEnabled: true})
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -1552,7 +1567,13 @@ func TestUpdateLoadBalancerServiceSourceRanges(t *testing.T) {
 					},
 				},
 			}
-			wantSvc, desired, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true)
+			wantSvc, desired, err := desiredLoadBalancerService(&ingressServiceConfigInput{
+				ic:                         ic,
+				deploymentRef:              deploymentRef,
+				platform:                   infraConfig.Status.PlatformStatus,
+				subnetsAWSEnabled:          true,
+				eipAllocationsAWSEnabled:   true,
+				nlbSecurityGroupAWSEnabled: true})
 			if err != nil {
 				t.Fatal(err)
 			}

pkg/operator/controller/ingress/status.go

@@ -49,6 +49,34 @@ type expectedCondition struct {
 	gracePeriod      time.Duration
 }
 
+// ingressServiceConfigInput represents the input parameters required to determine
+// the configuration of an ingress service in the cluster.
+//
+// Fields:
+//   - ic: The IngressController resource associated with the ingress service.
+//   - deploymentRef: A reference to the deployment that owns the ingress service.
+//   - service: The Service resource representing the ingress service.
+//   - platform: The platform status of the cluster, providing information about the
+//     underlying infrastructure.
+//   - secret: The Secret resource associated with the ingress service, typically used
+//     for TLS certificates or other sensitive data.
+//   - subnetsAWSEnabled: A boolean indicating whether AWS subnets are enabled for the
+//     ingress service.
+//   - eipAllocationsAWSEnabled: A boolean indicating whether AWS Elastic IP allocations
+//     are enabled for the ingress service.
+//   - nlbSecurityGroupAWSEnabled: A boolean indicating whether AWS Network Load Balancer
+//     (NLB) security groups are enabled for the ingress service.
+type ingressServiceConfigInput struct {
+	ic                         *operatorv1.IngressController
+	deploymentRef              metav1.OwnerReference
+	service                    *corev1.Service
+	platform                   *configv1.PlatformStatus
+	secret                     *corev1.Secret
+	subnetsAWSEnabled          bool
+	eipAllocationsAWSEnabled   bool
+	nlbSecurityGroupAWSEnabled bool
+}
+
 // syncIngressControllerStatus computes the current status of ic and
 // updates status upon any changes since last sync.
 func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressController, deployment *appsv1.Deployment, deploymentRef metav1.OwnerReference, pods []corev1.Pod, service *corev1.Service, operandEvents []corev1.Event, wildcardRecord *iov1.DNSRecord, dnsConfig *configv1.DNS, platformStatus *configv1.PlatformStatus) (error, bool) {
@@ -84,6 +112,8 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle
 		updateIngressControllerFloatingIPOpenStackStatus(updated, service)
 	}
 
+	// Question: Is there any requirement to collect security group created by CCM when the SG is managed?
+
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentAvailableCondition(deployment))
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentReplicasMinAvailableCondition(deployment, pods))
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentReplicasAllAvailableCondition(deployment))
@@ -96,7 +126,16 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle
 	errs = append(errs, err)
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressProgressingCondition(updated.Status.Conditions))
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, degradedCondition)
-	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled))
+	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(&ingressServiceConfigInput{
+		ic:                       ic,
+		deploymentRef:            deploymentRef,
+		service:                  service,
+		platform:                 platformStatus,
+		secret:                   secret,
+		subnetsAWSEnabled:        r.config.IngressControllerLBSubnetsAWSEnabled,
+		eipAllocationsAWSEnabled: r.config.IngressControllerEIPAllocationsAWSEnabled,
+		// TODO/Q: do we need SG condition upgrade?
+	}))
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressEvaluationConditionsDetectedCondition(ic, service))
 
 	updated.Status.Conditions = PruneConditions(updated.Status.Conditions)
@@ -646,13 +685,13 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition,
 }
 
 // computeIngressUpgradeableCondition computes the IngressController's "Upgradeable" status condition.
-func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, service *corev1.Service, platform *configv1.PlatformStatus, secret *corev1.Secret, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) operatorv1.OperatorCondition {
+func computeIngressUpgradeableCondition(in *ingressServiceConfigInput) operatorv1.OperatorCondition {
 	var errs []error
 
-	errs = append(errs, checkDefaultCertificate(secret, "*."+ic.Status.Domain))
+	errs = append(errs, checkDefaultCertificate(in.secret, "*."+in.ic.Status.Domain))
 
-	if service != nil {
-		errs = append(errs, loadBalancerServiceIsUpgradeable(ic, deploymentRef, service, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled))
+	if in.service != nil {
+		errs = append(errs, loadBalancerServiceIsUpgradeable(in))
 	}
 
 	if err := kerrors.NewAggregate(errs); err != nil {

pkg/operator/controller/ingress/status_test.go

@@ -3175,7 +3175,14 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) {
 					},
 				},
 			}
-			wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true)
+			wantSvc, service, err := desiredLoadBalancerService(&ingressServiceConfigInput{
+				ic:                         ic,
+				deploymentRef:              deploymentRef,
+				platform:                   platformStatus,
+				subnetsAWSEnabled:          true,
+				eipAllocationsAWSEnabled:   true,
+				nlbSecurityGroupAWSEnabled: true,
+			})
 			if err != nil {
 				t.Errorf("unexpected error from desiredLoadBalancerService: %v", err)
 				return
@@ -3197,7 +3204,15 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) {
 				expectedStatus = operatorv1.ConditionTrue
 			}
 
-			actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret, true, true)
+			actual := computeIngressUpgradeableCondition(&ingressServiceConfigInput{
+				ic:                         ic,
+				deploymentRef:              deploymentRef,
+				platform:                   platformStatus,
+				secret:                     secret,
+				subnetsAWSEnabled:          true,
+				eipAllocationsAWSEnabled:   true,
+				nlbSecurityGroupAWSEnabled: true,
+			})
 			if actual.Status != expectedStatus {
 				t.Errorf("expected Upgradeable to be %q, got %q", expectedStatus, actual.Status)
 			}
@@ -3285,7 +3300,14 @@ func Test_computeIngressEvaluationConditionsDetectedCondition(t *testing.T) {
 				},
 			}
 
-			wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true)
+			wantSvc, service, err := desiredLoadBalancerService(&ingressServiceConfigInput{
+				ic:                         ic,
+				deploymentRef:              deploymentRef,
+				platform:                   platformStatus,
+				subnetsAWSEnabled:          true,
+				eipAllocationsAWSEnabled:   true,
+				nlbSecurityGroupAWSEnabled: true,
+			})
 			if err != nil {
 				t.Fatalf("unexpected error from desiredLoadBalancerService: %v", err)
 			}