kube-rs / kube

Rust Kubernetes client and controller runtime
https://kube.rs
Apache License 2.0
2.95k stars 305 forks source link

shortcuts for Api::apply and Api::apply_status #649

Open clux opened 3 years ago

clux commented 3 years ago

Saw that these were added by client-go and did some investigation so thought i'd add an issue here for it to at least discuss them.

What is it: see client-go interface for (say) Node: https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/kubernetes/typed/core/v1/node.go#L53-L54

the implementation is just a dumb shortcut to calling the patch method, just coerces the patch into the type you asked for: https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/kubernetes/typed/core/v1/node.go#L191-L243

Their ApplyOptions are simple: https://github.com/kubernetes/apimachinery/blob/71bf7ba06cd490392e05cd6828ae24a69cb0ccaa/pkg/apis/meta/v1/generated.proto#L141-L160 basically the same as their PatchOptions..

the weirdest thing is they got alternate variants of their structs for apply builders(?). comparing the two:

they seem identical, so it must just be for the builders in applyconfigurations directory?

The shortcut does not deal with the awkward duplicate required metadata: { name: same_value_as_param } being part of the patch, and it technically allows mismatching patch types to the patch at the api level, so it doesn't provide any extra type safety for us i think? at least as long as patch options and apply options remain the same on the kubernetes side.

not sure how valuable this would be to us, but raising this here anyway so that can look back at an analysis, even if the result is a wontfix.

clux commented 3 years ago

Ah, one benefit on the client-go side is less arguments over its main Patch type which requires more. The new ones implicitly handle the .metadata.name (by forcing it in the builders, and presumably extracting from taht). The KEP is a little clearer in its goals: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/2155-clientgo-apply . pretty reasonable api, and it gets some typing safety:

appsv1apply.Deployment("ns", "nginx-deployment").
  Spec(appsv1apply.DeploymentSpec().
    Replicas(0).
    Template(
      v1apply.PodTemplate().
        Spec(v1apply.PodSpec().
          Containers(
            v1apply.Container().
              Name("nginx").
              Image("nginx:1.14.2"),
            v1apply.Container().
              Name("sidecar").
          )
        )
      )
    )
  )

over us just dumping it into json! and attempt-serializing at runtime.

clux commented 2 years ago

The KubeCon America talk; SIG API Machinery Deep Dive goes through why these structs (with full optionals) are needed (in the first half of that talk); some required fields get defaulted when serialized through apply (and thus we might overwrite something like replica numbers with int default 0 when we use Default::default on a openapi struct and pass it to apply).

This might be something we could resolve long term in codegen. But for now using json! is the easier way to do it in rust (to avoid accidentally passing on non-optional defaults with server side apply).