fix: align kube-rs with client-go config parsing

[goenning]

Signed-off-by: goenning me@goenning.net

Motivation

Fixes #1076

Solution

I went through each field on my kubeconfig and removed it / set it to null, then I tried both kubectl and cube-rs example kubectl. If it succeeded on kubectl but failed on kube-rs, I added a default and moved to the next field.

The part I struggled with was parsing of null values, such as

  - cluster:
    name: gke_oenning-sandbox_us-central1-c_cluster-1

I couldn't find how to do it with serve alone, so I had to add the serde_with crate.

Some properties like cluster.server are useless when empty, but the user might not be interacting with that cluster/context, so it should not be causing errors for something that might not impact its usage. If they try to use that cluster, they'll then have an error somewhere else.

Note: I'll add a unit test as well, but I want to get it out here for feedback early on in case someone disagree with the solution.

[codecov-commenter]

Codecov Report

Merging #1077 (8ecbd8f) into main (b60202c) will increase coverage by 0.03%.
The diff coverage is 58.97%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1077      +/-   ##
==========================================
+ Coverage   72.62%   72.66%   +0.03%     
==========================================
  Files          65       65              
  Lines        4819     4832      +13     
==========================================
+ Hits         3500     3511      +11     
- Misses       1319     1321       +2     

Impacted Files	Coverage Δ
kube-client/src/api/entry.rs	82.26% <0.00%> (ø)
kube-client/src/api/util/mod.rs	93.50% <ø> (ø)
kube-client/src/client/config_ext.rs	48.07% <0.00%> (-1.93%)	⬇️
kube-client/src/discovery/apigroup.rs	89.90% <0.00%> (ø)
kube-core/src/gvk.rs	82.60% <0.00%> (ø)
kube-core/src/schema.rs	89.70% <ø> (ø)
kube-core/src/subresource.rs	76.66% <ø> (ø)
kube-core/src/watch.rs	57.14% <0.00%> (ø)
kube-derive/src/custom_resource.rs	13.83% <0.00%> (ø)
kube-runtime/src/utils/mod.rs	64.15% <ø> (+1.18%)	⬆️
... and 14 more

[kazk]

Why was this changed?

[kazk]

I think we can clean this up to:

        let context_name = context
            .or_else(|| config.current_context.as_ref())
            .ok_or(KubeconfigError::CurrentContextNotSet)?;

[goenning]

That was removed because current-context can be null/empty, at least that's how it works on kubectl. Do you think we should not change this in kube-rs?

[kazk]

It can be null/empty when parsing, but should return an error when using the config, right?

[goenning]

It depends, if we want similar behavior as client-go/kubectl, then an empty current-context is a valid value, because context names can be empty too.

[kazk]

Confirmed kubectl works with a context with empty name when current context is empty.

We should align with client-go (library), and not kubectl (app), though.

---

• client-go's Config struct. (The link in file_config.rs is pointing to the struct after the conversion.)
• conversion from array of named objects to map. Errors on duplicate name. Missing name defaults to empty string because it's Go (nullable uses a pointer type).
• client-go has a validation to reject empty string context name. kubectl seems to work.
• What to do when there's no matching context. client-go has a validation to reject? kubectl seems to use a deprecated default?

We should probably use default value when missing instead of Option to align with client-go's Config as much as possible. Existing Option<String> fields should be fixed if it doesn't match client-go for consistency (not in this PR). We should also follow omitempty tags and skip serializing empty.

The problem is allowing null. Do we really need to? It just feels weird to me, and I don't think this happens in practice.

[goenning]

Maybe kubectl doesn't call ConfirmUsable?

---

I think this topic requires a bit more discussion and I don't want to rush into a solution, but I'd like to get the other changes on this PR merged soon.

As empty/null names are very edge cases scenarios, I'd like to propose that:

• revert changes to "name" fields and keep them as required Strings
• create an issue to discuss how do we want to handle those scenarios and implement that on a subsequent PR

What you think?

[kazk]

Yeah, let's do that.

[clux]

It's possible that all this lax parsing is due to supporting config merges. Configs that are invalid in isolation (e.g. no current context) are fine in the merged setting (where another file sets the current-context).

[goenning]

OK, I've reverted a bunch of changes so the PR is a lot simpler now. Still open for feedback! Thanks