limit manual sg configuration to 1, refactor code

controllers/ingress/group_controller.go

@@ -57,7 +57,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder
 		annotationParser, subnetsResolver,
 		authConfigBuilder, enhancedBackendBuilder, trackingProvider, elbv2TaggingManager,
 		cloud.VpcID(), config.ClusterName, config.DefaultTags, config.ExternalManagedTags,
-		config.DefaultSSLPolicy, backendSGProvider, logger)
+		config.DefaultSSLPolicy, backendSGProvider, config.EnableBackendSecurityGroup, logger)
 	stackMarshaller := deploy.NewDefaultStackMarshaller()
 	stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler,
 		config, ingressTagPrefix, logger)
@@ -142,15 +142,15 @@ func (r *groupReconciler) reconcile(ctx context.Context, req ctrl.Request) error
 		}
 	}
 
-	if len(ingGroup.InactiveMembers) > 0 {
-		if err := r.groupFinalizerManager.RemoveGroupFinalizer(ctx, ingGroupID, ingGroup.InactiveMembers); err != nil {
-			r.recordIngressGroupEvent(ctx, ingGroup, corev1.EventTypeWarning, k8s.IngressEventReasonFailedRemoveFinalizer, fmt.Sprintf("Failed remove finalizer due to %v", err))
+	if len(ingGroup.Members) == 0 {
+		if err := r.backendSGProvider.Release(ctx); err != nil {
 			return err
 		}
 	}
 
-	if len(ingGroup.Members) == 0 {
-		if err := r.backendSGProvider.Release(ctx); err != nil {
+	if len(ingGroup.InactiveMembers) > 0 {
+		if err := r.groupFinalizerManager.RemoveGroupFinalizer(ctx, ingGroupID, ingGroup.InactiveMembers); err != nil {
+			r.recordIngressGroupEvent(ctx, ingGroup, corev1.EventTypeWarning, k8s.IngressEventReasonFailedRemoveFinalizer, fmt.Sprintf("Failed remove finalizer due to %v", err))
 			return err
 		}
 	}

main.go

@@ -106,8 +106,8 @@ func main() {
 	vpcResolver := networking.NewDefaultVPCResolver(cloud.EC2(), cloud.VpcID(), ctrl.Log.WithName("vpc-resolver"))
 	tgbResManager := targetgroupbinding.NewDefaultResourceManager(mgr.GetClient(), cloud.ELBV2(), cloud.EC2(),
 		podInfoRepo, sgManager, sgReconciler, cloud.VpcID(), controllerCFG.ClusterName, mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log)
-	backendSGProvider := networking.NewBackendSGProvider(controllerCFG.ClusterName, controllerCFG.EnableBackendSecurityGroup,
-		controllerCFG.BackendSecurityGroups, cloud.VpcID(), cloud.EC2(), mgr.GetClient(), ctrl.Log.WithName("backend-sg-provider"))
+	backendSGProvider := networking.NewBackendSGProvider(controllerCFG.ClusterName, controllerCFG.BackendSecurityGroup,
+		cloud.VpcID(), cloud.EC2(), mgr.GetClient(), ctrl.Log.WithName("backend-sg-provider"))
 	ingGroupReconciler := ingress.NewGroupReconciler(cloud, mgr.GetClient(), mgr.GetEventRecorderFor("ingress"),
 		finalizerManager, sgManager, sgReconciler, subnetResolver,
 		controllerCFG, backendSGProvider, ctrl.Log.WithName("controllers").WithName("ingress"))

pkg/annotations/constants.go

@@ -45,7 +45,7 @@ const (
 	IngressSuffixAuthSessionCookie            = "auth-session-cookie"
 	IngressSuffixAuthSessionTimeout           = "auth-session-timeout"
 	IngressSuffixTargetNodeLabels             = "target-node-labels"
-	IngressSuffixManageSecurityGroupRules     = "manage-security-group-rules"
+	IngressSuffixManageSecurityGroupRules     = "manage-backend-security-group-rules"
 
 	// NLB annotation suffixes
 	// prefixes service.beta.kubernetes.io, service.kubernetes.io

pkg/config/controller_config.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -20,7 +21,7 @@ const (
 	flagTargetGroupBindingMaxExponentialBackoffDelay = "targetgroupbinding-max-exponential-backoff-delay"
 	flagDefaultSSLPolicy                             = "default-ssl-policy"
 	flagEnableBackendSG                              = "enable-backend-security-group"
-	flagBackendSecurityGroups                        = "backend-security-groups"
+	flagBackendSecurityGroup                         = "backend-security-group"
 	defaultLogLevel                                  = "info"
 	defaultMaxConcurrentReconciles                   = 3
 	defaultMaxExponentialBackoffDelay                = time.Second * 1000
@@ -75,9 +76,9 @@ type ControllerConfig struct {
 	// EnableBackendSecurityGroup specifies whether to use optimized security group rules
 	EnableBackendSecurityGroup bool
 
-	// BackendSecurityGroups specifies the configured backend security groups to use
+	// BackendSecurityGroups specifies the configured backend security group to use
 	// for optimized security group rules
-	BackendSecurityGroups []string
+	BackendSecurityGroup string
 }
 
 // BindFlags binds the command line flags to the fields in the config object
@@ -99,8 +100,8 @@ func (cfg *ControllerConfig) BindFlags(fs *pflag.FlagSet) {
 		"Default SSL policy for load balancers listeners")
 	fs.BoolVar(&cfg.EnableBackendSecurityGroup, flagEnableBackendSG, defaultEnableBackendSG,
 		"Enable sharing of security groups for backend traffic")
-	fs.StringSliceVar(&cfg.BackendSecurityGroups, flagBackendSecurityGroups, nil,
-		"Backend security groups to use for application traffic")
+	fs.StringVar(&cfg.BackendSecurityGroup, flagBackendSecurityGroup, "",
+		"Backend security group id to use for the ingress rules on the worker node SG")
 
 	cfg.AWSConfig.BindFlags(fs)
 	cfg.RuntimeConfig.BindFlags(fs)
@@ -125,6 +126,9 @@ func (cfg *ControllerConfig) Validate() error {
 	if err := cfg.validateExternalManagedTagsCollisionWithDefaultTags(); err != nil {
 		return err
 	}
+	if err := cfg.validateBackendSecurityGroupConfiguration(); err != nil {
+		return err
+	}
 	return nil
 }
 
@@ -155,3 +159,13 @@ func (cfg *ControllerConfig) validateExternalManagedTagsCollisionWithDefaultTags
 	}
 	return nil
 }
+
+func (cfg *ControllerConfig) validateBackendSecurityGroupConfiguration() error {
+	if len(cfg.BackendSecurityGroup) == 0 {
+		return nil
+	}
+	if !strings.HasPrefix(cfg.BackendSecurityGroup, "sg-") {
+		return errors.Errorf("invalid value %v for backend security group id", cfg.BackendSecurityGroup)
+	}
+	return nil
+}

pkg/ingress/model_build_load_balancer.go

@@ -242,64 +242,76 @@ func (t *defaultModelBuildTask) buildLoadBalancerSubnetMappings(ctx context.Cont
 }
 
 func (t *defaultModelBuildTask) buildLoadBalancerSecurityGroups(ctx context.Context, listenPortConfigByPort map[int64]listenPortConfig, ipAddressType elbv2model.IPAddressType) ([]core.StringToken, error) {
-	var explicitSGNameOrIDsList [][]string
-	for _, member := range t.ingGroup.Members {
-		var rawSGNameOrIDs []string
-		if exists := t.annotationParser.ParseStringSliceAnnotation(annotations.IngressSuffixSecurityGroups, &rawSGNameOrIDs, member.Ing.Annotations); !exists {
-			continue
-		}
-		explicitSGNameOrIDsList = append(explicitSGNameOrIDsList, rawSGNameOrIDs)
+	sgNameOrIDsViaAnnotation, err := t.buildFrontendSGNameOrIDsFromAnnotation(ctx)
+	if err != nil {
+		return nil, err
 	}
-	if len(explicitSGNameOrIDsList) == 0 {
-		sg, err := t.buildManagedSecurityGroup(ctx, listenPortConfigByPort, ipAddressType)
+	var lbSGTokens []core.StringToken
+	if len(sgNameOrIDsViaAnnotation) == 0 {
+		managedSG, err := t.buildManagedSecurityGroup(ctx, listenPortConfigByPort, ipAddressType)
 		if err != nil {
 			return nil, err
 		}
-		sgs := []core.StringToken{sg.GroupID()}
-
-		t.manageBackendSecurityGroupRules = true
-		t.backendSecurityGroups, err = t.backendSGProvider.Get(ctx)
+		lbSGTokens = append(lbSGTokens, managedSG.GroupID())
+		if !t.enableBackendSG {
+			t.backendSGIDToken = managedSG.GroupID()
+		} else {
+			backendSGID, err := t.backendSGProvider.Get(ctx)
+			if err != nil {
+				return nil, err
+			}
+			t.backendSGIDToken = core.LiteralStringToken((backendSGID))
+			lbSGTokens = append(lbSGTokens, t.backendSGIDToken)
+		}
+		t.logger.Info("Auto Create SG", "LB SGs", lbSGTokens, "backend SG", t.backendSGIDToken)
+	} else {
+		manageBackendSGRules, err := t.buildManageSecurityGroupRulesFlag(ctx)
+		if err != nil {
+			return nil, err
+		}
+		frontendSGIDs, err := t.resolveSecurityGroupIDsViaNameOrIDSlice(ctx, sgNameOrIDsViaAnnotation)
 		if err != nil {
 			return nil, err
 		}
-		for _, sg := range t.backendSecurityGroups {
-			sgs = append(sgs, core.LiteralStringToken(sg))
+		for _, sgID := range frontendSGIDs {
+			lbSGTokens = append(lbSGTokens, core.LiteralStringToken(sgID))
 		}
-		t.logger.Info("Auto create SG", "SGs", sgs, "backend SGs", t.backendSecurityGroups)
-		return sgs, nil
+
+		if manageBackendSGRules {
+			if !t.enableBackendSG {
+				return nil, errors.New("backendSG feature is required to manage worker node SG rules when frontendSG manually specified")
+			}
+			backendSGID, err := t.backendSGProvider.Get(ctx)
+			if err != nil {
+				return nil, err
+			}
+			t.backendSGIDToken = core.LiteralStringToken(backendSGID)
+			lbSGTokens = append(lbSGTokens, t.backendSGIDToken)
+		}
+		t.logger.Info("SG configured via annotation", "LB SGs", lbSGTokens, "backend SG", t.backendSGIDToken)
 	}
+	return lbSGTokens, nil
+}
 
+func (t *defaultModelBuildTask) buildFrontendSGNameOrIDsFromAnnotation(ctx context.Context) ([]string, error) {
+	var explicitSGNameOrIDsList [][]string
+	for _, member := range t.ingGroup.Members {
+		var rawSGNameOrIDs []string
+		if exists := t.annotationParser.ParseStringSliceAnnotation(annotations.IngressSuffixSecurityGroups, &rawSGNameOrIDs, member.Ing.Annotations); !exists {
+			continue
+		}
+		explicitSGNameOrIDsList = append(explicitSGNameOrIDsList, rawSGNameOrIDs)
+	}
+	if len(explicitSGNameOrIDsList) == 0 {
+		return nil, nil
+	}
 	chosenSGNameOrIDs := explicitSGNameOrIDsList[0]
 	for _, sgNameOrIDs := range explicitSGNameOrIDsList[1:] {
-		// securityGroups order might matters in the future(e.g. use the first securityGroup for traffic to nodeGroups)
 		if !cmp.Equal(chosenSGNameOrIDs, sgNameOrIDs) {
 			return nil, errors.Errorf("conflicting securityGroups: %v | %v", chosenSGNameOrIDs, sgNameOrIDs)
 		}
 	}
-	manageBackendSGRules, err := t.buildManageSecurityGroupRulesFlag(ctx)
-	if err != nil {
-		return nil, err
-	}
-	if manageBackendSGRules {
-		t.manageBackendSecurityGroupRules = true
-		t.backendSecurityGroups, err = t.backendSGProvider.Get(ctx)
-		if err != nil {
-			return nil, err
-		}
-		if len(t.backendSecurityGroups) == 0 {
-			return nil, errors.New("no backend sercurity groups configured")
-		}
-		chosenSGNameOrIDs = append(chosenSGNameOrIDs, t.backendSecurityGroups...)
-	}
-	chosenSGIDs, err := t.resolveSecurityGroupIDsViaNameOrIDSlice(ctx, chosenSGNameOrIDs)
-	if err != nil {
-		return nil, err
-	}
-	sgIDTokens := make([]core.StringToken, 0, len(chosenSGIDs))
-	for _, sgID := range chosenSGIDs {
-		sgIDTokens = append(sgIDTokens, core.LiteralStringToken(sgID))
-	}
-	return sgIDTokens, nil
+	return chosenSGNameOrIDs, nil
 }
 
 func (t *defaultModelBuildTask) buildLoadBalancerCOIPv4Pool(_ context.Context) (*string, error) {

pkg/ingress/model_build_managed_sg.go

@@ -24,7 +24,6 @@ func (t *defaultModelBuildTask) buildManagedSecurityGroup(ctx context.Context, l
 	}
 
 	sg := ec2model.NewSecurityGroup(t.stack, resourceIDManagedSecurityGroup, sgSpec)
-	t.managedSG = sg
 	return sg, nil
 }
 

pkg/ingress/model_build_target_group.go

@@ -6,7 +6,6 @@ import (
 	"encoding/hex"
 	"fmt"
 	"regexp"
-	"sigs.k8s.io/aws-load-balancer-controller/pkg/model/core"
 	"strconv"
 
 	"github.com/pkg/errors"
@@ -76,30 +75,20 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context,
 }
 
 func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Context) *elbv2model.TargetGroupBindingNetworking {
-	if !t.manageBackendSecurityGroupRules {
+	if t.backendSGIDToken == nil {
 		return nil
 	}
-	var networkingPeers []elbv2model.NetworkingPeer
-	if len(t.backendSecurityGroups) > 0 {
-		for _, sg := range t.backendSecurityGroups {
-			networkingPeers = append(networkingPeers, elbv2model.NetworkingPeer{
-				SecurityGroup: &elbv2model.SecurityGroup{
-					GroupID: core.LiteralStringToken(sg),
-				},
-			})
-		}
-	} else {
-		networkingPeers = append(networkingPeers, elbv2model.NetworkingPeer{
-			SecurityGroup: &elbv2model.SecurityGroup{
-				GroupID: t.managedSG.GroupID(),
-			},
-		})
-	}
 	protocolTCP := elbv2api.NetworkingProtocolTCP
 	return &elbv2model.TargetGroupBindingNetworking{
 		Ingress: []elbv2model.NetworkingIngressRule{
 			{
-				From: networkingPeers,
+				From: []elbv2model.NetworkingPeer{
+					{
+						SecurityGroup: &elbv2model.SecurityGroup{
+							GroupID: t.backendSGIDToken,
+						},
+					},
+				},
 				Ports: []elbv2api.NetworkingPort{
 					{
 						Protocol: &protocolTCP,

pkg/ingress/model_builder.go

@@ -17,7 +17,6 @@ import (
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/deploy/tracking"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/model/core"
-	ec2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/ec2"
 	elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
 	networkingpkg "sigs.k8s.io/aws-load-balancer-controller/pkg/networking"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -41,7 +40,7 @@ func NewDefaultModelBuilder(k8sClient client.Client, eventRecorder record.EventR
 	authConfigBuilder AuthConfigBuilder, enhancedBackendBuilder EnhancedBackendBuilder,
 	trackingProvider tracking.Provider, elbv2TaggingManager elbv2deploy.TaggingManager,
 	vpcID string, clusterName string, defaultTags map[string]string, externalManagedTags []string, defaultSSLPolicy string,
-	backendSGProvider networkingpkg.BackendSGProvider, logger logr.Logger) *defaultModelBuilder {
+	backendSGProvider networkingpkg.BackendSGProvider, enableBackendSG bool, logger logr.Logger) *defaultModelBuilder {
 	certDiscovery := NewACMCertDiscovery(acmClient, logger)
 	ruleOptimizer := NewDefaultRuleOptimizer(logger)
 	return &defaultModelBuilder{
@@ -62,6 +61,7 @@ func NewDefaultModelBuilder(k8sClient client.Client, eventRecorder record.EventR
 		defaultTags:            defaultTags,
 		externalManagedTags:    sets.NewString(externalManagedTags...),
 		defaultSSLPolicy:       defaultSSLPolicy,
+		enableBackendSG:        enableBackendSG,
 		logger:                 logger,
 	}
 }
@@ -89,6 +89,7 @@ type defaultModelBuilder struct {
 	defaultTags            map[string]string
 	externalManagedTags    sets.String
 	defaultSSLPolicy       string
+	enableBackendSG        bool
 
 	logger logr.Logger
 }
@@ -112,6 +113,7 @@ func (b *defaultModelBuilder) Build(ctx context.Context, ingGroup Group) (core.S
 		elbv2TaggingManager:    b.elbv2TaggingManager,
 		backendSGProvider:      b.backendSGProvider,
 		logger:                 b.logger,
+		enableBackendSG:        b.enableBackendSG,
 
 		ingGroup: ingGroup,
 		stack:    stack,
@@ -161,11 +163,11 @@ type defaultModelBuildTask struct {
 	elbv2TaggingManager    elbv2deploy.TaggingManager
 	logger                 logr.Logger
 
-	ingGroup                        Group
-	sslRedirectConfig               *SSLRedirectConfig
-	stack                           core.Stack
-	backendSecurityGroups           []string
-	manageBackendSecurityGroupRules bool
+	ingGroup          Group
+	sslRedirectConfig *SSLRedirectConfig
+	stack             core.Stack
+	backendSGIDToken  core.StringToken
+	enableBackendSG   bool
 
 	defaultTags                               map[string]string
 	externalManagedTags                       sets.String
@@ -185,7 +187,6 @@ type defaultModelBuildTask struct {
 	defaultHealthCheckMatcherGRPCCode         string
 
 	loadBalancer    *elbv2model.LoadBalancer
-	managedSG       *ec2model.SecurityGroup
 	tgByResID       map[string]*elbv2model.TargetGroup
 	backendServices map[types.NamespacedName]*corev1.Service
 }
@@ -400,7 +401,7 @@ func (t *defaultModelBuildTask) buildManageSecurityGroupRulesFlag(_ context.Cont
 		return false, nil
 	}
 	if len(explicitManageSGRulesFlag) > 1 {
-		return false, errors.New("conflicting enable managed security group rules")
+		return false, errors.New("conflicting manage backend security group rules settings")
 	}
 	return manageSGRules, nil
 }