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

Constrain Resource trait and Api::namespaced by Scope #956

Merged
merged 8 commits into from
Jul 20, 2022

Conversation

clux
Copy link
Member

@clux clux commented Jul 15, 2022

To prevent the namespace type errors users run into occassionally (like #952 ), and fixes #194 . k8s-openapi added these a year back but didn't have time to make everything work back then.

  • Extend Resource with Scope associated type
  • Add DynamicScope as an impl ResourceScope for dynamic/unstructured objs
  • Use #[kube(namespaced)] to decide scope for kube-derive
  • Some compile time tests + doc tests with compile_fail on negative case

Note the extra inlined bodies of Api::*namespaced methods now cannot call each other because the _with suffixed ones have a signature for dynamic scoped ones only.

The scope type is directly the ResourceScope trait from k8s-openapi. Long term maybe we can get this into a traits crate if we can get consensus, but afaikt it's not possible to have our own trait for this unless it's the same one used downstream. It's not a big deal now, but will be annoying in k8s-pb.


One thing we could do is to add an Api::cluster ctor with an explicit ClusterScope constraint. This might be good, but it leaves Api::all in a bad state. Do we put a NamespaceScope constraint on Api::all after years of users using it for cluster stuff? The migration path is horrible. I found it annoying to even do internally in kube; 8613992, and dynamic types struggle even more with it. Have opted not to do it for now. There are ultimately no user consequences / benefits in distinguishing cluster resources from all namespaces other than the slight semantic improvement.

To prevent the namespace type errors we've been seeing for ages that we
have the tools to prevent.

Early sketch.

Signed-off-by: clux <[email protected]>
@clux clux added the changelog-change changelog change category for prs label Jul 15, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2022

Codecov Report

Merging #956 (4af062b) into master (fa66405) will increase coverage by 0.04%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #956      +/-   ##
==========================================
+ Coverage   72.46%   72.50%   +0.04%     
==========================================
  Files          64       64              
  Lines        4412     4426      +14     
==========================================
+ Hits         3197     3209      +12     
- Misses       1215     1217       +2     
Impacted Files Coverage Δ
kube-core/src/dynamic.rs 66.66% <ø> (ø)
kube-core/src/object.rs 80.00% <ø> (ø)
kube-core/src/resource.rs 46.51% <ø> (ø)
kube-derive/src/custom_resource.rs 13.83% <0.00%> (-0.18%) ⬇️
kube-client/src/api/mod.rs 68.08% <100.00%> (+10.94%) ⬆️

@clux
Copy link
Member Author

clux commented Jul 17, 2022

Peripheral fix to cargo deny included (license case that as best i can tell does not apply to us - dtolnay/unicode-ident#9 - stole the wording from a similar pr in automerge/automerge#409). Can cherry pick to another branch if desired.

@clux clux marked this pull request as ready for review July 17, 2022 15:03
@clux clux requested a review from a team July 17, 2022 15:03
@clux clux added this to the 0.75.0 milestone Jul 18, 2022
@kazk kazk mentioned this pull request Jul 19, 2022
@kazk
Copy link
Member

kazk commented Jul 19, 2022

One thing we could do is to add an Api::cluster ctor with an explicit ClusterScope constraint. This might be good, but it leaves Api::all in a bad state. Do we put a NamespaceScope constraint on Api::all after years of users using it for cluster stuff? The migration path is horrible.

Yeah, this will be rough if we change Api::all.

Api::all is a confusing name, so maybe deprecate it and add new methods to allow incremental migration.

  • Add Api::cluster or Api::cluster_scoped with Scope = ClusterResourceScope
  • Add Api::all_namespaces with Scope = NamespaceResourceScope (should only allow a subset of methods, but that's not possible yet)
  • Deprecate Api::all and remove later

I think cluster_scoped is clearer and feels more natural in kube. all_namespaces is kind of awkward, but maybe it should be because it's different.

  • Api::namespaced
  • Api::default_namespaced
  • Api::cluster_scoped / Api::cluster
  • Api::all_namespaces / Api::all_namespaced / Api::namespaced_all

@clux
Copy link
Member Author

clux commented Jul 19, 2022

A bit torn on this, because while it might be slightly confusing semantically, there's no real penalty for having the ::all method serve as both "all resources in the cluster scope" and "all resources across all namespaces". In the api sense all here really matches the api concept very closely; "all" as in "do not restrict me to a namespace".

If we deprecate and introduce a longer named variant of the same thing, then I kind of think that is worse, because we force people to do the same migration anyway (just through warnings in their own time), and i don't think we actually need to do this:

The remaining error case (accidentally querying across too many namespaces) does not feel severe enough to warrant a migration. The most confusing user error is caught by constricting ::namespaced, so kind of feel this stands sufficiently on its own.

Long term though; I am getting more and more inclined to making query methods onto Client directly instead (i.e. one of the ideas from the Neokubism PR) and allowing users to bypass Api - which seems to be a reasonable path for many use-cases. With k8s-pb generics it might be possible.

@kazk
Copy link
Member

kazk commented Jul 19, 2022

this stands sufficiently on its own.

Yeah, I was just thinking of a way if we decide to add Api::cluster.

This is useful already. Other changes are probably not worth doing, and definitely not a blocker for this.

kube-core/src/resource.rs Outdated Show resolved Hide resolved
@clux clux merged commit e84cca0 into master Jul 20, 2022
@clux clux deleted the scope-constrain-api branch July 20, 2022 08:15
@clux clux mentioned this pull request Jul 28, 2022
@nightkr
Copy link
Member

nightkr commented Sep 22, 2022

I was pretty late on this, but...

Long term though; I am getting more and more inclined to making query methods onto Client directly instead (i.e. one of the ideas from the Neokubism PR) and allowing users to bypass Api - which seems to be a reasonable path for many use-cases. With k8s-pb generics it might be possible.

Yeah, this is how we do it at @stackabletech (we have our own Client type that creates short-lived Apis for each call) and IMO it's a pretty huge improvement. On the other hand, this change introduces some issues for that since we currently assume that we can always create the API the same way for all kinds...

@clux
Copy link
Member Author

clux commented Sep 22, 2022

Yeah, I can see it having ergonomic improvement in many places. I also don't think we need to break the Api type to accomplish this (because it also has value), as we can probably lift the methods into impls on Client, and make Api defer to those.

@imp
Copy link
Contributor

imp commented Sep 22, 2022

As an example - https://docs.rs/kube-client-ext/latest/kube_client_ext/trait.KubeClientExt.html
Makes life a little bit easier and code less verbose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resources are not scoped
5 participants