Change force-adoption annotation to adoption-policy

This change will allow us to support an `adopt-or-create` policy
in the future, where the controller will create the resource when
the resource does not exist and can't be adopted.

Before we support `adopt-or-create` we need a solution in code-gen
where we change how we handle the `PopulateResourceFromAnnotation`
which currently only allows the population of fields that are
required for a readOne operation, and they happen to be all scalar
fields (besides ARN, but we have a way of handling that), but the
required fields for create would need to be sometimes structs,
and this would require users to provide values in form of maps
eg.
Creating an EKS cluster requires a ResourceVPCConfig, which is a
struct that contains subnetIDs etc.
We can have a couple of ways to address this.
1. Accept these values in the spec, and return terminal error when
   we attempt a create and the create required fields are not provided
2. Accept these values in the `adoption-fields` annotation. This would
   need a code-gen change to allow reading from structs and assigning
   fields. but it would also make the annotation easy to make mistakes
   with when using yaml

Author: michaelhtm

apis/core/v1alpha1/annotations.go

@@ -81,11 +81,12 @@ const (
 	// the resource is read-only and should not be created/patched/deleted by the
 	// ACK service controller.
 	AnnotationReadOnly = AnnotationPrefix + "read-only"
-	// AnnotationForceAdoption is an annotation whose value is the identifier for whether
-	// we will force adoption or not. If this annotation is set to true on a CR, that
-	// means the user is indicating to the ACK service controller that it should
-	// force adoption of the resource.
-	AnnotationForceAdoption = AnnotationPrefix + "force-adoption"
+	// AnnotationAdoptionPolicy is an annotation whose value is the identifier for whether
+	// we will attempt adoption only (value = adopt-only) or attempt a create if resource 
+	// is not found (value adopt-or-create).
+	//
+	// NOTE (michaelhtm): Currently create-or-adopt is not supported
+	AnnotationAdoptionPolicy = AnnotationPrefix + "adoption-policy"
 	// AnnotationAdoptionFields is an annotation whose value contains a json-like
 	// format of the requied fields to do a ReadOne when attempting to force-adopt
 	// a Resource

pkg/featuregate/features.go

@@ -19,9 +19,9 @@ package featuregate
 import "fmt"
 
 const (
-	// AdoptResources is a feature gate for enabling forced adoption of resources
+	// ResourceAdoption is a feature gate for enabling forced adoption of resources
 	// by annotation
-	AdoptResources = "AdoptResources"
+	ResourceAdoption = "ResourceAdoption"
 
 	// ReadOnlyResources is a feature gate for enabling ReadOnly resources annotation.
 	ReadOnlyResources = "ReadOnlyResources"
@@ -36,7 +36,7 @@ const (
 // defaultACKFeatureGates is a map of feature names to Feature structs
 // representing the default feature gates for ACK controllers.
 var defaultACKFeatureGates = FeatureGates{
-	AdoptResources:    {Stage: Alpha, Enabled: false},
+	ResourceAdoption:  {Stage: Alpha, Enabled: false},
 	ReadOnlyResources: {Stage: Alpha, Enabled: false},
 	TeamLevelCARM:     {Stage: Alpha, Enabled: false},
 	ServiceLevelCARM:  {Stage: Alpha, Enabled: false},

pkg/runtime/reconciler.go

@@ -57,6 +57,10 @@ const (
 	// resource if the CARM cache is not synced yet, or if the roleARN is not
 	// available.
 	roleARNNotAvailableRequeueDelay = 15 * time.Second
+	// adoptOrCreate is an annotation field that decides whether to create the
+	// resource if it doesn't exist, or adopt the resource if it exists.
+	// value comes from getAdoptionPolicy
+	// adoptOrCreate = "adopt-or-create"
 )
 
 // reconciler describes a generic reconciler within ACK.
@@ -307,29 +311,23 @@ func (r *resourceReconciler) handleAdoption(
 	// If the resource is being adopted by force, we need to access
 	// the required field passed by annotation and attempt a read.
 
-	// continueReconcile will decide whether or not to continue the
-	// reconciliation. The cases where this would happen is when the
-	// resource is already adopted, or if we want to create the resource
-	// if user wants to create it when not found 
-	if !NeedAdoption(desired) {
-		return desired, nil
-	}
-
 	rlog.Info("Adopting Resource")
 	extractedFields, err := ExtractAdoptionFields(desired)
 	if err != nil {
 		return desired, ackerr.NewTerminalError(err)
 	}
 	if len(extractedFields) == 0 {
-		// TODO(michaelhtm) figure out error here
+		// TODO(michaelhtm) Here we need to figure out if we want to have an
+		// error or not. should we consider accepting values from Spec?
+		// And then we can let the ReadOne figure out if we have missing
+		// required fields for a Read
 		return nil, fmt.Errorf("failed extracting fields from annotation")
 	}
-	res := desired.DeepCopy()
-	err = res.PopulateResourceFromAnnotation(extractedFields)
+	resolved := desired.DeepCopy()
+	err = resolved.PopulateResourceFromAnnotation(extractedFields)
 	if err != nil {
 		return nil, err
 	}
-	resolved := res
 
 	rlog.Enter("rm.EnsureTags")
 	err = rm.EnsureTags(ctx, resolved, r.sc.GetMetadata())
@@ -340,37 +338,32 @@ func (r *resourceReconciler) handleAdoption(
 	rlog.Enter("rm.ReadOne")
 	latest, err := rm.ReadOne(ctx, resolved)
 	if err != nil {
-		return nil, err
+		return latest, err
 	}
 
-	if !r.rd.IsManaged(latest) {
-		if err = r.setResourceManaged(ctx, rm, latest); err != nil {
-			return nil, err
-		}
+	if err = r.setResourceManaged(ctx, rm, latest); err != nil {
+		return latest, err
+	}
 
-		// Ensure tags again after adding the finalizer and patching the
-		// resource. Patching desired resource omits the controller tags
-		// because they are not persisted in etcd. So we again ensure
-		// that tags are present before performing the create operation.
-		rlog.Enter("rm.EnsureTags")
-		err = rm.EnsureTags(ctx, latest, r.sc.GetMetadata())
-		rlog.Exit("rm.EnsureTags", err)
-		if err != nil {
-			return latest, err
-		}
+	// Ensure tags again after adding the finalizer and patching the
+	// resource. Patching desired resource omits the controller tags
+	// because they are not persisted in etcd. So we again ensure
+	// that tags are present before performing the create operation.
+	rlog.Enter("rm.EnsureTags")
+	err = rm.EnsureTags(ctx, latest, r.sc.GetMetadata())
+	rlog.Exit("rm.EnsureTags", err)
+	if err != nil {
+		return latest, err
 	}
 	r.rd.MarkAdopted(latest)
 	rlog.WithValues("is_adopted", "true")
 	latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest)
 	if err != nil {
 		return latest, err
 	}
-	err = r.patchResourceStatus(ctx, desired, latest)
-	if err != nil {
-		return latest, err
-	}
+
 	rlog.Info("Resource Adopted")
-	return latest, requeue.NeededAfter(nil, 5)
+	return latest, nil
 }
 
 // reconcile either cleans up a deleted resource or ensures that the supplied
@@ -435,10 +428,18 @@ func (r *resourceReconciler) Sync(
 	isAdopted := IsAdopted(desired)
 	rlog.WithValues("is_adopted", isAdopted)
 
-	if r.cfg.FeatureGates.IsEnabled(featuregate.AdoptResources) {
-		latest, err := r.handleAdoption(ctx, rm, desired, rlog)
-		if err != nil {
-			return latest, err
+	if r.cfg.FeatureGates.IsEnabled(featuregate.ResourceAdoption) {
+		if NeedAdoption(desired) && !r.rd.IsManaged(desired) {
+			latest, err := r.handleAdoption(ctx, rm, desired, rlog)
+
+			if err != nil {
+				// If we get an error, we want to return here
+				// TODO(michaelhtm): Change the handling of
+				// the error to allow Adopt or Create here
+				// when supported
+				return latest, err
+			}
+			return latest, nil
 		}
 	}
 

pkg/runtime/util.go

@@ -70,30 +70,29 @@ func IsReadOnly(res acktypes.AWSResource) bool {
 	return false
 }
 
-// hasAdoptAnnotation returns true if the supplied AWSResource has an annotation
-// indicating that it should be adopted
-func hasAdoptAnnotation(res acktypes.AWSResource) bool {
+// GetAdoptionPolicy returns the Adoption Policy of the resource
+// defined by the user in annotation. Possible values are: 
+// adopt-only | adopt-or-create
+// adopt-only keeps requing until the resource is found
+// adopt-or-create creates the resource if does not exist
+func GetAdoptionPolicy(res acktypes.AWSResource) string {
 	mo := res.MetaObject()
 	if mo == nil {
-		// Should never happen... if it does, it's buggy code.
-		panic("hasAdoptAnnotation received resource with nil RuntimeObject")
+		panic("getAdoptionPolicy received resource with nil RuntimeObject")
 	}
 	for k, v := range mo.GetAnnotations() {
-		if k == ackv1alpha1.AnnotationForceAdoption {
-			return strings.ToLower(v) == "true"
+		if k == ackv1alpha1.AnnotationAdoptionPolicy {
+			return v
 		}
 	}
-	return false
-}
 
-func createOrAdopt(res acktypes.AWSResource) bool {
-	return hasAdoptAnnotation(res) || IsAdopted(res)
+	return ""
 }
 
-// NeedAdoption returns true when the resource has 
+// NeedAdoption returns true when the resource has
 // adopt annotation but is not yet adopted
 func NeedAdoption(res acktypes.AWSResource) bool {
-	return hasAdoptAnnotation(res) && !IsAdopted(res)
+	return GetAdoptionPolicy(res) != "" && !IsAdopted(res) 
 }
 
 func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) {

pkg/runtime/util_test.go

@@ -74,26 +74,26 @@ func TestIsForcedAdoption(t *testing.T) {
 	res := &mocks.AWSResource{}
 	res.On("MetaObject").Return(&metav1.ObjectMeta{
 		Annotations: map[string]string{
-			ackv1alpha1.AnnotationForceAdoption: "true",
-			ackv1alpha1.AnnotationAdopted: "false",
+			ackv1alpha1.AnnotationAdoptionPolicy: "true",
+			ackv1alpha1.AnnotationAdopted:        "false",
 		},
 	})
 	require.True(ackrt.NeedAdoption(res))
 
 	res = &mocks.AWSResource{}
 	res.On("MetaObject").Return(&metav1.ObjectMeta{
 		Annotations: map[string]string{
-			ackv1alpha1.AnnotationForceAdoption: "true",
-			ackv1alpha1.AnnotationAdopted: "true",
+			ackv1alpha1.AnnotationAdoptionPolicy: "true",
+			ackv1alpha1.AnnotationAdopted:        "true",
 		},
 	})
 	require.False(ackrt.NeedAdoption(res))
 
 	res = &mocks.AWSResource{}
 	res.On("MetaObject").Return(&metav1.ObjectMeta{
 		Annotations: map[string]string{
-			ackv1alpha1.AnnotationForceAdoption: "false",
-			ackv1alpha1.AnnotationAdopted: "true",
+			ackv1alpha1.AnnotationAdoptionPolicy: "false",
+			ackv1alpha1.AnnotationAdopted:        "true",
 		},
 	})
 	require.False(ackrt.NeedAdoption(res))
@@ -111,10 +111,10 @@ func TestExtractAdoptionFields(t *testing.T) {
 			}`,
 		},
 	})
-	
+
 	expected := map[string]string{
 		"clusterName": "my-cluster",
-		"name": "ng-1234",
+		"name":        "ng-1234",
 	}
 	actual, err := ackrt.ExtractAdoptionFields(res)
 	require.NoError(err)