Set Terminal condition on updating unmanaged (#45)

ACK uses finalizers to determine whether it is currently managing a k8s resource. We set the finalizer when creating the resource and delete it when we have deleted the underlying AWS resource. The current code path also sets the finalizer if one is not applied when attempting to update a resource. If a user were to create a resource that has the same name as an existing AWS resource, it will attempt to call `updateResource` which would set the finalizer and treat the resource as if ACK created it. This behaviour circumvents the safer adoption mechanisms put in place to prevent overriding existing resources.

This pull request ensures that any resource which does not have a finalizer, when attempting to call `updateResource`, will be marked with a terminal condition. Adopted resources are now marked with the finalizer when they are created.

Author: RedbackThomson

pkg/condition/condition.go

@@ -21,6 +21,13 @@ import (
 	acktypes "github.com/aws-controllers-k8s/runtime/pkg/types"
 )
 
+var (
+	NotManagedMessage = "Resource already exists"
+	NotManagedReason  = "This resource already exists but is not managed by ACK. " +
+		"To bring the resource under ACK management, you should explicitly adopt " +
+		"the resource by creating a services.k8s.aws/AdoptedResource"
+)
+
 // Synced returns the Condition in the resource's Conditions collection that is
 // of type ConditionTypeResourceSynced. If no such condition is found, returns
 // nil.

pkg/runtime/adoption_reconciler.go

@@ -209,6 +209,7 @@ func (r *adoptionReconciler) sync(
 	}
 
 	described.SetObjectMeta(*targetMeta)
+	targetDescriptor.MarkManaged(described)
 	targetDescriptor.MarkAdopted(described)
 
 	if err := r.kc.Create(ctx, described.RuntimeObject()); err != nil {

pkg/runtime/reconciler.go

@@ -28,6 +28,7 @@ import (
 
 	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	"github.com/aws-controllers-k8s/runtime/pkg/condition"
 	ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config"
 	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
 	ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics"
@@ -312,8 +313,8 @@ func (r *resourceReconciler) updateResource(
 	exit := rlog.Trace("r.updateResource")
 	defer exit(err)
 
-	// Ensure the resource is always managed (adopted resources apply)
-	if err = r.setResourceManaged(ctx, desired); err != nil {
+	// Ensure the resource is managed
+	if err = r.failOnResourceUnmanaged(ctx, latest); err != nil {
 		return latest, err
 	}
 
@@ -333,7 +334,7 @@ func (r *resourceReconciler) updateResource(
 		}
 		// Ensure that we are patching any changes to the annotations/metadata and
 		// the Spec that may have been set by the resource manager's successful
-		// Create call above.
+		// Update call above.
 		err = r.patchResourceMetadataAndSpec(ctx, desired, latest)
 		if err != nil {
 			return latest, err
@@ -402,13 +403,9 @@ func (r *resourceReconciler) patchResourceMetadataAndSpec(
 	}
 
 	rlog.Enter("kc.Patch (metadata + spec)")
-	// It is necessary to use `DeepCopyObject` versions of `latest` when calling
-	// `Patch` as this method overrides all values as merged from `desired`.
-	// This may affect `latest` in later execution, as otherwise this would set
-	//`desired` == `latest` after calling this method.
 	err = r.kc.Patch(
 		ctx,
-		latest.RuntimeObject().DeepCopyObject(),
+		latest.RuntimeObject(),
 		client.MergeFrom(desired.RuntimeObject().DeepCopyObject()),
 	)
 	rlog.Exit("kc.Patch (metadata + spec)", err)
@@ -432,13 +429,9 @@ func (r *resourceReconciler) patchResourceStatus(
 	defer exit(err)
 
 	rlog.Enter("kc.Patch (status)")
-	// It is necessary to use `DeepCopyObject` versions of `latest` when calling
-	// `Patch` as this method overrides all values as merged from `desired`.
-	// This may affect `latest` in later execution, as otherwise this would set
-	//`desired` == `latest` after calling this method.
 	err = r.kc.Status().Patch(
 		ctx,
-		latest.RuntimeObject().DeepCopyObject(),
+		latest.RuntimeObject(),
 		client.MergeFrom(desired.RuntimeObject().DeepCopyObject()),
 	)
 	rlog.Exit("kc.Patch (status)", err)
@@ -517,21 +510,14 @@ func (r *resourceReconciler) setResourceManaged(
 	if r.rd.IsManaged(res) {
 		return nil
 	}
-
 	var err error
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("r.setResourceManaged")
 	defer exit(err)
 
 	orig := res.RuntimeObject().DeepCopyObject()
 	r.rd.MarkManaged(res)
-	rlog.Enter("kc.Patch (metadata + spec)")
-	err = r.kc.Patch(
-		ctx,
-		res.RuntimeObject(),
-		client.MergeFrom(orig),
-	)
-	rlog.Exit("kc.Patch (metadata + spec)", err)
+	err = r.patchResourceMetadataAndSpec(ctx, r.rd.ResourceFromRuntimeObject(orig), res)
 	if err != nil {
 		return err
 	}
@@ -557,20 +543,29 @@ func (r *resourceReconciler) setResourceUnmanaged(
 
 	orig := res.RuntimeObject().DeepCopyObject()
 	r.rd.MarkUnmanaged(res)
-	rlog.Enter("kc.Patch (metadata + spec)")
-	err = r.kc.Patch(
-		ctx,
-		res.RuntimeObject(),
-		client.MergeFrom(orig),
-	)
-	rlog.Exit("kc.Patch (metadata + spec)", err)
+	err = r.patchResourceMetadataAndSpec(ctx, r.rd.ResourceFromRuntimeObject(orig), res)
 	if err != nil {
 		return err
 	}
 	rlog.Debug("removed resource from management")
 	return nil
 }
 
+// failOnResourceUnmanaged ensures that the underlying CR in the supplied
+// AWSResource has a finalizer. If it does not, it will set a Terminal condition
+// and return with an error
+func (r *resourceReconciler) failOnResourceUnmanaged(
+	ctx context.Context,
+	res acktypes.AWSResource,
+) error {
+	if r.rd.IsManaged(res) {
+		return nil
+	}
+
+	condition.SetTerminal(res, corev1.ConditionTrue, &condition.NotManagedMessage, &condition.NotManagedReason)
+	return ackerr.Terminal
+}
+
 // getAWSResource returns an AWSResource representing the requested Kubernetes
 // namespaced object
 func (r *resourceReconciler) getAWSResource(

pkg/runtime/reconciler_test.go

@@ -20,8 +20,10 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zapcore"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sobj "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	k8srtschema "k8s.io/apimachinery/pkg/runtime/schema"
@@ -30,7 +32,9 @@ import (
 
 	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	"github.com/aws-controllers-k8s/runtime/pkg/condition"
 	ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config"
+	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
 	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"
@@ -96,10 +100,20 @@ func reconcilerMocks(
 	), kc
 }
 
+func managedResourceManagerFactoryMocks(
+	desired acktypes.AWSResource,
+	latest acktypes.AWSResource,
+) (
+	*ackmocks.AWSResourceManagerFactory,
+	*ackmocks.AWSResourceDescriptor,
+) {
+	return managerFactoryMocks(desired, latest, true)
+}
+
 func managerFactoryMocks(
 	desired acktypes.AWSResource,
 	latest acktypes.AWSResource,
-	delta *ackcompare.Delta,
+	isManaged bool,
 ) (
 	*ackmocks.AWSResourceManagerFactory,
 	*ackmocks.AWSResourceDescriptor,
@@ -114,7 +128,7 @@ func managerFactoryMocks(
 	rd.On("EmptyRuntimeObject").Return(
 		&fakeBook{},
 	)
-	rd.On("IsManaged", desired).Return(true)
+	rd.On("IsManaged", latest).Return(isManaged)
 
 	rmf := &ackmocks.AWSResourceManagerFactory{}
 	rmf.On("ResourceDescriptor").Return(rd)
@@ -150,7 +164,7 @@ func TestReconcilerUpdate(t *testing.T) {
 		latest, nil,
 	)
 
-	rmf, rd := managerFactoryMocks(desired, latest, delta)
+	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
 	rd.On("Delta", desired, latest).Return(
 		delta,
 	).Once()
@@ -201,7 +215,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) {
 	// Note the change in annotations
 	latestMetaObj.SetAnnotations(map[string]string{"a": "b"})
 
-	rmf, rd := managerFactoryMocks(desired, latest, delta)
+	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
 	rd.On("Delta", desired, latest).Return(
 		delta,
 	).Once()
@@ -251,7 +265,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) {
 	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
 	// Note no change to metadata...
 
-	rmf, rd := managerFactoryMocks(desired, latest, delta)
+	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
 	rd.On("Delta", desired, latest).Return(
 		delta,
 	)
@@ -301,7 +315,7 @@ func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) {
 
 	latestMetaObj.SetAnnotations(map[string]string{"a": "b"})
 
-	rmf, _ := managerFactoryMocks(desired, latest, delta)
+	rmf, _ := managedResourceManagerFactoryMocks(desired, latest)
 	r, kc := reconcilerMocks(rmf)
 
 	statusWriter := &ctrlrtclientmock.StatusWriter{}
@@ -325,7 +339,7 @@ func TestReconcilerHandleReconcilerError_NoPatchStatus_NoLatest(t *testing.T) {
 
 	desired, _, _ := resourceMocks()
 
-	rmf, _ := managerFactoryMocks(desired, nil, nil)
+	rmf, _ := managedResourceManagerFactoryMocks(desired, nil)
 	r, kc := reconcilerMocks(rmf)
 
 	statusWriter := &ctrlrtclientmock.StatusWriter{}
@@ -368,7 +382,7 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) {
 		latest, nil,
 	)
 
-	rmf, rd := managerFactoryMocks(desired, latest, delta)
+	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
 	rd.On("Delta", desired, latest).Return(
 		delta,
 	).Once()
@@ -393,3 +407,62 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) {
 	kc.AssertNotCalled(t, "Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj))
 	rm.AssertCalled(t, "LateInitialize", ctx, latest)
 }
+
+func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+
+	ctx := context.TODO()
+	arn := ackv1alpha1.AWSResourceName("mybook-arn")
+
+	delta := ackcompare.NewDelta()
+
+	desired, _, _ := resourceMocks()
+
+	ids := &ackmocks.AWSResourceIdentifiers{}
+	ids.On("ARN").Return(&arn)
+
+	latest, _, _ := resourceMocks()
+	latest.On("Identifiers").Return(ids)
+	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+
+	terminalCondition := ackv1alpha1.Condition{
+		Type:    ackv1alpha1.ConditionTypeTerminal,
+		Status:  corev1.ConditionTrue,
+		Reason:  &condition.NotManagedReason,
+		Message: &condition.NotManagedMessage,
+	}
+	latest.On("ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition")).Return([]*ackv1alpha1.Condition{&terminalCondition}).Run(func(args mock.Arguments) {
+		conditions := args.Get(0).([]*ackv1alpha1.Condition)
+		hasTerminal := false
+		for _, condition := range conditions {
+			if condition.Type != ackv1alpha1.ConditionTypeTerminal {
+				continue
+			}
+
+			hasTerminal = true
+			assert.Equal(condition.Message, terminalCondition.Message)
+			assert.Equal(condition.Reason, terminalCondition.Reason)
+		}
+
+		assert.True(hasTerminal)
+	})
+
+	rm := &ackmocks.AWSResourceManager{}
+	rm.On("ReadOne", ctx, desired).Return(
+		latest, nil,
+	)
+
+	rmf, rd := managerFactoryMocks(desired, latest, false)
+
+	r, _ := reconcilerMocks(rmf)
+
+	_, err := r.Sync(ctx, rm, desired)
+	// Assert the error from late initialization
+	require.NotNil(err)
+	assert.Equal(ackerr.Terminal, err)
+	rm.AssertCalled(t, "ReadOne", ctx, desired)
+	rd.AssertNotCalled(t, "Delta", desired, latest)
+	rm.AssertNotCalled(t, "Update", ctx, desired, latest, delta)
+	rm.AssertNotCalled(t, "LateInitialize", ctx, latest)
+}