Merge pull request #933 from lsviben/revert-873

Revert "Add MustCreate management policy"

Author: phisco

apis/common/policies.go

@@ -22,7 +22,7 @@ type ManagementPolicies []ManagementAction
 
 // A ManagementAction represents an action that the Crossplane controllers
 // can take on an external resource.
-// +kubebuilder:validation:Enum=Observe;Create;Update;Delete;LateInitialize;*;MustCreate
+// +kubebuilder:validation:Enum=Observe;Create;Update;Delete;LateInitialize;*
 type ManagementAction string
 
 const (
@@ -49,10 +49,6 @@ const (
 	// ManagementActionAll means that all of the above actions will be taken
 	// by the Crossplane controllers.
 	ManagementActionAll ManagementAction = "*"
-
-	// ManagementActionMustCreate means that the external resource MUST be created
-	// by the managed resource and if it already exists an error condition is raised.
-	ManagementActionMustCreate ManagementAction = "MustCreate"
 )
 
 // A DeletionPolicy determines what should happen to the underlying external

apis/common/v1/policies.go

@@ -52,10 +52,6 @@ const (
 	// ManagementActionAll means that all of the above actions will be taken
 	// by the Crossplane controllers.
 	ManagementActionAll = common.ManagementActionAll
-
-	// ManagementActionMustCreate means that the external resource must be created
-	// by this MR and if it already exists an error condition is raised and no further processing is executed.
-	ManagementActionMustCreate = common.ManagementActionMustCreate
 )
 
 // A DeletionPolicy determines what should happen to the underlying external

pkg/meta/meta.go

@@ -362,20 +362,6 @@ func ExternalCreateSucceededDuring(o metav1.Object, d time.Duration) bool {
 	return time.Since(t) < d
 }
 
-// ExternalCreateNotStarted returns true if the external resource was detected
-// during the initial Observe phase and was not created by this managed resource.
-func ExternalCreateNotStarted(o metav1.Object) bool {
-	pending := GetExternalCreatePending(o)
-	succeeded := GetExternalCreateSucceeded(o)
-	failed := GetExternalCreateFailed(o)
-
-	if pending.IsZero() && succeeded.IsZero() && failed.IsZero() {
-		return true
-	}
-
-	return false
-}
-
 // IsPaused returns true if the object has the AnnotationKeyReconciliationPaused
 // annotation set to `true`.
 func IsPaused(o metav1.Object) bool {

pkg/meta/meta_test.go

@@ -1228,66 +1228,3 @@ func TestIsPaused(t *testing.T) {
 		})
 	}
 }
-
-func TestExternalCreateNotStarted(t *testing.T) {
-	type args struct {
-		o metav1.Object
-	}
-
-	cases := map[string]struct {
-		args args
-		want bool
-	}{
-		"CreatePending": {
-			args: args{
-				o: func() metav1.Object {
-					o := &corev1.Pod{}
-					t := time.Now().Add(-30 * time.Second)
-					SetExternalCreatePending(o, t)
-
-					return o
-				}(),
-			},
-			want: false,
-		},
-		"CreateFailed": {
-			args: args{
-				o: func() metav1.Object {
-					o := &corev1.Pod{}
-					t := time.Now().Add(-30 * time.Second)
-					SetExternalCreateFailed(o, t)
-
-					return o
-				}(),
-			},
-			want: false,
-		},
-		"SuccessfullyCreated": {
-			args: args{
-				o: func() metav1.Object {
-					o := &corev1.Pod{}
-					t := time.Now().Add(-2 * time.Minute)
-					SetExternalCreateSucceeded(o, t)
-
-					return o
-				}(),
-			},
-			want: false,
-		},
-		"NotStarted": {
-			args: args{
-				o: &corev1.Pod{},
-			},
-			want: true,
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			got := ExternalCreateNotStarted(tc.args.o)
-			if diff := cmp.Diff(tc.want, got); diff != "" {
-				t.Errorf("ExternalCreateNotStarted(...): -want, +got:\n%s", diff)
-			}
-		})
-	}
-}

