Use original status when making api-server Status.Update call in adoption reconciler (#71)

Issue #, if available: aws-controllers-k8s/community#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.

Author: vijtrip2

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)
 			}

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,15 +155,15 @@ 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"))
 	ctx := context.TODO()
 	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())
 	}
 }