Skip to content

Commit

Permalink
Feedback Addressed
Browse files Browse the repository at this point in the history
  • Loading branch information
edibble21 committed Jan 8, 2025
1 parent 19af5b9 commit 47c0488
Show file tree
Hide file tree
Showing 8 changed files with 132 additions and 206 deletions.
2 changes: 0 additions & 2 deletions pkg/apis/v1/ec2nodeclass_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ const (
ConditionTypeSecurityGroupsReady = "SecurityGroupsReady"
ConditionTypeAMIsReady = "AMIsReady"
ConditionTypeInstanceProfileReady = "InstanceProfileReady"
ConditionTypeAuthorization = "AuthorizationReady"
ConditionTypeValidationSucceeded = "ValidationSucceeded"
)

Expand Down Expand Up @@ -95,7 +94,6 @@ func (in *EC2NodeClass) StatusConditions() status.ConditionSet {
ConditionTypeSubnetsReady,
ConditionTypeSecurityGroupsReady,
ConditionTypeInstanceProfileReady,
ConditionTypeAuthorization,
ConditionTypeValidationSucceeded,
).For(in)
}
Expand Down
61 changes: 0 additions & 61 deletions pkg/controllers/nodeclass/status/authorization.go

This file was deleted.

70 changes: 0 additions & 70 deletions pkg/controllers/nodeclass/status/authorization_test.go

This file was deleted.

5 changes: 1 addition & 4 deletions pkg/controllers/nodeclass/status/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ type Controller struct {
subnet *Subnet
securitygroup *SecurityGroup
validation *Validation
authorization *Authorization
readiness *Readiness //TODO : Remove this when we have sub status conditions
}

