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

get with Api::all() response can't be 404 #1276

Open
RWDai opened this issue Aug 10, 2023 · 5 comments
Open

get with Api::all() response can't be 404 #1276

RWDai opened this issue Aug 10, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@RWDai
Copy link

RWDai commented Aug 10, 2023

Current and expected behavior

I have an HttpRoute with the name "httproute_name" in the "default" namespace. When attempting to retrieve the API using the following code snippet:

let httproute_api: Api<HttpRoute> = Api::all(Client::try_default().await?);
let api = httproute_api.get("httproute_name").await;

The result of api is always a 404 error, indicating that the resource is not found. However, when I use the following code instead:

let httproute_api: Api<HttpRoute> = Api::namespaced(Client::try_default().await?, "default");
let api = httproute_api.get("httproute_name").await;

The api retrieval works as expected and returns the resource.

I'm puzzled by this behavior. My understanding was that the Api::all function should work across namespaces, and interestingly, when using Api::all, the list operation seems to return the correct results. It seems inconsistent that the get operation fails with a 404 when using Api::all, but works with Api::namespaced.

Could you please help me understand why this is happening? Is the all function not meant to be cross-namespace, and if so, why is the list operation returning expected results?

Update:

When utilizing the 'get' method with the 'all' API call, a 404 response should not be generated.

Possible solution

When using the 'get' function with the 'all' API call, instead of returning a 404 error, it would be more helpful to provide an error message that suggests using the namespaced API for more accurate results.

Additional context

No response

Environment

[~]# kubectl version --short
Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
Client Version: v1.25.0
Kustomize Version: v4.5.7
Server Version: v1.20.15
WARNING: version difference between client (1.25) and server (1.20) exceeds the supported minor version skew of +/-1
[~]# uname -a
Linux xxxxxx 3.10.0-1160.el7.x86_64 #1 SMP Mon Oct 19 16:18:59 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Configuration and features

kube = { version = "0.80", features = ["runtime", "derive"] }
k8s-openapi = { version = "0.17", features = ["v1_21"] }
k8s-gateway-api = { version = "0.11" }

Affected crates

No response

Would you like to work on fixing this bug?

None

@RWDai RWDai added the bug Something isn't working label Aug 10, 2023
@Dav1dde
Copy link
Member

Dav1dde commented Aug 10, 2023

This is expected and documented behavior, see the all and namespaced documentation.

You can only use the all client to get cluster wide resources or to list namespaced resources across all (multiple) namespaces. To retrieve a specific namespaced resource you need a namespaced api for that resource.

Your first example, what if a resource with this name existed in multiple namespaces, which one should it return?

@RWDai
Copy link
Author

RWDai commented Aug 10, 2023

Thank you for your reply. I have a better understanding now of why get cannot accurately find the resource when using all. However, I still find the return of a 404 error quite confusing. This is because the documentation for all() states that it's for cluster-level resources or resources viewed across all namespaces doc, which led me to believe that get could also work across namespaces when using all().

The 404 error is indeed confusing as it might give the impression that 'get' is unable to retrieve named resources, rather than indicating the absence of a specified namespace.Perhaps when using get with all, instead of returning a 404 error, a more precise error message could be provided, such as "Please the use of the namespaced API". #949

@RWDai RWDai changed the title Inconsistent Behavior Between 'all' and 'namespaced' get with Api::all() response can't be 404 Aug 10, 2023
@Dav1dde
Copy link
Member

Dav1dde commented Aug 10, 2023

We could improve the docs here, they are clear to me, but I am also quite experienced with the API.

We can't give a different error because the client doesn't know that there is no such cluster wide resource and we do not differentiate in the Api between cluster wide or namespaced (only in the Resource). You're sending a valid request to the api, it just doesn't make sense on a level the client doesn't know about.

There may be a future with another higher level client on top of API which is aware of this (maybe a trait with the scope as a associated type or a different type all together).

@clux
Copy link
Member

clux commented Aug 10, 2023

I am wondering if we should try to lean more on resource scopes in Api for Api::all style constructions.

Just like we limit Api::namespaced to a particular resource Scope, we could have specific getters for the two cases where:

  • K: Resource<Scope = ClusterResourceScope>
  • K: Resource<Scope = NamespaceResourceScope>

The trick lies in distinguishing:

  • "across all namespaces" variant of Api::all - only possible for NamespaceResourceScope
  • "cluster level only" variant of Api::all - only possible for ClusterResourceScope

We could probably introduce an Api::get_namespaced method that only works for NamespaceResourceScope, but allows an arbitrary namespace supplied. This would allow both an ::all and a ::namepaced bound Api to query from a namespace (even a different one to the one bound - which i think could be useful).

Other than that, maybe if we split Api::all into Api::all and Api::cluster and scope limit correctly, we could also return an error if people do Api::get on an "all scoped" namespace resource. That would be a small breaking change though.

(I did originally hope to iron out more of these issues with the on-client request method in #1032, but progress is slow and need more thought on the more difficult subresource cases - also i'm like 3 prs deep in there so need a new strategy)

@Dav1dde
Copy link
Member

Dav1dde commented Aug 10, 2023

Other than that, maybe if we split Api::all into Api::all and Api::cluster and scope limit correctly

I don't think we should do this in the Api in generic Code you need to work with either resource type and if the api is split there needs to be a trait to be able to work with them in generic code and some things will probably a lot harder then they already are (e.g. generically creating a Api is already pretty hard and requires two helper traits).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants