Config: make cluster/users/clusters optional

[goenning]

Motivation

This is a follow up on the previous work to make kube-rs parse kubeconfigs like client-go: #1076

We found another scenario where client-go allows us to have config files without clusters/users/contexts, while kube-rs does not. This mean a user could have all contexts/clusters in file A and keep users on file B.

Solution

Turn clusters/users/contexts into Option<Vec<T>>

[goenning]

Note: I'm still doing some ad-hoc tests on this, but I wanted to raise this for some early feedback/review.

[codecov-commenter]

Codecov Report

Merging #1120 (44a3279) into main (2d35a2f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1120      +/-   ##
==========================================
+ Coverage   72.63%   72.70%   +0.07%     
==========================================
  Files          65       65              
  Lines        4867     4877      +10     
==========================================
+ Hits         3535     3546      +11     
+ Misses       1332     1331       -1     

Impacted Files	Coverage Δ
kube-client/src/config/file_config.rs	77.14% <100.00%> (+1.38%)	⬆️
kube-runtime/src/wait.rs	77.35% <0.00%> (+1.88%)	⬆️

[goenning]

@clux I was doing a few more tests with this and it seems like the current solution doesn't work if the value as empty like this:

contexts:
users:
- name: admin@k3d-k3s-default
  user:
    client-certificate-data: aGVsbG8K
    client-key-data: aGVsbG8K

But it works if contexts key is removed. Do you know if there's any other attribute we're missing?

[clux]

Ah, yeah that is different. The serde default does not happen in that case, because you are technically setting contexts to null.

We would need something like https://docs.rs/serde_with/latest/serde_with/struct.DefaultOnNull.html to handle this.

Does something actually generate contexts: that we need to care about?

[goenning]

It's not the case of being generated with null, but the user manually modified the file and it worked fine kubectl, but not on apps using kube-rs

[clux]

Well, I guess this brings us back to the previous discussion on this, where DefaultOnNull was generally agreed upon as being surprising and non-idiomatic in rust.

However, we did not really consider that it would be cause different parsing behaviour from kubectl by taking this route, and the go world ultimately dictates the serialisation format here. Maybe we should use DefaultOnNull after all for these cases 🤔

[goenning]

Either that or Option<Vec<T>>, I'm fine either way, although I find the DefaultOrNull to simplify a lot, albeit less idiomatic.

The way go handles deserialisation is a lot more forgiving, which is specially useful when these files can be manually modified.

[kazk]

Yeah, I guess we should do DefaultOrNull then. And document that null defaults for compatibility.

[goenning]

Before I go ahead and work on this, should we do it on all fields that are currently Option<T> as well? Maybe not all, but at least all Vec and Strings?

[clux]

It's hard to give a blanket answer here because it looks to me like it's context dependent.

For instance, the conventions in the top config struct is different between similar fields: https://github.com/kubernetes/client-go/blob/228b004f4a926d43f53c0f8b6de3e1e2a3e50fcb/tools/clientcmd/api/types.go#L25-L54

Specifically:

// 1.struct, not `+optional` marked, but still is optional. ours is `Option<Preferences>`
// ...theirs might just have an implied default (all the fields of the struct is +optional)
Preferences Preferences `json:"preferences"`

// 2. map with pointer to cluster.. so `clusters: ~` should not be allowed as it looks illegal with yaml.
// ..i would be surprised if we needed DefaultOnNull here, but it shouldn't make a huge difference
// in our case they are all vecs, with `#[serde(default, skip_serializing_if = "Vec::is_empty")]` prefix
Clusters map[string]*Cluster `json:"clusters"`
AuthInfos map[string]*AuthInfo `json:"users"`
Contexts map[string]*Context `json:"contexts"`

// 3. this one has to exist, but it's nice for us to have option wrapped for merge alg
CurrentContext string `json:"current-context"`

// 4. explicit +optional and omitempty map. `DefaultOnNull` and Option wrapped probably makes sense here
// +optional
Extensions map[string]runtime.Object `json:"extensions,omitempty"`

Note the comment:

// Top level config objects and all values required for proper functioning are not "omitempty".  Any truly optional piece of config is allowed to be omitted.

