Skip to content

Revert "Add support for Orphan management policy" #931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
phisco merged 1 commit into from
Feb 17, 2026
+5 −274
Merged

Revert "Add support for Orphan management policy" #931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions apis/common/policies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;Orphan
// +kubebuilder:validation:Enum=Observe;Create;Update;Delete;LateInitialize;*;MustCreate
type ManagementAction string

const (
Expand Down Expand Up @@ -53,10 +53,6 @@ const (
// 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"

// ManagementActionOrphan means that all actions except Delete will be taken
// by the Crossplane controllers.
ManagementActionOrphan ManagementAction = "Orphan"
)

// A DeletionPolicy determines what should happen to the underlying external
Expand Down
4 changes: 0 additions & 4 deletions apis/common/v1/policies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ const (
// 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

// ManagementActionOrphan means that all actions except Delete will be taken
// by the Crossplane controllers.
ManagementActionOrphan ManagementAction = common.ManagementActionOrphan
)

// A DeletionPolicy determines what should happen to the underlying external
Expand Down
8 changes: 3 additions & 5 deletions pkg/reconciler/managed/policies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,6 @@ func defaultSupportedManagementPolicies() []sets.Set[xpv1.ManagementAction] {
// Useful when external resource lifecycle is managed elsewhere but you want
// to allow Crossplane to make updates and discover state changes.
sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve, xpv1.ManagementActionUpdate, xpv1.ManagementActionLateInitialize),
// Orphan allows all actions except Delete.
sets.New[xpv1.ManagementAction](xpv1.ManagementActionOrphan),
}
}

Expand Down Expand Up @@ -219,7 +217,7 @@ func (m *ManagementPoliciesResolver) ShouldCreate() bool {
return true
}

return m.managementPolicies.HasAny(xpv1.ManagementActionCreate, xpv1.ManagementActionAll, xpv1.ManagementActionMustCreate, xpv1.ManagementActionOrphan)
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
Expand All @@ -240,7 +238,7 @@ func (m *ManagementPoliciesResolver) ShouldUpdate() bool {
return true
}

return m.managementPolicies.HasAny(xpv1.ManagementActionUpdate, xpv1.ManagementActionAll, xpv1.ManagementActionOrphan)
return m.managementPolicies.HasAny(xpv1.ManagementActionUpdate, xpv1.ManagementActionAll)
}

// ShouldLateInitialize returns true if the LateInitialize action is allowed.
Expand All @@ -250,7 +248,7 @@ func (m *ManagementPoliciesResolver) ShouldLateInitialize() bool {
return true
}

return m.managementPolicies.HasAny(xpv1.ManagementActionLateInitialize, xpv1.ManagementActionAll, xpv1.ManagementActionOrphan)
return m.managementPolicies.HasAny(xpv1.ManagementActionLateInitialize, xpv1.ManagementActionAll)
}

