Skip to content

Try to integrate k8s-pb #1602

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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 added 2 commits October 13, 2024 11:32
Signed-off-by: clux <sszynrae@gmail.com>
starting step towards compilation

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux linked an issue Oct 13, 2024 that may be closed by this pull request
clux added 3 commits October 13, 2024 12:17
thankfully straightforward

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
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 added 4 commits October 14, 2024 21:25
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@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).

# for free to join this conversation on GitHub. Already have an account? # 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