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

Resource trait to expose resource Scope #65

Closed
clux opened this issue Mar 20, 2020 · 2 comments
Closed

Resource trait to expose resource Scope #65

clux opened this issue Mar 20, 2020 · 2 comments

Comments

@clux
Copy link

clux commented Mar 20, 2020

If this is possible, how do you feel about adding:

   const SCOPE: &'static str; // cluster or namespaced

To the Resource trait?

This would allow us to guide users towards using the right scope for a resource. Currently, we have some level of hardcoding of cluster-level resources in kube (so users don't try to get namespaced ClusterRoles etc), but the error exists both ways.

If the user chooses a cluster scope for a namespaced resource it leads to hard to decipher 405 errors. See kube-rs/kube#194

@Arnavion
Copy link
Owner

Arnavion commented Mar 20, 2020

The spec doesn't directly contain the information about whether a particular resource is cluster- or namespace-scoped. The golang code has this information of course, via +genclient:nonNamespaced annotations, but this isn't emitted in the spec.

The only way I can see is to indirectly infer it by first seeing if the resource has an associated read operation or create operation, and whether that operation takes a namespace parameter or not. (Some resources only have read and some only have create, so need to check both.)


As far as adding const SCOPE: &'static str to the Resource trait goes, I think this should be solved in the typesystem, by either

  • adding trait ClusterScopedResource: Resource { } trait NamespaceScopedResource: Resource { } instead of a member of Resource, or

  • instead adding const SCOPE: ResourceScope to Resource with trait ResourceScope { } struct ClusterScopedResource; impl ResourceScope for ClusterScopedResource { } struct NamespaceScopedResource; impl ResourceScope for NamespaceScopedResource { } struct NotScopedResource; impl ResourceScope for NotScopedResource { }

This way the constaint can be enforced at compile-time, via R: NamespaceScopedResource in the former and R: Resource<Scope = NamespaceScopedResource> in the latter.

(You would think the latter would also work better with Rust's coherence rules, since you cannot impl <R: NamespaceScopedResource> and <R: ClusterScopedResource> differently due to lack of specialization, but should be able to impl <R: Resource<Scope = NamespaceScopedResource>> and <R: Resource<Scope = ClusterScopedResource>> separately. Unfortunately the latter also runs afoul of coherence rules, as described in rust-lang/rust#20400 )

This is also important bcause there are some impls of Resource where neither create nor read is available, specifically subresources like Eviction and Status, so these are neither cluster- nor namespaced- scope. In the new traits case, they would impl neither; in the ResourceScope case they would use NotScopedResource.

@clux
Copy link
Author

clux commented Mar 28, 2020

The spec doesn't directly contain the information about whether a particular resource is cluster- or namespace-scoped. The golang code has this information of course, via +genclient:nonNamespaced annotations, but this isn't emitted in the spec.

The only way I can see is to indirectly infer it by first seeing if the resource has an associated read operation or create operation, and whether that operation takes a namespace parameter or not. (Some resources only have read and some only have create, so need to check both.)

That sounds annoying. Maybe we request that exposed upstream?

As far as adding const SCOPE: &'static str to the Resource trait goes, I think this should be solved in the typesystem....

Yeah, having messed around a bit (in the linked pr), your suggestions are a lot a more practical to implement than a new string. Even your first one gets us pretty much all the way there, except for having to do some of the safe-guarding at runtime rather than compile time due to the specialization snag you mentioned. It would be a big improvement over having opaque 405 errors regardless.

The second example sounds even better (if we can indeed do the type of R: Resource<Scope = NotScopedResource> restriction you suggest), although haven't gotten around to testing it out yet.

This is also important bcause there are some impls of Resource where neither create nor read is available, specifically subresources like Eviction and Status, so these are neither cluster- nor namespaced- scope. In the new traits case, they would impl neither; in the ResourceScope case they would use NotScopedResource.

This type of "tri-state" (cluster/namespaced/unscoped) is useful regardless because people might want to have a view of a namespaced resource for --all-namespaces (in which case we want to make sure they don't do a straight read/create on it).

BTW: I thought the subresources like Status and Evictiontechnically followed the same namespacing rules, just as children of the original resource? It's the same url as the parent resource but with an extra /status or /eviction suffix AFAIKT.

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 a pull request may close this issue.

2 participants