Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
379 stars 41 forks source link

Resource trait to expose resource Scope #65

Closed clux closed 3 years ago

clux commented 4 years ago

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 https://github.com/clux/kube-rs/issues/194

Arnavion commented 4 years ago

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

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 https://github.com/rust-lang/rust/issues/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 commented 4 years ago

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.