From 3fd1d42c2bab2125e8fb5cc4cb34d76a91e1eafe Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Wed, 13 Nov 2024 16:02:00 -0800 Subject: [PATCH 1/3] Add addoption by annotation feature These changes introduce a new feature gate called `ForceAdoptResources` which allows users to provide the read required fields in the annotation and an empty spec, and the controller would populate the resource and adopt from AWS --- apis/core/v1alpha1/annotations.go | 9 ++++ mocks/pkg/types/aws_resource.go | 14 ++++++ pkg/featuregate/features.go | 11 +++-- pkg/runtime/reconciler.go | 77 +++++++++++++++++++++++++++++++ pkg/runtime/util.go | 48 +++++++++++++++++++ pkg/runtime/util_test.go | 53 +++++++++++++++++++++ pkg/types/aws_resource.go | 3 ++ 7 files changed, 212 insertions(+), 3 deletions(-) diff --git a/apis/core/v1alpha1/annotations.go b/apis/core/v1alpha1/annotations.go index 75869bb..82fddd7 100644 --- a/apis/core/v1alpha1/annotations.go +++ b/apis/core/v1alpha1/annotations.go @@ -81,4 +81,13 @@ 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" + // 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 + AnnotationAdoptionFields = AnnotationPrefix + "adoption-fields" ) diff --git a/mocks/pkg/types/aws_resource.go b/mocks/pkg/types/aws_resource.go index 29cd765..7d14bfc 100644 --- a/mocks/pkg/types/aws_resource.go +++ b/mocks/pkg/types/aws_resource.go @@ -131,6 +131,20 @@ func (_m *AWSResource) SetIdentifiers(_a0 *v1alpha1.AWSIdentifiers) error { return r0 } +// PopulateResourceFromAnnotation provides a mock function with given fields: _a0 +func (_m *AWSResource) PopulateResourceFromAnnotation(_a0 map[string]string) error { + ret := _m.Called(_a0) + + var r0 error + if rf, ok := ret.Get(0).(func(map[string]string) error); ok { + r0 = rf(_a0) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // SetObjectMeta provides a mock function with given fields: meta func (_m *AWSResource) SetObjectMeta(meta v1.ObjectMeta) { _m.Called(meta) diff --git a/pkg/featuregate/features.go b/pkg/featuregate/features.go index 79ce3fd..0e53ac9 100644 --- a/pkg/featuregate/features.go +++ b/pkg/featuregate/features.go @@ -19,6 +19,10 @@ package featuregate import "fmt" const ( + // ForcedAdoptResources is a feature gate for enabling forced adoption of resources + // by annotation + ForcedAdoptResources = "ForcedAdoptResources" + // ReadOnlyResources is a feature gate for enabling ReadOnly resources annotation. ReadOnlyResources = "ReadOnlyResources" @@ -32,9 +36,10 @@ const ( // defaultACKFeatureGates is a map of feature names to Feature structs // representing the default feature gates for ACK controllers. var defaultACKFeatureGates = FeatureGates{ - ReadOnlyResources: {Stage: Alpha, Enabled: false}, - TeamLevelCARM: {Stage: Alpha, Enabled: false}, - ServiceLevelCARM: {Stage: Alpha, Enabled: false}, + ForcedAdoptResources: {Stage: Alpha, Enabled: false}, + ReadOnlyResources: {Stage: Alpha, Enabled: false}, + TeamLevelCARM: {Stage: Alpha, Enabled: false}, + ServiceLevelCARM: {Stage: Alpha, Enabled: false}, } // FeatureStage represents the development stage of a feature. diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 6a4f43a..c70a9bd 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -360,6 +360,81 @@ func (r *resourceReconciler) Sync( isAdopted := IsAdopted(desired) rlog.WithValues("is_adopted", isAdopted) + if r.cfg.FeatureGates.IsEnabled(featuregate.ForcedAdoptResources) { + + // If the resource is being adopted by force, we need to access + // the required field passed by annotation and attempt a read. + if NeedAdoption(desired) { + rlog.WithValues("is_forced_adoption", "true") + rlog.Info("Adopting Resource") + extractedFields, err := ExtractAdoptionFields(desired) + if err != nil { + return desired, err + } + if extractedFields == nil { + // TODO(michaelhtm) figure out error here + return nil, fmt.Errorf("Failed extracting fields from annotation") + } + res := desired.DeepCopy() + err = res.PopulateResourceFromAnnotation(extractedFields) + if err != nil { + return nil, err + } + resolved := res + if hasRef, ok := extractedFields["hasReferences"]; ok && hasRef == "true" { + rlog.Enter("rm.ResolveReferences") + resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, res) + rlog.Exit("rm.ResolveReferences", err) + if err != nil { + return ackcondition.WithReferencesResolvedCondition(res, err), err + } + if hasReferences { + resolved = ackcondition.WithReferencesResolvedCondition(resolved, err) + } + } + + rlog.Enter("rm.EnsureTags") + err = rm.EnsureTags(ctx, resolved, r.sc.GetMetadata()) + rlog.Exit("rm.EnsureTags", err) + if err != nil { + return resolved, err + } + rlog.Enter("rm.ReadOne") + latest, err = rm.ReadOne(ctx, resolved) + if err != nil { + return nil, err + } + + if !r.rd.IsManaged(latest) { + if err = r.setResourceManaged(ctx, rm, latest); err != nil { + return nil, 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) + 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) + } + } + if r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnlyResources) { isReadOnly := IsReadOnly(desired) rlog.WithValues("is_read_only", isReadOnly) @@ -367,6 +442,8 @@ func (r *resourceReconciler) Sync( // 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. + // + // NOTE(michaelhtm): Done, tnx :) // If the resource is read-only, we enter a different code path where we // only read the resource and patch the metadata and spec. diff --git a/pkg/runtime/util.go b/pkg/runtime/util.go index b2079e0..a3eb4b4 100644 --- a/pkg/runtime/util.go +++ b/pkg/runtime/util.go @@ -14,6 +14,7 @@ package runtime import ( + "encoding/json" "strings" corev1 "k8s.io/api/core/v1" @@ -68,3 +69,50 @@ func IsReadOnly(res acktypes.AWSResource) bool { } return false } + +// IsForcedAdoption returns true if the supplied AWSResource has an annotation +// indicating that it should be adopted +func isForcedAdoption(res acktypes.AWSResource) bool { + mo := res.MetaObject() + if mo == nil { + // Should never happen... if it does, it's buggy code. + panic("IsForcedAdoption received resource with nil RuntimeObject") + } + for k, v := range mo.GetAnnotations() { + if k == ackv1alpha1.AnnotationForceAdoption { + return strings.ToLower(v) == "true" + } + } + return false +} + +func NeedAdoption(res acktypes.AWSResource) bool { + return isForcedAdoption(res) && !IsAdopted(res) +} + +func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) { + fields := extractFieldsFromAnnotation(res) + + extractedFields := &map[string]string{} + err := json.Unmarshal([]byte(fields), extractedFields) + if err != nil { + return nil, err + } + + return *extractedFields, nil +} + +func extractFieldsFromAnnotation(res acktypes.AWSResource) string { + mo := res.MetaObject() + if mo == nil { + // Should never happen... if it does, it's buggy code. + panic("ExtractRequiredFields received resource with nil RuntimeObject") + } + + for k, v := range mo.GetAnnotations() { + if k == ackv1alpha1.AnnotationAdoptionFields { + return v + } + } + return "" +} diff --git a/pkg/runtime/util_test.go b/pkg/runtime/util_test.go index e9da364..1fbe66f 100644 --- a/pkg/runtime/util_test.go +++ b/pkg/runtime/util_test.go @@ -67,3 +67,56 @@ func TestIsSynced(t *testing.T) { }) require.False(ackrt.IsSynced(res)) } + +func TestIsForcedAdoption(t *testing.T) { + require := require.New(t) + + res := &mocks.AWSResource{} + res.On("MetaObject").Return(&metav1.ObjectMeta{ + Annotations: map[string]string{ + ackv1alpha1.AnnotationForceAdoption: "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", + }, + }) + require.False(ackrt.NeedAdoption(res)) + + res = &mocks.AWSResource{} + res.On("MetaObject").Return(&metav1.ObjectMeta{ + Annotations: map[string]string{ + ackv1alpha1.AnnotationForceAdoption: "false", + ackv1alpha1.AnnotationAdopted: "true", + }, + }) + require.False(ackrt.NeedAdoption(res)) +} + +func TestExtractAdoptionFields(t *testing.T) { + require := require.New(t) + + res := &mocks.AWSResource{} + res.On("MetaObject").Return(&metav1.ObjectMeta{ + Annotations: map[string]string{ + ackv1alpha1.AnnotationAdoptionFields: `{ + "clusterName": "my-cluster", + "name": "ng-1234" + }`, + }, + }) + + expected := map[string]string{ + "clusterName": "my-cluster", + "name": "ng-1234", + } + actual, err := ackrt.ExtractAdoptionFields(res) + require.NoError(err) + require.Equal(expected, actual) +} diff --git a/pkg/types/aws_resource.go b/pkg/types/aws_resource.go index 805c018..dd2673e 100644 --- a/pkg/types/aws_resource.go +++ b/pkg/types/aws_resource.go @@ -48,4 +48,7 @@ type AWSResource interface { SetStatus(AWSResource) // DeepCopy will return a copy of the resource DeepCopy() AWSResource + // PopulateResourceFromAnnotation will set the Spec or Status field that user + // provided from annotations + PopulateResourceFromAnnotation(fields map[string]string) error } From ec0734c20f98ab1f1c99896874c709400674790e Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Tue, 26 Nov 2024 10:45:32 -0800 Subject: [PATCH 2/3] Address PR comments and extract adoption logic to a seperate function --- pkg/featuregate/features.go | 12 +-- pkg/runtime/reconciler.go | 151 +++++++++++++++++++----------------- pkg/runtime/util.go | 18 +++-- 3 files changed, 97 insertions(+), 84 deletions(-) diff --git a/pkg/featuregate/features.go b/pkg/featuregate/features.go index 0e53ac9..4641881 100644 --- a/pkg/featuregate/features.go +++ b/pkg/featuregate/features.go @@ -19,9 +19,9 @@ package featuregate import "fmt" const ( - // ForcedAdoptResources is a feature gate for enabling forced adoption of resources + // AdoptResources is a feature gate for enabling forced adoption of resources // by annotation - ForcedAdoptResources = "ForcedAdoptResources" + AdoptResources = "AdoptResources" // ReadOnlyResources is a feature gate for enabling ReadOnly resources annotation. ReadOnlyResources = "ReadOnlyResources" @@ -36,10 +36,10 @@ const ( // defaultACKFeatureGates is a map of feature names to Feature structs // representing the default feature gates for ACK controllers. var defaultACKFeatureGates = FeatureGates{ - ForcedAdoptResources: {Stage: Alpha, Enabled: false}, - ReadOnlyResources: {Stage: Alpha, Enabled: false}, - TeamLevelCARM: {Stage: Alpha, Enabled: false}, - ServiceLevelCARM: {Stage: Alpha, Enabled: false}, + AdoptResources: {Stage: Alpha, Enabled: false}, + ReadOnlyResources: {Stage: Alpha, Enabled: false}, + TeamLevelCARM: {Stage: Alpha, Enabled: false}, + ServiceLevelCARM: {Stage: Alpha, Enabled: false}, } // FeatureStage represents the development stage of a feature. diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index c70a9bd..46a9367 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -298,6 +298,81 @@ func (r *resourceReconciler) handleCacheError( return r.HandleReconcileError(ctx, desired, latest, requeue.NeededAfter(err, roleARNNotAvailableRequeueDelay)) } +func (r *resourceReconciler) handleAdoption( + ctx context.Context, + rm acktypes.AWSResourceManager, + desired acktypes.AWSResource, + rlog acktypes.Logger, +) (acktypes.AWSResource, error) { + // 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 + return nil, fmt.Errorf("failed extracting fields from annotation") + } + res := desired.DeepCopy() + err = res.PopulateResourceFromAnnotation(extractedFields) + if err != nil { + return nil, err + } + resolved := res + + rlog.Enter("rm.EnsureTags") + err = rm.EnsureTags(ctx, resolved, r.sc.GetMetadata()) + rlog.Exit("rm.EnsureTags", err) + if err != nil { + return resolved, err + } + rlog.Enter("rm.ReadOne") + latest, err := rm.ReadOne(ctx, resolved) + if err != nil { + return nil, err + } + + if !r.rd.IsManaged(latest) { + if err = r.setResourceManaged(ctx, rm, latest); err != nil { + return nil, 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) +} + // reconcile either cleans up a deleted resource or ensures that the supplied // AWSResource's backing API resource matches the supplied desired state. // @@ -360,78 +435,10 @@ func (r *resourceReconciler) Sync( isAdopted := IsAdopted(desired) rlog.WithValues("is_adopted", isAdopted) - if r.cfg.FeatureGates.IsEnabled(featuregate.ForcedAdoptResources) { - - // If the resource is being adopted by force, we need to access - // the required field passed by annotation and attempt a read. - if NeedAdoption(desired) { - rlog.WithValues("is_forced_adoption", "true") - rlog.Info("Adopting Resource") - extractedFields, err := ExtractAdoptionFields(desired) - if err != nil { - return desired, err - } - if extractedFields == nil { - // TODO(michaelhtm) figure out error here - return nil, fmt.Errorf("Failed extracting fields from annotation") - } - res := desired.DeepCopy() - err = res.PopulateResourceFromAnnotation(extractedFields) - if err != nil { - return nil, err - } - resolved := res - if hasRef, ok := extractedFields["hasReferences"]; ok && hasRef == "true" { - rlog.Enter("rm.ResolveReferences") - resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, res) - rlog.Exit("rm.ResolveReferences", err) - if err != nil { - return ackcondition.WithReferencesResolvedCondition(res, err), err - } - if hasReferences { - resolved = ackcondition.WithReferencesResolvedCondition(resolved, err) - } - } - - rlog.Enter("rm.EnsureTags") - err = rm.EnsureTags(ctx, resolved, r.sc.GetMetadata()) - rlog.Exit("rm.EnsureTags", err) - if err != nil { - return resolved, err - } - rlog.Enter("rm.ReadOne") - latest, err = rm.ReadOne(ctx, resolved) - if err != nil { - return nil, err - } - - if !r.rd.IsManaged(latest) { - if err = r.setResourceManaged(ctx, rm, latest); err != nil { - return nil, 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) - 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) + if r.cfg.FeatureGates.IsEnabled(featuregate.AdoptResources) { + latest, err := r.handleAdoption(ctx, rm, desired, rlog) + if err != nil { + return latest, err } } diff --git a/pkg/runtime/util.go b/pkg/runtime/util.go index a3eb4b4..8275cc4 100644 --- a/pkg/runtime/util.go +++ b/pkg/runtime/util.go @@ -70,13 +70,13 @@ func IsReadOnly(res acktypes.AWSResource) bool { return false } -// IsForcedAdoption returns true if the supplied AWSResource has an annotation +// hasAdoptAnnotation returns true if the supplied AWSResource has an annotation // indicating that it should be adopted -func isForcedAdoption(res acktypes.AWSResource) bool { +func hasAdoptAnnotation(res acktypes.AWSResource) bool { mo := res.MetaObject() if mo == nil { // Should never happen... if it does, it's buggy code. - panic("IsForcedAdoption received resource with nil RuntimeObject") + panic("hasAdoptAnnotation received resource with nil RuntimeObject") } for k, v := range mo.GetAnnotations() { if k == ackv1alpha1.AnnotationForceAdoption { @@ -86,12 +86,18 @@ func isForcedAdoption(res acktypes.AWSResource) bool { return false } +func createOrAdopt(res acktypes.AWSResource) bool { + return hasAdoptAnnotation(res) || IsAdopted(res) +} + +// NeedAdoption returns true when the resource has +// adopt annotation but is not yet adopted func NeedAdoption(res acktypes.AWSResource) bool { - return isForcedAdoption(res) && !IsAdopted(res) + return hasAdoptAnnotation(res) && !IsAdopted(res) } func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) { - fields := extractFieldsFromAnnotation(res) + fields := getAdoptionFields(res) extractedFields := &map[string]string{} err := json.Unmarshal([]byte(fields), extractedFields) @@ -102,7 +108,7 @@ func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) return *extractedFields, nil } -func extractFieldsFromAnnotation(res acktypes.AWSResource) string { +func getAdoptionFields(res acktypes.AWSResource) string { mo := res.MetaObject() if mo == nil { // Should never happen... if it does, it's buggy code. From 4d783809bf6b7f0dd5af29cc37257ba6b3f3bd70 Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Fri, 29 Nov 2024 10:07:21 -0800 Subject: [PATCH 3/3] 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 --- apis/core/v1alpha1/annotations.go | 11 ++--- pkg/featuregate/features.go | 6 +-- pkg/runtime/reconciler.go | 73 ++++++++++++++++--------------- pkg/runtime/util.go | 25 +++++------ pkg/runtime/util_test.go | 16 +++---- 5 files changed, 66 insertions(+), 65 deletions(-) diff --git a/apis/core/v1alpha1/annotations.go b/apis/core/v1alpha1/annotations.go index 82fddd7..27c7673 100644 --- a/apis/core/v1alpha1/annotations.go +++ b/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 diff --git a/pkg/featuregate/features.go b/pkg/featuregate/features.go index 4641881..358a4f2 100644 --- a/pkg/featuregate/features.go +++ b/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}, diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 46a9367..e859b9f 100644 --- a/pkg/runtime/reconciler.go +++ b/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,24 +338,22 @@ 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") @@ -365,12 +361,9 @@ func (r *resourceReconciler) handleAdoption( 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 } } diff --git a/pkg/runtime/util.go b/pkg/runtime/util.go index 8275cc4..21c0481 100644 --- a/pkg/runtime/util.go +++ b/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) { diff --git a/pkg/runtime/util_test.go b/pkg/runtime/util_test.go index 1fbe66f..cf871e2 100644 --- a/pkg/runtime/util_test.go +++ b/pkg/runtime/util_test.go @@ -74,8 +74,8 @@ 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)) @@ -83,8 +83,8 @@ func TestIsForcedAdoption(t *testing.T) { 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)) @@ -92,8 +92,8 @@ func TestIsForcedAdoption(t *testing.T) { 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)