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

Do not guess resource endpoints #467

Closed
MikailBag opened this issue Mar 18, 2021 · 5 comments · Fixed by #481
Closed

Do not guess resource endpoints #467

MikailBag opened this issue Mar 18, 2021 · 5 comments · Fixed by #481
Assignees
Labels
api Api abstraction related

Comments

@MikailBag
Copy link
Contributor

Currently Resource (which is the only entity that is actually working with Kubernetes URLs) only known resource kind (such as "Pod"). However, URLs contain resource name instead (e.g. "pods" or "statefulsets"). Resource uses inflector crate to pluralize the kind and assumes that is it valid resource name. Although it works for most of resources, this means that is impossible to use Client with resources that do not follow this scheme (no matter are you using typed Api<k8s_openapi::Foo> or untyped Api<DynamicObject>). Just several examples of such resources:

  • discovery.k8s.io: EndpointSlice is served on endpointslices, but inflector guesses endpointsliceice.
  • metrics.k8s.io: NodeMetrics is served on nodes, and PodMetrics is served on pods. Impossible to guess.

It seems that in typed case we can't get rid of inflector until k8s-openapi provides us with this information, but at least dynamic API should work without guessing URLs.

@MikailBag
Copy link
Contributor Author

My proposal:

  • Add new resource_name (or endpoint, I'm not good at naming) method to the Meta trait.
  • Add a name field to GVK.
  • impl Meta for DynamicObject is changed to return this name.
  • impl Meta for K where K: k8s_openapi::Resource still uses inflector
  • When at some moment k8s-openapi provides resource name, we just change that method to use this provided information instead of the calls to inflector.

@clux
Copy link
Member

clux commented Mar 18, 2021

Yeah, we know this is bad. It's #284. Was hoping we'd get a fix in k8s-openapi, but no one has jumped on the issue there yet.

I think your proposal is reasonable. Now that Meta is unchained from k8s-openapi we can define what we want, and it seems a good place to start by fixing it there, and having the other one fallback to inflector for now.

We could potentially do one better by tackling cluster/namespaced scoping as part of the trait (see #199) as well (although one for a separate PR i think).

@clux
Copy link
Member

clux commented Mar 18, 2021

As for naming; plural is probably what we want. It's what's used in kube-derive.

@clux clux added the api Api abstraction related label Mar 18, 2021
@kazk
Copy link
Member

kazk commented Mar 18, 2021

whatisinternet/Inflector#73 fixes the iceice, but Inflector is no longer maintained. It's also a large dependency, and we don't need most of its features.

Do we have any irregular plurals to handle? I feel like we can use a much simpler function to handle basic pluralization instead.

@kazk kazk mentioned this issue Mar 19, 2021
clux added a commit that referenced this issue Mar 28, 2021
replaces #468.
closes #467

This changes kube-derive to no longer implement `k8s_openapi` traits.
We now instead implement `kube::Resource` instead so we can control the
plural.

Because the trait now picks up on an override when it is generating the
path_url, it is now correct in the Api urls.
clux added a commit that referenced this issue Mar 28, 2021
replaces #468.
closes #467

This changes kube-derive to no longer implement `k8s_openapi` traits.
We now instead implement `kube::Resource` instead so we can control the
plural.

Because the trait now picks up on an override when it is generating the
path_url, it is now correct in the Api urls.
@clux clux self-assigned this Mar 28, 2021
@clux clux closed this as completed in #481 Mar 29, 2021
@clux
Copy link
Member

clux commented Mar 31, 2021

No longer guessing unless we have to from 0.52.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related
Projects
None yet
3 participants