Resources are not scoped

[clux]

Currently people can instantiate any type of k8s_openapi::Resource and use it with Api::namespaced or Api::all, however, each resource only really works in one Scope.

So it's not a good pattern to have people figure out what is the correct scope, then only later getting an obscure 405 when picking it wrong like in #191 .

It would be useful if k8s_openapi::Resource also told us if the scope was cluster or namespaced, as it would avoid us having to either hardcode objects that are allowed to be used in this scope (we already do a bit of that in https://github.com/clux/kube-rs/blob/06f7b5cc7004ff3c6e23000747140171f03014c8/kube/src/api/resource.rs#L45-L50 )

It would also probably be better to rename Api::all to Api::cluster to signify that this is a scope selector not a --all-namespaces equivalent.

[nightkr]

It's perfectly valid to use ::all as an --all-namespaces equivalent for list/watch operations, at least.

[clux]

Yeah, that is true.. Maybe the best interface would just be having a third entrypoint:

Api::namespaced(ns) # namespaced scope, specified namespace
Api::all()        # namespaced scope, all namespaces
Api::cluster() # cluster scope, no namespace

and if we got the trait data, we could tell people they chose wrong?

[nightkr]

There is no difference for the URLs. We could handle it by adding a type argument and associated type like this..

// In k8s-openapi
trait Resource {
    type Namespace: Debug;
    // ...
}

struct RoleBinding { /* ... */ }
struct ClusterRoleBinding { /* ... */ }

impl Resource for RoleBinding {
    type Namespace = String;
    // ...
}

impl Resource for ClusterRoleBinding {
    type Namespace = void::Void;
    // ...
}

// In kube-rs
struct Api<K, NS> { /* ... */ }
// constructors
impl<K: Resource> Api<K, K::Namespace> {
    fn namespaced(ns: K::Namespace) -> Self { /* ... */ }
}
impl<K: Resource> Api<K, void::Void> {
    fn all() -> Self { /* ... */ }
}
// requires that we're in the correct scope for the resource
impl<K: Resource> Api<K, K::Namespace> {
    fn get(&self, name: &str) -> Result<Option<K>> {/* ... */}
}
// bulk operations, fine regardless of scope
impl<K: Resource, NS> Api<K, Ns> {
    fn list(&self, name: &str) -> Result<Vec<K>> {/* ... */}
}

That way we'd lock away invalid operations at the type level. The big downside is that we now need to drag this NS context around at the type level whenever we want to mention the Api's type.

[nightkr]

Regarding kube::api::Resource, IMO at that point you've already opted out of type safety and all hope is lost.. :P

What are the blockers for moving Informer and Reflector back to using kube::api::Api?

[clux]

Well, I was thinking we'd do it at the Resource level first:

impl Resource {
    /// Cluster level resources
    pub fn cluster<K: k8s_openapi::Resource>() -> Self {
        if K::SCOPE != "cluster" {
            panic!("{} is not a cluster scoped resource", K::KIND)
        }
        Self {
            api_version: K::API_VERSION.to_string(),
            kind: K::KIND.to_string(),
            group: K::GROUP.to_string(),
            version: K::VERSION.to_string(),
            namespace: None,
        }
    }

    /// Resources viewed across all namespaces
    pub fn all<K: k8s_openapi::Resource>() -> Self {
        if K::SCOPE != "namespaced" {
            panic!("{} is not a namespace scoped resource", K::KIND)
        }
        Self {
            api_version: K::API_VERSION.to_string(),
            kind: K::KIND.to_string(),
            group: K::GROUP.to_string(),
            version: K::VERSION.to_string(),
            namespace: None,
        }
    }

    /// Namespaced resource within a given namespace
    pub fn namespaced<K: k8s_openapi::Resource>(ns: &str) -> Self {
        if K::SCOPE != "namespaced" {
            panic!("{} is not a namespace scoped resource", K::KIND)
        }
        Self {
            api_version: K::API_VERSION.to_string(),
            kind: K::KIND.to_string(),
            group: K::GROUP.to_string(),
            version: K::VERSION.to_string(),
            namespace: Some(ns.to_string()),
        }
    }
}

While the LIST/GET url is ok for an all-namespaces alias, it is invalid to PUT/POST namespace scoped resources to a non-namespace-containing url.

[clux]

What are the blockers for moving Informer and Reflector back to using kube::api::Api?

I don't think there are any, unless there are lifetime/ownership issues associated with it. Think it felt nicer during the big refactor, but it's just duplicating Api logic AFAIKT now.

[nightkr]

While the LIST/GET url is ok for an all-namespaces alias, it is invalid to PUT/POST namespace scoped resources to a non-namespace-containing url.

Yeah.. so that requires another runtime check with the runtime approach. I'm also not a huge fan of hiding user-caused panics like this, even if you'd usually create all Apis pretty early on in your program.

[clux]

Yeah, I agree.

If there's a way to do it with some const-generics style impls that hides the complexity then I'd be all for a compile time check. But don't really want to force extra generic parameters on every program for an uncommon user error.

[nightkr]

I don't think const generics would help here, the syntax in https://github.com/rust-lang/rfcs/blob/master/text/2000-const-generics.md is effectively the same as for regular generics. We could move the generic to the constructor method, but that would still limit us to runtime checks for the operation methods.

The upside is that the NS type should usually be inferrable when constructing, so the usage changes like this:

// from
Api::<Pod>::namespaced("foo")
// to
Api::<Pod, _>::namespaced("foo")
// rather than
Api::<Pod, String>::namespaced("foo")

Users only have to interact with the extra generic when passing the Api around.

[clux]

I don't think const generics would help here, the syntax in https://github.com/rust-lang/rfcs/blob/master/text/2000-const-generics.md is effectively the same as for regular generics.

Well because k8s_openapi::Resource implements constant strings, we could potentially implement our own marker trait for namespaced resources with a single blanket implementation. Not sure about what features of const generics is necessary to do that (if that's even possible yet).

Maybe it would be easier if k8s_openapi implemented a Namespaced trait for namespace scoped resources instead? Then we could simply require Api::namespaced and Api::all to take a K: k8s_openapi::Resource + k8s_openapi::Namespaced.

[clux]

Users only have to interact with the extra generic when passing the Api around.

You do end up having to do that though , and it's not particularly intuitive why we are passing this marker trait around. Particularly if it can be done with a stronger constraint on K in the constructors.

[nightkr]

Maybe it would be easier if k8s_openapi implemented a Namespaced trait for namespace scoped resources instead?

Not really, since you can't have negative type constraints. The equivalent with my proposal would be to constrain to Resource<Namespace = String>. AFAIK we could define a type alias NamespacedResource for that.

Particularly if it can be done with a stronger constraint on K in the constructors.

You can do the current runtime check with a stronger constraint on K in the constructors (disallow creating an Api with namespaced scope for cluster-scoped resources). You can't block illegal method calls (like create() a namespaced resource with a cluster-scoped Api) without carrying the Api's scope somewhere in the type signature.

[clux]

Experimented a bit in #199 with one of Arnavion's suggestions. It does the current runtime check at compile time with the two extra traits he suggested (the easiest setup of the two).

It doesn't allow us to block Api::create at compile time, but we can at least do some book-keeping of the underlying Resource and block it in the case where we have Resource::all.

[clux]

This can now be tackled with k8s-openapi 0.12.
The Resource trait has been updated to have an associated trait for the scope, and these traits can be used to constrict type signatures for resources (e.g. deployment and its NamespaceResourceScope)