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

client: Api::all fails to create namespaced resources #1030

Open
olix0r opened this issue Sep 26, 2022 · 1 comment
Open

client: Api::all fails to create namespaced resources #1030

olix0r opened this issue Sep 26, 2022 · 1 comment
Labels
api Api abstraction related client http issues with the client

Comments

@olix0r
Copy link
Contributor

olix0r commented Sep 26, 2022

We have a test helper that worked in v0.74 and fails in v0.75:

pub async fn create<T>(client: &kube::Client, obj: T) -> T
where
    T: kube::Resource,
    T: serde::Serialize + serde::de::DeserializeOwned + Clone + std::fmt::Debug,
    T::DynamicType: Default,
{
    let params = kube::api::PostParams {
        field_manager: Some("linkerd-policy-test".to_string()),
        ..Default::default()
    };
    let api = kube::Api::<T>::all(client.clone());
    tracing::trace!(?obj, "Creating");
    api.create(&params, &obj)
        .await
        .expect("failed to create resource")
}
thread 'both' panicked at 'failed to create resource: Api(ErrorResponse { status: "Failure", message: "the server does not allow this method on the requested resource", reason: "MethodNotAllowed", code: 405 })', /workspaces/linkerd2/policy-test/src/lib.rs:34:10

This seems to be because we use kube::Api::all on a namespaced resource (ServiceAccount). Changing the code to use Api::namespaced fixes the issue, at least.

It's surprising that this compiles if it's not going to work properly. Should it work properly? It seems reasonable to have a cluster-wide client for a namespaced resource.

Otherwise, if this is by design, can we do anything to make it harder to build API clients that won't work?

@clux
Copy link
Member

clux commented Sep 26, 2022

That is the reverse of the problem that was fixed in 0.75 by making Api::namespaced only work on NamespaceResourceScope resources.

We decided against putting the hard constraint on Api::all at the time because Api::all can legitimately be used to both query resources across all namespaces if they are namespace scoped, or across the cluster if cluster scoped.

However, as you are noticing, Api::create on a resource will not work without a namespace on a resource that has NamespaceResourceScope, but it will also not will work with a namespace on a resource that has ClusterResourceScope. Thus using Api::all for an namespaced api instance that is meant to do more than lists/watch is currently something you are not supposed to do.

It's obviously not ideal, and afaik we can't specialise Api::create (or the other mutators) for these cases. Perhaps we should bite the breaking-change bullet and split Api::all into an Api::cluster (cluster scoped) and a new Api::all (namespace scoped).

@clux clux added api Api abstraction related client http issues with the client labels Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related client http issues with the client
Projects
None yet
Development

No branches or pull requests

2 participants