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

WIP: Native client apply support #1574

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

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Sep 9, 2024

Motivation

Add support for resource apply using unstable-client implementation.

This implementation addresses the need for https://pkg.go.dev/k8s.io/client-go/applyconfigurations, where the need for a custom DSL is justified by limitations of default values for the go structures.

It seems with exception of some resources where API is not designed to use SSA patch, and a separate patch definition needed, this abstraction can simplify usage of the SSA in general cases.

Equivalent structs in rust allow to distinguish a nil value from an ”” (empty string), as a pointer value is wrapped in Option<V>, thus rendering current state of structures sufficient to address limitation in native to object SSA patch usage.

None -> nil
Some(“”) -> “”
Some(“other”) -> “other”

However there is a second problem with TypeMeta. When constructing an object from a type, like &Pod::default(), the structure does not contain type meta from the beginning, although generated or k8s-openapi definitions have all the necessary information for constructing the field.

R::kind() -> kind
R::api_version() -> apiVersion

Solution

Provide a native to the client .apply and .apply_status methods. These can be used to pass a subset of resource changes to be applied via SSA patch.

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

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

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

Files with missing lines Patch % Lines
kube-client/src/client/client_ext.rs 0.0% 20 Missing ⚠️
kube-core/src/metadata.rs 0.0% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1574     +/-   ##
=======================================
- Coverage   75.2%   75.0%   -0.2%     
=======================================
  Files         82      82             
  Lines       7335    7355     +20     
=======================================
  Hits        5514    5514             
- Misses      1821    1841     +20     
Files with missing lines Coverage Δ
kube-core/src/metadata.rs 67.7% <0.0%> (ø)
kube-client/src/client/client_ext.rs 73.6% <0.0%> (-10.8%) ⬇️

@Danil-Grigorev Danil-Grigorev force-pushed the unstable-client-patch branch 2 times, most recently from 0288752 to 78f7608 Compare September 9, 2024 09:14
Comment on lines 432 to 475
let req = match subresource {
Subresource::Core => req.patch(name, pp, patch).map_err(Error::BuildRequest)?,
Subresource::Status => req
.patch_subresource("status", name, pp, patch)
.map_err(Error::BuildRequest)?,
Subresource::Scale => req
.patch_subresource("scale", name, pp, patch)
.map_err(Error::BuildRequest)?,
Subresource::Custom(subresource) => req
.patch_subresource(subresource, name, pp, patch)
.map_err(Error::BuildRequest)?,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting idea with the enum here. i am a little skeptical of it's benefit because it means the documentation must be smashed together for all the variants. separated is fairly easy to read on the client side, and doesn't require a new enum (that i'm pretty sure they'd only need to create for this call anyway).

the original style in api:

  • client.patch
  • client.patch_status
  • client.patch_scale
  • client.patch_subresource

feels preferable to me, unless you have any good reasons.

K: ResourceExt + Serialize + DeserializeOwned + Clone + Debug,
<K as Resource>::DynamicType: Default,
{
let name = resource.meta().name.as_ref().ok_or(Error::NameResolve)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name assumption actually severely constrains the usability of patch to only Patch::Apply and the subset of others that happen to be typed to contain the complete metadata.

I.e. it generally excludes merge patches, strategic merge patches and json patches

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea/end goal here is to simplify some of the pain points, occurring while using Apply patch, without introducing a custom DSL, like in https://pkg.go.dev/k8s.io/client-go/applyconfigurations, just to provide a apiVersion and kind for the apply patch.

It makes sense to address this incompatibility with other patch types. Need to think it through

@Danil-Grigorev Danil-Grigorev force-pushed the unstable-client-patch branch 2 times, most recently from 0928b11 to aa70e33 Compare October 6, 2024 21:02
@Danil-Grigorev Danil-Grigorev changed the title WIP: Native client patch WIP: Native client apply support Oct 6, 2024
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
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.

2 participants