Skip to content

Commit

Permalink
add limitation for 24 tags
Browse files Browse the repository at this point in the history 
Signed-off-by: chiragkyal <[email protected]>
  • Loading branch information
@chiragkyal
chiragkyal committed Sep 30, 2024
1 parent 697ad4b commit deb501d
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 39 deletions.
2 changes: 1 addition & 1 deletion pkg/controllers/awsloadbalancercontroller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func (r *AWSLoadBalancerControllerReconciler) BuildManagedController(mgr ctrl.Ma
handler.EnqueueRequestsFromMapFunc(clusterALBCInstance),
builder.WithPredicates(
predicate.NewPredicateFuncs(hasName(clusterInfrastructureName))))
log.Log.Info("Done with Builder")

return bldr
}

Expand Down
36 changes: 22 additions & 14 deletions pkg/controllers/awsloadbalancercontroller/deployment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,11 @@ func (r *AWSLoadBalancerControllerReconciler) ensureDeployment(ctx context.Conte
trustCAConfigMapHash = configMapHash
}

desired := r.desiredDeployment(deploymentName, crSecretName, servingSecretName, controller, platformStatus, sa, trustCAConfigMapName, trustCAConfigMapHash)
desired, err := r.desiredDeployment(deploymentName, crSecretName, servingSecretName, controller, platformStatus, sa, trustCAConfigMapName, trustCAConfigMapHash)
if err != nil {
return nil, fmt.Errorf("failed to get desired deployment %s: %w", deploymentName, err)
}

err = controllerutil.SetControllerReference(controller, desired, r.Scheme)
if err != nil {
return nil, fmt.Errorf("failed to set owner reference on deployment %s: %w", deploymentName, err)
Expand Down Expand Up @@ -122,7 +126,11 @@ func (r *AWSLoadBalancerControllerReconciler) ensureDeployment(ctx context.Conte
return current, nil
}

func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credentialsRequestSecretName, servingSecret string, controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, sa *corev1.ServiceAccount, trustedCAConfigMapName, trustedCAConfigMapHash string) *appsv1.Deployment {
func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credentialsRequestSecretName, servingSecret string, controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, sa *corev1.ServiceAccount, trustedCAConfigMapName, trustedCAConfigMapHash string) (*appsv1.Deployment, error) {
containerArgs, err := desiredContainerArgs(controller, platformStatus, r.ClusterName, r.VPCID)
if err != nil {
return nil, fmt.Errorf("failed to get container args: %w", err)
}
d := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -147,7 +155,7 @@ func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credential
{
Name: awsLoadBalancerControllerContainerName,
Image: r.Image,
Args: desiredContainerArgs(controller, platformStatus, r.ClusterName, r.VPCID),
Args: containerArgs,
Env: append([]corev1.EnvVar{
{
Name: awsRegionEnvVarName,
Expand Down Expand Up @@ -259,23 +267,21 @@ func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credential
})
}
}
return d
return d, nil
}

