From ec5baade68c1e34316e1019df7f3ee037d80e3a2 Mon Sep 17 00:00:00 2001 From: Vishwanath Taykhande Date: Mon, 25 Nov 2024 17:55:08 +0530 Subject: [PATCH 1/3] EKS Security Group tag updation from AWSManagedControlPlane --- pkg/cloud/services/eks/securitygroup.go | 77 ++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/pkg/cloud/services/eks/securitygroup.go b/pkg/cloud/services/eks/securitygroup.go index 829de2fcab..7bf36a972d 100644 --- a/pkg/cloud/services/eks/securitygroup.go +++ b/pkg/cloud/services/eks/securitygroup.go @@ -19,6 +19,8 @@ package eks import ( "context" "fmt" + "reflect" + "strings" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -73,11 +75,84 @@ func (s *Service) reconcileSecurityGroups(cluster *eks.Cluster) error { return fmt.Errorf("describing EKS cluster security group: %w", err) } - s.scope.ControlPlane.Status.Network.SecurityGroups[ekscontrolplanev1.SecurityGroupCluster] = infrav1.SecurityGroup{ + clusterSecurityGroup := infrav1.SecurityGroup{ ID: aws.StringValue(cluster.ResourcesVpcConfig.ClusterSecurityGroupId), Name: *output.SecurityGroups[0].GroupName, Tags: converters.TagsToMap(output.SecurityGroups[0].Tags), } + s.scope.ControlPlane.Status.Network.SecurityGroups[ekscontrolplanev1.SecurityGroupCluster] = clusterSecurityGroup + + additionalTags := s.scope.ControlPlane.Spec.AdditionalTags + if !reflect.DeepEqual(sg.Tags, desiredTags(sg.Tags, additionalTags)) { + if err = s.updateTagsForEKSManagedSecurityGroup(&sg.ID, sg.Tags, desiredTags(sg.Tags, additionalTags)); err != nil { + return err + } + } + + if !reflect.DeepEqual(clusterSecurityGroup.Tags, desiredTags(clusterSecurityGroup.Tags, additionalTags)) { + if err = s.updateTagsForEKSManagedSecurityGroup(&clusterSecurityGroup.ID, clusterSecurityGroup.Tags, desiredTags(clusterSecurityGroup.Tags, additionalTags)); err != nil { + return err + } + } + + return nil +} + +// desiredTags will return the default tags of EKS cluster with the spec.additionalTags from AWSManagedControlPlane +func desiredTags(existingTags, additionalTags infrav1.Tags) infrav1.Tags { + merged := make(infrav1.Tags) + + for key, value := range existingTags { + // since the cluster is created/managed by CAPA, existing tags will contain the default EKS security group tags + if key == "aws:eks:cluster-name" || key == "Name" || strings.Contains(key, infrav1.NameKubernetesAWSCloudProviderPrefix) { + merged[key] = value + } + } + + // additional tags from spec.additionalTags of AWSManagedControlPlane will be added/updated to desired tags considering them as source of truth + for key, value := range additionalTags { + merged[key] = value + } + + return merged +} + +// updateTagsForEKSManagedSecurityGroup will update the tags in the EKS security group with the desired tags via create/update/delete operations +func (s *Service) updateTagsForEKSManagedSecurityGroup(securityGroupID *string, existingTags, desiredTags infrav1.Tags) error { + tagsToDelete, newTags := getTagUpdates(existingTags, desiredTags) + + // Create tags for updating or adding + if len(newTags) > 0 { + desiredTags := converters.MapToTags(newTags) + _, err := s.EC2Client.CreateTags(&ec2.CreateTagsInput{ + Resources: []*string{securityGroupID}, + Tags: desiredTags, + }) + if err != nil { + return fmt.Errorf("failed to create/update tags: %v", err) + } + } + + // Delete tags + if len(tagsToDelete) > 0 { + var ec2TagKeys []*ec2.Tag + for _, key := range tagsToDelete { + // the default tags added to EKS cluster via AWS will not be deleted + if key != "aws:eks:cluster-name" && key != "Name" && !strings.Contains(key, infrav1.NameKubernetesAWSCloudProviderPrefix) { + ec2TagKeys = append(ec2TagKeys, &ec2.Tag{ + Key: aws.String(key), + }) + } + } + + _, err := s.EC2Client.DeleteTags(&ec2.DeleteTagsInput{ + Resources: []*string{securityGroupID}, + Tags: ec2TagKeys, + }) + if err != nil { + return fmt.Errorf("failed to delete tags: %v", err) + } + } return nil } From d023872a4c0e0da2c6cdaabb10990157670be292 Mon Sep 17 00:00:00 2001 From: Vishwanath Taykhande Date: Wed, 27 Nov 2024 08:46:15 +0530 Subject: [PATCH 2/3] Added tests --- pkg/cloud/services/eks/securitygroup_test.go | 224 +++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 pkg/cloud/services/eks/securitygroup_test.go diff --git a/pkg/cloud/services/eks/securitygroup_test.go b/pkg/cloud/services/eks/securitygroup_test.go new file mode 100644 index 0000000000..5d199b1ce3 --- /dev/null +++ b/pkg/cloud/services/eks/securitygroup_test.go @@ -0,0 +1,224 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package eks + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/golang/mock/gomock" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + + "github.com/aws/aws-sdk-go/service/ec2" + infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" + ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2" + + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/converters" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/eks/mock_eksiface" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/iamauth/mock_iamauth" + "sigs.k8s.io/cluster-api-provider-aws/v2/test/mocks" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestDesiredTags(t *testing.T) { + g := NewWithT(t) + + existingTags := infrav1.Tags{ + "aws:eks:cluster-name": "cluster-name", + "kubernetes.io/cluster/cluster-name": "owned", + "Name": "eks-cluster-sg-cluster-name-1488456827", + "tagKeyNotToBeIncluded": "tagValueNotToBeIncluded", + "sampleTag": "sampleOldValue", + } + + additionalTags := infrav1.Tags{ + "hello": "there", + "new": "tag", + "tagKey": "tagValue", + "sampleTag": "sampleNewValue", + } + + expectedDesiredTags := infrav1.Tags{ + "aws:eks:cluster-name": "cluster-name", + "kubernetes.io/cluster/cluster-name": "owned", + "Name": "eks-cluster-sg-cluster-name-1488456827", + "hello": "there", + "new": "tag", + "tagKey": "tagValue", + "sampleTag": "sampleNewValue", + } + + desiredTags := desiredTags(existingTags, additionalTags) + g.Expect(desiredTags).To(Equal(expectedDesiredTags)) +} + +func TestUpdateTagsForEKSManagedSecurityGroup(t *testing.T) { + g := NewWithT(t) + + mockControl := gomock.NewController(t) + defer mockControl.Finish() + + eksMock := mock_eksiface.NewMockEKSAPI(mockControl) + iamMock := mock_iamauth.NewMockIAMAPI(mockControl) + ec2Mock := mocks.NewMockEC2API(mockControl) + + scheme := runtime.NewScheme() + _ = infrav1.AddToScheme(scheme) + _ = ekscontrolplanev1.AddToScheme(scheme) + client := fake.NewClientBuilder().WithScheme(scheme).Build() + vpcSpec := infrav1.VPCSpec{ + IPv6: &infrav1.IPv6{ + CidrBlock: "2001:db8:85a3::/56", + }, + } + scope, err := scope.NewManagedControlPlaneScope(scope.ManagedControlPlaneScopeParams{ + Client: client, + Cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "cluster-name", + }, + }, + ControlPlane: &ekscontrolplanev1.AWSManagedControlPlane{ + Spec: ekscontrolplanev1.AWSManagedControlPlaneSpec{ + RoleName: ptr.To[string]("arn-role"), + Version: aws.String("1.29"), + Region: "us-east-1", + NetworkSpec: infrav1.NetworkSpec{ + Subnets: []infrav1.SubnetSpec{ + { + ID: "sub-1", + CidrBlock: "10.0.10.0/24", + AvailabilityZone: "us-west-2a", + IsPublic: true, + IsIPv6: true, + IPv6CidrBlock: "2001:db8:85a3:1::/64", + }, + { + ID: "sub-2", + CidrBlock: "10.0.10.0/24", + AvailabilityZone: "us-west-2b", + IsPublic: false, + IsIPv6: true, + IPv6CidrBlock: "2001:db8:85a3:2::/64", + }, + }, + VPC: vpcSpec, + }, + }, + }, + }) + g.Expect(err).To(BeNil()) + + s := NewService(scope) + s.EKSClient = eksMock + s.IAMClient = iamMock + s.EC2Client = ec2Mock + + existingTags := infrav1.Tags{ + "aws:eks:cluster-name": "cluster-name", + "kubernetes.io/cluster/cluster-name": "owned", + "Name": "eks-cluster-sg-cluster-name-1488456827", + } + + desiredTags := infrav1.Tags{ + "hello": "there", + "new": "tag", + "aws:eks:cluster-name": "cluster-name", + "kubernetes.io/cluster/cluster-name": "owned", + "Name": "eks-cluster-sg-cluster-name-1488456827", + "tagKey": "tagValue", + "sampleTag": "sampleNewValue", + } + + sampleEKSecurityGroupID := aws.String("sg-025f2495c64d5") + + createSGinput := &ec2.CreateSecurityGroupInput{ + GroupName: sampleEKSecurityGroupID, + TagSpecifications: []*ec2.TagSpecification{ + { + Tags: converters.MapToTags(desiredTags), + }, + }, + } + + ec2Mock.EXPECT().CreateSecurityGroup(createSGinput).Return(&ec2.CreateSecurityGroupOutput{ + GroupId: sampleEKSecurityGroupID, + }, nil) + + _, err = ec2Mock.CreateSecurityGroup(createSGinput) + g.Expect(err).To(BeNil()) + + ec2Mock.EXPECT().CreateTags(&ec2.CreateTagsInput{ + Resources: []*string{sampleEKSecurityGroupID}, + Tags: []*ec2.Tag{ + { + Key: aws.String("hello"), + Value: aws.String("there"), + }, + { + Key: aws.String("new"), + Value: aws.String("tag"), + }, + { + Key: aws.String("sampleTag"), + Value: aws.String("sampleNewValue"), + }, + { + Key: aws.String("tagKey"), + Value: aws.String("tagValue"), + }, + }, + }).Return(nil, nil) + + err = s.updateTagsForEKSManagedSecurityGroup(sampleEKSecurityGroupID, existingTags, desiredTags) + g.Expect(err).To(BeNil()) + + describeSGinput := &ec2.DescribeSecurityGroupsInput{ + GroupIds: []*string{sampleEKSecurityGroupID}, + } + + ec2Mock.EXPECT().DescribeSecurityGroups(describeSGinput).Return(&ec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []*ec2.SecurityGroup{ + { + GroupId: sampleEKSecurityGroupID, + Tags: converters.MapToTags(desiredTags), + Description: aws.String("EKS created security group applied to ENI that is attached to EKS Control Plane master nodes, as well as any managed workloads."), + }, + }, + }, nil) + + describeOutput, err := ec2Mock.DescribeSecurityGroups(&ec2.DescribeSecurityGroupsInput{ + GroupIds: []*string{sampleEKSecurityGroupID}, + }) + g.Expect(err).To(BeNil()) + + // Denotes that this EKS security group is created by AWS. + g.Expect(*describeOutput.SecurityGroups[0].Description).To(Equal("EKS created security group applied to ENI that is attached to EKS Control Plane master nodes, as well as any managed workloads.")) + + g.Expect(len(describeOutput.SecurityGroups[0].Tags)).To(Equal(len(desiredTags))) + + for key, value := range desiredTags { + g.Expect(converters.TagsToMap(describeOutput.SecurityGroups[0].Tags)).To(HaveKeyWithValue(key, value)) + } + +} From 8e4fcba9e65478db4d6361c2436bb72cb5e5cade Mon Sep 17 00:00:00 2001 From: Vishwanath Taykhande Date: Thu, 5 Dec 2024 21:09:32 +0530 Subject: [PATCH 3/3] Fixing lint --- pkg/cloud/services/eks/securitygroup.go | 4 ++-- pkg/cloud/services/eks/securitygroup_test.go | 8 +++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/cloud/services/eks/securitygroup.go b/pkg/cloud/services/eks/securitygroup.go index 7bf36a972d..2e52287c06 100644 --- a/pkg/cloud/services/eks/securitygroup.go +++ b/pkg/cloud/services/eks/securitygroup.go @@ -98,7 +98,7 @@ func (s *Service) reconcileSecurityGroups(cluster *eks.Cluster) error { return nil } -// desiredTags will return the default tags of EKS cluster with the spec.additionalTags from AWSManagedControlPlane +// desiredTags will return the default tags of EKS cluster with the spec.additionalTags from AWSManagedControlPlane. func desiredTags(existingTags, additionalTags infrav1.Tags) infrav1.Tags { merged := make(infrav1.Tags) @@ -117,7 +117,7 @@ func desiredTags(existingTags, additionalTags infrav1.Tags) infrav1.Tags { return merged } -// updateTagsForEKSManagedSecurityGroup will update the tags in the EKS security group with the desired tags via create/update/delete operations +// updateTagsForEKSManagedSecurityGroup will update the tags in the EKS security group with the desired tags via create/update/delete operations. func (s *Service) updateTagsForEKSManagedSecurityGroup(securityGroupID *string, existingTags, desiredTags infrav1.Tags) error { tagsToDelete, newTags := getTagUpdates(existingTags, desiredTags) diff --git a/pkg/cloud/services/eks/securitygroup_test.go b/pkg/cloud/services/eks/securitygroup_test.go index 5d199b1ce3..c7b1119297 100644 --- a/pkg/cloud/services/eks/securitygroup_test.go +++ b/pkg/cloud/services/eks/securitygroup_test.go @@ -20,23 +20,22 @@ import ( "testing" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" - "github.com/aws/aws-sdk-go/service/ec2" infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2" - "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/converters" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/eks/mock_eksiface" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/iamauth/mock_iamauth" "sigs.k8s.io/cluster-api-provider-aws/v2/test/mocks" - "sigs.k8s.io/controller-runtime/pkg/client/fake" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) func TestDesiredTags(t *testing.T) { @@ -220,5 +219,4 @@ func TestUpdateTagsForEKSManagedSecurityGroup(t *testing.T) { for key, value := range desiredTags { g.Expect(converters.TagsToMap(describeOutput.SecurityGroups[0].Tags)).To(HaveKeyWithValue(key, value)) } - }