Skip to content

Enable deleting objects when identity of manifest changes #349

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nkvoll
Copy link

@nkvoll nkvoll commented Mar 6, 2025

Description of your changes

This is an attempt to move the discussion of #287 forward.

I think some of the issue is that the interfaces provided by crossplane-runtime (e.g the managed.ExternalObservation) doesn't allow to neatly encode the Observe function wanting to return a the operations required for a rename (e.g a Delete + Create). Without changing interfaces etc, which may be a heavy ask for the needs of potentially a single provider, we might have to be a little creative.

At first, I tried dealing with the Deletion of the previous resource in Create, but its problematic because Observe will try to update atProvider, which overwrites the information we need to delete a previously managed object.

Doing the deletion in Observe is a less noisy option code-wise, but of course means that "Observing" might entail "Delete something", and I'd love to hear opinions on this with possible solutions (e.g like updating the interface/structs from the runtime, expanding the Object resource to include resource references etc -- these are larger lifts, but it would be great to have clarity whether doing the requisite work for that would end up with the work being merged before committing to it).

Fixes #287

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Running locally in a local dev environment.

@bobh66
Copy link
Contributor

bobh66 commented Mar 6, 2025

I'm not convinced that we want the provider to delete an existing resource and replace it with another one. We explicitly don't do that in the terraform-based providers when it's necessary to delete a resource to apply an update. I understand the scenarios involved, but if kubernetes requires a delete/create then I think provider-kubernetes should also require a delete/create, which can be handled at the Composition layer by changing the composite resource name.

It seems a little too much like magic and could have unintended consequences if someone doesn't understand that changing the name will cause the resource to be deleted and recreated.

It makes me wonder whether there should be CEL rules in the Object CRD to prevent changes to spec.manifest.apiVersion, spec.manifest.kind, spec.manifest.metadata.name and spec.manifest.metadata.namespace since all of these would trigger a delete/create if changed.

@nkvoll
Copy link
Author

nkvoll commented Mar 6, 2025

Preventing these changes would also solve my concern (inadvertently changing the identity, potentially leaving orphans behind without realizing it). 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing Object.spec.forProvider.manifest.metadata.name orphans previous resource
2 participants