From 65b9179df039caa5982bbf62436b509e2f563a08 Mon Sep 17 00:00:00 2001 From: Vijay Tripathi Date: Mon, 7 Feb 2022 13:10:53 -0800 Subject: [PATCH] Use original status when making api-server `Status.Update` call in adoption reconciler (#71) Issue #, if available: https://github.com/aws-controllers-k8s/community/issues/1161 Description of changes: * The Status update from AdoptionReconciler was not useful because the Create call before Status.Update was resetting status of the CustomResource. * Earlier this similar kind of problem was also present in `reconciler.go` and it was fixed by keeping an original copy of Status before making Create/Update calls * I used the same solution in AdoptionReconciler. ---------- * Discovered this issue while debugging SageMaker ModelPackage adoption test. ModelPackage uses ARN as identifier which needs to be set inside `Status.ACKResourceMetadata.ARN` field of CustomResource. * Since the Identifier was never getting correctly set, the `ReadOne` call inside `reconciler.go` was failing while reconciling the CustomResource. -------------- * Validated by running SageMaker end-to-end tests successfully. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --- pkg/runtime/adoption_reconciler.go | 8 +++- pkg/runtime/adoption_reconciler_test.go | 58 +++++++++++++++---------- 2 files changed, 42 insertions(+), 24 deletions(-) diff --git a/pkg/runtime/adoption_reconciler.go b/pkg/runtime/adoption_reconciler.go index 2724ee0..7dd8e92 100644 --- a/pkg/runtime/adoption_reconciler.go +++ b/pkg/runtime/adoption_reconciler.go @@ -224,10 +224,16 @@ func (r *adoptionReconciler) Sync( }, described.RuntimeObject()); err != nil { if apierrors.IsNotFound(err) { // If Adopted AWS resource was not found in k8s, create it. + + // Before creation, Keep the copy of original described object + // because after the create call, Status gets set to empty + describedCopy := described.DeepCopy() if err := r.kc.Create(ctx, described.RuntimeObject()); err != nil { return r.onError(ctx, desired, err) } - + // reset the status of described object to original value before + // making the Status Update call + described.SetStatus(describedCopy) if err := r.kc.Status().Update(ctx, described.RuntimeObject()); err != nil { return r.onError(ctx, desired, err) } diff --git a/pkg/runtime/adoption_reconciler_test.go b/pkg/runtime/adoption_reconciler_test.go index 935600a..013ec62 100644 --- a/pkg/runtime/adoption_reconciler_test.go +++ b/pkg/runtime/adoption_reconciler_test.go @@ -72,13 +72,14 @@ func mockReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Cli ), kc, apiReader } -func mockDescriptorAndAWSResource() (*ackmocks.AWSResourceDescriptor, *ackmocks.AWSResource) { +func mockDescriptorAndAWSResource() (*ackmocks.AWSResourceDescriptor, *ackmocks.AWSResource, *ackmocks.AWSResource) { des := &ackmocks.AWSResourceDescriptor{} emptyRuntimeObject := &ctrlrtclientmock.Object{} res := &ackmocks.AWSResource{} + resDeepCopy := &ackmocks.AWSResource{} des.On("EmptyRuntimeObject").Return(emptyRuntimeObject) des.On("ResourceFromRuntimeObject", emptyRuntimeObject).Return(res) - return des, res + return des, res, resDeepCopy } func mockManager() *ackmocks.AWSResourceManager { @@ -91,7 +92,11 @@ func setupMockClient(kc *ctrlrtclientmock.Client, statusWriter *ctrlrtclientmock kc.On("Patch", ctx, adoptedRes, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) } -func setupMockAwsResource(res *ackmocks.AWSResource, adoptedRes *ackv1alpha1.AdoptedResource) { +func setupMockAwsResource( + res *ackmocks.AWSResource, + resDeepCopy *ackmocks.AWSResource, + adoptedRes *ackv1alpha1.AdoptedResource, +) { res.On("SetIdentifiers", adoptedRes.Spec.AWS).Return(nil) res.On("SetObjectMeta", mock.AnythingOfType("ObjectMeta")).Run(func(args mock.Arguments) {}) @@ -108,6 +113,8 @@ func setupMockAwsResource(res *ackmocks.AWSResource, adoptedRes *ackv1alpha1.Ado rmo.On("GetFinalizers").Return(make([]string, 0)) rmo.On("GetOwnerReferences").Return(make([]v1.OwnerReference, 0)) rmo.On("GetGenerateName").Return("") + res.On("DeepCopy").Return(resDeepCopy) + res.On("SetStatus", resDeepCopy).Run(func(args mock.Arguments) {}) } func setupMockManager(manager *ackmocks.AWSResourceManager, ctx context.Context, res *ackmocks.AWSResource) { @@ -148,7 +155,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { require := require.New(t) // Mock resource creation r, kc, apiReader := mockReconciler() - descriptor, res := mockDescriptorAndAWSResource() + descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(Namespace, Name) res.On("SetIdentifiers", adoptedRes.Spec.AWS).Return(errors.New("unable to set Identifier")) @@ -156,7 +163,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { statusWriter := &ctrlrtclientmock.StatusWriter{} //Mock behavior setup - setupMockAwsResource(res, adoptedRes) + setupMockAwsResource(res, resDeepCopy, adoptedRes) setupMockClient(kc, statusWriter, ctx, adoptedRes) // Call @@ -176,7 +183,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { Namespace: Namespace, Name: Name, }, res.RuntimeObject()) - assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res) + assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) } @@ -186,14 +193,14 @@ func TestSync_FailureInReadOne(t *testing.T) { require := require.New(t) // Mock resource creation r, kc, apiReader := mockReconciler() - descriptor, res := mockDescriptorAndAWSResource() + descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(Namespace, Name) ctx := context.TODO() statusWriter := &ctrlrtclientmock.StatusWriter{} //Mock behavior setup - setupMockAwsResource(res, adoptedRes) + setupMockAwsResource(res, resDeepCopy, adoptedRes) setupMockClient(kc, statusWriter, ctx, adoptedRes) manager.On("ReadOne", ctx, res).Return(res, errors.New("failed to perform ReadOne")) @@ -213,7 +220,7 @@ func TestSync_FailureInReadOne(t *testing.T) { Namespace: Namespace, Name: Name, }, res.RuntimeObject()) - assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res) + assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) } @@ -223,14 +230,14 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { require := require.New(t) // Mock resource creation r, kc, apiReader := mockReconciler() - descriptor, res := mockDescriptorAndAWSResource() + descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(Namespace, Name) ctx := context.TODO() statusWriter := &ctrlrtclientmock.StatusWriter{} //Mock behavior setup - setupMockAwsResource(res, adoptedRes) + setupMockAwsResource(res, resDeepCopy, adoptedRes) setupMockClient(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) @@ -246,7 +253,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { //Assertions require.Nil(err) assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) - assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res) + assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertManaged(true, t, ctx, kc, adoptedRes) assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes) } @@ -256,14 +263,14 @@ func TestSync_APIReaderUnknownError(t *testing.T) { require := require.New(t) // Mock resource creation r, kc, apiReader := mockReconciler() - descriptor, res := mockDescriptorAndAWSResource() + descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(Namespace, Name) ctx := context.TODO() statusWriter := &ctrlrtclientmock.StatusWriter{} //Mock behavior setup - setupMockAwsResource(res, adoptedRes) + setupMockAwsResource(res, resDeepCopy, adoptedRes) setupMockClient(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) @@ -280,7 +287,7 @@ func TestSync_APIReaderUnknownError(t *testing.T) { require.NotNil(err) require.Equal("unknown error", err.Error()) assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) - assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res) + assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) } @@ -290,14 +297,14 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { require := require.New(t) // Mock resource creation r, kc, apiReader := mockReconciler() - descriptor, res := mockDescriptorAndAWSResource() + descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(Namespace, Name) ctx := context.TODO() statusWriter := &ctrlrtclientmock.StatusWriter{} //Mock behavior setup - setupMockAwsResource(res, adoptedRes) + setupMockAwsResource(res, resDeepCopy, adoptedRes) setupMockClient(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) @@ -323,14 +330,14 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { require := require.New(t) // Mock resource creation r, kc, apiReader := mockReconciler() - descriptor, res := mockDescriptorAndAWSResource() + descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(Namespace, Name) ctx := context.TODO() statusWriter := &ctrlrtclientmock.StatusWriter{} //Mock behavior setup - setupMockAwsResource(res, adoptedRes) + setupMockAwsResource(res, resDeepCopy, adoptedRes) setupMockClient(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) @@ -345,7 +352,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { require.NotNil(err) require.Equal("status update failure", err.Error()) assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) - assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res) + assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy) assertManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) } @@ -355,14 +362,14 @@ func TestSync_HappyCase(t *testing.T) { require := require.New(t) // Mock resource creation r, kc, apiReader := mockReconciler() - descriptor, res := mockDescriptorAndAWSResource() + descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(Namespace, Name) ctx := context.TODO() statusWriter := &ctrlrtclientmock.StatusWriter{} //Mock behavior setup - setupMockAwsResource(res, adoptedRes) + setupMockAwsResource(res, resDeepCopy, adoptedRes) setupMockClient(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) @@ -376,7 +383,7 @@ func TestSync_HappyCase(t *testing.T) { //Assertions require.Nil(err) assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) - assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res) + assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy) assertManaged(true, t, ctx, kc, adoptedRes) assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes) } @@ -432,12 +439,17 @@ func assertAWSResourceCreation( kc *ctrlrtclientmock.Client, statusWriter *ctrlrtclientmock.StatusWriter, res *ackmocks.AWSResource, + resDeepCopy *ackmocks.AWSResource, ) { if expectedCreation { kc.AssertCalled(t, "Create", ctx, res.RuntimeObject()) + res.AssertCalled(t, "DeepCopy") + res.AssertCalled(t, "SetStatus", resDeepCopy) statusWriter.AssertCalled(t, "Update", ctx, res.RuntimeObject()) } else { kc.AssertNotCalled(t, "Create", ctx, res.RuntimeObject()) + res.AssertNotCalled(t, "DeepCopy") + res.AssertNotCalled(t, "SetStatus", resDeepCopy) statusWriter.AssertNotCalled(t, "Update", ctx, res.RuntimeObject()) } }