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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from Dec 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
935c554
fix: align kube-rs with client-go config parsing
goenning Nov 13, 2022
d5f1325
remove serde_with, make field optional
goenning Nov 15, 2022
af53d50
remove commented test
goenning Nov 15, 2022
57f79ad
remove unnecessary default
goenning Nov 15, 2022
42c92fd
adjust based on PR review
goenning Nov 16, 2022
5964333
command and server are optional
goenning Nov 18, 2022
071bfd5
name on named structs is optional
goenning Nov 18, 2022
447e546
remove CurrentContextNotSet error
goenning Nov 18, 2022
772e9e1
run fmt
goenning Nov 18, 2022
a4bbc97
revert named strings to be mandatory
goenning Nov 24, 2022
850fb5b
other PR comments
goenning Nov 24, 2022
d86c2c6
revert
goenning Dec 3, 2022
1f51e4e
revert
goenning Dec 3, 2022
8ecbd8f
Merge branch 'main' into go/kubeconfig-default
clux Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions kube-client/src/client/auth/mod.rs
View file
Open in desktop
Expand Up @@ -76,6 +76,10 @@ pub enum Error {
#[error("failed to parse token-key")]
ParseTokenKey(#[source] serde_json::Error),

/// command was missing from exec config
#[error("command must be specified to use exec authentication plugin")]
MissingCommand,

/// OAuth error
#[cfg(feature = "oauth")]
#[cfg_attr(docsrs, doc(cfg(feature = "oauth")))]
Expand Down Expand Up @@ -461,7 +465,11 @@ pub struct ExecCredentialStatus {
}

fn auth_exec(auth: &ExecConfig) -> Result<ExecCredential, Error> {
let mut cmd = Command::new(&auth.command);
let mut cmd = match &auth.command {
Some(cmd) => Command::new(cmd),
None => return Err(Error::MissingCommand),
};

if let Some(args) = &auth.args {
cmd.args(args);
}
Expand Down Expand Up @@ -533,7 +541,7 @@ mod test {
);

let config: Kubeconfig = serde_yaml::from_str(&test_file).unwrap();
let auth_info = &config.auth_infos[0].auth_info;
let auth_info = config.auth_infos[0].auth_info.as_ref().unwrap();
match Auth::try_from(auth_info).unwrap() {
Auth::RefreshableToken(RefreshableToken::Exec(refreshable)) => {
let (token, _expire, info) = Arc::try_unwrap(refreshable).unwrap().into_inner();
Expand Down
131 changes: 81 additions & 50 deletions kube-client/src/config/file_config.rs
View file
Open in desktop
Expand Up @@ -66,27 +66,31 @@ pub struct Preferences {
#[cfg_attr(test, derive(PartialEq, Eq))]
pub struct NamedExtension {
/// Name of extension
pub name: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub name: Option<String>,
goenning marked this conversation as resolved.
Show resolved Hide resolved
/// Additional information for extenders so that reads and writes don't clobber unknown fields
pub extension: serde_json::Value,
}

/// NamedCluster associates name with cluster.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize, Default)]
#[cfg_attr(test, derive(PartialEq, Eq))]
pub struct NamedCluster {
/// Name of cluster
pub name: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub name: Option<String>,
/// Information about how to communicate with a kubernetes cluster
pub cluster: Cluster,
#[serde(skip_serializing_if = "Option::is_none")]
goenning marked this conversation as resolved.
Show resolved Hide resolved
pub cluster: Option<Cluster>,
}

/// Cluster stores information to connect Kubernetes cluster.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize, Default)]
#[cfg_attr(test, derive(PartialEq, Eq))]
pub struct Cluster {
/// The address of the kubernetes cluster (https://hostname:port).
pub server: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub server: Option<String>,
/// Skips the validity check for the server's certificate. This will make your HTTPS connections insecure.
#[serde(rename = "insecure-skip-tls-verify")]
#[serde(skip_serializing_if = "Option::is_none")]
Expand All @@ -109,14 +113,16 @@ pub struct Cluster {
}

/// NamedAuthInfo associates name with authentication.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize, Default)]
#[cfg_attr(test, derive(PartialEq))]
pub struct NamedAuthInfo {
/// Name of the user
pub name: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub name: Option<String>,
/// Information that describes identity of the user
#[serde(rename = "user")]
pub auth_info: AuthInfo,
#[serde(skip_serializing_if = "Option::is_none")]
pub auth_info: Option<AuthInfo>,
}

fn serialize_secretstring<S>(pw: &Option<SecretString>, serializer: S) -> Result<S::Ok, S::Error>
Expand All @@ -133,8 +139,9 @@ fn deserialize_secretstring<'de, D>(deserializer: D) -> Result<Option<SecretStri
where
D: Deserializer<'de>,
{
match String::deserialize(deserializer) {
Ok(secret) => Ok(Some(SecretString::new(secret))),
match Option::<String>::deserialize(deserializer) {
Ok(Some(secret)) => Ok(Some(SecretString::new(secret))),
Ok(None) => Ok(None),
Err(e) => Err(e),
}
}
Expand Down Expand Up @@ -218,8 +225,10 @@ impl PartialEq for AuthInfo {
#[cfg_attr(test, derive(PartialEq, Eq))]
pub struct AuthProviderConfig {
/// Name of the auth provider
#[serde(default)]
goenning marked this conversation as resolved.
Show resolved Hide resolved
pub name: String,
/// Auth provider configuration
#[serde(default)]
pub config: HashMap<String, String>,
}

Expand All @@ -234,7 +243,8 @@ pub struct ExecConfig {
#[serde(skip_serializing_if = "Option::is_none")]
pub api_version: Option<String>,
/// Command to execute.
pub command: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub command: Option<String>,
/// Arguments to pass to the command when executing it.
#[serde(skip_serializing_if = "Option::is_none")]
pub args: Option<Vec<String>>,
Expand All @@ -252,23 +262,27 @@ pub struct ExecConfig {
}

/// NamedContext associates name with context.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize, Default)]
#[cfg_attr(test, derive(PartialEq, Eq))]
pub struct NamedContext {
/// Name of the context
pub name: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub name: Option<String>,
/// Associations for the context
pub context: Context,
#[serde(skip_serializing_if = "Option::is_none")]
pub context: Option<Context>,
}

/// Context stores tuple of cluster and user information.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize, Default)]
#[cfg_attr(test, derive(PartialEq, Eq))]
pub struct Context {
/// Name of the cluster for this context
pub cluster: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub cluster: Option<String>,
/// Name of the `AuthInfo` for this context
pub user: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub user: Option<String>,
/// The default namespace to use on unspecified requests
#[serde(skip_serializing_if = "Option::is_none")]
pub namespace: Option<String>,
Expand All @@ -291,26 +305,30 @@ impl Kubeconfig {
for mut config in kubeconfig_from_yaml(&data)? {
if let Some(dir) = path.as_ref().parent() {
for named in config.clusters.iter_mut() {
if let Some(path) = &named.cluster.certificate_authority {
if let Some(abs_path) = to_absolute(dir, path) {
named.cluster.certificate_authority = Some(abs_path);
if let Some(cluster) = &mut named.cluster {
if let Some(path) = &cluster.certificate_authority {
if let Some(abs_path) = to_absolute(dir, path) {
cluster.certificate_authority = Some(abs_path);
}
}
}
}
for named in config.auth_infos.iter_mut() {
if let Some(path) = &named.auth_info.client_certificate {
if let Some(abs_path) = to_absolute(dir, path) {
named.auth_info.client_certificate = Some(abs_path);
if let Some(auth_info) = &mut named.auth_info {
if let Some(path) = &auth_info.client_certificate {
if let Some(abs_path) = to_absolute(dir, path) {
auth_info.client_certificate = Some(abs_path);
}
}
}
if let Some(path) = &named.auth_info.client_key {
if let Some(abs_path) = to_absolute(dir, path) {
named.auth_info.client_key = Some(abs_path);
if let Some(path) = &auth_info.client_key {
if let Some(abs_path) = to_absolute(dir, path) {
auth_info.client_key = Some(abs_path);
}
}
}
if let Some(path) = &named.auth_info.token_file {
if let Some(abs_path) = to_absolute(dir, path) {
named.auth_info.token_file = Some(abs_path);
if let Some(path) = &auth_info.token_file {
if let Some(abs_path) = to_absolute(dir, path) {
auth_info.token_file = Some(abs_path);
}
}
}
}
Expand Down Expand Up @@ -414,7 +432,7 @@ fn kubeconfig_from_yaml(text: &str) -> Result<Vec<Kubeconfig>, KubeconfigError>
#[allow(clippy::redundant_closure)]
fn append_new_named<T, F>(base: &mut Vec<T>, next: Vec<T>, f: F)
where
F: Fn(&T) -> &String,
F: Fn(&T) -> &Option<String>,
{
use std::collections::HashSet;
base.extend({
Expand Down Expand Up @@ -524,31 +542,31 @@ mod tests {
let kubeconfig1 = Kubeconfig {
current_context: Some("default".into()),
auth_infos: vec![NamedAuthInfo {
name: "red-user".into(),
auth_info: AuthInfo {
name: Some("red-user".into()),
auth_info: Some(AuthInfo {
token: Some(SecretString::from_str("first-token").unwrap()),
..Default::default()
},
}),
}],
..Default::default()
};
let kubeconfig2 = Kubeconfig {
current_context: Some("dev".into()),
auth_infos: vec![
NamedAuthInfo {
name: "red-user".into(),
auth_info: AuthInfo {
name: Some("red-user".into()),
auth_info: Some(AuthInfo {
token: Some(SecretString::from_str("second-token").unwrap()),
username: Some("red-user".into()),
..Default::default()
},
}),
},
NamedAuthInfo {
name: "green-user".into(),
auth_info: AuthInfo {
name: Some("green-user".into()),
auth_info: Some(AuthInfo {
token: Some(SecretString::from_str("new-token").unwrap()),
..Default::default()
},
}),
},
],
..Default::default()
Expand All @@ -558,19 +576,21 @@ mod tests {
// Preserves first `current_context`
assert_eq!(merged.current_context, Some("default".into()));
// Auth info with the same name does not overwrite
assert_eq!(merged.auth_infos[0].name, "red-user".to_owned());
assert_eq!(merged.auth_infos[0].name.as_ref().unwrap(), "red-user");
assert_eq!(
merged.auth_infos[0]
.auth_info
.as_ref()
.unwrap()
.token
.as_ref()
.map(|t| t.expose_secret().to_string()),
Some("first-token".to_string())
);
// Even if it's not conflicting
assert_eq!(merged.auth_infos[0].auth_info.username, None);
assert_eq!(merged.auth_infos[0].auth_info.as_ref().unwrap().username, None);
// New named auth info is appended
assert_eq!(merged.auth_infos[1].name, "green-user".to_owned());
assert_eq!(merged.auth_infos[1].name.as_ref().unwrap(), "green-user");
}

#[test]
Expand Down Expand Up @@ -632,10 +652,10 @@ users:

let config = Kubeconfig::from_yaml(config_yaml).unwrap();

assert_eq!(config.clusters[0].name, "eks");
assert_eq!(config.clusters[1].name, "minikube");
assert_eq!(config.clusters[0].name.as_ref().unwrap(), "eks");
assert_eq!(config.clusters[1].name.as_ref().unwrap(), "minikube");
assert_eq!(
config.clusters[1].cluster.extensions.as_ref().unwrap()[0]
config.clusters[1].cluster.as_ref().unwrap().extensions.as_ref().unwrap()[0]
.extension
.get("provider"),
Some(&Value::String("minikube.sigs.k8s.io".to_owned()))
Expand Down Expand Up @@ -688,8 +708,8 @@ users:
let cfg = Kubeconfig::from_yaml(config_yaml)?;

// Ensure we have data from both documents:
assert_eq!(cfg.clusters[0].name, "k3d-promstack");
assert_eq!(cfg.clusters[1].name, "k3d-k3s-default");
assert_eq!(cfg.clusters[0].name.as_ref().unwrap(), "k3d-promstack");
assert_eq!(cfg.clusters[1].name.as_ref().unwrap(), "k3d-k3s-default");
goenning marked this conversation as resolved.
Show resolved Hide resolved

Ok(())
}
Expand All @@ -701,6 +721,17 @@ users:
assert_eq!(cfg, Kubeconfig::default());
}

#[test]
fn authinfo_deserialize_null_secret() {
let authinfo_yaml = r#"
username: user
password:
"#;
let authinfo: AuthInfo = serde_yaml::from_str(authinfo_yaml).unwrap();
assert_eq!(authinfo.username, Some("user".to_string()));
assert!(authinfo.password.is_none());
}

#[test]
fn authinfo_debug_does_not_output_password() {
let authinfo_yaml = r#"
Expand Down
28 changes: 15 additions & 13 deletions kube-client/src/config/file_loader.rs
View file
Open in desktop
Expand Up @@ -59,40 +59,42 @@ impl ConfigLoader {
cluster: Option<&String>,
user: Option<&String>,
) -> Result<Self, KubeconfigError> {
let current_context_name = &config.current_context.clone().unwrap_or_default();
let context_name = if let Some(name) = context {
name
} else if let Some(name) = &config.current_context {
name
} else {
return Err(KubeconfigError::CurrentContextNotSet);
current_context_name
};
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can clean this up to:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@kazk kazk Nov 22, 2022 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's do that.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


kazk marked this conversation as resolved.
Show resolved Hide resolved
let current_context = config
.contexts
.iter()
.find(|named_context| &named_context.name == context_name)
.map(|named_context| &named_context.context)
.find(|named_context| &named_context.name.clone().unwrap_or_default() == context_name)
goenning marked this conversation as resolved.
Show resolved Hide resolved
.and_then(|named_context| named_context.context.clone())
.ok_or_else(|| KubeconfigError::LoadContext(context_name.clone()))?;

let cluster_name = cluster.unwrap_or(&current_context.cluster);
let current_cluster_name = &current_context.cluster.clone().unwrap_or_default();
let cluster_name = cluster.unwrap_or(current_cluster_name);
goenning marked this conversation as resolved.
Show resolved Hide resolved
let cluster = config
.clusters
.iter()
.find(|named_cluster| &named_cluster.name == cluster_name)
.map(|named_cluster| &named_cluster.cluster)
.find(|named_cluster| &named_cluster.name.clone().unwrap_or_default() == cluster_name)
.and_then(|named_cluster| named_cluster.cluster.clone())
.ok_or_else(|| KubeconfigError::LoadClusterOfContext(cluster_name.clone()))?;

let user_name = user.unwrap_or(&current_context.user);
let current_user_name = &current_context.user.clone().unwrap_or_default();
let user_name = user.unwrap_or(&current_user_name);
let user = config
.auth_infos
.iter()
.find(|named_user| &named_user.name == user_name)
.map(|named_user| &named_user.auth_info)
.find(|named_user| &named_user.name.clone().unwrap_or_default() == user_name)
.and_then(|named_user| named_user.auth_info.clone())
.ok_or_else(|| KubeconfigError::FindUser(user_name.clone()))?;

Ok(ConfigLoader {
current_context: current_context.clone(),
cluster: cluster.clone(),
user: user.clone(),
cluster,
user,
})
}

Expand Down
6 changes: 2 additions & 4 deletions kube-client/src/config/mod.rs
View file
Open in desktop
Expand Up @@ -29,10 +29,6 @@ pub struct InferConfigError {
/// Possible errors when loading kubeconfig
#[derive(Error, Debug)]
pub enum KubeconfigError {
/// Failed to determine current context
#[error("failed to determine current context")]
CurrentContextNotSet,

/// Kubeconfigs with mismatching kind cannot be merged
#[error("kubeconfigs with mismatching kind cannot be merged")]
KindMismatch,
Expand Down Expand Up @@ -299,6 +295,8 @@ impl Config {
let cluster_url = loader
.cluster
.server
.clone()
.unwrap_or_default()
goenning marked this conversation as resolved.
Show resolved Hide resolved
.parse::<http::Uri>()
.map_err(KubeconfigError::ParseClusterUrl)?;

Expand Down