Add read-only support for ACK resources via annotation

This commit adds support for making ACK resources read-only via an
annotation. A new `services.k8s.aws/read-only` annotation can be
added to a resource to instruct the ACK service controller to not
create/patch/delete the underlying AWS resource.

If the annotation is set to "true", the reconciler will:
- return an error on create if the resource doesn't exist
- only patch metadata/spec for updates but not make cloud API calls
- skip deletion of the underlying resource when the Kubernetes resource
  is deleted

This allows adopting externally-managed resources into ACK for read-only
purposes like referencing values, without ACK attempting to manage the
resource lifecycle.

Signed-off-by: Amine Hilaly <[email protected]>

apis/core/v1alpha1/annotations.go

@@ -66,4 +66,10 @@ const (
 	// resource manager will leave the AWS resource intact when the K8s resource
 	// is deleted.
 	AnnotationDeletionPolicy = AnnotationPrefix + "deletion-policy"
+	// AnnotationReadOnly is an annotation whose value is a boolean indicating
+	// whether the resource is read-only. If this annotation is set to true on a
+	// CR, that means the user is indicating to the ACK service controller that
+	// the resource is read-only and should not be created/patched/deleted by the
+	// ACK service controller.
+	AnnotationReadOnly = AnnotationPrefix + "read-only"
 )

pkg/errors/error.go

