From 75e6587783c558cf0aff84b624ca9849bfe29e4d Mon Sep 17 00:00:00 2001 From: oliviassss Date: Fri, 18 Oct 2024 12:36:45 -0700 Subject: [PATCH 1/4] add flag to specify resource prefix for resource tags --- controllers/ingress/group_controller.go | 7 +- controllers/service/service_controller.go | 5 +- pkg/config/controller_config.go | 102 ++++++++++++++---- pkg/config/controller_config_test.go | 84 ++++++++++++++- pkg/deploy/stack_deployer.go | 4 +- pkg/deploy/tracking/provider.go | 35 +++--- pkg/deploy/tracking/provider_test.go | 24 ++--- pkg/ingress/model_build_load_balancer_test.go | 2 +- pkg/ingress/model_builder_test.go | 2 +- pkg/service/model_build_load_balancer_test.go | 10 +- pkg/service/model_builder_test.go | 2 +- 11 files changed, 209 insertions(+), 68 deletions(-) diff --git a/controllers/ingress/group_controller.go b/controllers/ingress/group_controller.go index 175bbb6906..9bad1c759e 100644 --- a/controllers/ingress/group_controller.go +++ b/controllers/ingress/group_controller.go @@ -34,8 +34,7 @@ import ( ) const ( - ingressTagPrefix = "ingress.k8s.aws" - controllerName = "ingress" + controllerName = "ingress" // the groupVersion of used Ingress & IngressClass resource. ingressResourcesGroupVersion = "networking.k8s.io/v1" @@ -53,7 +52,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder authConfigBuilder := ingress.NewDefaultAuthConfigBuilder(annotationParser) enhancedBackendBuilder := ingress.NewDefaultEnhancedBackendBuilder(k8sClient, annotationParser, authConfigBuilder, controllerConfig.IngressConfig.TolerateNonExistentBackendService, controllerConfig.IngressConfig.TolerateNonExistentBackendAction) referenceIndexer := ingress.NewDefaultReferenceIndexer(enhancedBackendBuilder, authConfigBuilder, logger) - trackingProvider := tracking.NewDefaultProvider(ingressTagPrefix, controllerConfig.ClusterName) + trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.IngressTagPrefixKey], controllerConfig.ClusterName) modelBuilder := ingress.NewDefaultModelBuilder(k8sClient, eventRecorder, cloud.EC2(), cloud.ELBV2(), cloud.ACM(), annotationParser, subnetsResolver, @@ -63,7 +62,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder controllerConfig.EnableBackendSecurityGroup, controllerConfig.DisableRestrictedSGRules, controllerConfig.IngressConfig.AllowedCertificateAuthorityARNs, controllerConfig.FeatureGates.Enabled(config.EnableIPTargetType), logger) stackMarshaller := deploy.NewDefaultStackMarshaller() stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, - controllerConfig, ingressTagPrefix, logger) + controllerConfig, controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.IngressTagPrefixKey], logger) classLoader := ingress.NewDefaultClassLoader(k8sClient, true) classAnnotationMatcher := ingress.NewDefaultClassAnnotationMatcher(controllerConfig.IngressConfig.IngressClass) manageIngressesWithoutIngressClass := controllerConfig.IngressConfig.IngressClass == "" diff --git a/controllers/service/service_controller.go b/controllers/service/service_controller.go index 2ed7612b01..4972e43928 100644 --- a/controllers/service/service_controller.go +++ b/controllers/service/service_controller.go @@ -29,7 +29,6 @@ import ( const ( serviceFinalizer = "service.k8s.aws/resources" - serviceTagPrefix = "service.k8s.aws" serviceAnnotationPrefix = "service.beta.kubernetes.io" controllerName = "service" ) @@ -41,14 +40,14 @@ func NewServiceReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorde backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, logger logr.Logger) *serviceReconciler { annotationParser := annotations.NewSuffixAnnotationParser(serviceAnnotationPrefix) - trackingProvider := tracking.NewDefaultProvider(serviceTagPrefix, controllerConfig.ClusterName) + trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.ServiceTagPrefixKey], controllerConfig.ClusterName) serviceUtils := service.NewServiceUtils(annotationParser, serviceFinalizer, controllerConfig.ServiceConfig.LoadBalancerClass, controllerConfig.FeatureGates) modelBuilder := service.NewDefaultModelBuilder(annotationParser, subnetsResolver, vpcInfoProvider, cloud.VpcID(), trackingProvider, elbv2TaggingManager, cloud.EC2(), controllerConfig.FeatureGates, controllerConfig.ClusterName, controllerConfig.DefaultTags, controllerConfig.ExternalManagedTags, controllerConfig.DefaultSSLPolicy, controllerConfig.DefaultTargetType, controllerConfig.FeatureGates.Enabled(config.EnableIPTargetType), serviceUtils, backendSGProvider, sgResolver, controllerConfig.EnableBackendSecurityGroup, controllerConfig.DisableRestrictedSGRules, logger) stackMarshaller := deploy.NewDefaultStackMarshaller() - stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, controllerConfig, serviceTagPrefix, logger) + stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, controllerConfig, controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.ServiceTagPrefixKey], logger) return &serviceReconciler{ k8sClient: k8sClient, eventRecorder: eventRecorder, diff --git a/pkg/config/controller_config.go b/pkg/config/controller_config.go index 7d6b42e44d..da23df14f6 100644 --- a/pkg/config/controller_config.go +++ b/pkg/config/controller_config.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "strings" "time" @@ -16,6 +17,7 @@ const ( flagLogLevel = "log-level" flagK8sClusterName = "cluster-name" flagDefaultTags = "default-tags" + flagResourcePrefix = "resource-prefix" flagDefaultTargetType = "default-target-type" flagExternalManagedTags = "external-managed-tags" flagServiceTargetENISGTags = "service-target-eni-security-group-tags" @@ -27,24 +29,42 @@ const ( flagBackendSecurityGroup = "backend-security-group" flagEnableEndpointSlices = "enable-endpoint-slices" flagDisableRestrictedSGRules = "disable-restricted-sg-rules" - defaultLogLevel = "info" - defaultMaxConcurrentReconciles = 3 - defaultMaxExponentialBackoffDelay = time.Second * 1000 - defaultSSLPolicy = "ELBSecurityPolicy-2016-08" - defaultEnableBackendSG = true - defaultEnableEndpointSlices = false - defaultDisableRestrictedSGRules = false + + ClusterTagPrefixKey = "clusterTagPrefix" + IngressTagPrefixKey = "ingressTagPrefix" + ServiceTagPrefixKey = "serviceTagPrefix" + BackendSGNamePrefixKey = "backendSGNamePrefix" + ClusterSgRuleLabelPrefixKey = "clusterSgRuleLabelPrefix" + + defaultClusterTagPrefix = "elbv2.k8s.aws" + defaultIngressTagPrefix = "ingress.k8s.aws" + defaultServiceTagPrefix = "service.k8s.aws" + defaultBackendSGNamePrefix = "k8s-traffic" + defaultClusterSgRuleLabelPrefix = "elbv2.k8s.aws" + defaultLogLevel = "info" + defaultMaxConcurrentReconciles = 3 + defaultMaxExponentialBackoffDelay = time.Second * 1000 + defaultSSLPolicy = "ELBSecurityPolicy-2016-08" + defaultEnableBackendSG = true + defaultEnableEndpointSlices = false + defaultDisableRestrictedSGRules = false ) var ( - trackingTagKeys = sets.NewString( - "elbv2.k8s.aws/cluster", - "elbv2.k8s.aws/resource", - "ingress.k8s.aws/stack", - "ingress.k8s.aws/resource", - "service.k8s.aws/stack", - "service.k8s.aws/resource", + validPrefixKeys = sets.NewString( + ClusterTagPrefixKey, + IngressTagPrefixKey, + ServiceTagPrefixKey, + BackendSGNamePrefixKey, + ClusterSgRuleLabelPrefixKey, ) + defaultResourcePrefix = map[string]string{ + ClusterTagPrefixKey: defaultClusterTagPrefix, + IngressTagPrefixKey: defaultIngressTagPrefix, + ServiceTagPrefixKey: defaultServiceTagPrefix, + BackendSGNamePrefixKey: defaultBackendSGNamePrefix, + ClusterSgRuleLabelPrefixKey: defaultClusterSgRuleLabelPrefix, + } ) // ControllerConfig contains the controller configuration @@ -69,6 +89,9 @@ type ControllerConfig struct { // Default AWS Tags that will be applied to all AWS resources managed by this controller. DefaultTags map[string]string + // ResourcePrefix provides prefix for resource tags, backend SG name and worker node SG rules label. + ResourcePrefix map[string]string + // Default target type for Ingress and Service objects DefaultTargetType string @@ -134,10 +157,13 @@ func (cfg *ControllerConfig) BindFlags(fs *pflag.FlagSet) { "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") + fs.StringToStringVar(&cfg.ResourcePrefix, flagResourcePrefix, defaultResourcePrefix, + "the prefixes for resource tags, backend SG name and worker node SG rules label.") + + cfg.mergeDefaultResourcePrefixVal() cfg.FeatureGates.BindFlags(fs) cfg.AWSConfig.BindFlags(fs) cfg.RuntimeConfig.BindFlags(fs) - cfg.PodWebhookConfig.BindFlags(fs) cfg.IngressConfig.BindFlags(fs) cfg.AddonsConfig.BindFlags(fs) @@ -150,10 +176,23 @@ func (cfg *ControllerConfig) Validate() error { return errors.New("kubernetes cluster name must be specified") } - if err := cfg.validateDefaultTagsCollisionWithTrackingTags(); err != nil { + if err := cfg.validateResourcePrefixKeys(); err != nil { + return err + } + + trackingTagKeys := sets.New[string]( + cfg.ResourcePrefix[ClusterTagPrefixKey]+"/cluster", + cfg.ResourcePrefix[ClusterTagPrefixKey]+"/resource", + cfg.ResourcePrefix[IngressTagPrefixKey]+"/stack", + cfg.ResourcePrefix[IngressTagPrefixKey]+"/resource", + cfg.ResourcePrefix[ServiceTagPrefixKey]+"/stack", + cfg.ResourcePrefix[ServiceTagPrefixKey]+"/resource", + ) + + if err := cfg.validateDefaultTagsCollisionWithTrackingTags(trackingTagKeys); err != nil { return err } - if err := cfg.validateExternalManagedTagsCollisionWithTrackingTags(); err != nil { + if err := cfg.validateExternalManagedTagsCollisionWithTrackingTags(trackingTagKeys); err != nil { return err } if err := cfg.validateExternalManagedTagsCollisionWithDefaultTags(); err != nil { @@ -168,7 +207,7 @@ func (cfg *ControllerConfig) Validate() error { return nil } -func (cfg *ControllerConfig) validateDefaultTagsCollisionWithTrackingTags() error { +func (cfg *ControllerConfig) validateDefaultTagsCollisionWithTrackingTags(trackingTagKeys sets.Set[string]) error { for tagKey := range cfg.DefaultTags { if trackingTagKeys.Has(tagKey) { return errors.Errorf("tag key %v cannot be specified in %v flag", tagKey, flagDefaultTags) @@ -177,7 +216,7 @@ func (cfg *ControllerConfig) validateDefaultTagsCollisionWithTrackingTags() erro return nil } -func (cfg *ControllerConfig) validateExternalManagedTagsCollisionWithTrackingTags() error { +func (cfg *ControllerConfig) validateExternalManagedTagsCollisionWithTrackingTags(trackingTagKeys sets.Set[string]) error { for _, tagKey := range cfg.ExternalManagedTags { if trackingTagKeys.Has(tagKey) { return errors.Errorf("tag key %v cannot be specified in %v flag", tagKey, flagExternalManagedTags) @@ -214,3 +253,28 @@ func (cfg *ControllerConfig) validateBackendSecurityGroupConfiguration() error { } return nil } + +func (cfg *ControllerConfig) validateResourcePrefixKeys() error { + keys := make([]string, 0, len(cfg.ResourcePrefix)) + for key := range cfg.ResourcePrefix { + if !validPrefixKeys.Has(key) { + return fmt.Errorf("invalid key: %s. Valid keys are: %v", key, validPrefixKeys.List()) + } + keys = append(keys, key) + } + if len(keys) != len(validPrefixKeys.List()) { + return fmt.Errorf("invalid number of keys. Expected %d keys, but got %d keys", + len(validPrefixKeys.List()), len(keys)) + } + return nil +} + +// mergeDefaultResourcePrefixVal make sure the ResourcePrefix map always has default val for unspecified key in user-passed flag +func (cfg *ControllerConfig) mergeDefaultResourcePrefixVal() { + // Merge user-provided values with defaults + for key, defaultVal := range defaultResourcePrefix { + if _, exists := cfg.ResourcePrefix[key]; !exists { + cfg.ResourcePrefix[key] = defaultVal + } + } +} diff --git a/pkg/config/controller_config_test.go b/pkg/config/controller_config_test.go index 5b5fc8dbbb..7283118975 100644 --- a/pkg/config/controller_config_test.go +++ b/pkg/config/controller_config_test.go @@ -3,6 +3,7 @@ package config import ( "github.com/pkg/errors" "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/util/sets" "testing" ) @@ -46,7 +47,15 @@ func TestControllerConfig_validateDefaultTagsCollisionWithTrackingTags(t *testin cfg := &ControllerConfig{ DefaultTags: tt.fields.DefaultTags, } - err := cfg.validateDefaultTagsCollisionWithTrackingTags() + trackingTagKeys := sets.New[string]( + "elbv2.k8s.aws/cluster", + "elbv2.k8s.aws/resource", + "ingress.k8s.aws/stack", + "ingress.k8s.aws/resource", + "service.k8s.aws/stack", + "service.k8s.aws/resource", + ) + err := cfg.validateDefaultTagsCollisionWithTrackingTags(trackingTagKeys) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { @@ -92,7 +101,15 @@ func TestControllerConfig_validateExternalManagedTagsCollisionWithTrackingTags(t cfg := &ControllerConfig{ ExternalManagedTags: tt.fields.ExternalManagedTags, } - err := cfg.validateExternalManagedTagsCollisionWithTrackingTags() + trackingTagKeys := sets.New[string]( + "elbv2.k8s.aws/cluster", + "elbv2.k8s.aws/resource", + "ingress.k8s.aws/stack", + "ingress.k8s.aws/resource", + "service.k8s.aws/stack", + "service.k8s.aws/resource", + ) + err := cfg.validateExternalManagedTagsCollisionWithTrackingTags(trackingTagKeys) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { @@ -164,3 +181,66 @@ func TestControllerConfig_validateExternalManagedTagsCollisionWithDefaultTags(t }) } } + +func TestControllerConfig_validateResourcePrefixKeys(t *testing.T) { + type fields struct { + ResourcePrefix map[string]string + } + tests := []struct { + name string + fields fields + wantErr error + }{ + { + name: "resource prefix has all keys", + fields: fields{ + ResourcePrefix: map[string]string{ + "clusterTagPrefix": "elbv2.k8s.aws", + "ingressTagPrefix": "ingress.k8s.aws", + "serviceTagPrefix": "service.k8s.aws", + "backendSGNamePrefix": "k8s-traffic", + "clusterSgRuleLabelPrefix": "elbv2.k8s.aws", + }, + }, + wantErr: nil, + }, + { + name: "resource prefix has some invalid keys", + fields: fields{ + ResourcePrefix: map[string]string{ + "clusterTagPrefix": "elbv2.k8s.aws", + "ingressTagPrefix": "ingress.k8s.aws", + "serviceTagPrefix": "service.k8s.aws", + "backendSGNamePrefix": "k8s-traffic", + "myKey": "myVal", + }, + }, + wantErr: errors.New("invalid key: myKey. Valid keys are: [backendSGNamePrefix clusterSgRuleLabelPrefix clusterTagPrefix ingressTagPrefix serviceTagPrefix]"), + }, + { + name: "resource prefix is missing some valid keys", + fields: fields{ + ResourcePrefix: map[string]string{ + "clusterTagPrefix": "elbv2.k8s.aws", + "ingressTagPrefix": "ingress.k8s.aws", + "serviceTagPrefix": "service.k8s.aws", + "backendSGNamePrefix": "k8s-traffic", + }, + }, + wantErr: errors.New("invalid number of keys. Expected 5 keys, but got 4 keys"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &ControllerConfig{ + ResourcePrefix: tt.fields.ResourcePrefix, + } + err := cfg.validateResourcePrefixKeys() + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/pkg/deploy/stack_deployer.go b/pkg/deploy/stack_deployer.go index dda035adc3..c47fc2c2ef 100644 --- a/pkg/deploy/stack_deployer.go +++ b/pkg/deploy/stack_deployer.go @@ -26,9 +26,9 @@ type StackDeployer interface { func NewDefaultStackDeployer(cloud aws.Cloud, k8sClient client.Client, networkingSGManager networking.SecurityGroupManager, networkingSGReconciler networking.SecurityGroupReconciler, elbv2TaggingManager elbv2.TaggingManager, - config config.ControllerConfig, tagPrefix string, logger logr.Logger) *defaultStackDeployer { + config config.ControllerConfig, clusterTagPrefix string, resourceTagPrefix string, logger logr.Logger) *defaultStackDeployer { - trackingProvider := tracking.NewDefaultProvider(tagPrefix, config.ClusterName) + trackingProvider := tracking.NewDefaultProvider(clusterTagPrefix, resourceTagPrefix, config.ClusterName) ec2TaggingManager := ec2.NewDefaultTaggingManager(cloud.EC2(), networkingSGManager, cloud.VpcID(), logger) return &defaultStackDeployer{ diff --git a/pkg/deploy/tracking/provider.go b/pkg/deploy/tracking/provider.go index 32194e5b3d..e5dc0d373f 100644 --- a/pkg/deploy/tracking/provider.go +++ b/pkg/deploy/tracking/provider.go @@ -32,9 +32,6 @@ import ( // * `service.k8s.aws/stack-namespace: namespace` // * `service.k8s.aws/stack-name: serviceName` -// AWS TagKey for cluster resources. -const clusterNameTagKey = "elbv2.k8s.aws/cluster" - // Legacy AWS TagKey for cluster resources, which is used by AWSALBIngressController(v1.1.3+) const clusterNameTagKeyLegacy = "ingress.k8s.aws/cluster" @@ -63,10 +60,11 @@ type Provider interface { } // NewDefaultProvider constructs defaultProvider -func NewDefaultProvider(tagPrefix string, clusterName string) *defaultProvider { +func NewDefaultProvider(clusterTagPrefix string, resourceTagPrefix string, clusterName string) *defaultProvider { return &defaultProvider{ - tagPrefix: tagPrefix, - clusterName: clusterName, + clusterTagPrefix: clusterTagPrefix, + resourceTagPrefix: resourceTagPrefix, + clusterName: clusterName, } } @@ -74,19 +72,20 @@ var _ Provider = &defaultProvider{} // defaultImplementation for Provider type defaultProvider struct { - tagPrefix string - clusterName string + clusterTagPrefix string + resourceTagPrefix string + clusterName string } func (p *defaultProvider) ResourceIDTagKey() string { - return p.prefixedTrackingKey("resource") + return p.prefixedTrackingKey(p.resourceTagPrefix, "resource") } func (p *defaultProvider) StackTags(stack core.Stack) map[string]string { stackID := stack.StackID() return map[string]string{ - clusterNameTagKey: p.clusterName, - p.prefixedTrackingKey("stack"): stackID.String(), + p.prefixedTrackingKey(p.clusterTagPrefix, "cluster"): p.clusterName, + p.prefixedTrackingKey(p.resourceTagPrefix, "stack"): stackID.String(), } } @@ -102,20 +101,20 @@ func (p *defaultProvider) StackLabels(stack core.Stack) map[string]string { stackID := stack.StackID() if stackID.Namespace == "" { return map[string]string{ - p.prefixedTrackingKey("stack"): stackID.Name, + p.prefixedTrackingKey(p.resourceTagPrefix, "stack"): stackID.Name, } } return map[string]string{ - p.prefixedTrackingKey("stack-namespace"): stackID.Namespace, - p.prefixedTrackingKey("stack-name"): stackID.Name, + p.prefixedTrackingKey(p.resourceTagPrefix, "stack-namespace"): stackID.Namespace, + p.prefixedTrackingKey(p.resourceTagPrefix, "stack-name"): stackID.Name, } } func (p *defaultProvider) StackTagsLegacy(stack core.Stack) map[string]string { stackID := stack.StackID() return map[string]string{ - clusterNameTagKeyLegacy: p.clusterName, - p.prefixedTrackingKey("stack"): stackID.String(), + clusterNameTagKeyLegacy: p.clusterName, + p.prefixedTrackingKey(p.resourceTagPrefix, "stack"): stackID.String(), } } @@ -131,6 +130,6 @@ func (p *defaultProvider) LegacyTagKeys() []string { } } -func (p *defaultProvider) prefixedTrackingKey(tag string) string { - return fmt.Sprintf("%v/%v", p.tagPrefix, tag) +func (p *defaultProvider) prefixedTrackingKey(prefix string, tag string) string { + return fmt.Sprintf("%v/%v", prefix, tag) } diff --git a/pkg/deploy/tracking/provider_test.go b/pkg/deploy/tracking/provider_test.go index 2d1e926682..ba3ffabedb 100644 --- a/pkg/deploy/tracking/provider_test.go +++ b/pkg/deploy/tracking/provider_test.go @@ -14,12 +14,12 @@ func Test_defaultProvider_ResourceIDTagKey(t *testing.T) { }{ { name: "resourceTagKey for Ingress", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), want: "ingress.k8s.aws/resource", }, { name: "resourceTagKey for Service", - provider: NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), want: "service.k8s.aws/resource", }, } @@ -43,7 +43,7 @@ func Test_defaultProvider_StackTags(t *testing.T) { }{ { name: "stackTags for explicit IngressGroup", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "", Name: "awesome-group"})}, want: map[string]string{ "elbv2.k8s.aws/cluster": "cluster-name", @@ -52,7 +52,7 @@ func Test_defaultProvider_StackTags(t *testing.T) { }, { name: "stackTags for implicit IngressGroup", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "ingressName"})}, want: map[string]string{ "elbv2.k8s.aws/cluster": "cluster-name", @@ -61,7 +61,7 @@ func Test_defaultProvider_StackTags(t *testing.T) { }, { name: "stackTags for Service", - provider: NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "serviceName"})}, want: map[string]string{ "elbv2.k8s.aws/cluster": "cluster-name", @@ -94,7 +94,7 @@ func Test_defaultProvider_ResourceTags(t *testing.T) { }{ { name: "resourceTags for Ingress", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), args: args{ stack: stack, res: fakeRes, @@ -126,7 +126,7 @@ func Test_defaultProvider_StackLabels(t *testing.T) { }{ { name: "stackLabels for explicit IngressGroup", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "", Name: "awesome-group"})}, want: map[string]string{ "ingress.k8s.aws/stack": "awesome-group", @@ -134,7 +134,7 @@ func Test_defaultProvider_StackLabels(t *testing.T) { }, { name: "stackLabels for implicit IngressGroup", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "ingressName"})}, want: map[string]string{ "ingress.k8s.aws/stack-namespace": "namespace", @@ -143,7 +143,7 @@ func Test_defaultProvider_StackLabels(t *testing.T) { }, { name: "stackLabels for Service", - provider: NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "serviceName"})}, want: map[string]string{ "service.k8s.aws/stack-namespace": "namespace", @@ -171,7 +171,7 @@ func Test_defaultProvider_StackTagsLegacy(t *testing.T) { }{ { name: "stackTags for explicit IngressGroup", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "", Name: "awesome-group"})}, want: map[string]string{ "ingress.k8s.aws/cluster": "cluster-name", @@ -180,7 +180,7 @@ func Test_defaultProvider_StackTagsLegacy(t *testing.T) { }, { name: "stackTags for implicit IngressGroup", - provider: NewDefaultProvider("ingress.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "ingressName"})}, want: map[string]string{ "ingress.k8s.aws/cluster": "cluster-name", @@ -189,7 +189,7 @@ func Test_defaultProvider_StackTagsLegacy(t *testing.T) { }, { name: "stackTags for Service", - provider: NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "serviceName"})}, want: map[string]string{ "ingress.k8s.aws/cluster": "cluster-name", diff --git a/pkg/ingress/model_build_load_balancer_test.go b/pkg/ingress/model_build_load_balancer_test.go index 8b1c43a2dc..796e1e2953 100644 --- a/pkg/ingress/model_build_load_balancer_test.go +++ b/pkg/ingress/model_build_load_balancer_test.go @@ -1281,7 +1281,7 @@ func Test_defaultModelBuildTask_buildLoadBalancerSubnets(t *testing.T) { annotationParser: annotations.NewSuffixAnnotationParser("alb.ingress.kubernetes.io"), elbv2TaggingManager: taggingManager, subnetsResolver: subnetsResolver, - trackingProvider: tracking.NewDefaultProvider("ingress.k8s.aws", "test-cluster"), + trackingProvider: tracking.NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", "test-cluster"), } got, err := task.buildLoadBalancerSubnetMappings(context.Background(), elbv2.LoadBalancerSchemeInternetFacing) if err != nil { diff --git a/pkg/ingress/model_builder_test.go b/pkg/ingress/model_builder_test.go index 99d9fb066d..b261167cdc 100644 --- a/pkg/ingress/model_builder_test.go +++ b/pkg/ingress/model_builder_test.go @@ -3665,7 +3665,7 @@ func Test_defaultModelBuilder_Build(t *testing.T) { authConfigBuilder := NewDefaultAuthConfigBuilder(annotationParser) enhancedBackendBuilder := NewDefaultEnhancedBackendBuilder(k8sClient, annotationParser, authConfigBuilder, true, true) ruleOptimizer := NewDefaultRuleOptimizer(logr.New(&log.NullLogSink{})) - trackingProvider := tracking.NewDefaultProvider("ingress.k8s.aws", clusterName) + trackingProvider := tracking.NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", clusterName) stackMarshaller := deploy.NewDefaultStackMarshaller() backendSGProvider := networkingpkg.NewMockBackendSGProvider(ctrl) sgResolver := networkingpkg.NewDefaultSecurityGroupResolver(ec2Client, vpcID) diff --git a/pkg/service/model_build_load_balancer_test.go b/pkg/service/model_build_load_balancer_test.go index 65551cce1c..4b6118c624 100644 --- a/pkg/service/model_build_load_balancer_test.go +++ b/pkg/service/model_build_load_balancer_test.go @@ -958,7 +958,7 @@ func Test_defaultModelBuilderTask_buildLoadBalancerSubnets(t *testing.T) { name: "subnet auto-discovery", svc: &corev1.Service{}, scheme: elbv2.LoadBalancerSchemeInternal, - provider: tracking.NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: tracking.NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "serviceName"})}, listLoadBalancersCalls: []listLoadBalancerCall{listLoadBalancerCallForEmptyLB}, resolveViaDiscoveryCalls: []resolveSubnetResults{ @@ -996,7 +996,7 @@ func Test_defaultModelBuilderTask_buildLoadBalancerSubnets(t *testing.T) { }, }, scheme: elbv2.LoadBalancerSchemeInternal, - provider: tracking.NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: tracking.NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "serviceName"})}, resolveViaNameOrIDSliceCalls: []resolveSubnetResults{ { @@ -1027,7 +1027,7 @@ func Test_defaultModelBuilderTask_buildLoadBalancerSubnets(t *testing.T) { name: "subnet resolve via Name or ID, with existing LB and scheme wouldn't change", svc: &corev1.Service{}, scheme: elbv2.LoadBalancerSchemeInternal, - provider: tracking.NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: tracking.NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "serviceName"})}, listLoadBalancersCalls: []listLoadBalancerCall{ { @@ -1082,7 +1082,7 @@ func Test_defaultModelBuilderTask_buildLoadBalancerSubnets(t *testing.T) { name: "subnet auto discovery, with existing LB and scheme would change", svc: &corev1.Service{}, scheme: elbv2.LoadBalancerSchemeInternal, - provider: tracking.NewDefaultProvider("service.k8s.aws", "cluster-name"), + provider: tracking.NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "cluster-name"), args: args{stack: core.NewDefaultStack(core.StackID{Namespace: "namespace", Name: "serviceName"})}, listLoadBalancersCalls: []listLoadBalancerCall{ { @@ -1154,7 +1154,7 @@ func Test_defaultModelBuilderTask_buildLoadBalancerSubnets(t *testing.T) { annotationParser := annotations.NewSuffixAnnotationParser("service.beta.kubernetes.io") clusterName := "cluster-name" - trackingProvider := tracking.NewDefaultProvider("ingress.k8s.aws", clusterName) + trackingProvider := tracking.NewDefaultProvider("elbv2.k8s.aws", "ingress.k8s.aws", clusterName) featureGates := config.NewFeatureGates() builder := &defaultModelBuildTask{ diff --git a/pkg/service/model_builder_test.go b/pkg/service/model_builder_test.go index 998d46c1ab..0b56514aed 100644 --- a/pkg/service/model_builder_test.go +++ b/pkg/service/model_builder_test.go @@ -6429,7 +6429,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { } } annotationParser := annotations.NewSuffixAnnotationParser("service.beta.kubernetes.io") - trackingProvider := tracking.NewDefaultProvider("service.k8s.aws", "my-cluster") + trackingProvider := tracking.NewDefaultProvider("elbv2.k8s.aws", "service.k8s.aws", "my-cluster") elbv2TaggingManager := elbv2.NewMockTaggingManager(ctrl) for _, call := range tt.listLoadBalancerCalls { From 55e9ec4378b8f605f87032e8548a0db6b32a7634 Mon Sep 17 00:00:00 2001 From: oliviassss Date: Tue, 29 Oct 2024 16:44:27 -0700 Subject: [PATCH 2/4] build backend sg name and cluster sg rule label based on config --- main.go | 4 +- pkg/config/controller_config.go | 11 ----- pkg/networking/backend_sg_provider.go | 48 +++++++++--------- pkg/networking/backend_sg_provider_test.go | 45 +++++++++++------ pkg/targetgroupbinding/networking_manager.go | 49 ++++++++++--------- .../networking_manager_test.go | 10 +++- pkg/targetgroupbinding/resource_manager.go | 4 +- 7 files changed, 94 insertions(+), 77 deletions(-) diff --git a/main.go b/main.go index b6484f24fd..a0594fe5ab 100644 --- a/main.go +++ b/main.go @@ -115,9 +115,9 @@ func main() { tgbResManager := targetgroupbinding.NewDefaultResourceManager(mgr.GetClient(), cloud.ELBV2(), cloud.EC2(), podInfoRepo, sgManager, sgReconciler, vpcInfoProvider, multiClusterManager, cloud.VpcID(), controllerCFG.ClusterName, controllerCFG.FeatureGates.Enabled(config.EndpointsFailOpen), controllerCFG.EnableEndpointSlices, controllerCFG.DisableRestrictedSGRules, - controllerCFG.ServiceTargetENISGTags, mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log) + controllerCFG.ServiceTargetENISGTags, controllerCFG.ResourcePrefix[config.ClusterSgRuleLabelPrefixKey], 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")) + cloud.VpcID(), cloud.EC2(), mgr.GetClient(), controllerCFG.ResourcePrefix[config.ClusterTagPrefixKey], controllerCFG.ResourcePrefix[config.BackendSGNamePrefixKey], controllerCFG.DefaultTags, ctrl.Log.WithName("backend-sg-provider")) sgResolver := networking.NewDefaultSecurityGroupResolver(cloud.EC2(), cloud.VpcID()) elbv2TaggingManager := elbv2deploy.NewDefaultTaggingManager(cloud.ELBV2(), cloud.VpcID(), controllerCFG.FeatureGates, cloud.RGT(), ctrl.Log) ingGroupReconciler := ingress.NewGroupReconciler(cloud, mgr.GetClient(), mgr.GetEventRecorderFor("ingress"), diff --git a/pkg/config/controller_config.go b/pkg/config/controller_config.go index da23df14f6..4d24a71da1 100644 --- a/pkg/config/controller_config.go +++ b/pkg/config/controller_config.go @@ -160,7 +160,6 @@ func (cfg *ControllerConfig) BindFlags(fs *pflag.FlagSet) { fs.StringToStringVar(&cfg.ResourcePrefix, flagResourcePrefix, defaultResourcePrefix, "the prefixes for resource tags, backend SG name and worker node SG rules label.") - cfg.mergeDefaultResourcePrefixVal() cfg.FeatureGates.BindFlags(fs) cfg.AWSConfig.BindFlags(fs) cfg.RuntimeConfig.BindFlags(fs) @@ -268,13 +267,3 @@ func (cfg *ControllerConfig) validateResourcePrefixKeys() error { } return nil } - -// mergeDefaultResourcePrefixVal make sure the ResourcePrefix map always has default val for unspecified key in user-passed flag -func (cfg *ControllerConfig) mergeDefaultResourcePrefixVal() { - // Merge user-provided values with defaults - for key, defaultVal := range defaultResourcePrefix { - if _, exists := cfg.ResourcePrefix[key]; !exists { - cfg.ResourcePrefix[key] = defaultVal - } - } -} diff --git a/pkg/networking/backend_sg_provider.go b/pkg/networking/backend_sg_provider.go index 8b3900e523..f9588755b1 100644 --- a/pkg/networking/backend_sg_provider.go +++ b/pkg/networking/backend_sg_provider.go @@ -31,8 +31,6 @@ const ( defaultSGDeletionTimeout = 2 * time.Minute resourceTypeSecurityGroup = "security-group" - tagKeyK8sCluster = "elbv2.k8s.aws/cluster" - tagKeyResource = "elbv2.k8s.aws/resource" tagValueBackend = "backend-sg" explicitGroupFinalizerPrefix = "group.ingress.k8s.aws/" @@ -59,16 +57,19 @@ type BackendSGProvider interface { // NewBackendSGProvider constructs a new defaultBackendSGProvider func NewBackendSGProvider(clusterName string, backendSG string, vpcID string, - ec2Client services.EC2, k8sClient client.Client, defaultTags map[string]string, logger logr.Logger) *defaultBackendSGProvider { + ec2Client services.EC2, k8sClient client.Client, clusterTagPrefixKey string, backendSGNamePrefix string, defaultTags map[string]string, logger logr.Logger) *defaultBackendSGProvider { return &defaultBackendSGProvider{ - vpcID: vpcID, - clusterName: clusterName, - backendSG: backendSG, - defaultTags: defaultTags, - ec2Client: ec2Client, - k8sClient: k8sClient, - logger: logger, - mutex: sync.Mutex{}, + vpcID: vpcID, + clusterName: clusterName, + backendSG: backendSG, + tagKeyK8sCluster: clusterTagPrefixKey + "/cluster", + tagKeyResource: clusterTagPrefixKey + "/resource", + backendSGNamePrefix: backendSGNamePrefix, + defaultTags: defaultTags, + ec2Client: ec2Client, + k8sClient: k8sClient, + logger: logger, + mutex: sync.Mutex{}, checkIngressFinalizersFunc: func(finalizers []string) bool { for _, fin := range finalizers { @@ -100,12 +101,15 @@ type defaultBackendSGProvider struct { clusterName string mutex sync.Mutex - backendSG string - autoGeneratedSG string - defaultTags map[string]string - ec2Client services.EC2 - k8sClient client.Client - logger logr.Logger + backendSG string + autoGeneratedSG string + tagKeyK8sCluster string + tagKeyResource string + backendSGNamePrefix string + defaultTags map[string]string + ec2Client services.EC2 + k8sClient client.Client + logger logr.Logger // objectsMap keeps track of whether the backend SG is required for any tracked resources in the cluster. // If any entry in the map is true, or there are resources with this controller specific finalizers which // haven't been tracked in the map yet, controller doesn't delete the backend SG. If the controller has @@ -269,11 +273,11 @@ func (p *defaultBackendSGProvider) buildBackendSGTags(_ context.Context) []ec2ty ResourceType: resourceTypeSecurityGroup, Tags: append(defaultTags, []ec2types.Tag{ { - Key: awssdk.String(tagKeyK8sCluster), + Key: awssdk.String(p.tagKeyK8sCluster), Value: awssdk.String(p.clusterName), }, { - Key: awssdk.String(tagKeyResource), + Key: awssdk.String(p.tagKeyResource), Value: awssdk.String(tagValueBackend), }, }...), @@ -289,11 +293,11 @@ func (p *defaultBackendSGProvider) getBackendSGFromEC2(ctx context.Context, sgNa Values: []string{vpcID}, }, { - Name: awssdk.String(fmt.Sprintf("tag:%v", tagKeyK8sCluster)), + Name: awssdk.String(fmt.Sprintf("tag:%v", p.tagKeyK8sCluster)), Values: []string{p.clusterName}, }, { - Name: awssdk.String(fmt.Sprintf("tag:%v", tagKeyResource)), + Name: awssdk.String(fmt.Sprintf("tag:%v", p.tagKeyResource)), Values: []string{tagValueBackend}, }, }, @@ -342,7 +346,7 @@ func (p *defaultBackendSGProvider) getBackendSGName() string { _, _ = sgNameHash.Write([]byte(p.clusterName)) sgHash := hex.EncodeToString(sgNameHash.Sum(nil)) sanitizedClusterName := invalidSGNamePattern.ReplaceAllString(p.clusterName, "") - return fmt.Sprintf("k8s-traffic-%.232s-%.10s", sanitizedClusterName, sgHash) + return fmt.Sprintf("%v-%.232s-%.10s", p.backendSGNamePrefix, sanitizedClusterName, sgHash) } func isSecurityGroupDependencyViolationError(err error) bool { diff --git a/pkg/networking/backend_sg_provider_test.go b/pkg/networking/backend_sg_provider_test.go index 4850d7d30d..8eb4093a32 100644 --- a/pkg/networking/backend_sg_provider_test.go +++ b/pkg/networking/backend_sg_provider_test.go @@ -26,8 +26,12 @@ import ( ) const ( - defaultVPCID = "vpc-xxxyyy" - defaultClusterName = "testCluster" + defaultVPCID = "vpc-xxxyyy" + defaultClusterName = "testCluster" + defaultClusterTagPrefixKey = "elbv2.k8s.aws" + defaultBackendSGNamePrefix = "k8s-traffic" + defaultTagKeyK8sCluster = "elbv2.k8s.aws/cluster" + defaultTagKeyResource = "elbv2.k8s.aws/resource" ) func Test_defaultBackendSGProvider_Get(t *testing.T) { @@ -42,12 +46,14 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { err error } type fields struct { - backendSG string - ingResources []*networking.Ingress - svcResource *corev1.Service - defaultTags map[string]string - describeSGCalls []describeSecurityGroupsAsListCall - createSGCalls []createSecurityGroupWithContexCall + backendSG string + ingResources []*networking.Ingress + svcResource *corev1.Service + clusterTagPrefixKey string + backendSGNamePrefix string + defaultTags map[string]string + describeSGCalls []describeSecurityGroupsAsListCall + createSGCalls []createSecurityGroupWithContexCall } defaultEC2Filters := []ec2types.Filter{ { @@ -110,7 +116,8 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { }, }, }, - ingResources: []*networking.Ingress{ing, ing1}, + ingResources: []*networking.Ingress{ing, ing1}, + clusterTagPrefixKey: defaultClusterTagPrefixKey, }, want: "sg-autogen", }, @@ -152,7 +159,9 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { }, }, }, - ingResources: []*networking.Ingress{ing, ing1}, + ingResources: []*networking.Ingress{ing, ing1}, + clusterTagPrefixKey: defaultClusterTagPrefixKey, + backendSGNamePrefix: defaultBackendSGNamePrefix, }, want: "sg-newauto", }, @@ -206,6 +215,8 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { }, }, }, + clusterTagPrefixKey: defaultClusterTagPrefixKey, + backendSGNamePrefix: defaultBackendSGNamePrefix, defaultTags: map[string]string{ "zzzKey": "value", "KubernetesCluster": defaultClusterName, @@ -226,7 +237,9 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { err: &smithy.GenericAPIError{Code: "Some.Other.Error", Message: "describe security group as list error"}, }, }, - ingResources: []*networking.Ingress{ing}, + ingResources: []*networking.Ingress{ing}, + clusterTagPrefixKey: defaultClusterTagPrefixKey, + backendSGNamePrefix: defaultBackendSGNamePrefix, }, wantErr: errors.New("api error Some.Other.Error: describe security group as list error"), }, @@ -266,7 +279,9 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { err: &smithy.GenericAPIError{Code: "Create.Error", Message: "unable to create security group"}, }, }, - ingResources: []*networking.Ingress{ing1}, + ingResources: []*networking.Ingress{ing1}, + clusterTagPrefixKey: defaultClusterTagPrefixKey, + backendSGNamePrefix: defaultBackendSGNamePrefix, }, wantErr: errors.New("api error Create.Error: unable to create security group"), }, @@ -285,7 +300,7 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { } k8sClient := mock_client.NewMockClient(ctrl) sgProvider := NewBackendSGProvider(defaultClusterName, tt.fields.backendSG, - defaultVPCID, ec2Client, k8sClient, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) + defaultVPCID, ec2Client, k8sClient, tt.fields.clusterTagPrefixKey, tt.fields.backendSGNamePrefix, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) resourceType := ResourceTypeIngress var activeResources []types.NamespacedName @@ -329,6 +344,8 @@ func Test_defaultBackendSGProvider_Release(t *testing.T) { type fields struct { autogenSG string backendSG string + clusterTagPrefixKey string + backendSGNamePrefix string defaultTags map[string]string listIngressCalls []listIngressCall deleteSGCalls []deleteSecurityGroupWithContextCall @@ -732,7 +749,7 @@ func Test_defaultBackendSGProvider_Release(t *testing.T) { ec2Client := services.NewMockEC2(ctrl) k8sClient := mock_client.NewMockClient(ctrl) sgProvider := NewBackendSGProvider(defaultClusterName, tt.fields.backendSG, - defaultVPCID, ec2Client, k8sClient, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) + defaultVPCID, ec2Client, k8sClient, tt.fields.clusterTagPrefixKey, tt.fields.backendSGNamePrefix, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) if len(tt.fields.autogenSG) > 0 { sgProvider.backendSG = "" sgProvider.autoGeneratedSG = tt.fields.autogenSG diff --git a/pkg/targetgroupbinding/networking_manager.go b/pkg/targetgroupbinding/networking_manager.go index 21fa4b741f..9c952c15d8 100644 --- a/pkg/targetgroupbinding/networking_manager.go +++ b/pkg/targetgroupbinding/networking_manager.go @@ -27,7 +27,7 @@ import ( ) const ( - tgbNetworkingIPPermissionLabelKey = "elbv2.k8s.aws/targetGroupBinding" + //tgbNetworkingIPPermissionLabelKey = "elbv2.k8s.aws/targetGroupBinding" tgbNetworkingIPPermissionLabelValue = "shared" defaultTgbMinPort = int32(0) defaultTgbMaxPort = int32(65535) @@ -47,18 +47,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, serviceTargetENISGTags map[string]string, logger logr.Logger, disabledRestrictedSGRulesFlag bool) *defaultNetworkingManager { - + sgManager networking.SecurityGroupManager, sgReconciler networking.SecurityGroupReconciler, vpcID string, clusterName string, serviceTargetENISGTags map[string]string, clusterSgRuleLabelPrefix string, logger logr.Logger, disabledRestrictedSGRulesFlag bool) *defaultNetworkingManager { return &defaultNetworkingManager{ - k8sClient: k8sClient, - podENIResolver: podENIResolver, - nodeENIResolver: nodeENIResolver, - sgManager: sgManager, - sgReconciler: sgReconciler, - vpcID: vpcID, - clusterName: clusterName, - serviceTargetENISGTags: serviceTargetENISGTags, - logger: logger, + k8sClient: k8sClient, + podENIResolver: podENIResolver, + nodeENIResolver: nodeENIResolver, + sgManager: sgManager, + sgReconciler: sgReconciler, + vpcID: vpcID, + clusterName: clusterName, + serviceTargetENISGTags: serviceTargetENISGTags, + tgbNetworkingIPPermissionLabelKey: clusterSgRuleLabelPrefix + "/targetGroupBinding", + logger: logger, mutex: sync.Mutex{}, ingressPermissionsPerSGByTGB: make(map[types.NamespacedName]map[string][]networking.IPPermissionInfo), @@ -70,15 +70,16 @@ 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 - serviceTargetENISGTags map[string]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 + tgbNetworkingIPPermissionLabelKey string + logger logr.Logger // mutex will serialize our TargetGroup's networking reconcile requests. mutex sync.Mutex @@ -202,7 +203,7 @@ func (m *defaultNetworkingManager) reconcileWithIngressPermissionsPerSG(ctx cont computedForAllTGBs := m.consolidateIngressPermissionsPerSGByTGB(ctx, tgbsWithNetworking) aggregatedIngressPermissionsPerSG := m.computeAggregatedIngressPermissionsPerSG(ctx) - permissionSelector := labels.SelectorFromSet(labels.Set{tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelValue}) + permissionSelector := labels.SelectorFromSet(labels.Set{m.tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelValue}) var sgReconciliationErrors []error for sgID, permissions := range aggregatedIngressPermissionsPerSG { if err := m.sgReconciler.ReconcileIngress(ctx, sgID, permissions, @@ -421,7 +422,7 @@ func (m *defaultNetworkingManager) computePermissionsForPeerPort(ctx context.Con }) } - permissionLabels := map[string]string{tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelValue} + permissionLabels := map[string]string{m.tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelValue} if peer.SecurityGroup != nil { groupID := peer.SecurityGroup.GroupID permissions := make([]networking.IPPermissionInfo, 0, len(sdkFromToPortPairs)) @@ -484,7 +485,7 @@ func (m *defaultNetworkingManager) gcIngressPermissionsFromUnusedEndpointSGs(ctx usedEndpointSGs := sets.StringKeySet(ingressPermissionsPerSG) unusedEndpointSGs := endpointSGs.Difference(usedEndpointSGs) - permissionSelector := labels.SelectorFromSet(labels.Set{tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelValue}) + permissionSelector := labels.SelectorFromSet(labels.Set{m.tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelValue}) for sgID := range unusedEndpointSGs { err := m.sgReconciler.ReconcileIngress(ctx, sgID, nil, networking.WithPermissionSelector(permissionSelector)) diff --git a/pkg/targetgroupbinding/networking_manager_test.go b/pkg/targetgroupbinding/networking_manager_test.go index 74482a8439..35bda86f04 100644 --- a/pkg/targetgroupbinding/networking_manager_test.go +++ b/pkg/targetgroupbinding/networking_manager_test.go @@ -17,6 +17,8 @@ import ( "sigs.k8s.io/aws-load-balancer-controller/pkg/networking" ) +const tgbNetworkingIPPermissionLabelKey = "elbv2.k8s.aws/targetGroupBinding" + func Test_defaultNetworkingManager_computeIngressPermissionsForTGBNetworking(t *testing.T) { port8080 := intstr.FromInt(8080) port8443 := intstr.FromInt(8443) @@ -228,7 +230,9 @@ func Test_defaultNetworkingManager_computeIngressPermissionsForTGBNetworking(t * } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - m := &defaultNetworkingManager{} + m := &defaultNetworkingManager{ + tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelKey, + } got, err := m.computeIngressPermissionsForTGBNetworking(context.Background(), tt.args.tgbNetworking, tt.args.pods) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) @@ -476,7 +480,9 @@ func Test_defaultNetworkingManager_computePermissionsForPeerPort(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - m := &defaultNetworkingManager{} + m := &defaultNetworkingManager{ + tgbNetworkingIPPermissionLabelKey: tgbNetworkingIPPermissionLabelKey, + } got, err := m.computePermissionsForPeerPort(context.Background(), tt.args.peer, tt.args.port, tt.args.pods) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) diff --git a/pkg/targetgroupbinding/resource_manager.go b/pkg/targetgroupbinding/resource_manager.go index 78a664f0af..5266b59f17 100644 --- a/pkg/targetgroupbinding/resource_manager.go +++ b/pkg/targetgroupbinding/resource_manager.go @@ -39,7 +39,7 @@ func NewDefaultResourceManager(k8sClient client.Client, elbv2Client services.ELB podInfoRepo k8s.PodInfoRepo, sgManager networking.SecurityGroupManager, sgReconciler networking.SecurityGroupReconciler, vpcInfoProvider networking.VPCInfoProvider, multiClusterManager MultiClusterManager, vpcID string, clusterName string, failOpenEnabled bool, endpointSliceEnabled bool, disabledRestrictedSGRulesFlag bool, - endpointSGTags map[string]string, + endpointSGTags map[string]string, clusterSgRuleLabelPrefix string, eventRecorder record.EventRecorder, logger logr.Logger) *defaultResourceManager { targetsManager := NewCachedTargetsManager(elbv2Client, logger) endpointResolver := backend.NewDefaultEndpointResolver(k8sClient, podInfoRepo, failOpenEnabled, endpointSliceEnabled, logger) @@ -48,7 +48,7 @@ func NewDefaultResourceManager(k8sClient client.Client, elbv2Client services.ELB podENIResolver := networking.NewDefaultPodENIInfoResolver(k8sClient, ec2Client, nodeInfoProvider, vpcID, logger) nodeENIResolver := networking.NewDefaultNodeENIInfoResolver(nodeInfoProvider, logger) - networkingManager := NewDefaultNetworkingManager(k8sClient, podENIResolver, nodeENIResolver, sgManager, sgReconciler, vpcID, clusterName, endpointSGTags, logger, disabledRestrictedSGRulesFlag) + networkingManager := NewDefaultNetworkingManager(k8sClient, podENIResolver, nodeENIResolver, sgManager, sgReconciler, vpcID, clusterName, endpointSGTags, clusterSgRuleLabelPrefix, logger, disabledRestrictedSGRulesFlag) return &defaultResourceManager{ k8sClient: k8sClient, targetsManager: targetsManager, From b5df4ba4439f6ff588892a8702c846c15deac1a5 Mon Sep 17 00:00:00 2001 From: oliviassss Date: Wed, 30 Oct 2024 10:25:50 -0700 Subject: [PATCH 3/4] rename flag to resource-tracking-configuration --- controllers/ingress/group_controller.go | 4 ++-- controllers/service/service_controller.go | 4 ++-- main.go | 4 ++-- pkg/config/controller_config.go | 24 +++++++++++------------ pkg/config/controller_config_test.go | 10 +++++----- pkg/networking/backend_sg_provider.go | 2 ++ 6 files changed, 25 insertions(+), 23 deletions(-) diff --git a/controllers/ingress/group_controller.go b/controllers/ingress/group_controller.go index 9bad1c759e..e7aff011d3 100644 --- a/controllers/ingress/group_controller.go +++ b/controllers/ingress/group_controller.go @@ -52,7 +52,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder authConfigBuilder := ingress.NewDefaultAuthConfigBuilder(annotationParser) enhancedBackendBuilder := ingress.NewDefaultEnhancedBackendBuilder(k8sClient, annotationParser, authConfigBuilder, controllerConfig.IngressConfig.TolerateNonExistentBackendService, controllerConfig.IngressConfig.TolerateNonExistentBackendAction) referenceIndexer := ingress.NewDefaultReferenceIndexer(enhancedBackendBuilder, authConfigBuilder, logger) - trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.IngressTagPrefixKey], controllerConfig.ClusterName) + trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.IngressTagPrefixKey], controllerConfig.ClusterName) modelBuilder := ingress.NewDefaultModelBuilder(k8sClient, eventRecorder, cloud.EC2(), cloud.ELBV2(), cloud.ACM(), annotationParser, subnetsResolver, @@ -62,7 +62,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder controllerConfig.EnableBackendSecurityGroup, controllerConfig.DisableRestrictedSGRules, controllerConfig.IngressConfig.AllowedCertificateAuthorityARNs, controllerConfig.FeatureGates.Enabled(config.EnableIPTargetType), logger) stackMarshaller := deploy.NewDefaultStackMarshaller() stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, - controllerConfig, controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.IngressTagPrefixKey], logger) + controllerConfig, controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.IngressTagPrefixKey], logger) classLoader := ingress.NewDefaultClassLoader(k8sClient, true) classAnnotationMatcher := ingress.NewDefaultClassAnnotationMatcher(controllerConfig.IngressConfig.IngressClass) manageIngressesWithoutIngressClass := controllerConfig.IngressConfig.IngressClass == "" diff --git a/controllers/service/service_controller.go b/controllers/service/service_controller.go index 4972e43928..ff3339c208 100644 --- a/controllers/service/service_controller.go +++ b/controllers/service/service_controller.go @@ -40,14 +40,14 @@ func NewServiceReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorde backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, logger logr.Logger) *serviceReconciler { annotationParser := annotations.NewSuffixAnnotationParser(serviceAnnotationPrefix) - trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.ServiceTagPrefixKey], controllerConfig.ClusterName) + trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.ServiceTagPrefixKey], controllerConfig.ClusterName) serviceUtils := service.NewServiceUtils(annotationParser, serviceFinalizer, controllerConfig.ServiceConfig.LoadBalancerClass, controllerConfig.FeatureGates) modelBuilder := service.NewDefaultModelBuilder(annotationParser, subnetsResolver, vpcInfoProvider, cloud.VpcID(), trackingProvider, elbv2TaggingManager, cloud.EC2(), controllerConfig.FeatureGates, controllerConfig.ClusterName, controllerConfig.DefaultTags, controllerConfig.ExternalManagedTags, controllerConfig.DefaultSSLPolicy, controllerConfig.DefaultTargetType, controllerConfig.FeatureGates.Enabled(config.EnableIPTargetType), serviceUtils, backendSGProvider, sgResolver, controllerConfig.EnableBackendSecurityGroup, controllerConfig.DisableRestrictedSGRules, logger) stackMarshaller := deploy.NewDefaultStackMarshaller() - stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, controllerConfig, controllerConfig.ResourcePrefix[config.ClusterTagPrefixKey], controllerConfig.ResourcePrefix[config.ServiceTagPrefixKey], logger) + stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, controllerConfig, controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.ServiceTagPrefixKey], logger) return &serviceReconciler{ k8sClient: k8sClient, eventRecorder: eventRecorder, diff --git a/main.go b/main.go index a0594fe5ab..0a437df019 100644 --- a/main.go +++ b/main.go @@ -115,9 +115,9 @@ func main() { tgbResManager := targetgroupbinding.NewDefaultResourceManager(mgr.GetClient(), cloud.ELBV2(), cloud.EC2(), podInfoRepo, sgManager, sgReconciler, vpcInfoProvider, multiClusterManager, cloud.VpcID(), controllerCFG.ClusterName, controllerCFG.FeatureGates.Enabled(config.EndpointsFailOpen), controllerCFG.EnableEndpointSlices, controllerCFG.DisableRestrictedSGRules, - controllerCFG.ServiceTargetENISGTags, controllerCFG.ResourcePrefix[config.ClusterSgRuleLabelPrefixKey], mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log) + controllerCFG.ServiceTargetENISGTags, controllerCFG.ResourceTrackingConfiguration[config.ClusterSgRuleLabelPrefixKey], mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log) backendSGProvider := networking.NewBackendSGProvider(controllerCFG.ClusterName, controllerCFG.BackendSecurityGroup, - cloud.VpcID(), cloud.EC2(), mgr.GetClient(), controllerCFG.ResourcePrefix[config.ClusterTagPrefixKey], controllerCFG.ResourcePrefix[config.BackendSGNamePrefixKey], controllerCFG.DefaultTags, ctrl.Log.WithName("backend-sg-provider")) + cloud.VpcID(), cloud.EC2(), mgr.GetClient(), controllerCFG.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerCFG.ResourceTrackingConfiguration[config.BackendSGNamePrefixKey], controllerCFG.DefaultTags, ctrl.Log.WithName("backend-sg-provider")) sgResolver := networking.NewDefaultSecurityGroupResolver(cloud.EC2(), cloud.VpcID()) elbv2TaggingManager := elbv2deploy.NewDefaultTaggingManager(cloud.ELBV2(), cloud.VpcID(), controllerCFG.FeatureGates, cloud.RGT(), ctrl.Log) ingGroupReconciler := ingress.NewGroupReconciler(cloud, mgr.GetClient(), mgr.GetEventRecorderFor("ingress"), diff --git a/pkg/config/controller_config.go b/pkg/config/controller_config.go index 4d24a71da1..f3fd6e1d3f 100644 --- a/pkg/config/controller_config.go +++ b/pkg/config/controller_config.go @@ -17,7 +17,7 @@ const ( flagLogLevel = "log-level" flagK8sClusterName = "cluster-name" flagDefaultTags = "default-tags" - flagResourcePrefix = "resource-prefix" + flagResourceTrackingConfiguration = "resource-tracking-configuration" flagDefaultTargetType = "default-target-type" flagExternalManagedTags = "external-managed-tags" flagServiceTargetENISGTags = "service-target-eni-security-group-tags" @@ -89,8 +89,8 @@ type ControllerConfig struct { // Default AWS Tags that will be applied to all AWS resources managed by this controller. DefaultTags map[string]string - // ResourcePrefix provides prefix for resource tags, backend SG name and worker node SG rules label. - ResourcePrefix map[string]string + // ResourceTrackingConfiguration provides tracking prefix for resource tags, backend SG name and worker node SG rules label. + ResourceTrackingConfiguration map[string]string // Default target type for Ingress and Service objects DefaultTargetType string @@ -157,7 +157,7 @@ func (cfg *ControllerConfig) BindFlags(fs *pflag.FlagSet) { "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") - fs.StringToStringVar(&cfg.ResourcePrefix, flagResourcePrefix, defaultResourcePrefix, + fs.StringToStringVar(&cfg.ResourceTrackingConfiguration, flagResourceTrackingConfiguration, defaultResourcePrefix, "the prefixes for resource tags, backend SG name and worker node SG rules label.") cfg.FeatureGates.BindFlags(fs) @@ -180,12 +180,12 @@ func (cfg *ControllerConfig) Validate() error { } trackingTagKeys := sets.New[string]( - cfg.ResourcePrefix[ClusterTagPrefixKey]+"/cluster", - cfg.ResourcePrefix[ClusterTagPrefixKey]+"/resource", - cfg.ResourcePrefix[IngressTagPrefixKey]+"/stack", - cfg.ResourcePrefix[IngressTagPrefixKey]+"/resource", - cfg.ResourcePrefix[ServiceTagPrefixKey]+"/stack", - cfg.ResourcePrefix[ServiceTagPrefixKey]+"/resource", + cfg.ResourceTrackingConfiguration[ClusterTagPrefixKey]+"/cluster", + cfg.ResourceTrackingConfiguration[ClusterTagPrefixKey]+"/resource", + cfg.ResourceTrackingConfiguration[IngressTagPrefixKey]+"/stack", + cfg.ResourceTrackingConfiguration[IngressTagPrefixKey]+"/resource", + cfg.ResourceTrackingConfiguration[ServiceTagPrefixKey]+"/stack", + cfg.ResourceTrackingConfiguration[ServiceTagPrefixKey]+"/resource", ) if err := cfg.validateDefaultTagsCollisionWithTrackingTags(trackingTagKeys); err != nil { @@ -254,8 +254,8 @@ func (cfg *ControllerConfig) validateBackendSecurityGroupConfiguration() error { } func (cfg *ControllerConfig) validateResourcePrefixKeys() error { - keys := make([]string, 0, len(cfg.ResourcePrefix)) - for key := range cfg.ResourcePrefix { + keys := make([]string, 0, len(cfg.ResourceTrackingConfiguration)) + for key := range cfg.ResourceTrackingConfiguration { if !validPrefixKeys.Has(key) { return fmt.Errorf("invalid key: %s. Valid keys are: %v", key, validPrefixKeys.List()) } diff --git a/pkg/config/controller_config_test.go b/pkg/config/controller_config_test.go index 7283118975..a92547ee25 100644 --- a/pkg/config/controller_config_test.go +++ b/pkg/config/controller_config_test.go @@ -184,7 +184,7 @@ func TestControllerConfig_validateExternalManagedTagsCollisionWithDefaultTags(t func TestControllerConfig_validateResourcePrefixKeys(t *testing.T) { type fields struct { - ResourcePrefix map[string]string + ResourceTrackingConfiguration map[string]string } tests := []struct { name string @@ -194,7 +194,7 @@ func TestControllerConfig_validateResourcePrefixKeys(t *testing.T) { { name: "resource prefix has all keys", fields: fields{ - ResourcePrefix: map[string]string{ + ResourceTrackingConfiguration: map[string]string{ "clusterTagPrefix": "elbv2.k8s.aws", "ingressTagPrefix": "ingress.k8s.aws", "serviceTagPrefix": "service.k8s.aws", @@ -207,7 +207,7 @@ func TestControllerConfig_validateResourcePrefixKeys(t *testing.T) { { name: "resource prefix has some invalid keys", fields: fields{ - ResourcePrefix: map[string]string{ + ResourceTrackingConfiguration: map[string]string{ "clusterTagPrefix": "elbv2.k8s.aws", "ingressTagPrefix": "ingress.k8s.aws", "serviceTagPrefix": "service.k8s.aws", @@ -220,7 +220,7 @@ func TestControllerConfig_validateResourcePrefixKeys(t *testing.T) { { name: "resource prefix is missing some valid keys", fields: fields{ - ResourcePrefix: map[string]string{ + ResourceTrackingConfiguration: map[string]string{ "clusterTagPrefix": "elbv2.k8s.aws", "ingressTagPrefix": "ingress.k8s.aws", "serviceTagPrefix": "service.k8s.aws", @@ -233,7 +233,7 @@ func TestControllerConfig_validateResourcePrefixKeys(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cfg := &ControllerConfig{ - ResourcePrefix: tt.fields.ResourcePrefix, + ResourceTrackingConfiguration: tt.fields.ResourceTrackingConfiguration, } err := cfg.validateResourcePrefixKeys() if tt.wantErr != nil { diff --git a/pkg/networking/backend_sg_provider.go b/pkg/networking/backend_sg_provider.go index f9588755b1..ad556fd0a9 100644 --- a/pkg/networking/backend_sg_provider.go +++ b/pkg/networking/backend_sg_provider.go @@ -324,6 +324,7 @@ func (p *defaultBackendSGProvider) releaseSG(ctx context.Context) error { p.logger.V(1).Info("releaseSG ignore delete", "required", required, "err", err) return err } + p.logger.V(0).Info("releaseSG deleting sg %v", p.autoGeneratedSG) req := &ec2sdk.DeleteSecurityGroupInput{ GroupId: awssdk.String(p.autoGeneratedSG), } @@ -331,6 +332,7 @@ func (p *defaultBackendSGProvider) releaseSG(ctx context.Context) error { _, err := p.ec2Client.DeleteSecurityGroupWithContext(ctx, req) return err }); err != nil { + p.logger.V(0).Info("releaseSG failed to delete securityGroup", "sgID", p.autoGeneratedSG) return errors.Wrap(err, "failed to delete securityGroup") } p.logger.Info("deleted securityGroup", "ID", p.autoGeneratedSG) From fd70686938127d52303b1661ddbaf6d0e8e28653 Mon Sep 17 00:00:00 2001 From: oliviassss Date: Thu, 14 Nov 2024 14:02:44 -0800 Subject: [PATCH 4/4] replace flag by parameters --- controllers/ingress/group_controller.go | 6 +- controllers/service/service_controller.go | 7 ++- main.go | 9 ++- pkg/config/controller_config.go | 70 ++++------------------ pkg/config/controller_config_test.go | 63 ------------------- pkg/networking/backend_sg_provider.go | 42 +++++++------ pkg/networking/backend_sg_provider_test.go | 4 +- 7 files changed, 48 insertions(+), 153 deletions(-) diff --git a/controllers/ingress/group_controller.go b/controllers/ingress/group_controller.go index e7aff011d3..b838e93f59 100644 --- a/controllers/ingress/group_controller.go +++ b/controllers/ingress/group_controller.go @@ -39,6 +39,8 @@ const ( // the groupVersion of used Ingress & IngressClass resource. ingressResourcesGroupVersion = "networking.k8s.io/v1" ingressClassKind = "IngressClass" + clusterTagPrefix = "elbv2.k8s.aws" + ingressTagPrefix = "ingress.k8s.aws" ) // NewGroupReconciler constructs new GroupReconciler @@ -52,7 +54,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder authConfigBuilder := ingress.NewDefaultAuthConfigBuilder(annotationParser) enhancedBackendBuilder := ingress.NewDefaultEnhancedBackendBuilder(k8sClient, annotationParser, authConfigBuilder, controllerConfig.IngressConfig.TolerateNonExistentBackendService, controllerConfig.IngressConfig.TolerateNonExistentBackendAction) referenceIndexer := ingress.NewDefaultReferenceIndexer(enhancedBackendBuilder, authConfigBuilder, logger) - trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.IngressTagPrefixKey], controllerConfig.ClusterName) + trackingProvider := tracking.NewDefaultProvider(clusterTagPrefix, ingressTagPrefix, controllerConfig.ClusterName) modelBuilder := ingress.NewDefaultModelBuilder(k8sClient, eventRecorder, cloud.EC2(), cloud.ELBV2(), cloud.ACM(), annotationParser, subnetsResolver, @@ -62,7 +64,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder controllerConfig.EnableBackendSecurityGroup, controllerConfig.DisableRestrictedSGRules, controllerConfig.IngressConfig.AllowedCertificateAuthorityARNs, controllerConfig.FeatureGates.Enabled(config.EnableIPTargetType), logger) stackMarshaller := deploy.NewDefaultStackMarshaller() stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, - controllerConfig, controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.IngressTagPrefixKey], logger) + controllerConfig, clusterTagPrefix, ingressTagPrefix, logger) classLoader := ingress.NewDefaultClassLoader(k8sClient, true) classAnnotationMatcher := ingress.NewDefaultClassAnnotationMatcher(controllerConfig.IngressConfig.IngressClass) manageIngressesWithoutIngressClass := controllerConfig.IngressConfig.IngressClass == "" diff --git a/controllers/service/service_controller.go b/controllers/service/service_controller.go index ff3339c208..25e3d2dbfc 100644 --- a/controllers/service/service_controller.go +++ b/controllers/service/service_controller.go @@ -31,6 +31,9 @@ const ( serviceFinalizer = "service.k8s.aws/resources" serviceAnnotationPrefix = "service.beta.kubernetes.io" controllerName = "service" + + clusterTagPrefix = "elbv2.k8s.aws" + serviceTagPrefix = "service.k8s.aws" ) func NewServiceReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder record.EventRecorder, @@ -40,14 +43,14 @@ func NewServiceReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorde backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, logger logr.Logger) *serviceReconciler { annotationParser := annotations.NewSuffixAnnotationParser(serviceAnnotationPrefix) - trackingProvider := tracking.NewDefaultProvider(controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.ServiceTagPrefixKey], controllerConfig.ClusterName) + trackingProvider := tracking.NewDefaultProvider(clusterTagPrefix, serviceTagPrefix, controllerConfig.ClusterName) serviceUtils := service.NewServiceUtils(annotationParser, serviceFinalizer, controllerConfig.ServiceConfig.LoadBalancerClass, controllerConfig.FeatureGates) modelBuilder := service.NewDefaultModelBuilder(annotationParser, subnetsResolver, vpcInfoProvider, cloud.VpcID(), trackingProvider, elbv2TaggingManager, cloud.EC2(), controllerConfig.FeatureGates, controllerConfig.ClusterName, controllerConfig.DefaultTags, controllerConfig.ExternalManagedTags, controllerConfig.DefaultSSLPolicy, controllerConfig.DefaultTargetType, controllerConfig.FeatureGates.Enabled(config.EnableIPTargetType), serviceUtils, backendSGProvider, sgResolver, controllerConfig.EnableBackendSecurityGroup, controllerConfig.DisableRestrictedSGRules, logger) stackMarshaller := deploy.NewDefaultStackMarshaller() - stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, controllerConfig, controllerConfig.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerConfig.ResourceTrackingConfiguration[config.ServiceTagPrefixKey], logger) + stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, elbv2TaggingManager, controllerConfig, clusterTagPrefix, serviceTagPrefix, logger) return &serviceReconciler{ k8sClient: k8sClient, eventRecorder: eventRecorder, diff --git a/main.go b/main.go index 0a437df019..07502d8696 100644 --- a/main.go +++ b/main.go @@ -53,6 +53,11 @@ import ( // +kubebuilder:scaffold:imports ) +const ( + clusterSgRuleLabelPrefix = "elbv2.k8s.aws" + clusterTagPrefix = "elbv2.k8s.aws" +) + var ( scheme = k8sruntime.NewScheme() setupLog = ctrl.Log.WithName("setup") @@ -115,9 +120,9 @@ func main() { tgbResManager := targetgroupbinding.NewDefaultResourceManager(mgr.GetClient(), cloud.ELBV2(), cloud.EC2(), podInfoRepo, sgManager, sgReconciler, vpcInfoProvider, multiClusterManager, cloud.VpcID(), controllerCFG.ClusterName, controllerCFG.FeatureGates.Enabled(config.EndpointsFailOpen), controllerCFG.EnableEndpointSlices, controllerCFG.DisableRestrictedSGRules, - controllerCFG.ServiceTargetENISGTags, controllerCFG.ResourceTrackingConfiguration[config.ClusterSgRuleLabelPrefixKey], mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log) + controllerCFG.ServiceTargetENISGTags, clusterSgRuleLabelPrefix, mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log) backendSGProvider := networking.NewBackendSGProvider(controllerCFG.ClusterName, controllerCFG.BackendSecurityGroup, - cloud.VpcID(), cloud.EC2(), mgr.GetClient(), controllerCFG.ResourceTrackingConfiguration[config.ClusterTagPrefixKey], controllerCFG.ResourceTrackingConfiguration[config.BackendSGNamePrefixKey], controllerCFG.DefaultTags, ctrl.Log.WithName("backend-sg-provider")) + cloud.VpcID(), cloud.EC2(), mgr.GetClient(), clusterTagPrefix, controllerCFG.DefaultTags, ctrl.Log.WithName("backend-sg-provider")) sgResolver := networking.NewDefaultSecurityGroupResolver(cloud.EC2(), cloud.VpcID()) elbv2TaggingManager := elbv2deploy.NewDefaultTaggingManager(cloud.ELBV2(), cloud.VpcID(), controllerCFG.FeatureGates, cloud.RGT(), ctrl.Log) ingGroupReconciler := ingress.NewGroupReconciler(cloud, mgr.GetClient(), mgr.GetEventRecorderFor("ingress"), diff --git a/pkg/config/controller_config.go b/pkg/config/controller_config.go index f3fd6e1d3f..c0da47029e 100644 --- a/pkg/config/controller_config.go +++ b/pkg/config/controller_config.go @@ -1,7 +1,6 @@ package config import ( - "fmt" "strings" "time" @@ -14,10 +13,10 @@ import ( ) const ( - flagLogLevel = "log-level" - flagK8sClusterName = "cluster-name" - flagDefaultTags = "default-tags" - flagResourceTrackingConfiguration = "resource-tracking-configuration" + flagLogLevel = "log-level" + flagK8sClusterName = "cluster-name" + flagDefaultTags = "default-tags" + //flagResourceTrackingConfiguration = "resource-tracking-configuration" flagDefaultTargetType = "default-target-type" flagExternalManagedTags = "external-managed-tags" flagServiceTargetENISGTags = "service-target-eni-security-group-tags" @@ -30,17 +29,6 @@ const ( flagEnableEndpointSlices = "enable-endpoint-slices" flagDisableRestrictedSGRules = "disable-restricted-sg-rules" - ClusterTagPrefixKey = "clusterTagPrefix" - IngressTagPrefixKey = "ingressTagPrefix" - ServiceTagPrefixKey = "serviceTagPrefix" - BackendSGNamePrefixKey = "backendSGNamePrefix" - ClusterSgRuleLabelPrefixKey = "clusterSgRuleLabelPrefix" - - defaultClusterTagPrefix = "elbv2.k8s.aws" - defaultIngressTagPrefix = "ingress.k8s.aws" - defaultServiceTagPrefix = "service.k8s.aws" - defaultBackendSGNamePrefix = "k8s-traffic" - defaultClusterSgRuleLabelPrefix = "elbv2.k8s.aws" defaultLogLevel = "info" defaultMaxConcurrentReconciles = 3 defaultMaxExponentialBackoffDelay = time.Second * 1000 @@ -50,23 +38,6 @@ const ( defaultDisableRestrictedSGRules = false ) -var ( - validPrefixKeys = sets.NewString( - ClusterTagPrefixKey, - IngressTagPrefixKey, - ServiceTagPrefixKey, - BackendSGNamePrefixKey, - ClusterSgRuleLabelPrefixKey, - ) - defaultResourcePrefix = map[string]string{ - ClusterTagPrefixKey: defaultClusterTagPrefix, - IngressTagPrefixKey: defaultIngressTagPrefix, - ServiceTagPrefixKey: defaultServiceTagPrefix, - BackendSGNamePrefixKey: defaultBackendSGNamePrefix, - ClusterSgRuleLabelPrefixKey: defaultClusterSgRuleLabelPrefix, - } -) - // ControllerConfig contains the controller configuration type ControllerConfig struct { // Log level for the controller logs @@ -157,8 +128,6 @@ func (cfg *ControllerConfig) BindFlags(fs *pflag.FlagSet) { "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") - fs.StringToStringVar(&cfg.ResourceTrackingConfiguration, flagResourceTrackingConfiguration, defaultResourcePrefix, - "the prefixes for resource tags, backend SG name and worker node SG rules label.") cfg.FeatureGates.BindFlags(fs) cfg.AWSConfig.BindFlags(fs) @@ -175,17 +144,13 @@ func (cfg *ControllerConfig) Validate() error { return errors.New("kubernetes cluster name must be specified") } - if err := cfg.validateResourcePrefixKeys(); err != nil { - return err - } - trackingTagKeys := sets.New[string]( - cfg.ResourceTrackingConfiguration[ClusterTagPrefixKey]+"/cluster", - cfg.ResourceTrackingConfiguration[ClusterTagPrefixKey]+"/resource", - cfg.ResourceTrackingConfiguration[IngressTagPrefixKey]+"/stack", - cfg.ResourceTrackingConfiguration[IngressTagPrefixKey]+"/resource", - cfg.ResourceTrackingConfiguration[ServiceTagPrefixKey]+"/stack", - cfg.ResourceTrackingConfiguration[ServiceTagPrefixKey]+"/resource", + "elbv2.k8s.aws/cluster", + "elbv2.k8s.aws/resource", + "ingress.k8s.aws/stack", + "ingress.k8s.aws/resource", + "service.k8s.aws/stack", + "service.k8s.aws/resource", ) if err := cfg.validateDefaultTagsCollisionWithTrackingTags(trackingTagKeys); err != nil { @@ -252,18 +217,3 @@ func (cfg *ControllerConfig) validateBackendSecurityGroupConfiguration() error { } return nil } - -func (cfg *ControllerConfig) validateResourcePrefixKeys() error { - keys := make([]string, 0, len(cfg.ResourceTrackingConfiguration)) - for key := range cfg.ResourceTrackingConfiguration { - if !validPrefixKeys.Has(key) { - return fmt.Errorf("invalid key: %s. Valid keys are: %v", key, validPrefixKeys.List()) - } - keys = append(keys, key) - } - if len(keys) != len(validPrefixKeys.List()) { - return fmt.Errorf("invalid number of keys. Expected %d keys, but got %d keys", - len(validPrefixKeys.List()), len(keys)) - } - return nil -} diff --git a/pkg/config/controller_config_test.go b/pkg/config/controller_config_test.go index a92547ee25..8727ac5967 100644 --- a/pkg/config/controller_config_test.go +++ b/pkg/config/controller_config_test.go @@ -181,66 +181,3 @@ func TestControllerConfig_validateExternalManagedTagsCollisionWithDefaultTags(t }) } } - -func TestControllerConfig_validateResourcePrefixKeys(t *testing.T) { - type fields struct { - ResourceTrackingConfiguration map[string]string - } - tests := []struct { - name string - fields fields - wantErr error - }{ - { - name: "resource prefix has all keys", - fields: fields{ - ResourceTrackingConfiguration: map[string]string{ - "clusterTagPrefix": "elbv2.k8s.aws", - "ingressTagPrefix": "ingress.k8s.aws", - "serviceTagPrefix": "service.k8s.aws", - "backendSGNamePrefix": "k8s-traffic", - "clusterSgRuleLabelPrefix": "elbv2.k8s.aws", - }, - }, - wantErr: nil, - }, - { - name: "resource prefix has some invalid keys", - fields: fields{ - ResourceTrackingConfiguration: map[string]string{ - "clusterTagPrefix": "elbv2.k8s.aws", - "ingressTagPrefix": "ingress.k8s.aws", - "serviceTagPrefix": "service.k8s.aws", - "backendSGNamePrefix": "k8s-traffic", - "myKey": "myVal", - }, - }, - wantErr: errors.New("invalid key: myKey. Valid keys are: [backendSGNamePrefix clusterSgRuleLabelPrefix clusterTagPrefix ingressTagPrefix serviceTagPrefix]"), - }, - { - name: "resource prefix is missing some valid keys", - fields: fields{ - ResourceTrackingConfiguration: map[string]string{ - "clusterTagPrefix": "elbv2.k8s.aws", - "ingressTagPrefix": "ingress.k8s.aws", - "serviceTagPrefix": "service.k8s.aws", - "backendSGNamePrefix": "k8s-traffic", - }, - }, - wantErr: errors.New("invalid number of keys. Expected 5 keys, but got 4 keys"), - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cfg := &ControllerConfig{ - ResourceTrackingConfiguration: tt.fields.ResourceTrackingConfiguration, - } - err := cfg.validateResourcePrefixKeys() - if tt.wantErr != nil { - assert.EqualError(t, err, tt.wantErr.Error()) - } else { - assert.NoError(t, err) - } - }) - } -} diff --git a/pkg/networking/backend_sg_provider.go b/pkg/networking/backend_sg_provider.go index ad556fd0a9..dae4b54588 100644 --- a/pkg/networking/backend_sg_provider.go +++ b/pkg/networking/backend_sg_provider.go @@ -57,19 +57,18 @@ type BackendSGProvider interface { // NewBackendSGProvider constructs a new defaultBackendSGProvider func NewBackendSGProvider(clusterName string, backendSG string, vpcID string, - ec2Client services.EC2, k8sClient client.Client, clusterTagPrefixKey string, backendSGNamePrefix string, defaultTags map[string]string, logger logr.Logger) *defaultBackendSGProvider { + ec2Client services.EC2, k8sClient client.Client, clusterTagPrefixKey string, defaultTags map[string]string, logger logr.Logger) *defaultBackendSGProvider { return &defaultBackendSGProvider{ - vpcID: vpcID, - clusterName: clusterName, - backendSG: backendSG, - tagKeyK8sCluster: clusterTagPrefixKey + "/cluster", - tagKeyResource: clusterTagPrefixKey + "/resource", - backendSGNamePrefix: backendSGNamePrefix, - defaultTags: defaultTags, - ec2Client: ec2Client, - k8sClient: k8sClient, - logger: logger, - mutex: sync.Mutex{}, + vpcID: vpcID, + clusterName: clusterName, + backendSG: backendSG, + tagKeyK8sCluster: clusterTagPrefixKey + "/cluster", + tagKeyResource: clusterTagPrefixKey + "/resource", + defaultTags: defaultTags, + ec2Client: ec2Client, + k8sClient: k8sClient, + logger: logger, + mutex: sync.Mutex{}, checkIngressFinalizersFunc: func(finalizers []string) bool { for _, fin := range finalizers { @@ -101,15 +100,14 @@ type defaultBackendSGProvider struct { clusterName string mutex sync.Mutex - backendSG string - autoGeneratedSG string - tagKeyK8sCluster string - tagKeyResource string - backendSGNamePrefix string - defaultTags map[string]string - ec2Client services.EC2 - k8sClient client.Client - logger logr.Logger + backendSG string + autoGeneratedSG string + tagKeyK8sCluster string + tagKeyResource string + defaultTags map[string]string + ec2Client services.EC2 + k8sClient client.Client + logger logr.Logger // objectsMap keeps track of whether the backend SG is required for any tracked resources in the cluster. // If any entry in the map is true, or there are resources with this controller specific finalizers which // haven't been tracked in the map yet, controller doesn't delete the backend SG. If the controller has @@ -348,7 +346,7 @@ func (p *defaultBackendSGProvider) getBackendSGName() string { _, _ = sgNameHash.Write([]byte(p.clusterName)) sgHash := hex.EncodeToString(sgNameHash.Sum(nil)) sanitizedClusterName := invalidSGNamePattern.ReplaceAllString(p.clusterName, "") - return fmt.Sprintf("%v-%.232s-%.10s", p.backendSGNamePrefix, sanitizedClusterName, sgHash) + return fmt.Sprintf("k8s-traffic-%.232s-%.10s", sanitizedClusterName, sgHash) } func isSecurityGroupDependencyViolationError(err error) bool { diff --git a/pkg/networking/backend_sg_provider_test.go b/pkg/networking/backend_sg_provider_test.go index 8eb4093a32..4f82488e7f 100644 --- a/pkg/networking/backend_sg_provider_test.go +++ b/pkg/networking/backend_sg_provider_test.go @@ -300,7 +300,7 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) { } k8sClient := mock_client.NewMockClient(ctrl) sgProvider := NewBackendSGProvider(defaultClusterName, tt.fields.backendSG, - defaultVPCID, ec2Client, k8sClient, tt.fields.clusterTagPrefixKey, tt.fields.backendSGNamePrefix, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) + defaultVPCID, ec2Client, k8sClient, tt.fields.clusterTagPrefixKey, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) resourceType := ResourceTypeIngress var activeResources []types.NamespacedName @@ -749,7 +749,7 @@ func Test_defaultBackendSGProvider_Release(t *testing.T) { ec2Client := services.NewMockEC2(ctrl) k8sClient := mock_client.NewMockClient(ctrl) sgProvider := NewBackendSGProvider(defaultClusterName, tt.fields.backendSG, - defaultVPCID, ec2Client, k8sClient, tt.fields.clusterTagPrefixKey, tt.fields.backendSGNamePrefix, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) + defaultVPCID, ec2Client, k8sClient, tt.fields.clusterTagPrefixKey, tt.fields.defaultTags, logr.New(&log.NullLogSink{})) if len(tt.fields.autogenSG) > 0 { sgProvider.backendSG = "" sgProvider.autoGeneratedSG = tt.fields.autogenSG