Use status vaules for security groups

pkg/cloudprovider/drift.go

@@ -123,7 +123,7 @@ func (c *CloudProvider) areSecurityGroupsDrifted(ctx context.Context, ec2Instanc
 	if err != nil {
 		return "", err
 	}
-	securityGroupIds := sets.New(lo.Map(securitygroup, func(sg *ec2.SecurityGroup, _ int) string { return aws.StringValue(sg.GroupId) })...)
+	securityGroupIds := sets.New(lo.Map(securitygroup, func(sg v1beta1.SecurityGroup, _ int) string { return sg.ID })...)
 	if len(securityGroupIds) == 0 {
 		return "", fmt.Errorf("no security groups are discovered")
 	}

pkg/controllers/nodeclass/status/controller.go

@@ -63,7 +63,7 @@ func NewController(kubeClient client.Client, subnetProvider subnet.Provider, sec
 
 		ami:             &AMI{amiProvider: amiProvider},
 		subnet:          &Subnet{subnetProvider: subnetProvider},
-		securitygroup:   &SecurityGroup{securityGroupProvider: securityGroupProvider},
+		securitygroup:   &SecurityGroup{SecurityGroupProvider: securityGroupProvider},
 		instanceprofile: &InstanceProfile{instanceProfileProvider: instanceProfileProvider},
 		launchtemplate:  &LaunchTemplate{launchTemplateProvider: launchTemplateProvider},
 	})
@@ -77,6 +77,9 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClass *v1beta1.EC2NodeCl
 			return reconcile.Result{}, err
 		}
 	}
+	c.securitygroup.LockProvider()
+	defer c.securitygroup.UnlockProvider()
+
 	stored := nodeClass.DeepCopy()
 
 	var results []reconcile.Result

pkg/controllers/nodeclass/status/securitygroup.go

@@ -29,11 +29,11 @@ import (
 )
 
 type SecurityGroup struct {
-	securityGroupProvider securitygroup.Provider
+	SecurityGroupProvider securitygroup.Provider
 }
 
 func (sg *SecurityGroup) Reconcile(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) (reconcile.Result, error) {
-	securityGroups, err := sg.securityGroupProvider.List(ctx, nodeClass)
+	securityGroups, err := sg.SecurityGroupProvider.UpdateSecurityGroups(ctx, nodeClass)
 	if err != nil {
 		return reconcile.Result{}, err
 	}
@@ -52,3 +52,11 @@ func (sg *SecurityGroup) Reconcile(ctx context.Context, nodeClass *v1beta1.EC2No
 	})
 	return reconcile.Result{RequeueAfter: 5 * time.Minute}, nil
 }
+
+func (sg *SecurityGroup) LockProvider() {
+	sg.SecurityGroupProvider.LockProvider()
+}
+
+func (sg *SecurityGroup) UnlockProvider() {
+	sg.SecurityGroupProvider.UnlockProvider()
+}

pkg/controllers/nodeclass/status/securitygroup_test.go

