Skip to content
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

Porch should reject creating a PackageRevision that has an ownerReference whose blockOwnerDeletion flag is true #835

Open
kispaljr opened this issue Jan 16, 2025 · 0 comments
Labels
area/platform area/porch Porch related issues

Comments

@kispaljr
Copy link
Contributor

This is quite a nuanced corner case, that is caused by how the metadataStore in the porch-server is currently implemented.

Some relevant information before I describe the actual issue:

  • it is only possible to set an ownerReference pointing to an object of kind A with blockOwnerDeletion=true, if the creator has the permissions to update the finalizers on kind A (see details here)
  • when a PackageRevision is created with an ownerReference, the porch-server eventually creates another object of kind 'PackageRev' with the same ownerReference.

All of this implies the following:
If a client creates PackageRevision with a blockOwnerDeletion=true ownerReference to kind A, it will fail even if the client has the rights to update the finalizers of A, but the porch-server doesn't have the rights to update the finalizers of A. (Because the porch-server will fail to create the internal PackageRev object due to the missing permissions.) For anybody who doesn't know how the porch-server works internally it will be hard to understand why this is happening based on the current error message.

I can see multiple options to solve this:

  • the simplest is to reject all such ownerReferences with a clear error message, e.g: "It is not allowed to set ownerReferences on a PackageRevision with blockOwnerDeletion=true, because PackageRevision is a special resource."
  • Leave everything as-is. This will keep leading to confusion, but it leaves open the possibility to bind the necessary roles to the porch-server serviceaccount and thus enable creating blockOwnerDeletion=true ownerReferences (if really needed)
  • Refactor how the metadataStore is working

I think keeping the possibility open for blockOwnerDeletion=true type ownerReferences is not important at all, here is why:
an approved PackageRevsion cannot be just deleted, it has to be proposed for deletion before that. This implies that a controller that creates PackageRevisions cannot rely on the normal Kubernetes garbage collection process to clean up the "owned" PackageRevisons after the owner is deleted, but has to implement a special clean-up algorithm, that either orphans them, or proposes them for deletion (see the deletionPolicy in the PackageVariant controller).

All-in-all I propose to do what the title says: reject creating any PackageRevision that has an ownerReference whose blockOwnerDeletion flag is true.

@kispaljr kispaljr added area/platform area/porch Porch related issues labels Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform area/porch Porch related issues
Projects
None yet
Development

No branches or pull requests

1 participant