Skip to content

Commit c0254e4

Browse files
vijtrip2vijtrip2
authored
ensure Status.Conditions before patching resource status (#66)
Issue #, if available: aws-controllers-k8s/community#1133 Description of changes: * the logic for ensuring conditions was never been executed because the resource parameter(`latest`) passed in the defer call was always nil . See [Why](https://stackoverflow.com/a/42703862) * If the `desired` and `latest` state have no delta, set `ResourceSynced` condition to `True` * Set `ResourceSynced` condition to `False` when there is error in `LateInitialization` * For any reconciler error, except Terminal, if ResourceSynced condition is missing, set it to `False` * If there are no reconciler error, and ResourceSynced condition is missing, set it to `Unknown` * Revert aws-sdk-go upgrade and handle that in separate release. --- Tested these changes locally for iam-controller and tests pass successfully validating ResourceSynced condition. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 19c416f commit c0254e4

File tree

4 files changed

+74
-17
lines changed

4 files changed

+74
-17
lines changed

‎go.mod‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ module github.com/aws-controllers-k8s/runtime
33
go 1.17
44

55
require (
6-
github.com/aws/aws-sdk-go v1.42.35
6+
github.com/aws/aws-sdk-go v1.37.10
77
github.com/go-logr/logr v1.2.0
88
github.com/google/go-cmp v0.5.5
99
github.com/jaypipes/envutil v1.0.0
@@ -44,7 +44,7 @@ require (
4444
github.com/stretchr/objx v0.1.1 // indirect
4545
go.uber.org/atomic v1.7.0 // indirect
4646
go.uber.org/multierr v1.6.0 // indirect
47-
golang.org/x/net v0.0.0-20211216030914-fe4d6282115f // indirect
47+
golang.org/x/net v0.0.0-20210825183410-e898025ed96a // indirect
4848
golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f // indirect
4949
golang.org/x/sys v0.0.0-20211029165221-6e7872819dc8 // indirect
5050
golang.org/x/term v0.0.0-20210615171337-6886f2dfbf5b // indirect

‎go.sum‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ github.com/armon/circbuf v0.0.0-20150827004946-bbbad097214e/go.mod h1:3U/XgcO3hC
6464
github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da/go.mod h1:Q73ZrmVTwzkszR9V5SSuryQ31EELlFMUz1kKyl939pY=
6565
github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=
6666
github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
67-
github.com/aws/aws-sdk-go v1.42.35 h1:N4N9buNs4YlosI9N0+WYrq8cIZwdgv34yRbxzZlTvFs=
68-
github.com/aws/aws-sdk-go v1.42.35/go.mod h1:OGr6lGMAKGlG9CVrYnWYDKIyb829c6EVBRjxqjmPepc=
67+
github.com/aws/aws-sdk-go v1.37.10 h1:LRwl+97B4D69Z7tz+eRUxJ1C7baBaIYhgrn5eLtua+Q=
68+
github.com/aws/aws-sdk-go v1.37.10/go.mod h1:hcU610XS61/+aQV88ixoOzUoG7v3b31pl2zKMmprdro=
6969
github.com/benbjohnson/clock v1.0.3/go.mod h1:bGMdMPoPVvcYyt1gHDf4J2KE153Yf9BuiUKYMaxlTDM=
7070
github.com/benbjohnson/clock v1.1.0 h1:Q92kusRqC1XV2MjkWETPvjJVqKetz1OzxZB7mHJLju8=
7171
github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA=
@@ -590,9 +590,8 @@ golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96b
590590
golang.org/x/net v0.0.0-20210428140749-89ef3d95e781/go.mod h1:OJAsFXCWl8Ukc7SiCT/9KSuxbyM7479/AVlXFRxuMCk=
591591
golang.org/x/net v0.0.0-20210525063256-abc453219eb5/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
592592
golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
593+
golang.org/x/net v0.0.0-20210825183410-e898025ed96a h1:bRuuGXV8wwSdGTB+CtJf+FjgO1APK1CoO39T4BN/XBw=
593594
golang.org/x/net v0.0.0-20210825183410-e898025ed96a/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
594-
golang.org/x/net v0.0.0-20211216030914-fe4d6282115f h1:hEYJvxw1lSnWIl8X9ofsYMklzaDs90JI2az5YMd4fPM=
595-
golang.org/x/net v0.0.0-20211216030914-fe4d6282115f/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
596595
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
597596
golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
598597
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=

‎pkg/runtime/reconciler.go‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,9 @@ func (r *resourceReconciler) Sync(
208208
var latest acktypes.AWSResource // the newly created or mutated resource
209209

210210
r.resetConditions(ctx, desired)
211-
defer r.ensureConditions(ctx, latest, err)
211+
defer func() {
212+
r.ensureConditions(ctx, latest, err)
213+
}()
212214

213215
isAdopted := IsAdopted(desired)
214216
rlog.WithValues("is_adopted", isAdopted)
@@ -242,6 +244,15 @@ func (r *resourceReconciler) Sync(
242244
// Attempt to late initialize the resource. If there are no fields to
243245
// late initialize, this operation will be a no-op.
244246
if latest, err = r.lateInitializeResource(ctx, rm, latest); err != nil {
247+
// TODO(vijtrip2): move this condition handling to generated
248+
// AWSResourceManager.LateInitialize() method
249+
250+
// Whenever late initialization fails for a resource, set ACK.ResourceSynced
251+
// condition explicitly to "False"
252+
// Setting this explicitly to False is required because ACK.ResourceSynced
253+
// condition can be True due to successful Create/Update call OR no
254+
// Create/Update call in reconciler loop
255+
ackcondition.SetSynced(latest, corev1.ConditionFalse, nil, nil)
245256
return latest, err
246257
}
247258
return r.handleRequeues(ctx, latest)
@@ -287,11 +298,16 @@ func (r *resourceReconciler) ensureConditions(
287298
// stable sync state. Even if we got no error back from the
288299
// create/update operations, we can only set this to Unknown.
289300
condStatus := corev1.ConditionUnknown
290-
if reconcileErr == ackerr.Terminal {
291-
// A terminal condition by its very nature indicates a stable state
292-
// for a resource being synced. The resource is considered synced
293-
// because its state will not change.
294-
condStatus = corev1.ConditionTrue
301+
if reconcileErr != nil {
302+
if reconcileErr == ackerr.Terminal {
303+
// A terminal condition by its very nature indicates a stable state
304+
// for a resource being synced. The resource is considered synced
305+
// because its state will not change.
306+
condStatus = corev1.ConditionTrue
307+
} else {
308+
// For any other reconciler error, set synced condition to false
309+
condStatus = corev1.ConditionFalse
310+
}
295311
}
296312
ackcondition.SetSynced(res, condStatus, nil, nil)
297313
}
@@ -407,6 +423,10 @@ func (r *resourceReconciler) updateResource(
407423
return latest, err
408424
}
409425
rlog.Info("updated resource")
426+
} else {
427+
// If there is no delta between desired state and latest state, it is
428+
// safe to set ACK.ResourceSynced condition to true.
429+
ackcondition.SetSynced(latest, corev1.ConditionTrue, nil, nil)
410430
}
411431
return latest, nil
412432
}

‎pkg/runtime/reconciler_test.go‎

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) {
283283

284284
func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) {
285285
require := require.New(t)
286+
assert := assert.New(t)
286287

287288
ctx := context.TODO()
288289
arn := ackv1alpha1.AWSResourceName("mybook-arn")
@@ -299,10 +300,24 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) {
299300
latest, latestRTObj, _ := resourceMocks()
300301
latest.On("Identifiers").Return(ids)
301302
latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
303+
// ensureConditions method will add ACK.ResourceSynced condition as Unknown
302304
latest.On(
303305
"ReplaceConditions",
304306
mock.AnythingOfType("[]*v1alpha1.Condition"),
305-
).Return()
307+
).Return().Run(func(args mock.Arguments) {
308+
conditions := args.Get(0).([]*ackv1alpha1.Condition)
309+
hasSynced := false
310+
for _, condition := range conditions {
311+
if condition.Type != ackv1alpha1.ConditionTypeResourceSynced {
312+
continue
313+
}
314+
315+
hasSynced = true
316+
assert.Equal(condition.Status, corev1.ConditionUnknown)
317+
}
318+
319+
assert.True(hasSynced)
320+
})
306321
// Note no change to metadata...
307322

308323
rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
@@ -423,11 +438,29 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) {
423438

424439
latest, latestRTObj, _ := resourceMocks()
425440
latest.On("Identifiers").Return(ids)
426-
latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
441+
syncCondition := ackv1alpha1.Condition{
442+
Type: ackv1alpha1.ConditionTypeResourceSynced,
443+
Status: corev1.ConditionFalse,
444+
}
445+
latest.On("Conditions").Return([]*ackv1alpha1.Condition{}).Times(2)
446+
latest.On("Conditions").Return([]*ackv1alpha1.Condition{&syncCondition})
427447
latest.On(
428448
"ReplaceConditions",
429449
mock.AnythingOfType("[]*v1alpha1.Condition"),
430-
).Return()
450+
).Return().Run(func(args mock.Arguments) {
451+
conditions := args.Get(0).([]*ackv1alpha1.Condition)
452+
hasSynced := false
453+
for _, condition := range conditions {
454+
if condition.Type != ackv1alpha1.ConditionTypeResourceSynced {
455+
continue
456+
}
457+
458+
hasSynced = true
459+
assert.Equal(condition.Status, corev1.ConditionFalse)
460+
}
461+
462+
assert.True(hasSynced)
463+
})
431464

432465
rm := &ackmocks.AWSResourceManager{}
433466
rm.On("ResolveReferences", ctx, nil, desired).Return(
@@ -484,14 +517,20 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
484517

485518
latest, _, _ := resourceMocks()
486519
latest.On("Identifiers").Return(ids)
487-
latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
488520

489521
terminalCondition := ackv1alpha1.Condition{
490522
Type: ackv1alpha1.ConditionTypeTerminal,
491523
Status: corev1.ConditionTrue,
492524
Reason: &condition.NotManagedReason,
493525
Message: &condition.NotManagedMessage,
494526
}
527+
// Return empty conditions for first two times
528+
latest.On("Conditions").Return([]*ackv1alpha1.Condition{}).Times(2)
529+
// Once the terminal condition is added, return terminal condition
530+
// These calls will be made from ensureConditions method, which sets
531+
// ACK.ResourceSynced condition correctly
532+
latest.On("Conditions").Return([]*ackv1alpha1.Condition{&terminalCondition})
533+
495534
latest.On("ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition")).Return([]*ackv1alpha1.Condition{&terminalCondition}).Run(func(args mock.Arguments) {
496535
conditions := args.Get(0).([]*ackv1alpha1.Condition)
497536
hasTerminal := false
@@ -521,7 +560,6 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
521560
r, _ := reconcilerMocks(rmf)
522561

523562
_, err := r.Sync(ctx, rm, desired)
524-
// Assert the error from late initialization
525563
require.NotNil(err)
526564
assert.Equal(ackerr.Terminal, err)
527565
rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired)

0 commit comments

Comments
 (0)