From 3bce2be8045c65209153824da28d22017d22a725 Mon Sep 17 00:00:00 2001 From: Amanuel Engeda <74629455+engedaam@users.noreply.github.com> Date: Tue, 23 Jan 2024 16:28:47 -0800 Subject: [PATCH] fix: Subnet and Security Groups to use in-memory values for evaluating Drift (#5518) --- pkg/cloudprovider/drift.go | 39 ++++++++----- pkg/cloudprovider/suite_test.go | 100 +++++++++++++++++++++----------- 2 files changed, 93 insertions(+), 46 deletions(-) diff --git a/pkg/cloudprovider/drift.go b/pkg/cloudprovider/drift.go index 7fa085ebcd5a..fe259ef397d1 100644 --- a/pkg/cloudprovider/drift.go +++ b/pkg/cloudprovider/drift.go @@ -25,6 +25,9 @@ import ( corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1" "sigs.k8s.io/karpenter/pkg/cloudprovider" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1" "github.com/aws/karpenter-provider-aws/pkg/providers/amifamily" "github.com/aws/karpenter-provider-aws/pkg/providers/instance" @@ -51,11 +54,11 @@ func (c *CloudProvider) isNodeClassDrifted(ctx context.Context, nodeClaim *corev if err != nil { return "", fmt.Errorf("calculating ami drift, %w", err) } - securitygroupDrifted, err := c.areSecurityGroupsDrifted(instance, nodeClass) + securitygroupDrifted, err := c.areSecurityGroupsDrifted(ctx, instance, nodeClass) if err != nil { return "", fmt.Errorf("calculating securitygroup drift, %w", err) } - subnetDrifted, err := c.isSubnetDrifted(instance, nodeClass) + subnetDrifted, err := c.isSubnetDrifted(ctx, instance, nodeClass) if err != nil { return "", fmt.Errorf("calculating subnet drift, %w", err) } @@ -91,28 +94,38 @@ func (c *CloudProvider) isAMIDrifted(ctx context.Context, nodeClaim *corev1beta1 return "", nil } -// Checks if the security groups are drifted, by comparing the EC2NodeClass.Status.Subnets +// Checks if the security groups are drifted, by comparing the subnet returned from the subnetProvider // to the ec2 instance subnets -func (c *CloudProvider) isSubnetDrifted(instance *instance.Instance, nodeClass *v1beta1.EC2NodeClass) (cloudprovider.DriftReason, error) { - // If the node template status does not have subnets, wait for the subnets to be populated before continuing - if len(nodeClass.Status.Subnets) == 0 { - return "", fmt.Errorf("no subnets exist in status") +func (c *CloudProvider) isSubnetDrifted(ctx context.Context, instance *instance.Instance, nodeClass *v1beta1.EC2NodeClass) (cloudprovider.DriftReason, error) { + subnets, err := c.subnetProvider.List(ctx, nodeClass) + if err != nil { + return "", err } - _, found := lo.Find(nodeClass.Status.Subnets, func(subnet v1beta1.Subnet) bool { - return subnet.ID == instance.SubnetID + // subnets need to be found to check for drift + if len(subnets) == 0 { + return "", fmt.Errorf("no subnets are discovered") + } + + _, found := lo.Find(subnets, func(subnet *ec2.Subnet) bool { + return aws.StringValue(subnet.SubnetId) == instance.SubnetID }) + if !found { return SubnetDrift, nil } return "", nil } -// Checks if the security groups are drifted, by comparing the EC2NodeClass.Status.SecurityGroups +// Checks if the security groups are drifted, by comparing the security groups returned from the SecurityGroupProvider // to the ec2 instance security groups -func (c *CloudProvider) areSecurityGroupsDrifted(ec2Instance *instance.Instance, nodeClass *v1beta1.EC2NodeClass) (cloudprovider.DriftReason, error) { - securityGroupIds := sets.New(lo.Map(nodeClass.Status.SecurityGroups, func(sg v1beta1.SecurityGroup, _ int) string { return sg.ID })...) +func (c *CloudProvider) areSecurityGroupsDrifted(ctx context.Context, ec2Instance *instance.Instance, nodeClass *v1beta1.EC2NodeClass) (cloudprovider.DriftReason, error) { + securitygroup, err := c.securityGroupProvider.List(ctx, nodeClass) + if err != nil { + return "", err + } + securityGroupIds := sets.New(lo.Map(securitygroup, func(sg *ec2.SecurityGroup, _ int) string { return aws.StringValue(sg.GroupId) })...) if len(securityGroupIds) == 0 { - return "", fmt.Errorf("no security groups exist in status") + return "", fmt.Errorf("no security groups are discovered") } if !securityGroupIds.Equal(sets.New(ec2Instance.SecurityGroupIDs...)) { diff --git a/pkg/cloudprovider/suite_test.go b/pkg/cloudprovider/suite_test.go index 6fa50f8cbe5e..9cc73de9837e 100644 --- a/pkg/cloudprovider/suite_test.go +++ b/pkg/cloudprovider/suite_test.go @@ -210,31 +210,65 @@ var _ = Describe("CloudProvider", func() { ImageId: aws.String(armAMIID), Architecture: aws.String("arm64"), CreationDate: aws.String("2022-08-15T12:00:00Z"), + Tags: []*ec2.Tag{ + { + Key: aws.String("ami-key-1"), + Value: aws.String("ami-value-1"), + }, + }, }, { Name: aws.String(coretest.RandomName()), ImageId: aws.String(amdAMIID), Architecture: aws.String("x86_64"), CreationDate: aws.String("2022-08-15T12:00:00Z"), + Tags: []*ec2.Tag{ + { + Key: aws.String("ami-key-2"), + Value: aws.String("ami-value-2"), + }, + }, }, }, }) - nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{ - { - ID: validSecurityGroup, - Name: "test-securitygroup", - }, - } - nodeClass.Status.Subnets = []v1beta1.Subnet{ - { - ID: validSubnet1, - Zone: "zone-1", + awsEnv.EC2API.DescribeSecurityGroupsOutput.Set(&ec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []*ec2.SecurityGroup{ + { + GroupId: aws.String(validSecurityGroup), + GroupName: aws.String("test-securitygroup"), + Tags: []*ec2.Tag{ + { + Key: aws.String("sg-key"), + Value: aws.String("sg-value"), + }, + }, + }, }, - { - ID: validSubnet2, - Zone: "zone-2", + }) + awsEnv.EC2API.DescribeSubnetsOutput.Set(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{ + { + SubnetId: aws.String(validSubnet1), + AvailabilityZone: aws.String("zone-1"), + Tags: []*ec2.Tag{ + { + Key: aws.String("sn-key-1"), + Value: aws.String("sn-value-1"), + }, + }, + }, + { + SubnetId: aws.String(validSubnet2), + AvailabilityZone: aws.String("zone-2"), + Tags: []*ec2.Tag{ + { + Key: aws.String("sn-key-2"), + Value: aws.String("sn-value-2"), + }, + }, + }, }, - } + }) ExpectApplied(ctx, env.Client, nodePool, nodeClass) instanceTypes, err := cloudProvider.GetInstanceTypes(ctx, nodePool) Expect(err).ToNot(HaveOccurred()) @@ -305,8 +339,8 @@ var _ = Describe("CloudProvider", func() { Expect(isDrifted).To(Equal(cloudprovider.SubnetDrift)) }) It("should return an error if subnets are empty", func() { - nodeClass.Status.Subnets = []v1beta1.Subnet{} - ExpectApplied(ctx, env.Client, nodeClass) + awsEnv.SubnetCache.Flush() + awsEnv.EC2API.DescribeSubnetsOutput.Set(&ec2.DescribeSubnetsOutput{Subnets: []*ec2.Subnet{}}) _, err := cloudProvider.IsDrifted(ctx, nodeClaim) Expect(err).To(HaveOccurred()) }) @@ -316,39 +350,39 @@ var _ = Describe("CloudProvider", func() { Expect(isDrifted).To(BeEmpty()) }) It("should return an error if the security groups are empty", func() { - nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{} - ExpectApplied(ctx, env.Client, nodeClass) + awsEnv.EC2API.DescribeSecurityGroupsOutput.Set(&ec2.DescribeSecurityGroupsOutput{SecurityGroups: []*ec2.SecurityGroup{}}) // Instance is a reference to what we return in the GetInstances call instance.SecurityGroups = []*ec2.GroupIdentifier{{GroupId: aws.String(fake.SecurityGroupID())}} _, err := cloudProvider.IsDrifted(ctx, nodeClaim) Expect(err).To(HaveOccurred()) }) - It("should return drifted if the instance security groups doesn't match the status", func() { + It("should return drifted if the instance security groups doesn't match the discovered values", func() { // Instance is a reference to what we return in the GetInstances call instance.SecurityGroups = []*ec2.GroupIdentifier{{GroupId: aws.String(fake.SecurityGroupID())}} isDrifted, err := cloudProvider.IsDrifted(ctx, nodeClaim) Expect(err).ToNot(HaveOccurred()) Expect(isDrifted).To(Equal(cloudprovider.SecurityGroupDrift)) }) - It("should return drifted if there are more instance security groups present than in the status", func() { + It("should return drifted if there are more instance security groups present than in the discovered values", func() { // Instance is a reference to what we return in the GetInstances call instance.SecurityGroups = []*ec2.GroupIdentifier{{GroupId: aws.String(fake.SecurityGroupID())}, {GroupId: aws.String(validSecurityGroup)}} isDrifted, err := cloudProvider.IsDrifted(ctx, nodeClaim) Expect(err).ToNot(HaveOccurred()) Expect(isDrifted).To(Equal(cloudprovider.SecurityGroupDrift)) }) - It("should return drifted if more security groups are present than instance security groups", func() { - nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{ - { - ID: validSecurityGroup, - Name: "test-securitygroup", - }, - { - ID: fake.SecurityGroupID(), - Name: "test-securitygroup", + It("should return drifted if more security groups are present than instance security groups then discovered from nodeclass", func() { + awsEnv.EC2API.DescribeSecurityGroupsOutput.Set(&ec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []*ec2.SecurityGroup{ + { + GroupId: aws.String(validSecurityGroup), + GroupName: aws.String("test-securitygroup"), + }, + { + GroupId: aws.String(fake.SecurityGroupID()), + GroupName: aws.String("test-securitygroup"), + }, }, - } - ExpectApplied(ctx, env.Client, nodeClass) + }) isDrifted, err := cloudProvider.IsDrifted(ctx, nodeClaim) Expect(err).ToNot(HaveOccurred()) Expect(isDrifted).To(Equal(cloudprovider.SecurityGroupDrift)) @@ -422,8 +456,8 @@ var _ = Describe("CloudProvider", func() { Expect(err).NotTo(HaveOccurred()) Expect(isDrifted).To(BeEmpty()) }, - Entry("AMI Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AMISelectorTerms: []v1beta1.AMISelectorTerm{{Tags: map[string]string{"*": "*"}}}}}), - Entry("Subnet Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{SubnetSelectorTerms: []v1beta1.SubnetSelectorTerm{{ID: "subnet-test1"}}}}), + Entry("AMI Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AMISelectorTerms: []v1beta1.AMISelectorTerm{{Tags: map[string]string{"ami-key-1": "ami-value-1"}}}}}), + Entry("Subnet Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{SubnetSelectorTerms: []v1beta1.SubnetSelectorTerm{{Tags: map[string]string{"sn-key-1": "sn-value-1"}}}}}), Entry("SecurityGroup Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{SecurityGroupSelectorTerms: []v1beta1.SecurityGroupSelectorTerm{{Tags: map[string]string{"sg-key": "sg-value"}}}}}), ) It("should not return drifted if karpenter.k8s.aws/nodeclass-hash annotation is not present on the NodeClaim", func() {