Skip to content

Commit 62569b3

Browse files
jmdealjmdeal
committed
required condition logic update
1 parent 2142efc commit 62569b3

File tree

2 files changed

+68
-66
lines changed

2 files changed

+68
-66
lines changed

‎pkg/controllers/nodeclass/validation.go

+49-47
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,7 @@ var ValidationConditionMessages = map[string]string{
5353
}
5454

5555
type Validation struct {
56-
ec2api sdk.EC2API
57-
56+
ec2api sdk.EC2API
5857
amiProvider amifamily.Provider
5958
cache *cache.Cache
6059
}
@@ -69,22 +68,29 @@ func NewValidationReconciler(ec2api sdk.EC2API, amiProvider amifamily.Provider,
6968

7069
// nolint:gocyclo
7170
func (v *Validation) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass) (reconcile.Result, error) {
72-
for _, cond := range []string{
73-
v1.ConditionTypeAMIsReady,
74-
v1.ConditionTypeInstanceProfileReady,
75-
v1.ConditionTypeSecurityGroupsReady,
76-
v1.ConditionTypeSubnetsReady,
77-
} {
78-
if nodeClass.StatusConditions().Get(cond).IsTrue() {
79-
continue
80-
}
71+
if _, ok := lo.Find(v.requiredConditions(), func(cond string) bool {
72+
return nodeClass.StatusConditions().Get(cond).IsFalse()
73+
}); ok {
74+
// If any of the required status conditions are false, we know validation will fail regardless of the other values.
8175
nodeClass.StatusConditions().SetFalse(
8276
v1.ConditionTypeValidationSucceeded,
8377
"DependenciesNotReady",
8478
"Awaiting AMI, Instance Profile, Security Group, and Subnet resolution",
8579
)
8680
return reconcile.Result{}, nil
8781
}
82+
if _, ok := lo.Find(v.requiredConditions(), func(cond string) bool {
83+
return nodeClass.StatusConditions().Get(cond).IsUnknown()
84+
}); ok {
85+
// If none of the status conditions are false, but at least one is unknown, we should also consider the validation
86+
// state to be unknown. Once all required conditions collapse to a true or false state, we can test validation.
87+
nodeClass.StatusConditions().SetUnknownWithReason(
88+
v1.ConditionTypeValidationSucceeded,
89+
"DependenciesNotReady",
90+
"Awaiting AMI, Instance Profile, Security Group, and Subnet resolution",
91+
)
92+
return reconcile.Result{}, nil
93+
}
8894

8995
if val, ok := v.cache.Get(v.cacheKey(nodeClass)); ok {
9096
// We still update the status condition even if it's cached since we may have had a conflict error previously
@@ -113,32 +119,19 @@ func (v *Validation) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass)
113119
return reconcile.Result{}, reconcile.TerminalError(fmt.Errorf("validating tags, %w", err))
114120
}
115121

116-
type validator struct {
117-
isValid validatorFunc
118-
failureReason string
119-
}
120-
for _, val := range []validator{
121-
{
122-
isValid: v.validateCreateFleetAuthorization,
123-
failureReason: ConditionReasonCreateFleetAuthFailed,
124-
},
125-
{
126-
isValid: v.validateCreateLaunchTemplateAuthorization,
127-
failureReason: ConditionReasonCreateLaunchTemplateAuthFailed,
128-
},
129-
{
130-
isValid: v.validateRunInstancesAuthorization,
131-
failureReason: ConditionReasonRunInstancesAuthFailed,
132-
},
122+
for _, isValid := range []validatorFunc{
123+
v.validateCreateFleetAuthorization,
124+
v.validateCreateLaunchTemplateAuthorization,
125+
v.validateRunInstancesAuthorization,
133126
} {
134-
if ok, err := val.isValid(ctx, nodeClass, nodeClaim, tags); err != nil {
127+
if failureReason, err := isValid(ctx, nodeClass, nodeClaim, tags); err != nil {
135128
return reconcile.Result{}, err
136-
} else if !ok {
137-
v.cache.SetDefault(v.cacheKey(nodeClass), val.failureReason)
129+
} else if failureReason != "" {
130+
v.cache.SetDefault(v.cacheKey(nodeClass), failureReason)
138131
nodeClass.StatusConditions().SetFalse(
139132
v1.ConditionTypeValidationSucceeded,
140-
val.failureReason,
141-
ValidationConditionMessages[val.failureReason],
133+
failureReason,
134+
ValidationConditionMessages[failureReason],
142135
)
143136
return reconcile.Result{}, nil
144137
}
@@ -149,55 +142,55 @@ func (v *Validation) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass)
149142
return reconcile.Result{}, nil
150143
}
151144

152-
type validatorFunc func(context.Context, *v1.EC2NodeClass, *karpv1.NodeClaim, map[string]string) (bool, error)
145+
type validatorFunc func(context.Context, *v1.EC2NodeClass, *karpv1.NodeClaim, map[string]string) (string, error)
153146

154147
func (v *Validation) validateCreateFleetAuthorization(
155148
ctx context.Context,
156149
nodeClass *v1.EC2NodeClass,
157150
_ *karpv1.NodeClaim,
158151
tags map[string]string,
159-
) (bool, error) {
152+
) (reason string, err error) {
160153
createFleetInput := instance.GetCreateFleetInput(nodeClass, string(karpv1.CapacityTypeOnDemand), tags, mockLaunchTemplateConfig())
161154
createFleetInput.DryRun = aws.Bool(true)
162155
if _, err := v.ec2api.CreateFleet(ctx, createFleetInput); awserrors.IgnoreDryRunError(err) != nil {
163156
if awserrors.IgnoreUnauthorizedOperationError(err) != nil {
164157
// Dry run should only ever return UnauthorizedOperation or DryRunOperation so if we receive any other error
165158
// it would be an unexpected state
166-
return false, err
159+
return "", err
167160
}
168-
return false, nil
161+
return ConditionReasonCreateFleetAuthFailed, nil
169162
}
170-
return true, nil
163+
return "", nil
171164
}
172165

173166
func (v *Validation) validateCreateLaunchTemplateAuthorization(
174167
ctx context.Context,
175168
nodeClass *v1.EC2NodeClass,
176169
nodeClaim *karpv1.NodeClaim,
177170
tags map[string]string,
178-
) (bool, error) {
171+
) (reason string, err error) {
179172
createLaunchTemplateInput := launchtemplate.GetCreateLaunchTemplateInput(ctx, mockOptions(*nodeClaim, nodeClass, tags), corev1.IPv4Protocol, "")
180173
createLaunchTemplateInput.DryRun = aws.Bool(true)
181174
if _, err := v.ec2api.CreateLaunchTemplate(ctx, createLaunchTemplateInput); awserrors.IgnoreDryRunError(err) != nil {
182175
if awserrors.IgnoreUnauthorizedOperationError(err) != nil {
183176
// Dry run should only ever return UnauthorizedOperation or DryRunOperation so if we receive any other error
184177
// it would be an unexpected state
185-
return false, fmt.Errorf("validating ec2:CreateLaunchTemplates authorization, %w", err)
178+
return "", fmt.Errorf("validating ec2:CreateLaunchTemplates authorization, %w", err)
186179
}
187-
return false, nil
180+
return ConditionReasonCreateLaunchTemplateAuthFailed, nil
188181
}
189-
return true, nil
182+
return "", nil
190183
}
191184

192185
func (v *Validation) validateRunInstancesAuthorization(
193186
ctx context.Context,
194187
nodeClass *v1.EC2NodeClass,
195188
nodeClaim *karpv1.NodeClaim,
196189
tags map[string]string,
197-
) (bool, error) {
190+
) (reason string, err error) {
198191
// NOTE: Since we've already validated the AMI status condition is true, this should never occur
199192
if len(nodeClass.Status.AMIs) == 0 {
200-
return false, fmt.Errorf("no resolved amis in status")
193+
return "", fmt.Errorf("no resolved amis in status")
201194
}
202195

203196
var instanceType ec2types.InstanceType
@@ -243,11 +236,20 @@ func (v *Validation) validateRunInstancesAuthorization(
243236
if awserrors.IgnoreUnauthorizedOperationError(err) != nil {
244237
// Dry run should only ever return UnauthorizedOperation or DryRunOperation so if we receive any other error
245238
// it would be an unexpected state
246-
return false, fmt.Errorf("validating ec2:RunInstances authorization, %w", err)
239+
return "", fmt.Errorf("validating ec2:RunInstances authorization, %w", err)
247240
}
248-
return false, nil
241+
return ConditionReasonRunInstancesAuthFailed, nil
242+
}
243+
return "", nil
244+
}
245+
246+
func (*Validation) requiredConditions() []string {
247+
return []string{
248+
v1.ConditionTypeAMIsReady,
249+
v1.ConditionTypeInstanceProfileReady,
250+
v1.ConditionTypeSecurityGroupsReady,
251+
v1.ConditionTypeSubnetsReady,
249252
}
250-
return true, nil
251253
}
252254

253255
func (*Validation) cacheKey(nodeClass *v1.EC2NodeClass) string {

‎pkg/controllers/nodeclass/validation_test.go

+19-19
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/aws/smithy-go"
2222

2323
v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
24+
"github.com/aws/karpenter-provider-aws/pkg/controllers/nodeclass"
2425
"github.com/aws/karpenter-provider-aws/pkg/fake"
2526
"github.com/aws/karpenter-provider-aws/pkg/test"
2627

@@ -63,6 +64,7 @@ var _ = Describe("NodeClass Validation Status Controller", func() {
6364
Expect(err).To(HaveOccurred())
6465
nodeClass = ExpectExists(ctx, env.Client, nodeClass)
6566
Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsFalse()).To(BeTrue())
67+
Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).Reason).To(Equal("TagValidationFailed"))
6668
Expect(nodeClass.StatusConditions().Get(status.ConditionReady).IsFalse()).To(BeTrue())
6769
Expect(nodeClass.StatusConditions().Get(status.ConditionReady).Message).To(Equal("ValidationSucceeded=False"))
6870
},
@@ -84,31 +86,29 @@ var _ = Describe("NodeClass Validation Status Controller", func() {
8486
})
8587
Context("Authorization Validation", func() {
8688
DescribeTable("NodeClass validation failure conditions",
87-
func(setupFn func()) {
89+
func(setupFn func(), reason string) {
8890
ExpectApplied(ctx, env.Client, nodeClass)
8991
setupFn()
9092
ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)
9193
nodeClass = ExpectExists(ctx, env.Client, nodeClass)
9294
Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsFalse()).To(BeTrue())
95+
Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).Reason).To(Equal(reason))
9396
},
94-
Entry("should update status condition as NotReady when CreateFleet unauthorized",
95-
func() {
96-
awsEnv.EC2API.CreateFleetBehavior.Error.Set(&smithy.GenericAPIError{
97-
Code: "UnauthorizedOperation",
98-
}, fake.MaxCalls(1))
99-
}),
100-
Entry("should update status condition as NotReady when RunInstances unauthorized",
101-
func() {
102-
awsEnv.EC2API.RunInstancesBehavior.Error.Set(&smithy.GenericAPIError{
103-
Code: "UnauthorizedOperation",
104-
}, fake.MaxCalls(1))
105-
}),
106-
Entry("should update status condition as NotReady when CreateLaunchTemplate unauthorized",
107-
func() {
108-
awsEnv.EC2API.CreateLaunchTemplateBehavior.Error.Set(&smithy.GenericAPIError{
109-
Code: "UnauthorizedOperation",
110-
}, fake.MaxCalls(1))
111-
}),
97+
Entry("should update status condition as NotReady when CreateFleet unauthorized", func() {
98+
awsEnv.EC2API.CreateFleetBehavior.Error.Set(&smithy.GenericAPIError{
99+
Code: "UnauthorizedOperation",
100+
}, fake.MaxCalls(1))
101+
}, nodeclass.ConditionReasonCreateFleetAuthFailed),
102+
Entry("should update status condition as NotReady when RunInstances unauthorized", func() {
103+
awsEnv.EC2API.RunInstancesBehavior.Error.Set(&smithy.GenericAPIError{
104+
Code: "UnauthorizedOperation",
105+
}, fake.MaxCalls(1))
106+
}, nodeclass.ConditionReasonRunInstancesAuthFailed),
107+
Entry("should update status condition as NotReady when CreateLaunchTemplate unauthorized", func() {
108+
awsEnv.EC2API.CreateLaunchTemplateBehavior.Error.Set(&smithy.GenericAPIError{
109+
Code: "UnauthorizedOperation",
110+
}, fake.MaxCalls(1))
111+
}, nodeclass.ConditionReasonCreateLaunchTemplateAuthFailed),
112112
)
113113

114114
It("should update status condition as Ready when authorized", func() {

0 commit comments

Comments
 (0)