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

Try to integrate k8s-pb #1602

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Try to integrate k8s-pb #1602

wants to merge 9 commits into from

Conversation

clux
Copy link
Member

@clux clux commented Oct 13, 2024

Experiment to see how offensive this actually would look if we were to use an indirection layer (core::k8s) and route everything through that and the core traits (Resource + ResourceExt).

Annoying bits

  • need to make a feature selection for openapi vs pb, we can either:
    1. CURRENT force a choice (compile fail without pb or openapi), precedence to openapi (breaking for --no-default builds)
    2. NO feature gate out half the modules (feels like a big ergonomics loss for us)
  • Metadata is optional in all types in k8s_pb, but not in k8s-openapi:
    1. NO unwrap in k8s-pb for alignment Metadata trait should return guaranteed Metadata k8s-pb#47
    2. CURRENT unwrap in Resource trait to get it to compile - i don't like this either
    3. MAYBE make it truly optional make more optional return types (might be a lot)
    4. MAYBE hack k8s-pb types to coerce to non-option at serialisation layer?
  • Other core types are also slightly different
    1. CURRENT multiplex in Resource trait for ownerreferences/typemeta/labels/annotations
    2. NO hack k8s-pb more? ATM, this is not the biggest deal, but users would likely notice if they migrate.
  • Multiplexing between DeserializeOwned and prost::Message traits is non-trivial
    1. NO Wait for unstable trait-alias feature (progress) to allow Api bound to depend on an aliased DeserTrait
    2. CURRENT Hard-disable Api / discovery modules and anything using DeserializeOwned (current)
    3. MAYBE Some sort of indirection layer for for ser/de which can be traitified?
    4. MAYBE Let people contribute pb implementations to specifics as we go along?
  • Envelope implementation in client/mod for a PoC.
    1. CURRENT Thinking about it. Handling Envelope Wrapper k8s-pb#6

Easy bits

@clux clux linked an issue Oct 13, 2024 that may be closed by this pull request
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 16.12903% with 26 lines in your changes missing coverage. Please review.

Project coverage is 75.0%. Comparing base (95cf702) to head (be2911b).

Files with missing lines Patch % Lines
kube-core/src/resource.rs 13.4% 26 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1602     +/-   ##
=======================================
- Coverage   75.2%   75.0%   -0.1%     
=======================================
  Files         82      82             
  Lines       7335    7352     +17     
=======================================
  Hits        5514    5514             
- Misses      1821    1838     +17     
Files with missing lines Coverage Δ
kube-client/src/api/mod.rs 68.1% <ø> (ø)
kube-client/src/api/subresource.rs 52.7% <ø> (ø)
kube-client/src/api/util/csr.rs 66.7% <ø> (ø)
kube-client/src/api/util/mod.rs 93.4% <ø> (ø)
kube-client/src/client/mod.rs 75.2% <100.0%> (ø)
kube-client/src/discovery/apigroup.rs 87.2% <ø> (ø)
kube-client/src/discovery/parse.rs 94.6% <ø> (ø)
kube-client/src/lib.rs 94.1% <ø> (ø)
kube-core/src/crd.rs 83.0% <ø> (ø)
kube-core/src/dynamic.rs 77.1% <ø> (ø)
... and 9 more

@clux clux changed the title Integrate k8s-pb Try to integrate k8s-pb Oct 14, 2024
@nightkr
Copy link
Member

nightkr commented Nov 4, 2024

need to make a feature selection for openapi vs pb, we can either:

It feels weird to have these be mutually exclusive. AIUI, this mostly comes down to the current design where:

  1. The typing crates (k8s-openapi, k8s-pb) defines a resource trait
  2. We define our own resource trait
  3. We blanket-impl our own resource trait over the typing crate's resource trait

This requires a choice to be made since we can only have blanket impl to avoid ambiguities (since technically the same type could impl both k8s_openapi::Resource and k8s_pb::Resource). Instead, I'd rather move towards getting rid of the duplication, something like:

  1. We define a new trait-only crate (working title k8s-traits), moving kube::Resource into there
  2. Typings (k8s-pb) impl k8s_traits::Resource directly
    • We should talk to k8s-openapi, either k8s-openapi should also impl k8s_traits::Resource or we could spend our one blanket impl on it like kube currently does (behind a feature gate)
  3. Clients (kube) depends on k8s_traits::Resource directly

That way, you'd just pull in the kube and k8s-openapi versions you want and never worry about feature flags (for this). It should also help compilation parallelization a bit (since kube compilation would no longer be blocked by the selected typing impl (aside from if we end up doing the backwards compat hack for k8s-openapi)).

The only blocker here AIUI is ObjectMeta, and that could also be traitified for the fields we care about (like kube::Resource already kind of does).

Metadata is optional in all types in k8s_pb, but not in k8s-openapi:

Depending on how annoying this is to do with k8s-pb, another option here would be to return a blank ObjectMeta for the None case (just like the k8s-openapi variant of Resource::labels() already does).

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

Successfully merging this pull request may close these issues.

Integrate with kube under a feature
2 participants