From ab78f6317fa1ccf34638acd22ba25729f90da5c2 Mon Sep 17 00:00:00 2001 From: lsviben Date: Wed, 31 May 2023 10:24:54 +0200 Subject: [PATCH] implement granular managementPolicies Signed-off-by: lsviben --- apis/common/v1/policies.go | 42 +- apis/common/v1/resource.go | 8 +- apis/common/v1/zz_generated.deepcopy.go | 24 ++ pkg/meta/meta.go | 2 + pkg/reconciler/managed/reconciler.go | 118 +++--- pkg/reconciler/managed/reconciler_test.go | 449 +++++++++++++++++++--- 6 files changed, 528 insertions(+), 115 deletions(-) diff --git a/apis/common/v1/policies.go b/apis/common/v1/policies.go index 9e5898daf..4b85c7a23 100644 --- a/apis/common/v1/policies.go +++ b/apis/common/v1/policies.go @@ -16,23 +16,39 @@ limitations under the License. package v1 -// A ManagementPolicy determines how should Crossplane controllers manage an -// external resource. -// +kubebuilder:validation:Enum=FullControl;ObserveOnly;OrphanOnDelete -type ManagementPolicy string +// ManagementPolicy determines how should Crossplane controllers manage an +// external resource through an array of ManagementActions. +type ManagementPolicy []ManagementAction + +// A ManagementAction represents an action that the Crossplane controllers +// can take on an external resource. +// +kubebuilder:validation:Enum=Observe;Create;Update;Delete;LateInitialize;* +type ManagementAction string const ( - // ManagementFullControl means the external resource is fully controlled - // by Crossplane controllers, including its deletion. - ManagementFullControl ManagementPolicy = "FullControl" + // ManagementActionObserve means that the managed resource status.atProvider + // will be updated with the external resource state. + ManagementActionObserve ManagementAction = "Observe" + + // ManagementActionCreate means that the external resource will be created + // when using the managed resource spec.initProvider and spec.forProvider. + ManagementActionCreate ManagementAction = "Create" + + // ManagementActionUpdate means that the external resource will be updated + // when using the managed resource spec.forProvider. + ManagementActionUpdate ManagementAction = "Update" + + // ManagementActionDelete means that the external resource will be deleted + // when the managed resource is deleted. + ManagementActionDelete ManagementAction = "Delete" - // ManagementObserveOnly means the external resource will only be observed - // by Crossplane controllers, but not modified or deleted. - ManagementObserveOnly ManagementPolicy = "ObserveOnly" + // ManagementActionLateInitialize means that unspecified fields of the managed + // resource spec.forProvider will be updated with the external resource state. + ManagementActionLateInitialize ManagementAction = "LateInitialize" - // ManagementOrphanOnDelete means the external resource will be orphaned - // when its managed resource is deleted. - ManagementOrphanOnDelete ManagementPolicy = "OrphanOnDelete" + // ManagementActionAll means that all of the above actions will be taken + // by the Crossplane controllers. + ManagementActionAll ManagementAction = "*" ) // A DeletionPolicy determines what should happen to the underlying external diff --git a/apis/common/v1/resource.go b/apis/common/v1/resource.go index bf5f3ce47..1841b845c 100644 --- a/apis/common/v1/resource.go +++ b/apis/common/v1/resource.go @@ -205,14 +205,16 @@ type ResourceSpec struct { // THIS IS AN ALPHA FIELD. Do not use it in production. It is not honored // unless the relevant Crossplane feature flag is enabled, and may be // changed or removed without notice. - // ManagementPolicy specifies the level of control Crossplane has over the - // managed external resource. + // ManagementPolicy specifies the array of actions Crossplane is allowed to + // take on the managed and external resources. // This field is planned to replace the DeletionPolicy field in a future // release. Currently, both could be set independently and non-default // values would be honored if the feature flag is enabled. // See the design doc for more information: https://github.com/crossplane/crossplane/blob/499895a25d1a1a0ba1604944ef98ac7a1a71f197/design/design-doc-observe-only-resources.md?plain=1#L223 + // This field is replacing the ManagementPolicy alpha field. + // Only the ManagementPolicies field will be honored. // +optional - // +kubebuilder:default=FullControl + // +kubebuilder:default={"*"} ManagementPolicy ManagementPolicy `json:"managementPolicy,omitempty"` // DeletionPolicy specifies what will happen to the underlying external diff --git a/apis/common/v1/zz_generated.deepcopy.go b/apis/common/v1/zz_generated.deepcopy.go index 9b044bf60..64f7b0a9f 100644 --- a/apis/common/v1/zz_generated.deepcopy.go +++ b/apis/common/v1/zz_generated.deepcopy.go @@ -219,6 +219,25 @@ func (in *LocalSecretReference) DeepCopy() *LocalSecretReference { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in ManagementPolicy) DeepCopyInto(out *ManagementPolicy) { + { + in := &in + *out = make(ManagementPolicy, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ManagementPolicy. +func (in ManagementPolicy) DeepCopy() ManagementPolicy { + if in == nil { + return nil + } + out := new(ManagementPolicy) + in.DeepCopyInto(out) + return *out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MergeOptions) DeepCopyInto(out *MergeOptions) { *out = *in @@ -386,6 +405,11 @@ func (in *ResourceSpec) DeepCopyInto(out *ResourceSpec) { *out = new(Reference) (*in).DeepCopyInto(*out) } + if in.ManagementPolicy != nil { + in, out := &in.ManagementPolicy, &out.ManagementPolicy + *out = make(ManagementPolicy, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResourceSpec. diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index 67b4a15d9..e893750e0 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -423,6 +423,8 @@ func AllowsPropagationTo(from metav1.Object) map[types.NamespacedName]bool { // IsPaused returns true if the object has the AnnotationKeyReconciliationPaused // annotation set to `true`. +// Deprecated: Use reconciler.IsPaused instead to check if the Crossplane managed resource is paused as it includes +// the check for management policies. func IsPaused(o metav1.Object) bool { return o.GetAnnotations()[AnnotationKeyReconciliationPaused] == "true" } diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 0972997c3..a1f7c15f3 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -676,13 +676,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco ) // Check the pause annotation and return if it has the value "true" - // after logging, publishing an event and updating the SYNC status condition - if meta.IsPaused(managed) { - log.Debug("Reconciliation is paused via the pause annotation", "annotation", meta.AnnotationKeyReconciliationPaused, "value", "true") - record.Event(managed, event.Normal(reasonReconciliationPaused, "Reconciliation is paused via the pause annotation")) + // or if the management policies are enabled and the ManagementPolicy is empty. + // Log, publish an event and update the SYNC status condition. + if IsPaused(managed, r.managementPoliciesEnabled) { + log.Debug("Reconciliation is paused through the management policy or the pause annotation") + record.Event(managed, event.Normal(reasonReconciliationPaused, "Reconciliation is paused")) managed.SetConditions(xpv1.ReconcilePaused()) - // if the pause annotation is removed, we will have a chance to reconcile again and resume - // and if status update fails, we will reconcile again to retry to update the status + // if the pause annotation is removed or the management policies changed, we will have a chance to reconcile + // again and resume and if status update fails, we will reconcile again to retry to update the status return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -693,7 +694,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // not realize that the controller is still trying to reconcile // (and modify or delete) the resource since they forgot to enable the // feature flag. - if !r.managementPoliciesEnabled && (managed.GetManagementPolicy() == xpv1.ManagementObserveOnly || managed.GetManagementPolicy() == xpv1.ManagementOrphanOnDelete) { + if !r.managementPoliciesEnabled && (managed.GetManagementPolicy() != nil && !ContainsOnlyManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionAll)) { log.Debug(errManagementPolicy, "policy", managed.GetManagementPolicy()) record.Event(managed, event.Warning(reasonManagementPolicyNotEnabled, errors.New(errManagementPolicy))) managed.SetConditions(xpv1.ReconcileError(errors.New(errManagementPolicy))) @@ -816,41 +817,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } - if r.managementPoliciesEnabled && managed.GetManagementPolicy() == xpv1.ManagementObserveOnly { - // In the observe-only mode, !observation.ResourceExists will be an error - // case, and we will explicitly return this information to the user. - if !observation.ResourceExists { - record.Event(managed, event.Warning(reasonCannotObserve, errors.New(errExternalResourceNotExist))) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) - } - - // It is a valid use case to Observe a resource to get its connection - // details, so we publish them here. - if _, err := r.managed.PublishConnection(ctx, managed, observation.ConnectionDetails); err != nil { - // If this is the first time we encounter this issue we'll be - // requeued implicitly when we update our status with the new error - // condition. If not, we requeue explicitly, which will trigger - // backoff. - log.Debug("Cannot publish connection details", "error", err) - record.Event(managed, event.Warning(reasonCannotPublish, err)) - managed.SetConditions(xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) - } - - // Since we're in the ObserveOnly mode, we don't want to update the spec - // of the managed resource for any reason including the late - // initialization of fields. So, we ignore `observation.ResourceLateInitialized` - // and do not make an `Update` call on the managed resource ensuring any - // spec change is ignored. - - // We are returning a ReconcileSuccess here because we have observed the - // resource successfully, and we don't need any further action in this - // reconcile. - log.Debug("Observed the resource successfully with management policy ObserveOnly", "requeue-after", time.Now().Add(r.pollInterval)) - managed.SetConditions(xpv1.ReconcileSuccess()) - return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + // In the observe-only mode, !observation.ResourceExists will be an error + // case, and we will explicitly return this information to the user. + if r.managementPoliciesEnabled && !observation.ResourceExists && ContainsOnlyManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionObserve) { + record.Event(managed, event.Warning(reasonCannotObserve, errors.New(errExternalResourceNotExist))) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + // If this resource has a non-zero creation grace period we want to wait // for that period to expire before we trust that the resource really // doesn't exist. This is because some external APIs are eventually @@ -865,9 +839,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco if meta.WasDeleted(managed) { log = log.WithValues("deletion-timestamp", managed.GetDeletionTimestamp()) - // We'll only reach this point if deletion policy is not orphan, so we - // are safe to call external deletion if external resource exists. - if observation.ResourceExists { + if observation.ResourceExists && !shouldOrphan(r.managementPoliciesEnabled, managed) { if err := external.Delete(externalCtx, managed); err != nil { // We'll hit this condition if we can't delete our external // resource, for example if our provider credentials don't have @@ -940,7 +912,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } - if !observation.ResourceExists { + if !observation.ResourceExists && (!r.managementPoliciesEnabled || ContainsManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionCreate)) { // We write this annotation for two reasons. Firstly, it helps // us to detect the case in which we fail to persist critical // information (like the external name) that may be set by the @@ -1030,7 +1002,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } - if observation.ResourceLateInitialized { + if observation.ResourceLateInitialized && (!r.managementPoliciesEnabled || ContainsManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionLateInitialize)) { // Note that this update may reset any pending updates to the status of // the managed resource from when it was observed above. This is because // the API server replies to the update with its unchanged view of the @@ -1062,6 +1034,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco log.Debug("External resource differs from desired state", "diff", observation.Diff) } + // skip the update if the management policy is set to ignore updates + if r.managementPoliciesEnabled && !ContainsManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionUpdate) { + log.Debug("Reconciliation succeeded", "requeue-after", time.Now().Add(r.pollInterval)) + managed.SetConditions(xpv1.ReconcileSuccess()) + return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + } + update, err := external.Update(externalCtx, managed) if err != nil { // We'll hit this condition if we can't update our external resource, @@ -1096,28 +1075,55 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } +// ContainsManagementAction returns true if the management policy contains the management action or ManagementActionAll. +func ContainsManagementAction(managementPolicy xpv1.ManagementPolicy, action xpv1.ManagementAction) bool { + for _, a := range managementPolicy { + if a == action || a == xpv1.ManagementActionAll { + return true + } + } + return false +} + +// ContainsOnlyManagementAction returns true if the management policy contains only the management action. +func ContainsOnlyManagementAction(managementPolicy xpv1.ManagementPolicy, action xpv1.ManagementAction) bool { + return len(managementPolicy) == 1 && managementPolicy[0] == action +} + // We need to be careful until we completely remove the deletionPolicy in favor // of managementPolicies which conflicts with the managementPolicy regarding -// orphaning of the external resource. This function implement the proposal in +// orphaning of the external resource. This function implements the proposal in // the Observe Only design doc under the "Deprecation of `deletionPolicy`" // section by triggering external resource deletion only when the deletionPolicy -// is set to "Delete" and the managementPolicy is set to "FullControl". +// is set to "Delete" and the managementPolicy includes ManagementPolicyDeleteAction. func shouldOrphan(managementPoliciesEnabled bool, managed resource.Managed) bool { if !managementPoliciesEnabled { return managed.GetDeletionPolicy() == xpv1.DeletionOrphan } - if managed.GetDeletionPolicy() == xpv1.DeletionDelete && managed.GetManagementPolicy() == xpv1.ManagementFullControl { - // This is the only case where we should delete the external resource, - // so do not orphan it. + + // delete external resource if both the deletionPolicy and the managementPolicy are set to delete + if managed.GetDeletionPolicy() == xpv1.DeletionDelete && ContainsManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionDelete) { + return false + } + // if the managementPolicy is not default, and it contains the deletion action, we should delete the external resource + if !ContainsOnlyManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionAll) && + ContainsManagementAction(managed.GetManagementPolicy(), xpv1.ManagementActionDelete) { return false } + // For all other cases, we should orphan the external resource. // Obvious cases: - // DeletionOrphan && ManagementOrphanOnDelete - // DeletionOrphan && ManagementObserveOnly + // DeletionOrphan && ManagementPolicy without Delete Action // Conflicting cases: - // DeletionOrphan && ManagementFullControl (obeys non-default configuration) - // DeletionDelete && ManagementObserveOnly (obeys non-default configuration) - // DeletionDelete && ManagementOrphanOnDelete (obeys non-default configuration) + // DeletionOrphan && Management Policy ["*"] (obeys non-default configuration) + // DeletionDelete && ManagementPolicy that does not include the Delete Action (obeys non-default configuration) return true } + +// IsPaused returns true if the object has the AnnotationKeyReconciliationPaused +// annotation set to `true` or if the managed policies alpha feature is enabled and +// the `spec.managementPolicy` array is empty. +func IsPaused(o resource.Managed, managementPoliciesEnabled bool) bool { + return o.GetAnnotations()[meta.AnnotationKeyReconciliationPaused] == "true" || + (managementPoliciesEnabled && len(o.GetManagementPolicy()) == 0) +} diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 2c3cc82bb..10551413e 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -1104,6 +1104,39 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{}}, }, + "ManagementPolicyReconciliationPausedSuccessful": { + reason: `If a managed resource has the pause annotation with value "true", there should be no further requeue requests.`, + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := obj.(*fake.Managed) + mg.SetManagementPolicy(xpv1.ManagementPolicy{}) + return nil + }), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := &fake.Managed{} + want.SetManagementPolicy(xpv1.ManagementPolicy{}) + want.SetConditions(xpv1.ReconcilePaused()) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := `If managed resource has the pause annotation with value "true", it should acquire "Synced" status condition with the status "False" and the reason "ReconcilePaused".` + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithManagementPolicies(), + WithInitializers(), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{}}, + }, "ReconciliationResumes": { reason: `If a managed resource has the pause annotation with some value other than "true" and the Synced=False/ReconcilePaused status condition, reconciliation should resume with requeueing.`, args: args{ @@ -1176,12 +1209,12 @@ func TestReconciler(t *testing.T) { Client: &test.MockClient{ MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { mg := obj.(*fake.Managed) - mg.SetManagementPolicy(xpv1.ManagementObserveOnly) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionCreate}) return nil }), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetManagementPolicy(xpv1.ManagementObserveOnly) + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionCreate}) want.SetConditions(xpv1.ReconcileError(errors.New(errManagementPolicy))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := `If managed resource has a non default management policy but feature not enabled, it should return a proper error.` @@ -1197,18 +1230,18 @@ func TestReconciler(t *testing.T) { want: want{result: reconcile.Result{}}, }, "ObserveOnlyResourceDoesNotExist": { - reason: "With ObserveOnly, observing a resource that does not exist should be reported as a conditioned status error.", + reason: "With only Observe management action, observing a resource that does not exist should be reported as a conditioned status error.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { mg := obj.(*fake.Managed) - mg.SetManagementPolicy(xpv1.ManagementObserveOnly) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve}) return nil }), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetManagementPolicy(xpv1.ManagementObserveOnly) + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve}) want.SetConditions(xpv1.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Resource does not exist should be reported as a conditioned status when ObserveOnly." @@ -1236,18 +1269,18 @@ func TestReconciler(t *testing.T) { want: want{result: reconcile.Result{Requeue: true}}, }, "ObserveOnlyPublishConnectionDetailsError": { - reason: "With ObserveOnly, errors publishing connection details after observation should trigger a requeue after a short wait.", + reason: "With Observe, errors publishing connection details after observation should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { mg := obj.(*fake.Managed) - mg.SetManagementPolicy(xpv1.ManagementObserveOnly) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve}) return nil }), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetManagementPolicy(xpv1.ManagementObserveOnly) + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve}) want.SetConditions(xpv1.ReconcileError(errBoom)) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Errors publishing connection details after observation should be reported as a conditioned status." @@ -1280,18 +1313,18 @@ func TestReconciler(t *testing.T) { want: want{result: reconcile.Result{Requeue: true}}, }, "ObserveOnlySuccessfulObserve": { - reason: "With ObserveOnly, a successful managed resource observe should trigger a requeue after a long wait.", + reason: "With Observe, a successful managed resource observe should trigger a requeue after a long wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { mg := obj.(*fake.Managed) - mg.SetManagementPolicy(xpv1.ManagementObserveOnly) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve}) return nil }), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetManagementPolicy(xpv1.ManagementObserveOnly) + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve}) want.SetConditions(xpv1.ReconcileSuccess()) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "With ObserveOnly, a successful managed resource observation should be reported as a conditioned status." @@ -1319,6 +1352,267 @@ func TestReconciler(t *testing.T) { return false, nil }, }), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{RequeueAfter: defaultpollInterval}}, + }, + "ManagementPolicyAllCreateSuccessful": { + reason: "Successful managed resource creation using management policy all should trigger a requeue after a short wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := obj.(*fake.Managed) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionAll}) + return nil + }), + MockUpdate: test.NewMockUpdateFn(nil), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := &fake.Managed{} + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionAll}) + meta.SetExternalCreatePending(want, time.Now()) + meta.SetExternalCreateSucceeded(want, time.Now()) + want.SetConditions(xpv1.ReconcileSuccess()) + want.SetConditions(xpv1.Creating()) + if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { + reason := "Successful managed resource creation should be reported as a conditioned status." + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnecter(&NopConnecter{}), + WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(ctx context.Context, o client.Object) error { return nil })), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{Requeue: true}}, + }, + "ManagementPolicyCreateCreateSuccessful": { + reason: "Successful managed resource creation using management policy Create should trigger a requeue after a short wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := obj.(*fake.Managed) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionCreate}) + return nil + }), + MockUpdate: test.NewMockUpdateFn(nil), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := &fake.Managed{} + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionCreate}) + meta.SetExternalCreatePending(want, time.Now()) + meta.SetExternalCreateSucceeded(want, time.Now()) + want.SetConditions(xpv1.ReconcileSuccess()) + want.SetConditions(xpv1.Creating()) + if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { + reason := "Successful managed resource creation should be reported as a conditioned status." + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnecter(&NopConnecter{}), + WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(ctx context.Context, o client.Object) error { return nil })), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{Requeue: true}}, + }, + "ManagementPolicySkipCreate": { + reason: "Successful reconciliation skipping create should trigger a requeue after a long wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := obj.(*fake.Managed) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionUpdate, xpv1.ManagementActionDelete}) + return nil + }), + MockUpdate: test.NewMockUpdateFn(nil), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := &fake.Managed{} + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionUpdate, xpv1.ManagementActionDelete}) + want.SetConditions(xpv1.ReconcileSuccess()) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := `Managed resource should acquire Synced=False/ReconcileSuccess status condition after a resume.` + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: false, ResourceUpToDate: true}, nil + }, + CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) { + return ExternalCreation{}, errBoom + }, + } + return c, nil + })), + WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(ctx context.Context, o client.Object) error { return errBoom })), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{RequeueAfter: defaultpollInterval}}, + }, + "ManagementPolicyAllUpdateSuccessful": { + reason: "A successful managed resource update using management policies should trigger a requeue after a long wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := obj.(*fake.Managed) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionAll}) + return nil + }), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := &fake.Managed{} + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionAll}) + want.SetConditions(xpv1.ReconcileSuccess()) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := "A successful managed resource update should be reported as a conditioned status." + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil + }, + UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) { + return ExternalUpdate{}, nil + }, + } + return c, nil + })), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{RequeueAfter: defaultpollInterval}}, + }, + "ManagementPolicyUpdateUpdateSuccessful": { + reason: "A successful managed resource update using management policies should trigger a requeue after a long wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := obj.(*fake.Managed) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionUpdate}) + return nil + }), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := &fake.Managed{} + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionUpdate}) + want.SetConditions(xpv1.ReconcileSuccess()) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := "A successful managed resource update should be reported as a conditioned status." + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil + }, + UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) { + return ExternalUpdate{}, nil + }, + } + return c, nil + })), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{RequeueAfter: defaultpollInterval}}, + }, + "ManagementPolicySkipLateInitialize": { + reason: "Should skip updating a managed resource to persist late initialized fields and should trigger a requeue after a long wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := obj.(*fake.Managed) + mg.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve, xpv1.ManagementActionUpdate, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) + return nil + }), + MockUpdate: test.NewMockUpdateFn(errBoom), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := &fake.Managed{} + want.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve, xpv1.ManagementActionUpdate, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) + want.SetConditions(xpv1.ReconcileSuccess()) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := "Errors updating a managed resource should be reported as a conditioned status." + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: true, ResourceUpToDate: true, ResourceLateInitialized: true}, nil + }, + } + return c, nil + })), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, }, want: want{result: reconcile.Result{RequeueAfter: defaultpollInterval}}, @@ -1378,8 +1672,8 @@ func TestShouldOrphan(t *testing.T) { }, want: want{orphan: false}, }, - "DeletionDeleteManagementFullControl": { - reason: "Should not orphan if management policies are enabled and deletion policy is set to Delete and management policy is set to FullControl.", + "DeletionDeleteManagementActionAll": { + reason: "Should not orphan if management policies are enabled and deletion policy is set to Delete and management policy is set to All.", args: args{ managementPoliciesEnabled: true, managed: &fake.Managed{ @@ -1387,14 +1681,14 @@ func TestShouldOrphan(t *testing.T) { Policy: xpv1.DeletionDelete, }, Manageable: fake.Manageable{ - Policy: xpv1.ManagementFullControl, + Policy: xpv1.ManagementPolicy{xpv1.ManagementActionAll}, }, }, }, want: want{orphan: false}, }, - "DeletionOrphanManagementOrphanOnDelete": { - reason: "Should orphan if management policies are enabled and deletion policy is set to Orphan and management policy is set to OrphanOnDelete.", + "DeletionOrphanManagementActionAll": { + reason: "Should orphan if management policies are enabled and deletion policy is set to Orphan and management policy is set to All.", args: args{ managementPoliciesEnabled: true, managed: &fake.Managed{ @@ -1402,29 +1696,29 @@ func TestShouldOrphan(t *testing.T) { Policy: xpv1.DeletionOrphan, }, Manageable: fake.Manageable{ - Policy: xpv1.ManagementOrphanOnDelete, + Policy: xpv1.ManagementPolicy{xpv1.ManagementActionAll}, }, }, }, want: want{orphan: true}, }, - "DeletionOrphanManagementObserveOnly": { - reason: "Should orphan if management policies are enabled and deletion policy is set to Orphan and management policy is set to ObserveOnly.", + "DeletionDeleteManagementActionDelete": { + reason: "Should not orphan if management policies are enabled and deletion policy is set to Delete and management policy has action Delete.", args: args{ managementPoliciesEnabled: true, managed: &fake.Managed{ Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionOrphan, + Policy: xpv1.DeletionDelete, }, Manageable: fake.Manageable{ - Policy: xpv1.ManagementObserveOnly, + Policy: xpv1.ManagementPolicy{xpv1.ManagementActionDelete}, }, }, }, - want: want{orphan: true}, + want: want{orphan: false}, }, - "DeletionOrphanManagementFullControl": { - reason: "Should orphan if management policies are enabled and deletion policy is set to Orphan and management policy is set to FullControl.", + "DeletionOrphanManagementActionDelete": { + reason: "Should not orphan if management policies are enabled and deletion policy is set to Orphan and management policy has action Delete.", args: args{ managementPoliciesEnabled: true, managed: &fake.Managed{ @@ -1432,14 +1726,14 @@ func TestShouldOrphan(t *testing.T) { Policy: xpv1.DeletionOrphan, }, Manageable: fake.Manageable{ - Policy: xpv1.ManagementFullControl, + Policy: xpv1.ManagementPolicy{xpv1.ManagementActionDelete}, }, }, }, - want: want{orphan: true}, + want: want{orphan: false}, }, - "DeletionDeleteManagementObserveOnly": { - reason: "Should orphan if management policies are enabled and deletion policy is set to Delete and management policy is set to ObserveOnly.", + "DeletionDeleteManagementActionNoDelete": { + reason: "Should orphan if management policies are enabled and deletion policy is set to Delete and management policy does not have action Delete.", args: args{ managementPoliciesEnabled: true, managed: &fake.Managed{ @@ -1447,32 +1741,101 @@ func TestShouldOrphan(t *testing.T) { Policy: xpv1.DeletionDelete, }, Manageable: fake.Manageable{ - Policy: xpv1.ManagementObserveOnly, + Policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize}, }, }, }, want: want{orphan: true}, }, - "DeletionDeleteManagementOrphanOnDelete": { - reason: "Should orphan if management policies are enabled and deletion policy is set to Delete and management policy is set to OrphanOnDelete.", + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + if diff := cmp.Diff(tc.want.orphan, shouldOrphan(tc.args.managementPoliciesEnabled, tc.args.managed)); diff != "" { + t.Errorf("\nReason: %s\nshouldOrphan(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestIsPaused(t *testing.T) { + + type args struct { + o resource.Managed + mgmtEnabled bool + } + cases := map[string]struct { + args args + want bool + }{ + "HasPauseAnnotationSetTrue": { + args: args{ + o: func() resource.Managed { + p := &fake.Managed{} + p.SetAnnotations(map[string]string{ + meta.AnnotationKeyReconciliationPaused: "true", + }) + return p + }(), + }, + want: true, + }, + "NoPauseAnnotation": { args: args{ - managementPoliciesEnabled: true, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionDelete, - }, - Manageable: fake.Manageable{ - Policy: xpv1.ManagementOrphanOnDelete, - }, - }, + o: &fake.Managed{}, }, - want: want{orphan: true}, + want: false, + }, + "HasEmptyPauseAnnotation": { + args: args{ + o: func() resource.Managed { + p := &fake.Managed{} + p.SetAnnotations(map[string]string{ + meta.AnnotationKeyReconciliationPaused: "", + }) + return p + }(), + }, + want: false, + }, + "HasEmptyManagementPolicyEnabled": { + args: args{ + o: func() resource.Managed { + p := &fake.Managed{} + p.SetManagementPolicy(xpv1.ManagementPolicy{}) + return p + }(), + mgmtEnabled: true, + }, + want: true, + }, + "HasEmptyManagementPolicyDisabled": { + args: args{ + o: func() resource.Managed { + p := &fake.Managed{} + p.SetManagementPolicy(xpv1.ManagementPolicy{}) + return p + }(), + mgmtEnabled: false, + }, + want: false, + }, + "HasNonEmptyManagementPolicyEnabled": { + args: args{ + o: func() resource.Managed { + p := &fake.Managed{} + p.SetManagementPolicy(xpv1.ManagementPolicy{xpv1.ManagementActionObserve}) + return p + }(), + mgmtEnabled: true, + }, + want: false, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - if diff := cmp.Diff(tc.want.orphan, shouldOrphan(tc.args.managementPoliciesEnabled, tc.args.managed)); diff != "" { - t.Errorf("\nReason: %s\nshouldOrphan(...): -want, +got:\n%s", tc.reason, diff) + got := IsPaused(tc.args.o, tc.args.mgmtEnabled) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("IsPaused(...): -want, +got:\n%s", diff) } }) }