pkg/reconciler/managed/policies.go

@@ -60,8 +60,6 @@ func defaultSupportedManagementPolicies() []sets.Set[xpv1.ManagementAction] {
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionAll),
 		// All actions explicitly set, the same as default.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionDelete),
-		// All actions explicitly set with MustCreate instead of Create.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize, xpv1.ManagementActionDelete),
 		// ObserveOnly, just observe action is done, the external resource is
 		// considered as read-only.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve),
@@ -71,55 +69,29 @@ func defaultSupportedManagementPolicies() []sets.Set[xpv1.ManagementAction] {
 		// No LateInitialize filling in the spec.forProvider, allowing some
 		// external resource fields to be managed externally.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionDelete),
-		// No LateInitialize filling in the spec.forProvider, allowing some
-		// external resource fields to be managed externally. With MustCreate.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionDelete),
 		// No Delete, the external resource is not deleted when the managed
 		// resource is deleted.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize),
-		// No Delete, the external resource is not deleted when the managed
-		// resource is deleted.  With MustCreate.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize),
 		// No Delete and no LateInitialize, the external resource is not deleted
 		// when the managed resource is deleted and the spec.forProvider is not
 		// late initialized.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate),
-		// No Delete and no LateInitialize, the external resource is not deleted
-		// when the managed resource is deleted and the spec.forProvider is not
-		// late initialized. With MustCreate.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionUpdate),
 		// No Update, the external resource is not updated when the managed
 		// resource is updated. Useful for immutable external resources.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete, xpv1.ManagementActionLateInitialize),
-		// No Update, the external resource is not updated when the managed
-		// resource is updated. Useful for immutable external resources. With MustCreate.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionDelete, xpv1.ManagementActionLateInitialize),
 		// No Update and no Delete, the external resource is not updated
 		// when the managed resource is updated and the external resource
 		// is not deleted when the managed resource is deleted.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionLateInitialize),
-		// No Update and no Delete, the external resource is not updated
-		// when the managed resource is updated and the external resource
-		// is not deleted when the managed resource is deleted. With MustCreate.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionLateInitialize),
 		// No Update and no LateInitialize, the external resource is not updated
 		// when the managed resource is updated and the spec.forProvider is not
 		// late initialized.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate, xpv1.ManagementActionDelete),
-		// No Update and no LateInitialize, the external resource is not updated
-		// when the managed resource is updated and the spec.forProvider is not
-		// late initialized. With MustCreate.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionDelete),
 		// No Update, no Delete and no LateInitialize, the external resource is
 		// not updated when the managed resource is updated, the external resource
 		// is not deleted when the managed resource is deleted and the
 		// spec.forProvider is not late initialized.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionCreate),
-		// No Update, no Delete and no LateInitialize, the external resource is
-		// not updated when the managed resource is updated, the external resource
-		// is not deleted when the managed resource is deleted and the
-		// spec.forProvider is not late initialized.  With MustCreate.
-		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate),
 		// Like ObserveOnly, but the external resource is deleted when the
 		// managed resource is deleted.
 		sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionDelete),
@@ -217,18 +189,7 @@ func (m *ManagementPoliciesResolver) ShouldCreate() bool {
 		return true
 	}
 
-	return m.managementPolicies.HasAny(xpv1.ManagementActionCreate, xpv1.ManagementActionAll, xpv1.ManagementActionMustCreate)
-}
-
-// MustCreate returns true if the Create action is required.  If the resource already exists an error will
-// be raised in the reconciler.
-// If the management policy feature is disabled, it returns false.
-func (m *ManagementPoliciesResolver) MustCreate() bool {
-	if !m.enabled {
-		return false
-	}
-
-	return m.managementPolicies.Has(xpv1.ManagementActionMustCreate)
+	return m.managementPolicies.HasAny(xpv1.ManagementActionCreate, xpv1.ManagementActionAll)
 }
 
 // ShouldUpdate returns true if the Update action is allowed.

pkg/reconciler/managed/reconciler.go