1 and 2 are probably fine to add DefaultOnNull to. Preferences is packed in an Option (NB: a raw struct with Default could work, but feels ugly), and all of the ones in 2 need to exist in the end and are unioned at merge time. I am not sure about 4 -Extensions - because it has a different serialisation format than category 2. Sounds like it can fully omitted without defaulting, so probably should not default to an empty vec, and should fully override during merge (i.e. keep the Option<Vec<..>> and then add DefaultOnNull).

---

Note this is only for the top level structs. How deep we want to follow this approach needs to be dictated by kubectlclient-go's merge behaviour. Does it fill in partially filled in structs (e.g. in Cluster) across two kubeconfigs refer to the same cluster.name? Does merging cause appending of shared vectors?

Such behaviour would mean making our merge smarter:

kube/kube-client/src/config/file_config.rs

Lines 409 to 439 in e0a4e49

	/// Merge kubeconfig file according to the rules described in
	/// <https://kubernetes.io/docs/concepts/configuration/organize-cluster-access-kubeconfig/#merging-kubeconfig-files>
	///
	/// > Merge the files listed in the `KUBECONFIG` environment variable according to these rules:
	/// >
	/// > - Ignore empty filenames.
	/// > - Produce errors for files with content that cannot be deserialized.
	/// > - The first file to set a particular value or map key wins.
	/// > - Never change the value or map key.
	/// > Example: Preserve the context of the first file to set `current-context`.
	/// > Example: If two files specify a `red-user`, use only values from the first file's `red-user`.
	/// > Even if the second file has non-conflicting entries under `red-user`, discard them.
	pub fn merge(mut self, next: Kubeconfig) -> Result<Self, KubeconfigError> {
	if self.kind.is_some() && next.kind.is_some() && self.kind != next.kind {
	return Err(KubeconfigError::KindMismatch);
	}
	if self.api_version.is_some() && next.api_version.is_some() && self.api_version != next.api_version {
	return Err(KubeconfigError::ApiVersionMismatch);
	}
	self.kind = self.kind.or(next.kind);
	self.api_version = self.api_version.or(next.api_version);
	self.preferences = self.preferences.or(next.preferences);
	append_new_named(&mut self.clusters, next.clusters, |x| &x.name);
	append_new_named(&mut self.auth_infos, next.auth_infos, |x| &x.name);
	append_new_named(&mut self.contexts, next.contexts, |x| &x.name);
	self.current_context = self.current_context.or(next.current_context);
	self.extensions = self.extensions.or(next.extensions);
	Ok(self)
	}
	}

So it's worth doing some testing and checking upstream how client-go merges configs before adding DefaultOnNull to nested vectors or things that are not option packed. For option packed things it should be fine to add though.

[goenning]

Thanks, I'll do some testing and compare the behavior between both libs and open an issue before doing any change. I think what can be confusing here is that omitempty is only used for serialization, but Go will generally deserialize null to empty values, which is why I think the clusters: ~ works fine on client-go.

But I'm actually surprised to see clusters/contexts/users as a map instead of an array, how the hell does that work? Unless they have a custom deserializer somewhere.

[kazk]

Just remember that we should align with client-go and not kubectl.

But I'm actually surprised to see clusters/contexts/users as a map instead of an array, how the hell does that work? Unless they have a custom deserializer somewhere.

Those are arrays in clientcmd/api/v1 Config.

The link in file_config.rs is kind of misleading because it points to clientcmd/api Config which is after the conversion.

https://github.com/kubernetes/client-go/blob/7697067af71046b18e03dbda04e01a5bb17f9809/tools/clientcmd/api/v1/conversion.go#L28 converts an array of named clusters to a map.

https://github.com/kubernetes/client-go/blob/7697067af71046b18e03dbda04e01a5bb17f9809/tools/clientcmd/loader.go seems to be where it reads the file and deserializes the struct.

[clux]

Ah, interesting. So client-go effectively has a way to distinguish between more ergonomic types after conversion, and a disk format.

..It could similarly make sense to have a lax "pre-merge Kubeconfig" struct that is more option heavy, that is later converted into one where we have all properties (and invariants baked into errors returned by merge / conversion), plus convenience getters for map-style vectors. That would solve the ergonomics issues with options when we know the properties are required. That's more work though, and requires some care in terms of breaking changes.