Merge pull request #3147 from carflo/abitrary-tags-for-nlb-sg

k8s-ci-robot · web-flow · commit 074beee570a0 · 2023-08-31T11:32:48.000-07:00
Enables providing multiple tags for worker node security group discovery
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -7,7 +7,7 @@ _As contributors and maintainers of this project, and in the interest of fosteri
 ## Getting Started
 
 ### Building the project
-[Controller developement documentation](/docs/controller-devel.md) has instructions on how to build the project and project specific expectations.
+[Controller development documentation](/docs/controller-devel.md) has instructions on how to build the project and project specific expectations.
 
 ### Contributing to docs
 
diff --git a/docs/guide/service/nlb.md b/docs/guide/service/nlb.md
@@ -3,8 +3,8 @@
 The AWS Load Balancer Controller (LBC) supports reconciliation for Kubernetes Service resources of type `LoadBalancer` by provisioning an AWS Network Load Balancer (NLB) with an `instance` or `ip` [target type](https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-target-groups.html#target-type).
 
 !!! info "Secure by default"
-    Since the [:octicons-tag-24: v2.2.0](https://github.com/kubernetes-sigs/aws-load-balancer-controller/releases/tag/v2.2.0) release, the LBC provisions an `internal` NLB by default. 
-    
+    Since the [:octicons-tag-24: v2.2.0](https://github.com/kubernetes-sigs/aws-load-balancer-controller/releases/tag/v2.2.0) release, the LBC provisions an `internal` NLB by default.
+
     To create an `internet-facing` NLB, the following annotation is required on your service:
 
     ```yaml
@@ -28,16 +28,16 @@ The AWS Load Balancer Controller (LBC) supports reconciliation for Kubernetes Se
 
 ## Configuration
 
-By default, Kubernetes Service resources of type `LoadBalancer` get reconciled by the Kubernetes controller built into the `CloudProvider` component of the `kube-controller-manager` or the `cloud-controller-manager`(also known as the in-tree controller). 
+By default, Kubernetes Service resources of type `LoadBalancer` get reconciled by the Kubernetes controller built into the `CloudProvider` component of the `kube-controller-manager` or the `cloud-controller-manager`(also known as the in-tree controller).
 
 In order for the LBC to manage the reconciliation of Kubernetes Service resources of type `LoadBalancer`, you need to offload the reconciliation from the in-tree controller to the LBC, explicitly.
 
 
 === "With LoadBalancerClass"
-    The LBC supports the `LoadBalancerClass` feature since the [:octicons-tag-24: v2.4.0](https://github.com/kubernetes-sigs/aws-load-balancer-controller/releases/tag/v2.4.0) release for Kubernetes v1.22+ clusters. 
-     
+    The LBC supports the `LoadBalancerClass` feature since the [:octicons-tag-24: v2.4.0](https://github.com/kubernetes-sigs/aws-load-balancer-controller/releases/tag/v2.4.0) release for Kubernetes v1.22+ clusters.
+
     The `LoadBalancerClass` feature provides a `CloudProvider` agnostic way of offloading the reconciliation for Kubernetes Service resources of type `LoadBalancer` to an external controller.
-    
+
     When you specify the `spec.loadBalancerClass` to be `service.k8s.aws/nlb` on a Kubernetes Service resource of type `LoadBalancer`, the LBC takes charge of reconciliation by provisioning an NLB.
 
     !!! warning
@@ -89,22 +89,22 @@ In order for the LBC to manage the reconciliation of Kubernetes Service resource
         ```
 
 === "With `service.beta.kubernetes.io/aws-load-balancer-type` annotation"
-    The AWS in-tree controller supports an AWS specific way of offloading the reconciliation for Kubernetes Service resources of type `LoadBalancer` to an external controller. 
+    The AWS in-tree controller supports an AWS specific way of offloading the reconciliation for Kubernetes Service resources of type `LoadBalancer` to an external controller.
 
     When you specify the [`service.beta.kubernetes.io/aws-load-balancer-type` annotation](./annotations.md#lb-type) to be `external` on a Kubernetes Service resource of type `LoadBalancer`, the in-tree controller ignores the Service resource. In addition, if you specify the [`service.beta.kubernetes.io/aws-load-balancer-nlb-target-type` annotation](./annotations.md#nlb-target-type) on the Service resource, the LBC takes charge of reconciliation by provisioning an NLB.
 
     !!! warning
         - It's not recommended to modify or add the `service.beta.kubernetes.io/aws-load-balancer-type` annotation on an existing Service resource. If a change is desired, delete the existing Service resource and create a new one instead of modifying an existing Service.
 
-        - If you modify this annotation on an existing Service resource, you might end up with leaked LBC resources. 
+        - If you modify this annotation on an existing Service resource, you might end up with leaked LBC resources.
 
     !!! note "backwards compatibility for `nlb-ip` type"
         For backwards compatibility, both the in-tree and LBC controller supports `nlb-ip` as a value for the `service.beta.kubernetes.io/aws-load-balancer-type` annotation. The controllers treats it as if you specified both of the following annotations:
         ```
         service.beta.kubernetes.io/aws-load-balancer-type: external
         service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: ip
         ```
-    
+
     !!! example "Example: instance mode"
         ```yaml hl_lines="6 7"
         apiVersion: v1
@@ -144,7 +144,7 @@ In order for the LBC to manage the reconciliation of Kubernetes Service resource
         ```
 
 ## Protocols
-The LBC supports both TCP and UDP protocols. The controller also configures TLS termination on your NLB if you configure the Service with a certificate annotation. 
+The LBC supports both TCP and UDP protocols. The controller also configures TLS termination on your NLB if you configure the Service with a certificate annotation.
 
 In the case of TCP, an NLB with IP targets doesn't pass the client source IP address, unless you specifically configure it to using target group attributes. Your application pods might not see the actual client IP address, even if the NLB passes it along. For example, if you're using instance mode with `externalTrafficPolicy` set to `Cluster`.
 In such cases, you can configure [NLB proxy protocol v2](https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-target-groups.html#proxy-protocol) using an [annotation](https://kubernetes.io/docs/concepts/services-networking/service/#proxy-protocol-support-on-aws) if you need visibility into
@@ -183,6 +183,8 @@ The controller automatically selects the worker node security groups that it mod
 
     `${cluster-name}` is the name of the Kubernetes cluster.
 
+If it is possible for multiple security groups with the tag `kubernetes.io/cluster/${cluster-name}` to be on a target ENI, you may use the `--service-target-eni-security-group-tags` flag to specify additional tags that must also match in order for a security group to be used.
+
 ### Worker node security groups rules
 
 === "When client IP preservation is enabled"
diff --git a/main.go b/main.go
@@ -108,7 +108,7 @@ func main() {
 	tgbResManager := targetgroupbinding.NewDefaultResourceManager(mgr.GetClient(), cloud.ELBV2(), cloud.EC2(),
 		podInfoRepo, sgManager, sgReconciler, vpcInfoProvider,
 		cloud.VpcID(), controllerCFG.ClusterName, controllerCFG.FeatureGates.Enabled(config.EndpointsFailOpen), controllerCFG.EnableEndpointSlices, controllerCFG.DisableRestrictedSGRules,
-		mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log)
+		controllerCFG.ServiceTargetENISGTags, mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log)
 	backendSGProvider := networking.NewBackendSGProvider(controllerCFG.ClusterName, controllerCFG.BackendSecurityGroup,
 		cloud.VpcID(), cloud.EC2(), mgr.GetClient(), controllerCFG.DefaultTags, ctrl.Log.WithName("backend-sg-provider"))
 	sgResolver := networking.NewDefaultSecurityGroupResolver(cloud.EC2(), cloud.VpcID())
diff --git a/pkg/config/controller_config.go b/pkg/config/controller_config.go
@@ -18,6 +18,7 @@ const (
 	flagDefaultTags                                  = "default-tags"
 	flagDefaultTargetType                            = "default-target-type"
 	flagExternalManagedTags                          = "external-managed-tags"
+	flagServiceTargetENISGTags                       = "service-target-eni-security-group-tags"
 	flagServiceMaxConcurrentReconciles               = "service-max-concurrent-reconciles"
 	flagTargetGroupBindingMaxConcurrentReconciles    = "targetgroupbinding-max-concurrent-reconciles"
 	flagTargetGroupBindingMaxExponentialBackoffDelay = "targetgroupbinding-max-exponential-backoff-delay"
@@ -74,6 +75,9 @@ type ControllerConfig struct {
 	// List of Tag keys on AWS resources that will be managed externally.
 	ExternalManagedTags []string
 
+	// ServiceTargetENISGTags are AWS tags, in addition to the cluster tags, for finding the target ENI security group to which to add inbound rules from NLBs.
+	ServiceTargetENISGTags map[string]string
+
 	// Default SSL Policy that will be applied to all ingresses or services that do not have
 	// the SSL Policy annotation.
 	DefaultSSLPolicy string
@@ -128,7 +132,8 @@ func (cfg *ControllerConfig) BindFlags(fs *pflag.FlagSet) {
 		"Enable EndpointSlices for IP targets instead of Endpoints")
 	fs.BoolVar(&cfg.DisableRestrictedSGRules, flagDisableRestrictedSGRules, defaultDisableRestrictedSGRules,
 		"Disable the usage of restricted security group rules")
-
+	fs.StringToStringVar(&cfg.ServiceTargetENISGTags, flagServiceTargetENISGTags, nil,
+		"AWS Tags, in addition to cluster tags, for finding the target ENI security group to which to add inbound rules from NLBs")
 	cfg.FeatureGates.BindFlags(fs)
 	cfg.AWSConfig.BindFlags(fs)
 	cfg.RuntimeConfig.BindFlags(fs)
diff --git a/pkg/targetgroupbinding/networking_manager.go b/pkg/targetgroupbinding/networking_manager.go
@@ -46,17 +46,18 @@ type NetworkingManager interface {
 
 // NewDefaultNetworkingManager constructs defaultNetworkingManager.
 func NewDefaultNetworkingManager(k8sClient client.Client, podENIResolver networking.PodENIInfoResolver, nodeENIResolver networking.NodeENIInfoResolver,
-	sgManager networking.SecurityGroupManager, sgReconciler networking.SecurityGroupReconciler, vpcID string, clusterName string, logger logr.Logger, disabledRestrictedSGRulesFlag bool) *defaultNetworkingManager {
+	sgManager networking.SecurityGroupManager, sgReconciler networking.SecurityGroupReconciler, vpcID string, clusterName string, serviceTargetENISGTags map[string]string, logger logr.Logger, disabledRestrictedSGRulesFlag bool) *defaultNetworkingManager {
 
 	return &defaultNetworkingManager{
-		k8sClient:       k8sClient,
-		podENIResolver:  podENIResolver,
-		nodeENIResolver: nodeENIResolver,
-		sgManager:       sgManager,
-		sgReconciler:    sgReconciler,
-		vpcID:           vpcID,
-		clusterName:     clusterName,
-		logger:          logger,
+		k8sClient:              k8sClient,
+		podENIResolver:         podENIResolver,
+		nodeENIResolver:        nodeENIResolver,
+		sgManager:              sgManager,
+		sgReconciler:           sgReconciler,
+		vpcID:                  vpcID,
+		clusterName:            clusterName,
+		serviceTargetENISGTags: serviceTargetENISGTags,
+		logger:                 logger,
 
 		mutex:                         sync.Mutex{},
 		ingressPermissionsPerSGByTGB:  make(map[types.NamespacedName]map[string][]networking.IPPermissionInfo),
@@ -68,14 +69,15 @@ func NewDefaultNetworkingManager(k8sClient client.Client, podENIResolver network
 
 // default implementation for NetworkingManager.
 type defaultNetworkingManager struct {
-	k8sClient       client.Client
-	podENIResolver  networking.PodENIInfoResolver
-	nodeENIResolver networking.NodeENIInfoResolver
-	sgManager       networking.SecurityGroupManager
-	sgReconciler    networking.SecurityGroupReconciler
-	vpcID           string
-	clusterName     string
-	logger          logr.Logger
+	k8sClient              client.Client
+	podENIResolver         networking.PodENIInfoResolver
+	nodeENIResolver        networking.NodeENIInfoResolver
+	sgManager              networking.SecurityGroupManager
+	sgReconciler           networking.SecurityGroupReconciler
+	vpcID                  string
+	clusterName            string
+	serviceTargetENISGTags map[string]string
+	logger                 logr.Logger
 
 	// mutex will serialize our TargetGroup's networking reconcile requests.
 	mutex sync.Mutex
@@ -526,19 +528,33 @@ func (m *defaultNetworkingManager) resolveEndpointSGForENI(ctx context.Context,
 		return "", err
 	}
 	clusterResourceTagKey := fmt.Sprintf("kubernetes.io/cluster/%s", m.clusterName)
-	sgIDsWithClusterTag := sets.NewString()
+	sgIDsWithMatchingEndpointSGTags := sets.NewString()
 	for sgID, sgInfo := range sgInfoByID {
 		if _, ok := sgInfo.Tags[clusterResourceTagKey]; ok {
-			sgIDsWithClusterTag.Insert(sgID)
+			matchesEndpointSGTags := true
+			for endpointSGTagKey, endpointSGTagValue := range m.serviceTargetENISGTags {
+				if _, ok := sgInfo.Tags[endpointSGTagKey]; !ok || sgInfo.Tags[endpointSGTagKey] != endpointSGTagValue {
+					matchesEndpointSGTags = false
+					break
+				}
+			}
+			if matchesEndpointSGTags {
+				sgIDsWithMatchingEndpointSGTags.Insert(sgID)
+			}
 		}
 	}
-	if len(sgIDsWithClusterTag) != 1 {
+
+	if len(sgIDsWithMatchingEndpointSGTags) != 1 {
 		// user may provide incorrect `--cluster-name` at bootstrap or modify the tag key unexpectedly, it is hard to find out if no clusterName included in error message.
 		// having `clusterName` included in error message might be helpful for shorten the troubleshooting time spent.
-		return "", errors.Errorf("expect exactly one securityGroup tagged with %v for eni %v, got: %v (clusterName: %v)",
-			clusterResourceTagKey, eniInfo.NetworkInterfaceID, sgIDsWithClusterTag.List(), m.clusterName)
+		if len(m.serviceTargetENISGTags) == 0 {
+			return "", errors.Errorf("expected exactly one securityGroup tagged with %v for eni %v, got: %v (clusterName: %v)",
+				clusterResourceTagKey, eniInfo.NetworkInterfaceID, sgIDsWithMatchingEndpointSGTags.List(), m.clusterName)
+		}
+		return "", errors.Errorf("expected exactly one securityGroup tagged with %v and %v for eni %v, got: %v (clusterName: %v)",
+			clusterResourceTagKey, m.serviceTargetENISGTags, eniInfo.NetworkInterfaceID, sgIDsWithMatchingEndpointSGTags.List(), m.clusterName)
 	}
-	sgID, _ := sgIDsWithClusterTag.PopAny()
+	sgID, _ := sgIDsWithMatchingEndpointSGTags.PopAny()
 	return sgID, nil
 }
 
diff --git a/pkg/targetgroupbinding/networking_manager_test.go b/pkg/targetgroupbinding/networking_manager_test.go
diff --git a/pkg/targetgroupbinding/resource_manager.go b/pkg/targetgroupbinding/resource_manager.go