// ShouldOnlyObserve returns true if the Observe action is allowed and all
Expand Down
139 changes: 1 addition & 138 deletions pkg/reconciler/managed/reconciler_legacy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestReconciler(t *testing.T) {
},
want: want{result: reconcile.Result{}},
},
"UnpublishConnectionDetailsDeletionPolicyDeleteOrphan": {
"UnpublishConnectionDetailsDeletionPolicyDeleteOrpahn": {
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Typo in test case name: "Orphahn" → "Orphan".

Hey, small catch here — it looks like a transposition crept in during the revert. The test case key reads "UnpublishConnectionDetailsDeletionPolicyDeleteOrpahn" but should be "UnpublishConnectionDetailsDeletionPolicyDeleteOrphan". Could you fix this? 🙂

🔤 Proposed fix 
-		"UnpublishConnectionDetailsDeletionPolicyDeleteOrpahn": {
+		"UnpublishConnectionDetailsDeletionPolicyDeleteOrphan": {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"UnpublishConnectionDetailsDeletionPolicyDeleteOrpahn": {
"UnpublishConnectionDetailsDeletionPolicyDeleteOrphan": {
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/reconciler/managed/reconciler_legacy_test.go` at line 89, The test case
map contains a typo in the key
"UnpublishConnectionDetailsDeletionPolicyDeleteOrpahn"; rename this key to
"UnpublishConnectionDetailsDeletionPolicyDeleteOrphan" in the test table within
reconciler_legacy_test.go so the test name is spelled correctly (search for the
existing map entry with the incorrect key and replace it with the corrected
string); no other code logic changes are required.

reason: "Errors unpublishing connection details should trigger a requeue after a short wait.",
args: args{
m: &fake.Manager{
Expand Down Expand Up @@ -1858,45 +1858,6 @@ func TestReconciler(t *testing.T) {
},
want: want{result: reconcile.Result{Requeue: true}},
},
"ManagementPolicyOrphanCreateSuccessful": {
reason: "Successful managed resource creation using management policy Orphan 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.ManagementActionOrphan})
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.ManagementActionOrphan})
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}},
},
"ManagementPolicyCreateCreateSuccessful": {
reason: "Successful managed resource creation using management policy Create should trigger a requeue after a short wait.",
args: args{
Expand Down Expand Up @@ -2129,53 +2090,6 @@ func TestReconciler(t *testing.T) {
},
want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}},
},
"ManagementPolicyOrphanUpdateSuccessful": {
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 := asLegacyManaged(obj, 42)
mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionOrphan})
return nil
}),
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))
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.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}, nil
},
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
},
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}},
},
"ManagementPolicyUpdateUpdateSuccessful": {
reason: "A successful managed resource update using management policies should trigger a requeue after a long wait.",
args: args{
Expand Down Expand Up @@ -2492,14 +2406,6 @@ func TestLegacyManagementPoliciesResolverShouldCreate(t *testing.T) {
},
want: true,
},
"ManagementPoliciesEnabledHasCreateOrphan": {
reason: "Should return true if management policies are enabled and managementPolicies has action Orphan",
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicies{xpv1.ManagementActionOrphan},
},
want: true,
},
"ManagementPoliciesEnabledActionNotAllowed": {
reason: "Should return false if management policies are enabled and managementPolicies does not have Create",
args: args{
Expand Down Expand Up @@ -2561,14 +2467,6 @@ func TestLegacyManagementPoliciesResolverShouldUpdate(t *testing.T) {
},
want: false,
},
"ManagementPoliciesEnabledHasUpdateOrphan": {
reason: "Should return true if management policies are enabled and managementPolicies has action Orphan",
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicies{xpv1.ManagementActionOrphan},
},
want: true,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
Expand Down Expand Up @@ -2622,14 +2520,6 @@ func TestLegacyManagementPoliciesResolverShouldLateInitialize(t *testing.T) {
},
want: false,
},
"ManagementPoliciesEnabledHasLateInitializeOrphan": {
reason: "Should return true if management policies are enabled and managementPolicies has action Orphan",
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicies{xpv1.ManagementActionOrphan},
},
want: true,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
Expand Down Expand Up @@ -2800,33 +2690,6 @@ func TestLegacyShouldDelete(t *testing.T) {
},
want: want{delete: false},
},
"DeletionDeleteManagementActionOrphan": {
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.LegacyManaged{
Orphanable: fake.Orphanable{
Policy: xpv1.DeletionDelete,
},
Manageable: fake.Manageable{
Policy: xpv1.ManagementPolicies{xpv1.ManagementActionOrphan},
},
},
},
want: want{delete: false},
},
"DeletionManagementActionOrphan": {
reason: "Should orphan if management policies are enabled and deletion policy is not set and management policy does not have action Delete.",
args: args{
managementPoliciesEnabled: true,
managed: &fake.LegacyManaged{
Manageable: fake.Manageable{
Policy: xpv1.ManagementPolicies{xpv1.ManagementActionOrphan},
},
},
},
want: want{delete: false},
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
Expand Down
Loading
Loading