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

data/data/install.openshift.io_installconfigs.yaml

@@ -4264,12 +4264,41 @@ spec:
                       different account than the rest of the cluster resources.
                       If HostedZoneRole is set, HostedZone must also be set.
                     type: string
+                  ingressController:
+                    description: IngressController is an optional extra configuration
+                      for the Ingress Controllers.
+                    properties:
+                      securityGroupEnabled:
+                        description: |-
+                          SecurityGroupEnabled is an optional field to enable security groups
+                          when using load balancer type Network Load Balancer (NLB).
+                          When this field is enabled with LBType NLB, all ingresscontrollers (including the
+                          default ingresscontroller) will be created using security group in the NLBs
+                          by default.
+
+                          If this field is not set explicitly, it defaults to no security groups in the NLB.
+                          This default is subject to change over time.
+                        type: boolean
+                      securityGroups:
+                        description: |-
+                          resources created by installer.
+
+                          SecurityGroups is an optional list of security groups to be added to the IngressController
+                          when using load balancer type Network Load Balancer (NLB).
+
+                          When this field is used with SecurityGroupEnabled and LBType NLB, the security
+                          group will be attached to the IngressControllers.
+                        items:
+                          type: string
+                        type: array
+                    type: object
                   lbType:
                     description: |-
                       LBType is an optional field to specify a load balancer type.
                       When this field is specified, all ingresscontrollers (including the
                       default ingresscontroller) will be created using the specified load-balancer
                       type by default.
+                      Q: Should we move LBType to here?
 
                       Following are the accepted values:
 

go.mod

@@ -352,3 +352,7 @@ replace github.com/vmware-tanzu/vm-operator/pkg/constants/testlabels => github.c
 
 // This is to force capi back for the older provider version
 replace sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.19.3
+
+// 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

@@ -659,6 +659,8 @@ github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjY
 github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 h1:n6/2gBQ3RWajuToeY6ZtZTIKv2v7ThUy5KKusIT0yc0=
 github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00/go.mod h1:Pm3mSP3c5uWn86xMLZ5Sa7JB9GsEZySvHYXCTK4E9q4=
 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-20120707110453-a547fc61f48d/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
 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=
@@ -699,8 +701,6 @@ github.com/opencontainers/image-spec v1.1.0 h1:8SG7/vwALn54lVB/0yZ/MMwhFrPYtpEHQ
 github.com/opencontainers/image-spec v1.1.0/go.mod h1:W4s4sFTMaBeK1BQLXbG4AdM2szdn85PY75RI83NrTrM=
 github.com/opencontainers/runtime-spec v1.2.0 h1:z97+pHb3uELt/yiAWD691HNHQIF07bE7dzrbT927iTk=
 github.com/opencontainers/runtime-spec v1.2.0/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
-github.com/openshift/api v0.0.0-20250313134101-8a7efbfb5316 h1:iJ1OkAUvFbQPB6qWRDxrH1jj8iA9GA/Jx2vYz7o+i1E=
-github.com/openshift/api v0.0.0-20250313134101-8a7efbfb5316/go.mod h1:yk60tHAmHhtVpJQo3TwVYq2zpuP70iJIFDCmeKMIzPw=
 github.com/openshift/assisted-image-service v0.0.0-20240607085136-02df2e56dde6 h1:U6ve+dnHlHhAELoxX+rdFOHVhoaYl0l9qtxwYtsO6C0=
 github.com/openshift/assisted-image-service v0.0.0-20240607085136-02df2e56dde6/go.mod h1:o2H5VwQhUD8P6XsK6dRmKpCCJqVvv12KJQZBXmcCXCU=
 github.com/openshift/assisted-service v1.0.10-0.20230830164851-6573b5d7021d h1:CKw2Y4EdaFsMoqAdr2Tq0nlYTaaXmCRdP0gOu7pN64U=

pkg/asset/manifests/cloudproviderconfig.go