@@ -50,7 +50,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 	})
 	It("Should update EC2NodeClass status for Security Groups", func() {
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -78,7 +77,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 			},
 		}
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -99,7 +97,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 			},
 		}
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -111,7 +108,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 	})
 	It("Should update Security Groups status when the Security Groups selector gets updated by tags", func() {
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -138,7 +134,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 			},
 		}
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -154,7 +149,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 	})
 	It("Should update Security Groups status when the Security Groups selector gets updated by ids", func() {
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -178,7 +172,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 			},
 		}
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -195,14 +188,12 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 			},
 		}
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileFailed(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(BeNil())
 	})
 	It("Should not resolve a invalid selectors for an updated Security Groups selector", func() {
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(Equal([]v1beta1.SecurityGroup{
@@ -226,7 +217,6 @@ var _ = Describe("NodeClass Security Group Status Controller", func() {
 			},
 		}
 		ExpectApplied(ctx, env.Client, nodeClass)
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 		ExpectReconcileFailed(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		Expect(nodeClass.Status.SecurityGroups).To(BeNil())

pkg/controllers/nodeclass/status/suite_test.go

@@ -73,7 +73,6 @@ var _ = BeforeEach(func() {
 	ctx = coreoptions.ToContext(ctx, coretest.Options())
 	nodeClass = test.EC2NodeClass()
 	awsEnv.Reset()
-	Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 })
 
 var _ = AfterEach(func() {

pkg/operator/operator.go

@@ -133,7 +133,7 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont
 
 	unavailableOfferingsCache := awscache.NewUnavailableOfferings()
 	subnetProvider := subnet.NewDefaultProvider(ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
-	securityGroupProvider := securitygroup.NewDefaultProvider(ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
+	securityGroupProvider := securitygroup.NewDefaultProvider(operator.GetClient(), ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
 	instanceProfileProvider := instanceprofile.NewDefaultProvider(*sess.Config.Region, iam.New(sess), cache.New(awscache.InstanceProfileTTL, awscache.DefaultCleanupInterval))
 	pricingProvider := pricing.NewDefaultProvider(
 		ctx,

pkg/providers/instance/suite_test.go

@@ -26,16 +26,19 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1"
 	corecloudprovider "sigs.k8s.io/karpenter/pkg/cloudprovider"
 	"sigs.k8s.io/karpenter/pkg/events"
+	"sigs.k8s.io/karpenter/pkg/operator/controller"
 	coreoptions "sigs.k8s.io/karpenter/pkg/operator/options"
 	"sigs.k8s.io/karpenter/pkg/operator/scheme"
 	coretest "sigs.k8s.io/karpenter/pkg/test"
 
 	"github.com/aws/karpenter-provider-aws/pkg/apis"
 	"github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1"
 	"github.com/aws/karpenter-provider-aws/pkg/cloudprovider"
+	"github.com/aws/karpenter-provider-aws/pkg/controllers/nodeclass/status"
 	"github.com/aws/karpenter-provider-aws/pkg/fake"
 	"github.com/aws/karpenter-provider-aws/pkg/operator/options"
 	"github.com/aws/karpenter-provider-aws/pkg/providers/instance"
@@ -51,6 +54,7 @@ var ctx context.Context
 var env *coretest.Environment
 var awsEnv *test.Environment
 var cloudProvider *cloudprovider.CloudProvider
+var statusController controller.Controller
 
 func TestAWS(t *testing.T) {
 	ctx = TestContextWithLogger(t)
@@ -65,6 +69,14 @@ var _ = BeforeSuite(func() {
 	awsEnv = test.NewEnvironment(ctx, env)
 	cloudProvider = cloudprovider.New(awsEnv.InstanceTypesProvider, awsEnv.InstanceProvider, events.NewRecorder(&record.FakeRecorder{}),
 		env.Client, awsEnv.AMIProvider, awsEnv.SecurityGroupProvider, awsEnv.SubnetProvider)
+	statusController = status.NewController(
+		env.Client,
+		awsEnv.SubnetProvider,
+		awsEnv.SecurityGroupProvider,
+		awsEnv.AMIProvider,
+		awsEnv.InstanceProfileProvider,
+		awsEnv.LaunchTemplateProvider,
+	)
 })
 
 var _ = AfterSuite(func() {
@@ -117,7 +129,8 @@ var _ = Describe("InstanceProvider", func() {
 			{CapacityType: corev1beta1.CapacityTypeSpot, InstanceType: "m5.xlarge", Zone: "test-zone-1a"},
 			{CapacityType: corev1beta1.CapacityTypeSpot, InstanceType: "m5.xlarge", Zone: "test-zone-1b"},
 		})
-		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
+		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
+		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		instanceTypes, err := cloudProvider.GetInstanceTypes(ctx, nodePool)
 		Expect(err).ToNot(HaveOccurred())