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

provide rustls implementation to sign request to gke #123

Merged
merged 1 commit into from Feb 9, 2020

Conversation

davidB
Copy link
Contributor

@davidB davidB commented Feb 8, 2020

related #120

I refactor a bit the code:

  • to reduce / to remove the duplication between fragment for native-tls and rustls-tls
  • to test the signing function

for my quick test native-tls and rustls-tls behave the same, but

  • I fail to use rustls as is with my test gke server because the server url was with an ip (like https://35.187.173.xxx/) and that is not supported by rustls see Missing support for ipv4/ipv6 subjects · Issue #56 · ctz/hyper-rustls (I workaround by modifing etc/hosts and .kube/config to use the url https://kubernetes.default.
  • require GOOGLE_APPLICATION_CREDENTIALS is not friendly and the behavior of kubectl (nor go-client), I will investigate to make a PR later to fix that
  • I was not able to connect my cluster after expiration of my token (same error by both tls)

So I guess that the lib is not usable as is with gke

FYI, currently in my kubectl plugin, I don't let kube-rs manage token (and refresh) because

  • it doesn't update .kube/config after refresh (and that cause trouble with kubectl (and other
    plugin))
  • it doesn't handle well some case
    So I call kubectl cluster-infobefore.
async fn load_kube_config() -> Result<config::Configuration> {
    //HACK force refresh token by calling "kubectl cluster-info before loading configuration"
    use std::process::Command;
    let output = Command::new("kubectl")
        .arg("cluster-info")
        .output()
        .with_context(|| "failed to executed 'kubectl cluster-info'")?;
    if !output.status.success() {
        return Err(anyhow!("`kubectl cluster-info` failed with: {:?}", &output));
    }
    config::load_kube_config()
        .await
        .with_context(|| "failed to load kubeconfig")
}

Sorry for long text

@clux
Copy link
Member

clux commented Feb 9, 2020

I refactor a bit the code to reduce / to remove the duplication

It's very good, and looks clean. Thank you so much. Have tested it against EKS and will merge right away.

Oh wow. That's annoying. Thanks for finding the root issue.

  • require GOOGLE_APPLICATION_CREDENTIALS is not friendly and the behavior of kubectl (nor go-client), I will investigate to make a PR later to fix that

👍

FYI, currently in my kubectl plugin, I don't let kube-rs manage token (and refresh) because it doesn't update .kube/config after refresh (and that cause trouble with kubectl (and other plugin)) [..] so I call kubectl cluster-infobefore.

Do you feel that perhaps we should patch the ~/.kube/config file with refresh tokens?

@clux clux merged commit 7e7e2aa into kube-rs:master Feb 9, 2020
@clux
Copy link
Member

clux commented Feb 27, 2020

Opened part of your workaround as a bug on here: #153

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.

None yet

2 participants