@@ -110,6 +110,7 @@ func (cpc *CloudProviderConfig) Generate(ctx context.Context, dependencies asset
 		// https://bugzilla.redhat.com/show_bug.cgi?id=1926975.
 		// Note that the newline is required in order to be valid yaml.
 		cm.Data[cloudProviderConfigDataKey] = `[Global]
+nlbSecurityGroupEnabled=true
 `
 	case openstacktypes.Name:
 		cloudProviderConfigData, cloudProviderConfigCABundleData, err := openstackmanifests.GenerateCloudProviderConfig(ctx, *installConfig.Config)

pkg/asset/manifests/ingress.go

@@ -39,6 +39,7 @@ func (*Ingress) Name() string {
 func (*Ingress) Dependencies() []asset.Asset {
 	return []asset.Asset{
 		&installconfig.InstallConfig{},
+		&installconfig.ClusterID{},
 	}
 }
 
@@ -51,6 +52,7 @@ func (*Ingress) Dependencies() []asset.Asset {
 // to use the internal publishing strategy.
 func (ing *Ingress) Generate(_ context.Context, dependencies asset.Parents) error {
 	installConfig := &installconfig.InstallConfig{}
+	clusterID := &installconfig.ClusterID{}
 	dependencies.Get(installConfig)
 
 	ing.FileList = []*asset.File{}
@@ -64,7 +66,7 @@ func (ing *Ingress) Generate(_ context.Context, dependencies asset.Parents) erro
 		Data:     clusterConfig,
 	})
 
-	defaultIngressController, err := ing.generateDefaultIngressController(installConfig.Config)
+	defaultIngressController, err := ing.generateDefaultIngressController(installConfig.Config, clusterID.InfraID)
 	if err != nil {
 		return errors.Wrap(err, "failed to create default ingresscontroller")
 	}
@@ -135,7 +137,7 @@ func (ing *Ingress) generateClusterConfig(config *types.InstallConfig) ([]byte,
 	return yaml.Marshal(obj)
 }
 
-func (ing *Ingress) generateDefaultIngressController(config *types.InstallConfig) ([]byte, error) {
+func (ing *Ingress) generateDefaultIngressController(config *types.InstallConfig, infraID string) ([]byte, error) {
 	// getDefaultIngressController returns an object representing the default Ingress Controller
 	// with empty LoadBalancer spec.
 	getDefaultIngressController := func() *operatorv1.IngressController {
@@ -161,6 +163,41 @@ func (ing *Ingress) generateDefaultIngressController(config *types.InstallConfig
 
 	switch config.Platform.Name() {
 	case aws.Name:
+		// default ingress
+		obj = getDefaultIngressController()
+		lbSpec := obj.Spec.EndpointPublishingStrategy.LoadBalancer
+
+		if config.PublicIngress() {
+			lbSpec.Scope = operatorv1.ExternalLoadBalancer
+		} else {
+			lbSpec.Scope = operatorv1.InternalLoadBalancer
+		}
+		if config.AWS.LBType == configv1.NLB {
+			lbSpec.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
+				Type: operatorv1.AWSLoadBalancerProvider,
+				AWS: &operatorv1.AWSLoadBalancerParameters{
+					Type:                          operatorv1.AWSNetworkLoadBalancer,
+					NetworkLoadBalancerParameters: &operatorv1.AWSNetworkLoadBalancerParameters{},
+				},
+			}
+		} else {
+			lbSpec.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
+				Type: operatorv1.AWSLoadBalancerProvider,
+				AWS: &operatorv1.AWSLoadBalancerParameters{
+					Type:                          operatorv1.AWSNetworkLoadBalancer,
+					ClassicLoadBalancerParameters: &operatorv1.AWSClassicLoadBalancerParameters{},
+				},
+			}
+		}
+
+		// Enable features
+		// Feature: Enable Security Group on NLB
+		if config.AWS.LBType == configv1.NLB &&
+			config.AWS.IngressController != nil {
+			lbSpec.ProviderParameters.AWS.NetworkLoadBalancerParameters.ManagedSecurityGroup = config.AWS.IngressController.SecurityGroupEnabled
+		}
+
+		// Feature: BYO VPC/subnet
 		subnetIDsByRole := make(map[aws.SubnetRoleType][]operatorv1.AWSSubnetID)
 		for _, subnet := range config.AWS.VPC.Subnets {
 			for _, role := range subnet.Roles {
@@ -170,42 +207,17 @@ func (ing *Ingress) generateDefaultIngressController(config *types.InstallConfig
 
 		// BYO-subnet install case and subnet roles are specified.
 		if len(subnetIDsByRole) > 0 {
-			obj = getDefaultIngressController()
-			lbSpec := obj.Spec.EndpointPublishingStrategy.LoadBalancer
-
-			if config.PublicIngress() {
-				lbSpec.Scope = operatorv1.ExternalLoadBalancer
-			} else {
-				lbSpec.Scope = operatorv1.InternalLoadBalancer
-			}
-
 			if config.AWS.LBType == configv1.NLB {
-				lbSpec.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
-					Type: operatorv1.AWSLoadBalancerProvider,
-					AWS: &operatorv1.AWSLoadBalancerParameters{
-						Type: operatorv1.AWSNetworkLoadBalancer,
-						NetworkLoadBalancerParameters: &operatorv1.AWSNetworkLoadBalancerParameters{
-							Subnets: &operatorv1.AWSSubnets{
-								IDs: subnetIDsByRole[aws.IngressControllerLBSubnetRole],
-							},
-						},
-					},
+				lbSpec.ProviderParameters.AWS.NetworkLoadBalancerParameters.Subnets = &operatorv1.AWSSubnets{
+					IDs: subnetIDsByRole[aws.IngressControllerLBSubnetRole],
 				}
 			} else {
-				lbSpec.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
-					Type: operatorv1.AWSLoadBalancerProvider,
-					AWS: &operatorv1.AWSLoadBalancerParameters{
-						Type: operatorv1.AWSClassicLoadBalancer,
-						ClassicLoadBalancerParameters: &operatorv1.AWSClassicLoadBalancerParameters{
-							Subnets: &operatorv1.AWSSubnets{
-								IDs: subnetIDsByRole[aws.IngressControllerLBSubnetRole],
-							},
-						},
-					},
+				lbSpec.ProviderParameters.AWS.ClassicLoadBalancerParameters.Subnets = &operatorv1.AWSSubnets{
+					IDs: subnetIDsByRole[aws.IngressControllerLBSubnetRole],
 				}
 			}
-			break
 		}
+
 		// Fall back to existing logic similar to other platforms otherwise.
 		// i.e. managed subnets or no subnet roles are specified.
 		fallthrough

pkg/infrastructure/aws/clusterapi/ami.go

@@ -15,6 +15,7 @@ import (
 
 // copyAMIToRegion copies the AMI to the region configured in the installConfig if needed.
 func copyAMIToRegion(ctx context.Context, installConfig *installconfig.InstallConfig, infraID string, rhcosImage *rhcos.Image) (string, error) {
+	logrus.Infof("Copying AMI to region....")
 	osImage := strings.SplitN(rhcosImage.ControlPlane, ",", 2)
 	amiID, amiRegion := osImage[0], osImage[1]
 

pkg/infrastructure/aws/clusterapi/aws.go

@@ -104,21 +104,36 @@ func (p Provider) Ignition(ctx context.Context, in clusterapi.IgnitionInput) ([]
 	return ignSecrets, nil
 }
 
-// InfraReady creates private hosted zone and DNS records.
-func (*Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput) error {
-	awsCluster := &capa.AWSCluster{}
-	key := k8sClient.ObjectKey{
-		Name:      in.InfraID,
-		Namespace: capiutils.Namespace,
-	}
-	if err := in.Client.Get(ctx, key, awsCluster); err != nil {
-		return fmt.Errorf("failed to get AWSCluster: %w", err)
-	}
-
-	awsSession, err := in.InstallConfig.AWS.Session(ctx)
-	if err != nil {
-		return fmt.Errorf("failed to get aws session: %w", err)
-	}
+// infraReadyHookDNS is a hook function that ensures the DNS infrastructure is properly set up
+// for an AWS-based cluster. It performs the following tasks:
+//
+// 1. Retrieves the VPC ID associated with the subnets if not explicitly provided.
+// 2. Skips DNS record creation if the user has opted for a user-provisioned DNS solution.
+// 3. Creates a private hosted zone in Route53 if it does not already exist.
+// 4. Creates DNS records for the control plane load balancer in both public and private zones.
+//
+// Parameters:
+// - ctx: The context for managing request-scoped values, cancellation, and deadlines.
+// - in: An input structure containing infrastructure-related configuration and metadata.
+// - awsSession: An AWS session used to interact with AWS services.
+// - awsCluster: The AWSCluster object containing cluster-specific configuration and status.
+//
+// Returns:
+// - An error if any step in the DNS setup process fails, or nil if successful.
+//
+// Notes:
+//   - If the user has enabled user-provisioned DNS, the function skips DNS record creation.
+//   - The function handles potential conflicts when creating a private hosted zone, such as
+//     the "ConflictingDomainExists" error, by logging a warning and proceeding.
+//   - The function creates or updates DNS records for both the public API endpoint and the
+//     internal API endpoint (api-int) in the appropriate hosted zones.
+func (p *Provider) infraReadyHookDNS(
+	ctx context.Context,
+	in clusterapi.InfraReadyInput,
+	awsSession *session.Session,
+	awsCluster *capa.AWSCluster,
+) (err error) {
+	client := awsconfig.NewClient(awsSession)
 
 	subnetIDs := make([]string, 0, len(awsCluster.Spec.NetworkSpec.Subnets))
 	for _, s := range awsCluster.Spec.NetworkSpec.Subnets {
@@ -134,8 +149,6 @@ func (*Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput)
 		}
 	}
 
-	client := awsconfig.NewClient(awsSession)
-
 	// The user has selected to provision their own DNS solution. Skip the creation of the
 	// Hosted Zone(s) and the records for those zones.
 	if in.InstallConfig.Config.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled {
@@ -147,8 +160,10 @@ func (*Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput)
 
 	phzID := in.InstallConfig.Config.AWS.HostedZone
 	if len(phzID) == 0 {
+		// TODO(mtulio): file a bug when REENTRANT=true: need to check if PHZ has been already created before creating
+		// or handle error/exception ConflictingDomainExists. Full error:
+		//  ERROR failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed provisioning resources after infrastructure ready: failed to create private hosted zone: error creating private hosted zone: ConflictingDomainExists: The VPC vpc-006ef9b0ada34e881 in region us-east-1 has already been associated with the hosted zone Z04314343JJ3BIPAD8WRA with the same domain name.
 		logrus.Debugln("Creating private Hosted Zone")
-
 		res, err := client.CreateHostedZone(ctx, &awsconfig.HostedZoneInput{
 			InfraID:  in.InfraID,
 			VpcID:    vpcID,
@@ -158,7 +173,12 @@ func (*Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput)
 			UserTags: awsCluster.Spec.AdditionalTags,
 		})
 		if err != nil {
-			return fmt.Errorf("failed to create private hosted zone: %w", err)
+			var awsErr awserr.Error
+			if errors.As(err, &awsErr) && awsErr.Code() == "ConflictingDomainExists" {
+				logrus.Warnf("Private hosted zone already exists: %v", err)
+			} else {
+				return fmt.Errorf("failed to create private hosted zone: %w", err)
+			}
 		}
 		phzID = aws.StringValue(res.Id)
 		logrus.Infoln("Created private Hosted Zone")
@@ -223,6 +243,32 @@ func (*Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput)
 		return fmt.Errorf("failed to create records for api-int in private zone: %w", err)
 	}
 	logrus.Debugln("Created private API record in private zone")
+	return nil
+}
+
+// InfraReady creates additional infrastructure resources not covered by Cluster API,
+// such as private hosted zone and DNS records, extra ingress security groups, etc.
+func (p *Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput) error {
+	awsCluster := &capa.AWSCluster{}
+	key := k8sClient.ObjectKey{
+		Name:      in.InfraID,
+		Namespace: capiutils.Namespace,
+	}
+	if err := in.Client.Get(ctx, key, awsCluster); err != nil {
+		return fmt.Errorf("failed to get AWSCluster: %w", err)
+	}
+
+	awsSession, err := in.InstallConfig.AWS.Session(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to get aws session: %w", err)
+	}
+
+	// InfraReady for feature-specific
+
+	// Provision DNS
+	if err := p.infraReadyHookDNS(ctx, in, awsSession, awsCluster); err != nil {
+		return err
+	}
 
 	return nil
 }

pkg/types/aws/platform.go

@@ -95,6 +95,7 @@ type Platform struct {
 	// When this field is specified, all ingresscontrollers (including the
 	// default ingresscontroller) will be created using the specified load-balancer
 	// type by default.
+	// Q: Should we move LBType to here?
 	//
 	// Following are the accepted values:
 	//
@@ -134,6 +135,10 @@ type Platform struct {
 	// +default="Disabled"
 	// +kubebuilder:validation:Enum="Enabled";"Disabled"
 	UserProvisionedDNS dns.UserProvisionedDNS `json:"userProvisionedDNS,omitempty"`
+
+	// IngressController is an optional extra configuration for the Ingress Controllers.
+	// +optional
+	IngressController *IngressController `json:"ingressController,omitempty"`
 }
 
 // ServiceEndpoint store the configuration for services to
@@ -232,6 +237,21 @@ const (
 	ControlPlaneInternalLBSubnetRole SubnetRoleType = "ControlPlaneInternalLB"
 )
 
+// IngressController specifies the additional ingress controller configuration.
+type IngressController struct {
+	// SecurityGroupEnabled is an optional field to enable security groups
+	// when using load balancer type Network Load Balancer (NLB).
+	// When this field is enabled with LBType NLB, all ingresscontrollers (including the
+	// default ingresscontroller) will be created using security group in the NLBs
+	// by default.
+	//
+	// If this field is not set explicitly, it defaults to no security groups in the NLB.
+	// This default is subject to change over time.
+	//
+	// +optional
+	SecurityGroupEnabled bool `json:"securityGroupEnabled,omitempty"`
+}
+
 // IsSecretRegion returns true if the region is part of either the ISO or ISOB partitions.
 func IsSecretRegion(region string) bool {
 	partition, ok := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), region)