feat/aws/svc-nlb: Enable config to manage security group

Enforce CCM to manage Security Group by default for
security compliance and best practices on Service type-loadBalancer
when using Network Load Balancer (NLB).

Fixes INI files with sections sorted co-authored by Claude.

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Nolan Brubaker <[email protected]>

Author: mtulio

pkg/cloud/aws/aws_config_transformer.go

@@ -11,6 +11,7 @@ import (
 	"gopkg.in/ini.v1"
 
 	awsconfig "k8s.io/cloud-provider-aws/pkg/providers/v1/config"
+	"k8s.io/klog/v2"
 
 	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 )
@@ -28,7 +29,7 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, netwo
 		return "", fmt.Errorf("failed to read the cloud.conf: %w", err)
 	}
 
-	setOpenShiftDefaults(cfg)
+	setOpenShiftDefaults(cfg, features)
 
 	return marshalAWSConfig(cfg)
 }
@@ -76,24 +77,64 @@ func marshalAWSConfig(cfg *awsconfig.CloudConfig) (string, error) {
 	}
 
 	// Ensure service override sections are last and ordered numerically.
-	sort.Slice(file.Sections(), func(i, j int) bool {
-		return file.Sections()[i].Name() < file.Sections()[j].Name()
+	// We need to create a new file with sorted sections because file.Sections()
+	// returns a new slice each time, so sorting it doesn't modify the original.
+	sections := file.Sections()
+	sort.Slice(sections, func(i, j int) bool {
+		return sections[i].Name() < sections[j].Name()
 	})
 
+	// Create a new INI file with sections in sorted order
+	sortedFile := ini.Empty()
+	for _, section := range sections {
+		newSection, err := sortedFile.NewSection(section.Name())
+		if err != nil {
+			return "", fmt.Errorf("failed to create section: %w", err)
+		}
+		for _, key := range section.Keys() {
+			newSection.Key(key.Name()).SetValue(key.Value())
+		}
+	}
+
 	buf := &bytes.Buffer{}
 
-	if _, err := file.WriteTo(buf); err != nil {
+	if _, err := sortedFile.WriteTo(buf); err != nil {
 		return "", fmt.Errorf("failed to write INI file: %w", err)
 	}
 
 	return buf.String(), nil
 }
 
-func setOpenShiftDefaults(cfg *awsconfig.CloudConfig) {
+// isFeatureGateEnabled safely checks if a feature gate is enabled without panicking
+// if the feature is not registered. Returns false if features is nil or if the
+// feature is not in the known features list.
+func isFeatureGateEnabled(features featuregates.FeatureGate, featureName string) bool {
+	// features.Enabled returns panic if the feature is not registered in FeatureGates,
+	// this functions prevents the panic by returning false if the feature is not registered in FeatureGates.
+	if features == nil || len(featureName) == 0 {
+		return false
+	}
+	for _, known := range features.KnownFeatures() {
+		if string(known) == featureName {
+			return features.Enabled(known)
+		}
+	}
+	return false
+}
+
+func setOpenShiftDefaults(cfg *awsconfig.CloudConfig, features featuregates.FeatureGate) {
 	if cfg.Global.ClusterServiceLoadBalancerHealthProbeMode == "" {
 		// OpenShift uses Shared mode by default.
 		// This attaches the health check for Cluster scope services to the "kube-proxy"
 		// health check endpoint served by OVN.
 		cfg.Global.ClusterServiceLoadBalancerHealthProbeMode = "Shared"
 	}
+	if isFeatureGateEnabled(features, "AWSServiceLBNetworkSecurityGroup") {
+		if cfg.Global.NLBSecurityGroupMode != awsconfig.NLBSecurityGroupModeManaged {
+			// OpenShift enforces to CCM manage security group by default when deploying
+			// Service type-LoadBalancer Network Load Balancer (NLB).
+			klog.Infof("Enforcing cloud provider AWS configuration NLBSecurityGroupMode to Managed")
+			cfg.Global.NLBSecurityGroupMode = awsconfig.NLBSecurityGroupModeManaged
+		}
+	}
 }

pkg/cloud/aws/aws_config_transformer_test.go

@@ -5,14 +5,81 @@ import (
 
 	. "github.com/onsi/gomega"
 
+	configv1 "github.com/openshift/api/config/v1"
+
 	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 )
 
+var mockEmptyFeatureGates = featuregates.NewFeatureGate([]configv1.FeatureGateName{}, []configv1.FeatureGateName{})
+var mockEnabledFeatureGates = featuregates.NewFeatureGate([]configv1.FeatureGateName{"AWSServiceLBNetworkSecurityGroup"}, []configv1.FeatureGateName{})
+var mockDisabledFeatureGates = featuregates.NewFeatureGate([]configv1.FeatureGateName{}, []configv1.FeatureGateName{"AWSServiceLBNetworkSecurityGroup"})
+
+func TestIsFeatureGateEnabled(t *testing.T) {
+	testCases := []struct {
+		name        string
+		features    featuregates.FeatureGate
+		featureName string
+		expected    bool
+	}{
+		{
+			name:        "returns false when features is nil",
+			features:    nil,
+			featureName: "AWSServiceLBNetworkSecurityGroup",
+			expected:    false,
+		},
+		{
+			name:        "returns false when feature name is empty string",
+			features:    mockEnabledFeatureGates,
+			featureName: "",
+			expected:    false,
+		},
+		{
+			name:        "returns false when feature is not registered (empty feature gate)",
+			features:    mockEmptyFeatureGates,
+			featureName: "AWSServiceLBNetworkSecurityGroup",
+			expected:    false,
+		},
+		{
+			name:        "returns true when feature is enabled",
+			features:    mockEnabledFeatureGates,
+			featureName: "AWSServiceLBNetworkSecurityGroup",
+			expected:    true,
+		},
+		{
+			name:        "returns false when feature is explicitly disabled",
+			features:    mockDisabledFeatureGates,
+			featureName: "AWSServiceLBNetworkSecurityGroup",
+			expected:    false,
+		},
+		{
+			name:        "returns false when feature is not in known features list",
+			features:    featuregates.NewFeatureGate([]configv1.FeatureGateName{"SomeOtherFeature"}, []configv1.FeatureGateName{}),
+			featureName: "AWSServiceLBNetworkSecurityGroup",
+			expected:    false,
+		},
+		{
+			name:        "returns false for unknown feature with disabled features registered",
+			features:    featuregates.NewFeatureGate([]configv1.FeatureGateName{}, []configv1.FeatureGateName{"SomeOtherFeature"}),
+			featureName: "AWSServiceLBNetworkSecurityGroup",
+			expected:    false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			result := isFeatureGateEnabled(tc.features, tc.featureName)
+			g.Expect(result).To(Equal(tc.expected), "Expected isFeatureGateEnabled to return %v for feature '%s'", tc.expected, tc.featureName)
+		})
+	}
+}
+
 func TestCloudConfigTransformer(t *testing.T) {
 	testCases := []struct {
 		name     string
 		source   string
 		expected string
+		features featuregates.FeatureGate
 	}{
 		{
 			name: "default source",
@@ -23,6 +90,7 @@ DisableSecurityGroupIngress                     = false
 ClusterServiceLoadBalancerHealthProbeMode       = Shared
 ClusterServiceSharedLoadBalancerHealthProbePort = 0
 `,
+			features: mockEmptyFeatureGates,
 		},
 		{
 			name:   "completely empty source",
@@ -32,6 +100,7 @@ DisableSecurityGroupIngress                     = false
 ClusterServiceLoadBalancerHealthProbeMode       = Shared
 ClusterServiceSharedLoadBalancerHealthProbePort = 0
 `,
+			features: mockEmptyFeatureGates,
 		},
 		{
 			name: "with existing configuration",
@@ -45,6 +114,7 @@ DisableSecurityGroupIngress                     = true
 ClusterServiceLoadBalancerHealthProbeMode       = Shared
 ClusterServiceSharedLoadBalancerHealthProbePort = 0
 `, // Ordered based on the order of fields in the AWS CloudConfig struct.
+			features: mockEmptyFeatureGates,
 		},
 		{
 			name: "with existing configuration and overrides",
@@ -82,14 +152,44 @@ Region        = us-west-1
 URL           = https://s3.foo.bar
 SigningRegion = signing_region
 `, // Ordered based on the order of fields in the AWS CloudConfig struct.
+			features: mockEmptyFeatureGates,
+		},
+		{
+			name: "with AWSServiceLBNetworkSecurityGroup feature gate enabled",
+			source: `[Global]
+DisableSecurityGroupIngress = true
+Zone                        = Foo
+`,
+			expected: `[Global]
+Zone                                            = Foo
+DisableSecurityGroupIngress                     = true
+ClusterServiceLoadBalancerHealthProbeMode       = Shared
+ClusterServiceSharedLoadBalancerHealthProbePort = 0
+NLBSecurityGroupMode                            = Managed
+`,
+			features: mockEnabledFeatureGates,
+		},
+		{
+			name: "with AWSServiceLBNetworkSecurityGroup feature gate disabled",
+			source: `[Global]
+DisableSecurityGroupIngress = true
+Zone                        = Foo
+`,
+			expected: `[Global]
+Zone                                            = Foo
+DisableSecurityGroupIngress                     = true
+ClusterServiceLoadBalancerHealthProbeMode       = Shared
+ClusterServiceSharedLoadBalancerHealthProbePort = 0
+`,
+			features: mockDisabledFeatureGates,
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			gotConfig, err := CloudConfigTransformer(tc.source, nil, nil, featuregates.NewFeatureGate(nil, nil)) // No Infra or Network are required for the current functionality.
+			gotConfig, err := CloudConfigTransformer(tc.source, nil, nil, tc.features) // No Infra or Network are required for the current functionality.
 			g.Expect(err).ToNot(HaveOccurred())
 
 			g.Expect(gotConfig).To(Equal(tc.expected))

pkg/controllers/cloud_config_sync_controller_test.go

@@ -206,7 +206,7 @@ var _ = Describe("Cloud config sync controller", func() {
 				ManagedNamespace: targetNamespaceName,
 			},
 			Scheme:            scheme.Scheme,
-			FeatureGateAccess: featuregates.NewHardcodedFeatureGateAccessForTesting(nil, nil, nil, nil),
+			FeatureGateAccess: featuregates.NewHardcodedFeatureGateAccessForTesting(nil, []configv1.FeatureGateName{"AWSServiceLBNetworkSecurityGroup"}, nil, nil),
 		}
 		Expect(reconciler.SetupWithManager(mgr)).To(Succeed())
 
@@ -408,7 +408,7 @@ var _ = Describe("Cloud config sync reconciler", func() {
 				ManagedNamespace: targetNamespaceName,
 			},
 			Scheme:            scheme.Scheme,
-			FeatureGateAccess: featuregates.NewHardcodedFeatureGateAccessForTesting(nil, nil, nil, nil),
+			FeatureGateAccess: featuregates.NewHardcodedFeatureGateAccessForTesting(nil, []configv1.FeatureGateName{"AWSServiceLBNetworkSecurityGroup"}, nil, nil),
 		}
 
 		networkResource := makeNetworkResource()