@@ -70,8 +70,6 @@ const (
 	errExternalResourceNotExist = "external resource does not exist"
 
 	errManagedNotImplemented = "managed resource does not implement connection details"
-
-	errMustCreate = "managed resource has mustcreate policy but external resource already exists"
 )
 
 // Event reasons.
@@ -117,8 +115,6 @@ type ManagementPoliciesChecker interface { //nolint:interfacebloat // This has t
 	ShouldOnlyObserve() bool
 	// ShouldCreate returns true if the Create action is allowed.
 	ShouldCreate() bool
-	// MustCreate returns true if the create action is required to be executed
-	MustCreate() bool
 	// ShouldLateInitialize returns true if the LateInitialize action is
 	// allowed.
 	ShouldLateInitialize() bool
@@ -1148,16 +1144,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 		return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
 	}
 
-	// If the resource already exists, the MustCreate policy is set, and there are no create annotations then
-	// this MR did not create the resource and an error is raised.
-	if observation.ResourceExists && policy.MustCreate() && meta.ExternalCreateNotStarted(managed) {
-		log.Debug(errMustCreate)
-		record.Event(managed, event.Warning(reasonCannotCreate, errors.New(errMustCreate)))
-		status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(errors.New(errMustCreate)))
-
-		return reconcile.Result{Requeue: false}, 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

pkg/reconciler/managed/reconciler_legacy_test.go

@@ -1900,93 +1900,6 @@ func TestReconciler(t *testing.T) {
 			},
 			want: want{result: reconcile.Result{Requeue: true}},
 		},
-		"ManagementPolicyMustCreateCreateSuccessful": {
-			reason: "Successful managed resource creation using management policy MustCreate 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 := asLegacyManaged(obj, 42)
-							mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionDelete})
-							return nil
-						}),
-						MockUpdate: test.NewMockUpdateFn(nil),
-						MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
-							want := newLegacyManaged(42)
-							want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionDelete})
-							meta.SetExternalCreatePending(want, time.Now())
-							meta.SetExternalCreateSucceeded(want, time.Now())
-							want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42))
-							want.SetConditions(xpv1.Creating().WithObservedGeneration(42))
-							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.LegacyManaged{}),
-				},
-				mg: resource.ManagedKind(fake.GVK(&fake.LegacyManaged{})),
-				o: []ReconcilerOption{
-					WithInitializers(),
-					WithManagementPolicies(),
-					WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
-					WithExternalConnector(&NopConnector{}),
-					WithCriticalAnnotationUpdater(CriticalAnnotationUpdateFn(func(_ context.Context, _ client.Object) error { return nil })),
-					WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}),
-				},
-			},
-			want: want{result: reconcile.Result{Requeue: true}},
-		},
-		"ResourceExistsMustCreateError": {
-			reason: "When a resource exists and the policy is MustCreate an error condition should be raised.",
-			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.ManagementActionMustCreate, xpv1.ManagementActionDelete})
-							return nil
-						}),
-						MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error {
-							want := newLegacyManaged(42)
-							want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve, xpv1.ManagementActionMustCreate, xpv1.ManagementActionDelete})
-							want.SetConditions(xpv1.Creating().WithObservedGeneration(42), xpv1.ReconcileError(errors.New(errMustCreate)).WithObservedGeneration(42).WithObservedGeneration(42))
-							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
-								reason := "With MustCreate, a successful managed resource observation with no creation annotations should be reported as an error 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}, nil
-							},
-							DisconnectFn: func(_ context.Context) error {
-								return nil
-							},
-						}
-						return c, nil
-					})),
-					withLocalConnectionPublishers(LocalConnectionPublisherFns{
-						PublishConnectionFn: func(_ context.Context, _ resource.LocalConnectionSecretOwner, _ ConnectionDetails) (bool, error) {
-							return false, nil
-						},
-					}),
-					WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}),
-				},
-			},
-			want: want{result: reconcile.Result{Requeue: false}},
-		},
 		"ManagementPolicyImmutable": {
 			reason: "Successful reconciliation skipping update should trigger a requeue after a long wait.",
 			args: args{