diff --git a/apis/common/condition.go b/apis/common/condition.go index 03f67110c..3619bf0c2 100644 --- a/apis/common/condition.go +++ b/apis/common/condition.go @@ -60,9 +60,10 @@ const ( // Reasons a resource is or is not synced. const ( - ReasonReconcileSuccess ConditionReason = "ReconcileSuccess" - ReasonReconcileError ConditionReason = "ReconcileError" - ReasonReconcilePaused ConditionReason = "ReconcilePaused" + ReasonReconcileSuccess ConditionReason = "ReconcileSuccess" + ReasonReconcileError ConditionReason = "ReconcileError" + ReasonReconcilePaused ConditionReason = "ReconcilePaused" + ReasonReconcileForbidden ConditionReason = "ReconcileForbidden" ) // See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties @@ -312,3 +313,15 @@ func ReconcilePaused() Condition { Reason: ReasonReconcilePaused, } } + +// ReconcileForbidden returns a condition that indicates reconciliation on +// the managed resource is forbidden because managementPolicy 'Update' is missing +// and a diff between spec and the external resource exists. +func ReconcileForbidden() Condition { + return Condition{ + Type: TypeSynced, + Status: corev1.ConditionFalse, + LastTransitionTime: metav1.Now(), + Reason: ReasonReconcileForbidden, + } +} diff --git a/apis/common/v1/condition.go b/apis/common/v1/condition.go index c84c18f76..6a1bf9a38 100644 --- a/apis/common/v1/condition.go +++ b/apis/common/v1/condition.go @@ -134,3 +134,10 @@ func ReconcileError(err error) Condition { func ReconcilePaused() Condition { return common.ReconcilePaused() } + +// ReconcileForbidden returns a condition that indicates reconciliation on +// the managed resource is forbidden because managementPolicy 'Update' is missing +// and a diff between spec and the external resource exists. +func ReconcileForbidden() Condition { + return common.ReconcileForbidden() +} diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index d4bbf6877..8ed328a33 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -18,6 +18,7 @@ package managed import ( "context" + "fmt" "math/rand" "strings" "time" @@ -56,6 +57,7 @@ const ( const ( errFmtManagementPolicyNonDefault = "`spec.managementPolicies` is set to a non-default value but the feature is not enabled: %s" errFmtManagementPolicyNotSupported = "`spec.managementPolicies` is set to a value(%s) which is not supported. Check docs for supported policies" + errExternalResourceDiffNoUpdate = "External resource differs from desired state, but will not update due to missing 'Update' managementPolicy." errGetManaged = "cannot get managed resource" errUpdateManagedAnnotations = "cannot update managed resource annotations" @@ -1470,7 +1472,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu if !policy.ShouldUpdate() { reconcileAfter := r.pollIntervalHook(managed, r.pollInterval) log.Debug("Skipping update due to managementPolicies. Reconciliation succeeded", "requeue-after", time.Now().Add(reconcileAfter)) - status.MarkConditions(xpv1.ReconcileSuccess()) + // since there is a diff between spec and upstream, we want to inform the user + // about the diff, but that we will not update it - indicating that we WOULD perform + // changes if we could. This is helpful in migration scenarios to crossplane where + // the diff between actual and desired must be analyzed for potential impacts first, + // before giving Crossplane control over the resource. + msg := errExternalResourceDiffNoUpdate + if observation.Diff != "" { + msg = fmt.Sprintf("%s Diff:\n%s", errExternalResourceDiffNoUpdate, observation.Diff) + } + status.MarkConditions(xpv1.ReconcileForbidden().WithMessage(msg)) return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } diff --git a/pkg/reconciler/managed/reconciler_legacy_test.go b/pkg/reconciler/managed/reconciler_legacy_test.go index 0009a9fa6..b3246d80e 100644 --- a/pkg/reconciler/managed/reconciler_legacy_test.go +++ b/pkg/reconciler/managed/reconciler_legacy_test.go @@ -1765,7 +1765,7 @@ func TestReconciler(t *testing.T) { want: want{result: reconcile.Result{Requeue: true}}, }, "ObserveOnlySuccessfulObserve": { - reason: "With Observe, a successful managed resource observe should trigger a requeue after a long wait.", + reason: "With Observe, a successfully managed resource observe should trigger a requeue after a long wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ @@ -1778,10 +1778,10 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve}) - want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42)) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := "With ObserveOnly, a successful managed resource observation should be reported as a conditioned status." + reason := "With ObserveOnly, a successfully managed resource observation should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } @@ -1797,7 +1797,59 @@ func TestReconciler(t *testing.T) { WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) { c := &ExternalClientFns{ ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{ResourceExists: true}, nil + return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil + }, + DisconnectFn: func(_ context.Context) error { + return nil + }, + } + + return c, nil + })), + withConnectionPublishers(ConnectionPublisherFns{ + PublishConnectionFn: func(_ context.Context, _ resource.ConnectionSecretOwner, _ ConnectionDetails) (bool, error) { + return false, nil + }, + }), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}}, + }, + "ObserveOnlyWithDiffSyncError": { + reason: "With Observe, if the external resource is not up to date with desired, it should show a Synced=False + ReconcileForbidden error condition, 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 := asLegacyManaged(obj, 42) + mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve}) + + return nil + }), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := newLegacyManaged(42) + want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve}) + want.SetConditions(xpv1.ReconcileForbidden().WithObservedGeneration(42).WithMessage("External resource differs from desired state, but will not update due to missing 'Update' managementPolicy. Diff:\nmock diff")) + + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := "With ObserveOnly, a managed resource observation with a diff to upstream should be reported as a ReconcileForbidden + Synced=False condition" + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.LegacyManaged{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.LegacyManaged{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "mock diff"}, nil }, DisconnectFn: func(_ context.Context) error { return nil @@ -2026,7 +2078,7 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: false}}, }, - "ManagementPolicyImmutable": { + "ManagementPolicyImmutableNoDiff": { reason: "Successful reconciliation skipping update should trigger a requeue after a long wait.", args: args{ m: &fake.Manager{ @@ -2044,7 +2096,7 @@ func TestReconciler(t *testing.T) { want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42)) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := `Managed resource should acquire Synced=False/ReconcileSuccess status condition.` + reason := `Managed resource should acquire Synced=True/ReconcileSuccess status condition.` t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } @@ -2061,7 +2113,59 @@ func TestReconciler(t *testing.T) { WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) { c := &ExternalClientFns{ ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil + return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil + }, + UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) { + return ExternalUpdate{}, errBoom + }, + DisconnectFn: func(_ context.Context) error { + return nil + }, + } + + return c, nil + })), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}}, + }, + "ManagementPolicyImmutableWithDiff": { + reason: "Reconciliation without being allowed to update should trigger a ReconcileForbidden condition and a requeue after a long wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := asLegacyManaged(obj, 42) + mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) + + return nil + }), + MockUpdate: test.NewMockUpdateFn(errBoom), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := newLegacyManaged(42) + want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) + want.SetConditions(xpv1.ReconcileForbidden().WithObservedGeneration(42).WithMessage("External resource differs from desired state, but will not update due to missing 'Update' managementPolicy. Diff:\nmock diff")) + + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := `Managed resource should acquire Synced=False/ReconcileForbidden status condition when update is not allowed and a diff exists.` + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.LegacyManaged{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.LegacyManaged{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "mock diff"}, nil }, UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) { return ExternalUpdate{}, errBoom @@ -2092,7 +2196,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) - want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42)) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "A successful managed resource update should be reported as a conditioned status." @@ -2142,7 +2246,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := newLegacyManaged(42) want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionOrphan}) - want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) + want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42)) 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) diff --git a/pkg/reconciler/managed/reconciler_modern_test.go b/pkg/reconciler/managed/reconciler_modern_test.go index 833910074..4384996bf 100644 --- a/pkg/reconciler/managed/reconciler_modern_test.go +++ b/pkg/reconciler/managed/reconciler_modern_test.go @@ -1770,7 +1770,7 @@ func TestModernReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: true}}, }, - "ObserveOnlySuccessfulObserve": { + "ObserveOnlySuccessfulObserveNoDiff": { reason: "With Observe, a successful managed resource observe should trigger a requeue after a long wait.", args: args{ m: &fake.Manager{ @@ -1787,7 +1787,7 @@ func TestModernReconciler(t *testing.T) { want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42)) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := "With ObserveOnly, a successful managed resource observation should be reported as a conditioned status." + reason := "With ObserveOnly, a successful managed resource observation without any diff between external resource and desired spec should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } @@ -1803,7 +1803,7 @@ func TestModernReconciler(t *testing.T) { WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) { c := &ExternalClientFns{ ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{ResourceExists: true}, nil + return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil }, DisconnectFn: func(_ context.Context) error { return nil @@ -2032,8 +2032,8 @@ func TestModernReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: false}}, }, - "ManagementPolicyImmutable": { - reason: "Successful reconciliation skipping update should trigger a requeue after a long wait.", + "ManagementPolicyImmutableNoDiff": { + reason: "Successful reconciliation without upstream diff and skipping update should trigger a requeue after a long wait", args: args{ m: &fake.Manager{ Client: &test.MockClient{ @@ -2050,7 +2050,7 @@ func TestModernReconciler(t *testing.T) { want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42)) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { - reason := `Managed resource should acquire Synced=False/ReconcileSuccess status condition.` + reason := `Managed resource should acquire Synced=True/ReconcileSuccess status condition.` t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) } @@ -2067,7 +2067,59 @@ func TestModernReconciler(t *testing.T) { WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) { c := &ExternalClientFns{ ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { - return ExternalObservation{ResourceExists: true, ResourceUpToDate: false}, nil + return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil + }, + UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) { + return ExternalUpdate{}, errBoom + }, + DisconnectFn: func(_ context.Context) error { + return nil + }, + } + + return c, nil + })), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}}, + }, + "ManagementPolicyImmutableWithDiff": { + reason: "Successful reconciliation with upstream diff and skipping update 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 := asModernManaged(obj, 42) + mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) + + return nil + }), + MockUpdate: test.NewMockUpdateFn(errBoom), + MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { + want := newModernManaged(42) + want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete}) + want.SetConditions(xpv1.ReconcileForbidden().WithObservedGeneration(42).WithMessage("External resource differs from desired state, but will not update due to missing 'Update' managementPolicy. Diff:\nmock diff")) + + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := `Managed resource should acquire Synced=False/ReconcileForbidden status condition.` + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.ModernManaged{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.ModernManaged{})), + o: []ReconcilerOption{ + WithInitializers(), + WithManagementPolicies(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: true, ResourceUpToDate: false, Diff: "mock diff"}, nil }, UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) { return ExternalUpdate{}, errBoom