Add securitygroup controller

pkg/cloudprovider/suite_test.go

@@ -140,6 +140,7 @@ var _ = Describe("CloudProvider", func() {
 		})
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
+		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 	})
 	It("should return an ICE error when there are no instance types to launch", func() {
 		// Specify no instance types and expect to receive a capacity error

pkg/controllers/nodeclass/status/securitygroup_test.go

@@ -50,6 +50,7 @@ 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{
@@ -77,6 +78,7 @@ 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{
@@ -97,6 +99,7 @@ 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{
@@ -108,6 +111,7 @@ 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{
@@ -134,6 +138,7 @@ 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{
@@ -149,6 +154,7 @@ 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{
@@ -172,6 +178,7 @@ 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{
@@ -188,12 +195,14 @@ 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{
@@ -217,6 +226,7 @@ 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,6 +73,7 @@ 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/controllers/providers/securtiygroup/controller.go

@@ -0,0 +1,60 @@
+/*
+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 securitygroup
+
+import (
+	"context"
+	"time"
+
+	controllerruntime "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/manager"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	corecontroller "sigs.k8s.io/karpenter/pkg/operator/controller"
+
+	"github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1"
+	"github.com/aws/karpenter-provider-aws/pkg/providers/securitygroup"
+)
+
+type Controller struct {
+	securitygroupProvider securitygroup.Provider
+}
+
+func NewController(securitygroupProvider securitygroup.Provider) *Controller {
+	return &Controller{
+		securitygroupProvider: securitygroupProvider,
+	}
+}
+
+func (c *Controller) Reconcile(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) (reconcile.Result, error) {
+	if err := c.securitygroupProvider.UpdateSecurityGroup(ctx, nodeClass); err != nil {
+		return reconcile.Result{}, err
+	}
+	return reconcile.Result{RequeueAfter: 5 * time.Minute}, nil
+}
+
+func (c *Controller) Name() string {
+	return "providers.securitygroup"
+}
+
+func (c *Controller) Builder(_ context.Context, m manager.Manager) corecontroller.Builder {
+	return corecontroller.Adapt(controllerruntime.
+		NewControllerManagedBy(m).
+		For(&v1beta1.EC2NodeClass{}).
+		WithOptions(controller.Options{
+			MaxConcurrentReconciles: 10,
+		}))
+}

pkg/controllers/providers/securtiygroup/suite_test.go

@@ -0,0 +1,59 @@
+/*
+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 securitygroup_test
+
+import (
+	"context"
+	"testing"
+
+	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"
+	controllerspricing "github.com/aws/karpenter-provider-aws/pkg/controllers/providers/pricing"
+	"github.com/aws/karpenter-provider-aws/pkg/operator/options"
+	"github.com/aws/karpenter-provider-aws/pkg/test"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	. "knative.dev/pkg/logging/testing"
+)
+
+var ctx context.Context
+var stop context.CancelFunc
+var env *coretest.Environment
+var awsEnv *test.Environment
+var controller *controllerspricing.Controller
+
+func TestAWS(t *testing.T) {
+	ctx = TestContextWithLogger(t)
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "SecurityGroup")
+}
+
+var _ = BeforeSuite(func() {
+	env = coretest.NewEnvironment(scheme.Scheme, coretest.WithCRDs(apis.CRDs...))
+	ctx = coreoptions.ToContext(ctx, coretest.Options())
+	ctx = options.ToContext(ctx, test.Options())
+	ctx, stop = context.WithCancel(ctx)
+	awsEnv = test.NewEnvironment(ctx, env)
+	controller = controllerspricing.NewController(awsEnv.PricingProvider)
+})
+
+var _ = AfterSuite(func() {
+	stop()
+	Expect(env.Stop()).To(Succeed(), "Failed to stop environment")
+})

pkg/providers/instancetype/suite_test.go

@@ -157,6 +157,8 @@ var _ = Describe("InstanceTypeProvider", func() {
 		})
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
+		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
+		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, windowsNodeClass)).To(BeNil())
 	})
 
 	It("should support individual instance type labels", func() {

pkg/providers/launchtemplate/suite_test.go

@@ -148,6 +148,7 @@ var _ = Describe("LaunchTemplate Provider", func() {
 		})
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
+		Expect(awsEnv.SecurityGroupProvider.UpdateSecurityGroup(ctx, nodeClass)).To(BeNil())
 	})
 	It("should create unique launch templates for multiple identical nodeClasses", func() {
 		nodeClass2 := test.EC2NodeClass()

pkg/providers/securitygroup/securitygroup.go

@@ -34,10 +34,11 @@ import (
 
 type Provider interface {
 	List(context.Context, *v1beta1.EC2NodeClass) ([]*ec2.SecurityGroup, error)
+	UpdateSecurityGroup(context.Context, *v1beta1.EC2NodeClass) error
 }
 
 type DefaultProvider struct {
-	sync.Mutex
+	sync.RWMutex
 	ec2api ec2iface.EC2API
 	cache  *cache.Cache
 	cm     *pretty.ChangeMonitor
@@ -53,45 +54,58 @@ func NewDefaultProvider(ec2api ec2iface.EC2API, cache *cache.Cache) *DefaultProv
 }
 
 func (p *DefaultProvider) List(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) ([]*ec2.SecurityGroup, error) {
-	p.Lock()
-	defer p.Unlock()
+	p.RLock()
+	defer p.RUnlock()
 
 	// Get SecurityGroups
 	filterSets := getFilterSets(nodeClass.Spec.SecurityGroupSelectorTerms)
-	securityGroups, err := p.getSecurityGroups(ctx, filterSets)
+	hash, err := p.securitygroupHash(filterSets)
 	if err != nil {
 		return nil, err
 	}
-	if p.cm.HasChanged(fmt.Sprintf("security-groups/%s", nodeClass.Name), securityGroups) {
-		logging.FromContext(ctx).
-			With("security-groups", lo.Map(securityGroups, func(s *ec2.SecurityGroup, _ int) string {
-				return aws.StringValue(s.GroupId)
-			})).
-			Debugf("discovered security groups")
+	if sg, ok := p.cache.Get(hash); ok {
+		return sg.([]*ec2.SecurityGroup), nil
 	}
-	return securityGroups, nil
+	return []*ec2.SecurityGroup{}, nil
 }
 
-func (p *DefaultProvider) getSecurityGroups(ctx context.Context, filterSets [][]*ec2.Filter) ([]*ec2.SecurityGroup, error) {
-	hash, err := hashstructure.Hash(filterSets, hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true})
+func (p *DefaultProvider) UpdateSecurityGroup(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) error {
+	p.Lock()
+	defer p.Unlock()
+
+	filterSets := getFilterSets(nodeClass.Spec.SecurityGroupSelectorTerms)
+	hash, err := p.securitygroupHash(filterSets)
 	if err != nil {
-		return nil, err
-	}
-	if sg, ok := p.cache.Get(fmt.Sprint(hash)); ok {
-		return sg.([]*ec2.SecurityGroup), nil
+		return err
 	}
 	securityGroups := map[string]*ec2.SecurityGroup{}
 	for _, filters := range filterSets {
 		output, err := p.ec2api.DescribeSecurityGroupsWithContext(ctx, &ec2.DescribeSecurityGroupsInput{Filters: filters})
 		if err != nil {
-			return nil, fmt.Errorf("describing security groups %+v, %w", filterSets, err)
+			return fmt.Errorf("describing security groups %+v, %w", filterSets, err)
 		}
 		for i := range output.SecurityGroups {
 			securityGroups[lo.FromPtr(output.SecurityGroups[i].GroupId)] = output.SecurityGroups[i]
 		}
 	}
 	p.cache.SetDefault(fmt.Sprint(hash), lo.Values(securityGroups))
-	return lo.Values(securityGroups), nil
+
+	if p.cm.HasChanged(fmt.Sprintf("security-groups/%s", nodeClass.Name), securityGroups) {
+		logging.FromContext(ctx).
+			With("security-groups", lo.Map(lo.Values(securityGroups), func(s *ec2.SecurityGroup, _ int) string {
+				return aws.StringValue(s.GroupId)
+			})).
+			Debugf("discovered security groups")
+	}
+	return nil
+}
+
+func (p *DefaultProvider) securitygroupHash(filterSets [][]*ec2.Filter) (string, error) {
+	hash, err := hashstructure.Hash(filterSets, hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true})
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("%d", hash), nil
 }
 
 func getFilterSets(terms []v1beta1.SecurityGroupSelectorTerm) (res [][]*ec2.Filter) {