@@ -24,6 +24,9 @@ var (
 	// information that the resource being checked for existence was
 	// previously-created out of band from ACK
 	AdoptedResourceNotFound = fmt.Errorf("adopted resource not found")
+	// ReadOnlyResourceNotFound is like NotFound but provides the caller with
+	// information that the resource is on read-only mode and was not found
+	ReadOnlyResourceNotFound = fmt.Errorf("read-only resource not found")
 	// MissingNameIdentifier indicates an unexpected nil name identifier pointer
 	MissingNameIdentifier = fmt.Errorf("expected name identifier, found nil")
 	// NotAdoptable is to indicate the current resource has been explicitly

pkg/runtime/reconciler.go

@@ -236,8 +236,9 @@ func (r *resourceReconciler) reconcile(
 	res acktypes.AWSResource,
 ) (acktypes.AWSResource, error) {
 	if res.IsBeingDeleted() {
-		// Determine whether we should retain or delete the resource
-		if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete {
+		// We only delete resources that are not read-only and have a deletion
+		// policy set to delete.
+		if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete && !IsReadOnly(res) {
 			// Resolve references before deleting the resource.
 			// Ignore any errors while resolving the references
 			resolved, _, _ := rm.ResolveReferences(ctx, r.apiReader, res)
@@ -284,6 +285,58 @@ func (r *resourceReconciler) Sync(
 	isAdopted := IsAdopted(desired)
 	rlog.WithValues("is_adopted", isAdopted)
 
+	isReadOnly := IsReadOnly(desired)
+	rlog.WithValues("is_read_only", isReadOnly)
+
+	// NOTE(a-hilaly): When the time comes to support adopting resources
+	// using annotations, we will need to think a little bit more about
+	// the case where a user, wants to adopt a resource as read-only.
+
+	// If the resource is read-only, we enter a different code path where we
+	// only read the resource and patch the metadata and spec.
+	if isReadOnly {
+		rlog.Enter("rm.ResolveReferences")
+		resolved, _, _ := rm.ResolveReferences(ctx, r.apiReader, desired)
+		// NOTE(a-hilaly): One might be wondering why are we ignoring the
+		// error here. The reason is that the current implementation of
+		// ResolveReference doesn't take into consideration the read-only
+		// resources (and their fields).
+		//
+		// In a nutshell, `ResolveReferences` implementation was designed to
+		// be used for the create/update operations, and not for the read-only
+		// resources. This means that the implementation of `ResolveReferences`
+		// returns an error when any of the required references (for create)
+		// are not resolved. This is not helpful for read-only resources.
+		//
+		// We need to refactor the `ResolveReferences` implementation to
+		// be able to resolve read-only required fields. For example, EKS
+		// NodeGroups, only require the `ClusterName` field to be resolved.
+		// This is not the case for create/update operations where we need to
+		// resolve all the references (e.g. `ClusterName`, `NodeRoleArn`, etc.).
+		// See https://github.com/aws-controllers-k8s/eks-controller/blob/main/pkg/resource/nodegroup/references.go#L72-L114
+		//
+		// What's the worst that can happen? If we don't resolve the references
+		// for read-only resources, ReadOne call will fail with an error, causing
+		// the resource to be requeued.
+		//
+		// TODO(a-hilaly): Implement resolve references for read-only resources,
+		// maybe part of the DynamicReferences work.
+		rlog.Exit("rm.ResolveReferences", err)
+
+		rlog.Enter("rm.ReadOne")
+		latest, err = rm.ReadOne(ctx, resolved)
+		rlog.Exit("rm.ReadOne", err)
+		if err != nil {
+			if err == ackerr.NotFound {
+				return nil, ackerr.ReadOnlyResourceNotFound
+			}
+			return latest, err
+		}
+
+		err = r.patchResourceStatus(ctx, desired, latest)
+		return latest, err
+	}
+
 	rlog.Enter("rm.ResolveReferences")
 	resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired)
 	rlog.Exit("rm.ResolveReferences", err)

pkg/runtime/reconciler_test.go

@@ -24,6 +24,7 @@ import (
 	"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"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	k8srtschema "k8s.io/apimachinery/pkg/runtime/schema"
@@ -198,6 +199,69 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) {
 	rm.AssertNumberOfCalls(t, "ReadOne", 6)
 }
 
+func TestReconcilerReadOnlyResource(t *testing.T) {
+	require := require.New(t)
+
+	ctx := context.TODO()
+	arn := ackv1alpha1.AWSResourceName("my-read-only-book-arn")
+
+	desired, _, metaObj := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
+	metaObj.SetAnnotations(map[string]string{
+		ackv1alpha1.AnnotationReadOnly: "true",
+	})
+
+	ids := &ackmocks.AWSResourceIdentifiers{}
+	ids.On("ARN").Return(&arn)
+
+	latest, latestRTObj, _ := resourceMocks()
+	latest.On("Identifiers").Return(ids)
+	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On("MetaObject").Return(metav1.ObjectMeta{
+		Annotations: map[string]string{
+			ackv1alpha1.AnnotationReadOnly: "true",
+		},
+	})
+	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
+
+	rm := &ackmocks.AWSResourceManager{}
+	rm.On("ResolveReferences", ctx, nil, desired).Return(
+		desired, false, nil,
+	).Times(2)
+	rm.On("ClearResolvedReferences", desired).Return(desired)
+	rm.On("ClearResolvedReferences", latest).Return(latest)
+	rm.On("ReadOne", ctx, desired).Return(
+		latest, nil,
+	).Once()
+	rm.On("Create", ctx, desired).Return(
+		latest, nil,
+	)
+	rm.On("IsSynced", ctx, latest).Return(true, nil)
+	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
+
+	rm.On("LateInitialize", ctx, latest).Return(latest, nil)
+	rd.On("IsManaged", desired).Return(true)
+	rd.On("Delta", desired, latest).Return(ackcompare.NewDelta())
+	rd.On("Delta", latest, latest).Return(ackcompare.NewDelta())
+
+	r, kc, scmd := reconcilerMocks(rmf)
+	rm.On("EnsureTags", ctx, desired, scmd).Return(nil)
+	statusWriter := &ctrlrtclientmock.SubResourceWriter{}
+	kc.On("Status").Return(statusWriter)
+	statusWriter.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
+	_, err := r.Sync(ctx, rm, desired)
+	require.Nil(err)
+	rm.AssertNumberOfCalls(t, "ReadOne", 1)
+	// Assert that the resource is not created or updated
+	rm.AssertNotCalled(t, "Create", 0)
+	rm.AssertNotCalled(t, "Update", 0)
+	rm.AssertNotCalled(t, "Delta", 0)
+}
+
 func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testing.T) {
 	require := require.New(t)
 

pkg/runtime/util.go

@@ -52,3 +52,19 @@ func IsSynced(res acktypes.AWSResource) bool {
 	}
 	return false
 }
+
+// IsReadOnly returns true if the supplied AWSResource has an annotation
+// indicating that it is in read-only mode.
+func IsReadOnly(res acktypes.AWSResource) bool {
+	mo := res.MetaObject()
+	if mo == nil {
+		// Should never happen... if it does, it's buggy code.
+		panic("IsReadOnly received resource with nil RuntimeObject")
+	}
+	for k, v := range mo.GetAnnotations() {
+		if k == ackv1alpha1.AnnotationReadOnly {
+			return strings.ToLower(v) == "true"
+		}
+	}
+	return false
+}