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

Generate GCP tokens through cmd-path config #410

Merged
merged 1 commit into from May 2, 2019

Conversation

lucasmazza
Copy link
Contributor

@lucasmazza lucasmazza commented Apr 17, 2019

This commit pushes the Kubeclient::GoogleApplicationDefaultCredentials implementation down to a GCPAuthProvider that handle both code paths of how to generate GCP tokens.

Related to #210 (comment) and #210 (comment)

EDIT(cben): relevant docs in k8s Go source:
https://github.com/kubernetes/kubernetes/blob/v1.14.1/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go#L58-L109

@cben
Copy link
Collaborator

cben commented Apr 22, 2019

It's a bit sad cmd-path generally duplicates generic exec functionality (#363). Do you happen to know if configs emitted by gcloud will start using exec in future?

Anyway, thanks 👍, generally looks sane, but I'm too tired to merge atm 😴 will revisit soon when I'm back from vacation.

if config.key?('cmd-path')
Kubeclient::GCPCommandCredentials.token(config)
else
Kubeclient::GoogleApplicationDefaultCredentials.token
Copy link
Contributor

Choose a reason for hiding this comment

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

In the interests of hardening, would you be opposed to adding retries here? We have begun observing intermittent Signet::RemoteServerError exceptions that, according to Google, we can/should retry on.

I don't think pushing up retryable errors to the caller is the right thing to do; we should handle them here and only throw once we've exhausted our retry limit.

Full error we observe:

"error": {
  "code": 503,
  "message": "The service is currently unavailable.",
  "status": "UNAVAILABLE"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timothysmith0609 sounds like something that the googleauth gem should be dealing with, not kubeclient itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're absolutely right. It turns out googleauth does retry, but only when using the GCE node service account (no idea why that's the case). It looks like it should be possible to wrap the other case in their retry routine so I'll try to solve this upstream.

@lucasmazza
Copy link
Contributor Author

Do you happen to know if configs emitted by gcloud will start using exec in future?

I have no clue :( all my knowledge on this space is due source diving and local testing. Would be awesome to have a person/channel on the k8s side of things to help explain these authentication strategies for client libraries.

@cben
Copy link
Collaborator

cben commented Apr 28, 2019

Sorry for delay. Re-read, looks good 👍. But remembered one requirement:
This should support the opt-out we have, like #375. I don't know if anybody has use cases for it, but I feel on general principles libraries should support a "safe mode" separating "parse this config" from "act on this config"...

And a related question (not merge blocker, but worth figuring out): what if cmd-path is a relative/path/to/gcloud? What if it's just gcloud with no path? Is it correct to look it up relative to current working directory, or should it be relative to kubeconfig? Is it correct to do $PATH lookup?
Does that even matter — maybe gcloud always generates configs with absolute path?

Is there a Go reference implementation to compare?
https://github.com/kubernetes/kubernetes/blob/v1.14.1/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go#L87-L90
Hmm, doesn't say and I don't see any special logic for to relative paths, the command and args are fed as-is into exec.Command() which handles $PATH, absolute paths, and relative to cwd.
https://github.com/kubernetes/kubernetes/blob/v1.14.1/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go#L298
So it's almost like ext_command_path() behavior from #375 for exec, except relative to cwd rather than config file's directory.

(Code-wise, you could "push up" the if from GCPAuthProvider back into Config#fetch_user_auth_options, then you can reuse allow_external_lookups? and/or ext_command_path(). Or pass some param down to GCPAuthProvider...)

@cben
Copy link
Collaborator

cben commented Apr 28, 2019

(Historical note: The specific way one opts out — constructing Config directly with base directory arg set to nil — is pretty arbitrary, originally grew out of "opt out from reading files from file system", and involved a couple mistakes on my part. I documented it before it actually worked 🤣 and then stuck to it 🤷‍♂️)

This commit pushes the `Kubeclient::GoogleApplicationDefaultCredentials`
implementation down to a `GCPAuthProvider` that handle both code paths of
how to generate GCP tokens.
@lucasmazza
Copy link
Contributor Author

@cben thanks for the review, the code is now using the ext_command_path around the cmd-path config

Copy link
Collaborator

@cben cben left a comment

Choose a reason for hiding this comment

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

Neat!

@cben cben merged commit 56c55d9 into ManageIQ:master May 2, 2019
@lucasmazza
Copy link
Contributor Author

@cben thanks for the merge, let me know when you cut a new release so I can get shopify/kubernetes-deploy up to date with this ✨

cben added a commit to cben/kubeclient that referenced this pull request May 3, 2019
@cben cben mentioned this pull request May 3, 2019
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

3 participants