func desiredContainerArgs(controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, clusterName, vpcID string) []string {
func desiredContainerArgs(controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, clusterName, vpcID string) ([]string, error) {
var args []string
args = append(args, fmt.Sprintf("--webhook-cert-dir=%s", webhookTLSDir))
args = append(args, fmt.Sprintf("--aws-vpc-id=%s", vpcID))
args = append(args, fmt.Sprintf("--cluster-name=%s", clusterName))

tags := mergeTags(controller, platformStatus)
// AWS supports a maximum of 50 tags per resource
// FIXME: AWSLoadBalancerController reserves 3 tags and allows 24 user tags
// platformStatus.AWS.ResourceTags allows 25 user tags
// (3+24+25) > 50 -- fix is needed
if len(tags) > 50 {
log.Log.Error(fmt.Errorf("max 50 tags are allowed"), "got", "tags", len(tags))
// TODO return error
// `--default-tags` arg allows a maximum of 24 user tags, but the combination of
// controller.Spec.AdditionalResourceTags and platformStatus.AWS.ResourceTags can result in more.
// Ensure a maximum of 24 merged tags are added.
if len(tags) > 24 {
return nil, fmt.Errorf("exceeded maximum of 24 allowed tags, got %d", len(tags))
}
if len(tags) > 0 {
sort.Strings(tags)
Expand Down Expand Up @@ -308,7 +314,7 @@ func desiredContainerArgs(controller *albo.AWSLoadBalancerController, platformSt
args = append(args, fmt.Sprintf("--ingress-class=%s", controller.Spec.IngressClass))
args = append(args, "--feature-gates=EnableIPTargetType=false")
sort.Strings(args)
return args
return args, nil
}

func (r *AWSLoadBalancerControllerReconciler) currentDeployment(ctx context.Context, name string, namespace string) (bool, *appsv1.Deployment, error) {
Expand Down Expand Up @@ -549,6 +555,9 @@ func buildMapHash(data map[string]string) (string, error) {
return hex.EncodeToString(hash.Sum(nil)), nil
}

// mergeTags merges tags from an AWSLoadBalancerController and PlatformStatus into a slice of strings.
// Tags from `controller.Spec.AdditionalResourceTags` take precedence over those in `platformStatus.AWS.ResourceTags`.
// If a tag key already exists, the value from AdditionalResourceTags will overwrite the one from ResourceTags.
func mergeTags(controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus) []string {
// tagMap holds tags with unique keys
tagMap := make(map[string]string)
Expand All @@ -562,7 +571,6 @@ func mergeTags(controller *albo.AWSLoadBalancerController, platformStatus *confi

// Add tags from platformStatus.AWS.ResourceTags to the map, only if the key doesn't exist
if platformStatus.AWS != nil && len(platformStatus.AWS.ResourceTags) > 0 {
log.Log.Info("@chirag: platform.AWS.ResourceTags", "tags", platformStatus.AWS.ResourceTags)
for _, t := range platformStatus.AWS.ResourceTags {
if len(t.Key) > 0 {
if _, exists := tagMap[t.Key]; !exists {
Expand Down
75 changes: 51 additions & 24 deletions pkg/controllers/awsloadbalancercontroller/deployment_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,13 @@ const (
testAWSRegion = "us-east-1"
)

// TODO: example platformStatus
var platformStatus = &configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSPlatformStatus{
ResourceTags: []configv1.AWSResourceTag{
// {
// Key: "Key1",
// Value: "Value1",
// },
},
},
}

func TestDesiredArgs(t *testing.T) {
for _, tc := range []struct {
name string
controller *albo.AWSLoadBalancerController
expectedArgs sets.Set[string]
platformStatus *configv1.PlatformStatus
expectedArgs sets.Set[string]
expectedError bool
}{
{
name: "non-default ingress class",
Expand Down Expand Up @@ -222,6 +210,21 @@ func TestDesiredArgs(t *testing.T) {
"--default-tags=op-key1=op-value1,op-key2=op-value2,plat-key1=plat-value1,plat-key2=plat-value2",
),
},
{
name: "when merged tags exceeded maximum of 24 allowed tags",
controller: &albo.AWSLoadBalancerController{
Spec: albo.AWSLoadBalancerControllerSpec{
AdditionalResourceTags: []albo.AWSResourceTag{},
},
},
platformStatus: &configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSPlatformStatus{
ResourceTags: generateAWSResourceTags(25),
},
},
expectedError: true,
},
} {
t.Run(tc.name, func(t *testing.T) {
defaultArgs := sets.New[string](
Expand All @@ -239,12 +242,16 @@ func TestDesiredArgs(t *testing.T) {
if tc.platformStatus == nil {
tc.platformStatus = &configv1.PlatformStatus{}
}
args := desiredContainerArgs(tc.controller, tc.platformStatus, "test-cluster", "test-vpc")

expected := sets.List(expectedArgs)
sort.Strings(expected)
if diff := cmp.Diff(expected, args); diff != "" {
t.Fatalf("unexpected arguments\n%s", diff)
args, gotErr := desiredContainerArgs(tc.controller, tc.platformStatus, "test-cluster", "test-vpc")
if (gotErr != nil) != tc.expectedError {
t.Fatalf("expected errors to be %t, but got %t", tc.expectedError, gotErr != nil)
}
if !tc.expectedError {
expected := sets.List(expectedArgs)
sort.Strings(expected)
if diff := cmp.Diff(expected, args); diff != "" {
t.Fatalf("unexpected arguments\n%s", diff)
}
}
})
}
Expand Down Expand Up @@ -840,11 +847,15 @@ func TestEnsureDeployment(t *testing.T) {
VPCID: "test-vpc",
AWSRegion: testAWSRegion,
}
_, err := r.ensureDeployment(context.Background(), tc.serviceAccount, "test-credentials", "test-serving", tc.controller, platformStatus, tc.trustedCAConfigMap)
_, err := r.ensureDeployment(context.Background(), tc.serviceAccount, "test-credentials", "test-serving", tc.controller, &configv1.PlatformStatus{}, tc.trustedCAConfigMap)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
tc.expectedDeployment.Spec.Template.Spec.Containers[0].Args = desiredContainerArgs(tc.controller, platformStatus, "test-cluster", "test-vpc")
args, err := desiredContainerArgs(tc.controller, &configv1.PlatformStatus{}, "test-cluster", "test-vpc")
if err != nil {
t.Fatalf("failed to get container args: %v", err)
}
tc.expectedDeployment.Spec.Template.Spec.Containers[0].Args = args
var deployment appsv1.Deployment
err = client.Get(context.Background(), types.NamespacedName{Namespace: "test-namespace", Name: fmt.Sprintf("%s-%s", controllerResourcePrefix, tc.controller.Name)}, &deployment)
if err != nil {
Expand Down Expand Up @@ -951,11 +962,15 @@ func TestEnsureDeploymentEnvVars(t *testing.T) {
VPCID: "test-vpc",
AWSRegion: testAWSRegion,
}
_, err := r.ensureDeployment(context.Background(), tc.serviceAccount, "test-credentials", "test-serving", tc.controller, platformStatus, nil)
_, err := r.ensureDeployment(context.Background(), tc.serviceAccount, "test-credentials", "test-serving", tc.controller, &configv1.PlatformStatus{}, nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
tc.expectedDeployment.Spec.Template.Spec.Containers[0].Args = desiredContainerArgs(tc.controller, platformStatus, "test-cluster", "test-vpc")
args, err := desiredContainerArgs(tc.controller, &configv1.PlatformStatus{}, "test-cluster", "test-vpc")
if err != nil {
t.Fatalf("failed to get container args: %v", err)
}
tc.expectedDeployment.Spec.Template.Spec.Containers[0].Args = args
var deployment appsv1.Deployment
err = client.Get(context.Background(), types.NamespacedName{Namespace: "test-namespace", Name: fmt.Sprintf("%s-%s", controllerResourcePrefix, tc.controller.Name)}, &deployment)
if err != nil {
Expand Down Expand Up @@ -1330,3 +1345,15 @@ func (b *testContainerBuilder) build() corev1.Container {
SecurityContext: b.securityContext,
}
}

// generateAWSResourceTags generates AWSResourceTags with a length of `n`.
func generateAWSResourceTags(n int) []configv1.AWSResourceTag {
tags := make([]configv1.AWSResourceTag, n)
for i := 0; i < n; i++ {
tags[i] = configv1.AWSResourceTag{
Key: fmt.Sprintf("key-%d", i),
Value: fmt.Sprintf("value-%d", i),
}
}
return tags
}

0 comments on commit deb501d

Please sign in to comment.