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

kube-config - Allow arbitrary 'extension' objects #425

Merged
merged 3 commits into from
Feb 22, 2021

Conversation

kppullin
Copy link
Contributor

Prior to this change the type of NamedExtension.extension was String.

However, the k8s golang type defines this as a runtime.RawExtension,
which allows for setting arbitrary objects as the value of the extension
key.

For example, minikube sets the value to:

- extension:
     last-update: 'Thu, 18 Feb 2021 16:59:26 PST'
     provider: minikube.sigs.k8s.io
     version: v1.17.1
  name: context_info

When kube-rs tries to parse this as a String you'll end up with an error like this:

thread 'config::file_config::tests::kubeconfig_deserialize' panicked at 'called `Result::unwrap()`
on an `Err` value: ParseYaml(Message("invalid type: map, expected a string", 
Some(Pos { marker: Marker { index: 312, line: 12, col: 23 }, path: 
"clusters[1].cluster.extensions[0].extension" })))', kube/src/config/file_config.rs:407:14

This PR changes the type from String to serde_yaml::Value
to allow for parsing, reading, writing, and round-tripping any
valid yaml value.

Further, the Cluster struct is filled with all known existing fields defined here: https://github.com/kubernetes/client-go/blob/30548acd0a9e231d42d406433aef55b111b36240/tools/clientcmd/api/v1/types.go#L63 , with the extensions field most relevant to this PR as it is set by minikube.

Prior to this change the type of `NamedExtension.extension` was `String`.

However, the k8s golang type defines this as a `runtime.RawExtension`,
which allows for setting arbitrary objects as the value of the extension
key.

For example, minikube sets the value to:

```
- extension:
     last-update: 'Thu, 18 Feb 2021 16:59:26 PST'
     provider: minikube.sigs.k8s.io
     version: v1.17.1
  name: context_info
```

When `kube-rs` tries to parse this as a `String` you'll end up with an error like this:

```
thread 'config::file_config::tests::kubeconfig_deserialize' panicked at 'called `Result::unwrap()` on an `Err` value: ParseYaml(Message("invalid type: map, expected a string", Some(Pos { marker: Marker { index: 312, line: 12, col: 23 }, path: "clusters[1].cluster.extensions[0].extension" })))', kube/src/config/file_config.rs:407:14
```

This PR changes the type from `String` to `serde_yaml::Value`
to allow for parsing, reading, writing, and round-tripping any
valid yaml value."
Change the `extensions` type from `serde_yaml::Mapping` to
`serde_json::Value`.
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

small comments

#[serde(rename = "insecure-skip-tls-verify")]
pub insecure_skip_tls_verify: Option<bool>,
#[serde(rename = "certificate-authority")]
pub certificate_authority: Option<String>,
#[serde(rename = "certificate-authority-data")]
pub certificate_authority_data: Option<String>,
#[serde(rename = "proxy-url")]
pub proxy_url: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

oh, is this new? i didn't see any good info from a quick google. are we meant to actually proxy if this is set?

Copy link
Member

Choose a reason for hiding this comment

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

Technically we don't support proxying at the moment. We could include this in the config, but we should have a comment caveating that it's not used in our client yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be better to omit these for now? My goal here is to just avoid the panics with the minikube generated config : )

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably best to avoid it to minimise confusion until we have support for it :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -38,7 +38,7 @@ pub struct Preferences {
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct NamedExtension {
pub name: String,
pub extension: String,
pub extension: serde_json::Value,
Copy link
Member

Choose a reason for hiding this comment

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

Are there specs for this anywhere? It's probably fine to have that as dynamic, though it is technically a breaking change for previous String extensions.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't found much in the way of RawExtension specs beyond the links you've listed above and some of the references in this thread: kubernetes/apimachinery#102 and the recent PR for minikube that began using the extensions: kubernetes/minikube#10126 (which seems to have required a similar change to this one in the C# client: kubernetes-client/csharp#556)

@clux clux merged commit 6bffd12 into kube-rs:master Feb 22, 2021
@clux
Copy link
Member

clux commented Feb 28, 2021

Released in kube 0.51.0, thanks again!

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.

4 participants