Allow cluster-scoped instance CRDs via schema scope #885
Conversation
Adds spec.schema.scope (Namespaced|Cluster, immutable, default Namespaced) to ResourceGraphDefinition. CRD synthesis now sets scope accordingly, and instance reconciler handles cluster-scoped instances without requiring a namespace. Includes unit tests and docs note. Fixes: kubernetes-sigs#806
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: antcybersec The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @antcybersec. Thanks for your PR. I'm waiting for a github.com 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/M
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
a-hilaly
left a comment
a-hilaly
left a comment
There was a problem hiding this comment.
Thank you @antcybersec! i do have one comment on the implementation. however I do think that this feature deserves a KREP-XXX document that shares benefits, tradeoffs and (maybe) potential impact.
| if ns := obj.GetNamespace(); ns != "" { | ||
| namespacedClient = instanceClient.Namespace(ns) | ||
| } |
There was a problem hiding this comment.
runtime, has already information about the resource scope.
There was a problem hiding this comment.
Thanks for the feedback! Just to confirm — are you suggesting that instead of checking obj.GetNamespace() here, I should pull the scope from the runtime (via RESTMapper / GVK mapping) and choose the correct client accordingly?
If runtime scope is authoritative in this context, I can update the implementation to rely on it and remove this namespaced check.
There was a problem hiding this comment.
Since we already know the scope at RGD compile time, we can store that information in https://github.com/kubernetes-sigs/kro/blob/main/pkg/graph/graph.go#L30-L31 - which has a namespaced bool https://github.com/kubernetes-sigs/kro/blob/main/pkg/graph/resource.go#L60-L63.
Later we can just retrieve from runtime using https://github.com/kubernetes-sigs/kro/blob/main/pkg/runtime/interfaces.go#L113-L115
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@antcybersec can you please write up a KREP document (doesn't have to be large) that shares benefits, tradeoffs and (maybe) potential impact. Just documenting decisions here. |
k8s-ci-robot
added
size/L
| descriptor := igr.runtime.ResourceDescriptor(resourceID) | ||
| gvr := descriptor.GetGroupVersionResource() | ||
|
|
||
| var dynResource dynamic.ResourceInterface | ||
| if restMapping.Scope.Name() == meta.RESTScopeNameNamespace { | ||
| if descriptor.IsNamespaced() { | ||
| namespace := igr.getResourceNamespace(resourceID) | ||
| dynResource = igr.client.Resource(restMapping.Resource).Namespace(namespace) | ||
| dynResource = igr.client.Resource(gvr).Namespace(namespace) | ||
| } else { | ||
| dynResource = igr.client.Resource(restMapping.Resource) | ||
| dynResource = igr.client.Resource(gvr) |
There was a problem hiding this comment.
How is this change related to allowing cluster-scoped RGDs?
There was a problem hiding this comment.
hey @tjamet I have added a KREP document (
docs/design/proposals/resource-descriptor-scope-resolution.md
) in the latest commit that details the motivation and design for this change.
| instanceClient := igr.client.Resource(igr.gvr) | ||
| var namespacedClient dynamic.ResourceInterface = instanceClient | ||
| if ns := obj.GetNamespace(); ns != "" { | ||
| namespacedClient = instanceClient.Namespace(ns) | ||
| } |
There was a problem hiding this comment.
namespacedClient name is misleading IMO It could be cluster-scoped or namespaced client depending on the resource
I would suggest defining a getGVRClient used here, in the setUnmanaged and in the getResourceClient function to ensure namespacing is well-managed everywhere
4 participants
Adds spec.schema.scope (Namespaced|Cluster, immutable, default Namespaced) to ResourceGraphDefinition. CRD synthesis now sets scope accordingly, and instance reconciler handles cluster-scoped instances without requiring a namespace. Includes unit tests and docs note.
Fixes: #806