Expand All @@ -67,8 +66,7 @@ func NewController(kubeClient client.Client, subnetProvider subnet.Provider, sec
subnet: &Subnet{subnetProvider: subnetProvider},
securitygroup: &SecurityGroup{securityGroupProvider: securityGroupProvider},
instanceprofile: &InstanceProfile{instanceProfileProvider: instanceProfileProvider},
validation: &Validation{},
authorization: &Authorization{cloudProvider: cloudProvider, instanceProvider: instanceProvider},
validation: &Validation{cloudProvider: cloudProvider, instanceProvider: instanceProvider},
readiness: &Readiness{launchTemplateProvider: launchTemplateProvider},
}
}
Expand Down Expand Up @@ -100,7 +98,6 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass)
c.securitygroup,
c.instanceprofile,
c.validation,
c.authorization,
c.readiness,
} {
res, err := reconciler.Reconcile(ctx, nodeClass)
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/nodeclass/status/readiness_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ var _ = Describe("NodeClass Status Condition Controller", func() {
ExpectApplied(ctx, env.Client, nodeClass)
ExpectObjectReconciled(ctx, env.Client, statusController, nodeClass)
nodeClass = ExpectExists(ctx, env.Client, nodeClass)
Expect(nodeClass.Status.Conditions).To(HaveLen(7))
Expect(nodeClass.Status.Conditions).To(HaveLen(6))
Expect(nodeClass.StatusConditions().Get(status.ConditionReady).IsTrue()).To(BeTrue())
})
It("should update status condition as Not Ready", func() {
Expand Down
27 changes: 3 additions & 24 deletions pkg/controllers/nodeclass/status/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,25 +56,6 @@ var _ = BeforeSuite(func() {
ctx = coreoptions.ToContext(ctx, coretest.Options())
ctx = options.ToContext(ctx, test.Options())
awsEnv = test.NewEnvironment(ctx, env)

instanceTypesProvider := awsEnv.InstanceTypesProvider

Expect(instanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
Expect(instanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())

cloudProvider = lo.FromPtr(cloudprovider.New(instanceTypesProvider, awsEnv.InstanceProvider, coretest.NewEventRecorder(),
env.Client, awsEnv.AMIProvider, awsEnv.SecurityGroupProvider))

statusController = status.NewController(
env.Client,
awsEnv.SubnetProvider,
awsEnv.SecurityGroupProvider,
awsEnv.AMIProvider,
awsEnv.InstanceProfileProvider,
awsEnv.LaunchTemplateProvider,
cloudProvider,
awsEnv.InstanceProvider,
)
})

var _ = AfterSuite(func() {
Expand All @@ -86,12 +67,10 @@ var _ = BeforeEach(func() {
nodeClass = test.EC2NodeClass()
awsEnv.Reset()

instanceTypesProvider := awsEnv.InstanceTypesProvider

Expect(instanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
Expect(instanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())

cloudProvider = lo.FromPtr(cloudprovider.New(instanceTypesProvider, awsEnv.InstanceProvider, coretest.NewEventRecorder(),
cloudProvider = lo.FromPtr(cloudprovider.New(awsEnv.InstanceTypesProvider, awsEnv.InstanceProvider, coretest.NewEventRecorder(),
env.Client, awsEnv.AMIProvider, awsEnv.SecurityGroupProvider))

statusController = status.NewController(
Expand Down
37 changes: 36 additions & 1 deletion pkg/controllers/nodeclass/status/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,27 @@ package status

import (
"context"
"errors"
"fmt"

"github.com/samber/lo"

"sigs.k8s.io/controller-runtime/pkg/reconcile"
corecloudprovider "sigs.k8s.io/karpenter/pkg/cloudprovider"
coretest "sigs.k8s.io/karpenter/pkg/test"

v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
"github.com/aws/karpenter-provider-aws/pkg/cloudprovider"
"github.com/aws/karpenter-provider-aws/pkg/providers/instance"
)

type Validation struct{}
type Validation struct {
cloudProvider cloudprovider.CloudProvider
instanceProvider instance.Provider
}

func (n Validation) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass) (reconcile.Result, error) {
//Tag Validation
if offendingTag, found := lo.FindKeyBy(nodeClass.Spec.Tags, func(k string, v string) bool {
for _, exp := range v1.RestrictedTagPatterns {
if exp.MatchString(k) {
Expand All @@ -40,6 +49,32 @@ func (n Validation) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass) (
fmt.Sprintf("%q tag does not pass tag validation requirements", offendingTag))
return reconcile.Result{}, reconcile.TerminalError(fmt.Errorf("%q tag does not pass tag validation requirements", offendingTag))
}
//Auth Validation
//validates createfleet, describelaunchtemplate, createtags, and terminateinstances
//nolint:ineffassign, staticcheck
ctx = context.WithValue(ctx, "DryRun", lo.ToPtr(true))
if nodeClass.StatusConditions().Get(v1.ConditionTypeSubnetsReady).IsFalse() || nodeClass.StatusConditions().Get(v1.ConditionTypeAMIsReady).IsFalse() {
return reconcile.Result{}, nil
}
nodeClaim := coretest.NodeClaim()
nodeClaim.Spec.NodeClassRef.Name = nodeClass.Name
var errs []error
//create checks createfleet, and describelaunchtemplate
if _, err := n.cloudProvider.Create(ctx, nodeClaim); err != nil {
errs = append(errs, fmt.Errorf("create: %w", err))
}

if err := n.instanceProvider.Delete(ctx, "mock-id"); err != nil {
errs = append(errs, fmt.Errorf("delete: %w", err))
}

if err := n.instanceProvider.CreateTags(ctx, "mock-id", map[string]string{"mock-tag": "mock-tag-value"}); err != nil {
errs = append(errs, fmt.Errorf("create tags: %w", err))
}
if corecloudprovider.IsNodeClassNotReadyError(errors.Join(errs...)) {
nodeClass.StatusConditions().SetFalse(v1.ConditionTypeValidationSucceeded, "NodeClassNotReady", fmt.Sprintf("unauthorized operation %v", errors.Join(errs...)))
return reconcile.Result{}, fmt.Errorf("unauthorized operation %w", errors.Join(errs...))
}
nodeClass.StatusConditions().SetTrue(v1.ConditionTypeValidationSucceeded)
return reconcile.Result{}, nil
}
Loading

0 comments on commit 47c0488

Please sign in to comment.