diff --git a/apis/core/v1alpha1/conditions.go b/apis/core/v1alpha1/conditions.go index dbef4bf..c06a5e5 100644 --- a/apis/core/v1alpha1/conditions.go +++ b/apis/core/v1alpha1/conditions.go @@ -45,6 +45,13 @@ const ( // Examples include // - Modifying an immutable field after it was created ConditionTypeAdvisory ConditionType = "ACK.Advisory" + // ConditionTypeLateInitialized indicates whether the late initialization + // of fields is completed or is in progress. + // The absence of this condition indicates there is no late initalization + // needed for the k8s resource. + // "True" status indicates that the resource fields have been late initialized + // "False" status indicates that the resource fields are in process of being late initialized. + ConditionTypeLateInitialized ConditionType = "ACK.LateInitialized" ) // Condition is the common struct used by all CRDs managed by ACK service diff --git a/mocks/pkg/types/aws_resource.go b/mocks/pkg/types/aws_resource.go index 968f5de..107e1cf 100644 --- a/mocks/pkg/types/aws_resource.go +++ b/mocks/pkg/types/aws_resource.go @@ -135,3 +135,8 @@ func (_m *AWSResource) SetIdentifiers(_a0 *v1alpha1.AWSIdentifiers) error { func (_m *AWSResource) SetObjectMeta(meta v1.ObjectMeta) { _m.Called(meta) } + +// SetStatus provides a mock function with given fields: _a0 +func (_m *AWSResource) SetStatus(_a0 types.AWSResource) { + _m.Called(_a0) +} diff --git a/mocks/pkg/types/aws_resource_manager.go b/mocks/pkg/types/aws_resource_manager.go index c081352..1395dd6 100644 --- a/mocks/pkg/types/aws_resource_manager.go +++ b/mocks/pkg/types/aws_resource_manager.go @@ -77,6 +77,29 @@ func (_m *AWSResourceManager) Delete(_a0 context.Context, _a1 types.AWSResource) return r0, r1 } +// LateInitialize provides a mock function with given fields: _a0, _a1 +func (_m *AWSResourceManager) LateInitialize(_a0 context.Context, _a1 types.AWSResource) (types.AWSResource, error) { + ret := _m.Called(_a0, _a1) + + var r0 types.AWSResource + if rf, ok := ret.Get(0).(func(context.Context, types.AWSResource) types.AWSResource); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.AWSResource) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, types.AWSResource) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // ReadOne provides a mock function with given fields: _a0, _a1 func (_m *AWSResourceManager) ReadOne(_a0 context.Context, _a1 types.AWSResource) (types.AWSResource, error) { ret := _m.Called(_a0, _a1) diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index fd4e42e..a6f95b8 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -35,6 +35,13 @@ func Terminal(subject acktypes.ConditionManager) *ackv1alpha1.Condition { return FirstOfType(subject, ackv1alpha1.ConditionTypeTerminal) } +// LateInitialized returns the Condition in the resource's Conditions collection that +// is of type ConditionTypeLateInitialized. If no such condition is found, returns +// nil. +func LateInitialized(subject acktypes.ConditionManager) *ackv1alpha1.Condition { + return FirstOfType(subject, ackv1alpha1.ConditionTypeLateInitialized) +} + // FirstOfType returns the first Condition in the resource's Conditions // collection of the supplied type. If no such condition is found, returns nil. func FirstOfType( @@ -111,3 +118,35 @@ func SetTerminal( c.Reason = reason subject.ReplaceConditions(allConds) } + +// SetLateInitialized sets the resource's Condition of type ConditionTypeLateInitialized to +// the supplied status, optional message and reason. +func SetLateInitialized( + subject acktypes.ConditionManager, + status corev1.ConditionStatus, + message *string, + reason *string, +) { + allConds := subject.Conditions() + var c *ackv1alpha1.Condition + if c = LateInitialized(subject); c == nil { + c = &ackv1alpha1.Condition{ + Type: ackv1alpha1.ConditionTypeLateInitialized, + } + allConds = append(allConds, c) + } + now := metav1.Now() + c.LastTransitionTime = &now + c.Status = status + c.Message = message + c.Reason = reason + subject.ReplaceConditions(allConds) +} + +// LateInitializationInProgress return true if ConditionTypeLateInitialized has "False" status +// False status means that resource has LateInitializationConfig but has not been completely +// late initialized yet. +func LateInitializationInProgress(subject acktypes.ConditionManager) bool { + c := LateInitialized(subject) + return c != nil && c.Status == corev1.ConditionFalse +} diff --git a/pkg/requeue/requeue.go b/pkg/requeue/requeue.go index eb4b983..57e9c91 100644 --- a/pkg/requeue/requeue.go +++ b/pkg/requeue/requeue.go @@ -53,13 +53,16 @@ type RequeueNeeded struct { } func (e *RequeueNeeded) Error() string { - if e.err == nil { + if e == nil || e.err == nil { return "" } return e.err.Error() } func (e *RequeueNeeded) Unwrap() error { + if e == nil { + return nil + } return e.err } @@ -78,17 +81,23 @@ type RequeueNeededAfter struct { } func (e *RequeueNeededAfter) Error() string { - if e.err == nil { + if e == nil || e.err == nil { return "" } return e.err.Error() } func (e *RequeueNeededAfter) Duration() time.Duration { + if e == nil { + return time.Duration(0)*time.Second + } return e.duration } func (e *RequeueNeededAfter) Unwrap() error { + if e == nil { + return nil + } return e.err } diff --git a/pkg/requeue/requeue_test.go b/pkg/requeue/requeue_test.go index 82ff66d..51801cb 100644 --- a/pkg/requeue/requeue_test.go +++ b/pkg/requeue/requeue_test.go @@ -17,10 +17,9 @@ import ( "testing" "time" + "github.com/aws-controllers-k8s/runtime/pkg/requeue" "github.com/pkg/errors" "github.com/stretchr/testify/assert" - - "github.com/aws-controllers-k8s/runtime/pkg/requeue" ) func TestRequeueNeeded(t *testing.T) { @@ -109,3 +108,15 @@ func TestRequeueNeededAfter(t *testing.T) { }) } } + +func TestRequeueNeededAfter_Nil(t *testing.T) { + assert := assert.New(t) + var nilRequeueNeededAfter *requeue.RequeueNeededAfter + assert.Empty(nilRequeueNeededAfter.Error()) + assert.Nil(nilRequeueNeededAfter.Unwrap()) + assert.Equal("0s", nilRequeueNeededAfter.Duration().String()) + + var nilRequeueNeeded *requeue.RequeueNeeded + assert.Empty(nilRequeueNeeded.Error()) + assert.Nil(nilRequeueNeeded.Unwrap()) +} diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 8dec793..2b495ac 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -221,6 +221,11 @@ func (r *resourceReconciler) Sync( return latest, err } } + // Attempt to late initialize the resource. If there are no fields to + // late initialize, this operation will be a no-op. + if latest, err = r.lateInitializeResource(ctx, rm, latest); err != nil { + return latest, err + } return r.handleRequeues(ctx, latest) } @@ -338,6 +343,43 @@ func (r *resourceReconciler) updateResource( return latest, nil } +// lateInitializeResource calls AWSResourceManager.LateInitialize() method and +// returns the AWSResource with late initialized fields. +// +// When the late initialization is delayed for an AWSResource, an error is returned +// with specific requeue delay to attempt lateInitialization again. +// +// This method also adds an annotation to K8s CR, indicating the number of +// late initialization attempts to correctly calculate exponential backoff delay +// +// This method also adds Condition to CR's status indicating status of late initialization. +func (r *resourceReconciler) lateInitializeResource( + ctx context.Context, + rm acktypes.AWSResourceManager, + latest acktypes.AWSResource, +) (acktypes.AWSResource, error) { + var err error + rlog := ackrtlog.FromContext(ctx) + exit := rlog.Trace("r.lateInitializeResource") + defer exit(err) + + rlog.Enter("rm.LateInitialize") + lateInitializedLatest, err := rm.LateInitialize(ctx, latest) + rlog.Exit("rm.LateInitialize", err) + // Always patch after late initialize because some fields may have been initialized while + // others require a retry after some delay. + // This patching does not hurt because if there is no diff then 'patchResourceMetadataAndSpec' + // acts as a no-op. + if ackcompare.IsNotNil(lateInitializedLatest) { + patchErr := r.patchResourceMetadataAndSpec(ctx, latest, lateInitializedLatest) + // Throw the patching error if reconciler is unable to patch the resource with late initializations + if patchErr != nil { + err = patchErr + } + } + return lateInitializedLatest, err +} + // patchResourceMetadataAndSpec patches the custom resource in the Kubernetes API to match the // supplied latest resource's metadata and spec. func (r *resourceReconciler) patchResourceMetadataAndSpec( diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index 7233dd4..3bb8942 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -15,8 +15,11 @@ package runtime_test import ( "context" + "errors" "testing" + "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap/zapcore" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -29,6 +32,7 @@ import ( ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare" ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config" ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics" + "github.com/aws-controllers-k8s/runtime/pkg/requeue" ackrt "github.com/aws-controllers-k8s/runtime/pkg/runtime" ackrtcache "github.com/aws-controllers-k8s/runtime/pkg/runtime/cache" acktypes "github.com/aws-controllers-k8s/runtime/pkg/types" @@ -152,6 +156,9 @@ func TestReconcilerUpdate(t *testing.T) { ).Once() rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) + rm.On("LateInitialize", ctx, latest).Return(latest, nil) + rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) + r, kc := reconcilerMocks(rmf) kc.On("Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)).Return(nil) @@ -170,6 +177,7 @@ func TestReconcilerUpdate(t *testing.T) { kc.AssertNotCalled(t, "Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)) // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus kc.AssertNotCalled(t, "Status") + rm.AssertCalled(t, "LateInitialize", ctx, latest) } func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { @@ -206,6 +214,8 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { rm.On("Update", ctx, desired, latest, delta).Return( latest, nil, ) + rm.On("LateInitialize", ctx, latest).Return(latest, nil) + rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc := reconcilerMocks(rmf) @@ -219,6 +229,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { kc.AssertCalled(t, "Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)) // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus kc.AssertNotCalled(t, "Status") + rm.AssertCalled(t, "LateInitialize", ctx, latest) } func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { @@ -252,6 +263,8 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { rm.On("Update", ctx, desired, latest, delta).Return( latest, nil, ) + rm.On("LateInitialize", ctx, latest).Return(latest, nil) + rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc := reconcilerMocks(rmf) @@ -265,6 +278,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { kc.AssertCalled(t, "Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)) // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus kc.AssertNotCalled(t, "Status") + rm.AssertCalled(t, "LateInitialize", ctx, latest) } func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) { @@ -326,3 +340,56 @@ func TestReconcilerHandleReconcilerError_NoPatchStatus_NoLatest(t *testing.T) { // patch the spec/metadata... kc.AssertNotCalled(t, "Patch") } + +func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + + ctx := context.TODO() + arn := ackv1alpha1.AWSResourceName("mybook-arn") + + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + + desired, desiredRTObj, _ := resourceMocks() + + ids := &ackmocks.AWSResourceIdentifiers{} + ids.On("ARN").Return(&arn) + + latest, latestRTObj, _ := resourceMocks() + latest.On("Identifiers").Return(ids) + latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) + + rm := &ackmocks.AWSResourceManager{} + rm.On("ReadOne", ctx, desired).Return( + latest, nil, + ) + rm.On("Update", ctx, desired, latest, delta).Return( + latest, nil, + ) + + rmf, rd := managerFactoryMocks(desired, latest, delta) + rd.On("Delta", desired, latest).Return( + delta, + ).Once() + rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) + + requeueError := requeue.NeededAfter(errors.New("error from late initialization"), time.Duration(0)*time.Second) + rm.On("LateInitialize", ctx, latest).Return(latest, requeueError) + rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) + + r, kc := reconcilerMocks(rmf) + + kc.On("Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)).Return(nil) + + _, err := r.Sync(ctx, rm, desired) + // Assert the error from late initialization + require.NotNil(err) + assert.Equal(requeueError, err) + rm.AssertCalled(t, "ReadOne", ctx, desired) + rd.AssertCalled(t, "Delta", desired, latest) + rm.AssertCalled(t, "Update", ctx, desired, latest, delta) + // No difference in desired, latest metadata and spec + kc.AssertNotCalled(t, "Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)) + rm.AssertCalled(t, "LateInitialize", ctx, latest) +} diff --git a/pkg/types/aws_resource_manager.go b/pkg/types/aws_resource_manager.go index bb6acfd..487fa8a 100644 --- a/pkg/types/aws_resource_manager.go +++ b/pkg/types/aws_resource_manager.go @@ -67,6 +67,13 @@ type AWSResourceManager interface { // GetAttributes operations but all we have (for new CRs at least) is a // name for the resource ARNFromName(string) string + // LateInitialize returns an AWS Resource after setting the late initialized + // fields from the ReadOne call. This method will initialize the optional fields + // which were not provided by the k8s user but were defaulted by the AWS service. + // If there are no such fields to be initialized, the returned object is identical to + // object passed in the parameter. + // This method also adds/updates the ConditionTypeLateInitialized for the AWSResource. + LateInitialize(context.Context, AWSResource) (AWSResource, error) } // AWSResourceManagerFactory returns an AWSResourceManager that can be used to