-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
🐛 cross-namespace owner references should be disallowed #675
🐛 cross-namespace owner references should be disallowed #675
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @boylee1111! |
Hi @boylee1111. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
735a877
to
4d0aa95
Compare
/ok-to-test |
/assign @droot |
@@ -60,6 +60,10 @@ func SetControllerReference(owner, object metav1.Object, scheme *runtime.Scheme) | |||
return fmt.Errorf("%T is not a runtime.Object, cannot call SetControllerReference", owner) | |||
} | |||
|
|||
if owner.GetNamespace() != "" && owner.GetNamespace() != object.GetNamespace() { | |||
return fmt.Errorf("cross-namespace owner references are disallowed, owner's namespace %s, obj's namespace %s", owner.GetNamespace(), object.GetNamespace()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens to (accidentally?) also cover the case of "cluster-scoped resource must not have a namespace-scoped owner". However in that case the error is confusing, because its not a cross-namespace ref.
Would you mind returning a different error message in case object.GetNamesapce() == ""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my intention was to cover both cases. I had separated them now, it's more clear. thx for suggestion.
4d0aa95
to
58a6142
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks!
c0853ea
to
7c75b06
Compare
7c75b06
to
6f5721d
Compare
Sorry, I accidentally pushed something wrong to this pr, I had removed it now. PTAL. |
/lgtm thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: boylee1111, DirectXMan12 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I stumbled upon this PR while migrating from Operator Framework (there is no such limitation) to Kubebuilder. I understand why this cross namespace limitation makes sense for standard K8s resources like StatefulSets or Deployments. However, why has this to be enforced in a general framework, where also more abstract functionalities are implemented? In my case an operator creates developer workspaces, implemented as Namespaces and a Pod in each of them. These workspaces are defined by a namespaced CRs in the operator namespace. I do not see any requirements to introduce a cluster CRD for that. |
@FaKod this is a limitation of Kubernetes itself, check the link in the PR description |
@alvaroaleman A call to |
Yes, prior to this PR it was possible to create a defunct owner Ref. |
Based on https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#owners-and-dependents, cross-namespace owner references are disallowed by design. The
SetControllerReference()
should prevent owner reference from setting on dependents in different namespace.