gcp automatic default credentials #394

timothysmith0609 · 2019-02-19T18:58:40Z

Follow-up of #392

Adds a check for gcp auth provider and defaults to using GoogleApplicationDefaultCredentials.
Also added a specific error if googleauth gem isn't present on the importing application.
Brought in Mocha as a dev dependency for mocking. I can use a bare mock if you prefer
Add a unit test + test kubeconfig file

cc @cben

tsontario · 2019-02-20T02:38:47Z

Metrics/PerceivedComplexity: Perceived complexity for fetch_user_auth_options is too high. [8/7]

This rubocop violation feels a bit too strict for me

lib/kubeclient/google_application_default_credentials.rb

cben · 2019-02-20T07:47:51Z

Looks great! 👏
No objection to mocha.
Yeah, rubocop is too draconian. Feel free to bump PerceivedComplexity limit to what you consider sane in .rubocop.yml. ⬆️
Can you add a note in CHANGELOG?

timothysmith0609

CHANGELOG updated and wrapped the LoadError warning into a reraised contextual error. This is looking good to go.

timothysmith0609 · 2019-02-20T15:57:02Z

lib/kubeclient/config.rb

-        options[:bearer_token] = Kubeclient::ExecCredentials.token(exec_opts)
+    if user.key?('token')
+      options[:bearer_token] = user['token']
+    elsif user.key?('exec')


Moved exec above the gcp check in anticipation of its future release (though I can't seem to track down anything concrete about this). Might be unnecessary future-proofing but I don't see the harm here

remove hidden file test + mocha + gcp rubocop cleanup

cben · 2019-02-21T14:36:25Z

rebased to solve conflict, fixed some indentation, force-pushed your branch.

rubocop warns "Lint/InheritException: Inherit from RuntimeError instead of LoadError."
Introduced rubocop/rubocop#3427, motivation seems to be that default rescue copes with StandardError and won't catch this (and specific preference for RuntimeError is just that's a safer change rubocop/rubocop#3127 (comment)).
Since this was LoadError before, nothing changes and GoogleDependencyError < LoadError is fine 👍

rubocop warns "Lint/InheritException: Inherit from RuntimeError instead of LoadError." Check introduced rubocop/rubocop#3427, motivation seems to be that default `rescue` copes with StandardError and won't catch this (and specific preference for RuntimeError is just that's a safer change rubocop/rubocop#3127 (comment)). Since this was LoadError before, nothing changes and this is OK.

cben · 2019-02-21T15:00:19Z

Thanks LGTM 💯

I just pushed an update to the README, can you please review that?

cben · 2019-02-21T15:01:26Z

cc @jeremywadsack, if you still care about gcp, please review (at least the README)

cben · 2019-02-21T15:03:26Z

README.md

+#### Google's Application Default Credentials
+
+On Google Compute Engine, Google App Engine, or Google Cloud Functions, as well as `gcloud`-configured systems
+with [application default credentials](https://developers.google.com/identity/protocols/application-default-credentials),


Can we claim all these 4 scenarios (GCE, AppEngine, Functions, gcloud) should work out of the box?
I wasn't sure so wrote "if ... kubeconfig file has user: {auth-provider: {name: gcp}}" to be safe...

All those systems are documented by Google to include application default credentials.

If the environment variable isn't set, ADC uses the default service account that Compute Engine, Kubernetes Engine, App Engine, and Cloud Functions provide, for applications that run on those services.

https://cloud.google.com/docs/authentication/production#providing_credentials_to_your_application

FYI, I think the link above should be updated to this: https://cloud.google.com/docs/authentication/production#providing_credentials_to_your_application (which is where it redirects)

cben · 2019-02-21T15:05:26Z

README.md

+```
+
+Note that this returns token good for one hour. If your code runs for longer than that, you should plan to
+acquire a new one.


Q: how do you acquire a new one, in the automagic Config gcp scenario?
Calling .context again will do this, right?

Call Kubeclient::GoogleApplicationDefaultCredentials.token again.

I think we've discussed before that kubeclient doesn't have any support for renewing tokens. Is that something that's now covered with exec functionality?

Calling .context will generate a new token, since it will end up calling Kubeclient::GoogleApplicationDefaultCredentials.token, as Jeremy points out. I think the farthest we should go in that direction is to capture the error, if possible, and let the caller handle it.

README.md

jeremywadsack · 2019-02-21T18:20:50Z

@timothysmith0609 @cben I'm really glad to see this change. I think that having the gem "just work" in many environments makes it easier for people to use. It helps people get started with the gem and then they can tune more if they need to.

Tangentially, I would suggest that we configure this to "just work" inside a Kubernetes cluster. That is detect and implement the code in the README section "Inside a Kubernetes Cluster" when no other authorization is provided.

timothysmith0609 · 2019-02-22T14:26:38Z

That is detect and implement the code in the README section "Inside a Kubernetes Cluster" when no other authorization is provided.

That sounds like a good idea. Looking at how client-go implements rest.InClusterConfig, looks like we can just lift that wholesale. In fact, that's pretty much what I've seen most apps doing (unsurprisingly).

I propose to push that work out to a separate issue, however, and keep this change squarely scoped to just the gcp change. I think adding the in-cluster ability as a new method on Kubeclient::Config should be sufficient, and we'll leave the responsibility of calling the appropriate method to the caller (since sometimes clients are run in-cluster, but with a mounted kubeconfig to speak with other clusters). This has the nice benefit of mirroring the conventions of the client-go package as well

Co-Authored-By: timothysmith0609 <31742287+timothysmith0609@users.noreply.github.com>

timothysmith0609 · 2019-02-22T14:49:24Z

@cben README changes seem good to me (made a minor edit for clarity re: LoadError). Are there any outstanding concerns? As mentioned above, @jeremywadsack 's suggestion to provide a simple mechanism for in-cluster config is a good idea (and the next logical step); let's push that out into the next issue.

jeremywadsack · 2019-02-22T15:54:59Z

@timothysmith0609 Agree that in-cluster should be a separate PR. For reference see also our gem that implements this (in ruby): https://github.com/keylimetoolbox/resque-kubernetes/blob/v2.0.0/lib/resque/kubernetes/context/well_known.rb

And yes, we should do this as a last resort. If there's a provided configuration use that; if not, check for GCP; final step is to check for in-cluster.

cben · 2019-02-24T15:44:14Z

Hi, was sick for a few days, thanks for the polish 😄
Added a test that .context() does call .token (and .auth_options doesn't) and documented this.
Going to merge & release.

cben · 2019-02-24T15:53:44Z

Opened #395 for in-cluster config.
Opened #393 for authentication renewal. I'll brain-dump my thoughts there later, but any ideas welcome.

cben · 2019-03-03T11:09:32Z

released in gem 4.3.0.

ManageIQ/kubeclient#394 (comment)

cben reviewed Feb 20, 2019

View reviewed changes

lib/kubeclient/google_application_default_credentials.rb Outdated Show resolved Hide resolved

timothysmith0609 commented Feb 20, 2019

View reviewed changes

timothysmith0609 added 6 commits February 21, 2019 16:21

gcp automatic default credentials

8fe08a0

remove hidden file test + mocha + gcp rubocop cleanup

dig not avail on 2.2

8bd3200

bump cyclomaticcomplexity limit

e771cfa

Add rubocop directive for perceived complexity

d0ad476

check exec key first, update CHANGELOG

f2aa932

subclass loaderror for googleauth

0e0b596

cben force-pushed the gcp_default_auth branch from ebfe94b to 0e0b596 Compare February 21, 2019 14:32

cben added 2 commits February 21, 2019 16:39

Update README about gcp auth-provider

9d2ece7

cben reviewed Feb 21, 2019

View reviewed changes

jeremywadsack reviewed Feb 21, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

jeremywadsack and others added 2 commits February 22, 2019 09:32

Update README.md

bdab5d3

Co-Authored-By: timothysmith0609 <31742287+timothysmith0609@users.noreply.github.com>

small README edit

d18ab1a

gcp auth: Add a test that each .context() obtains new token

0cbf5db

cben merged commit d0248c1 into ManageIQ:master Feb 24, 2019

cben mentioned this pull request Feb 24, 2019

Make in-cluster configuration easy #395

Open

This was referenced Feb 24, 2019

Automatically support GCP ApplicationDefaultCredentials #392

Closed

Implement OIDC Auth Provider #396

Merged

Don't promise that Config#context is what acts on exec:/auth-provider: #400

Merged

Release 4.3.0 #401

Merged

jeremywadsack mentioned this pull request Apr 9, 2019

Authentication from "gcp" provider #210

Closed

benlangfeld added a commit to powerhome/krane that referenced this pull request Apr 16, 2019

Removes special treatment of GCP authentication

7fe393f

ManageIQ/kubeclient#394 (comment)

benlangfeld mentioned this pull request Apr 16, 2019

Removes special treatment of GCP authentication Shopify/krane#465

Merged

benlangfeld added a commit to powerhome/krane that referenced this pull request Apr 26, 2019

Removes special treatment of GCP authentication

fcef44e

ManageIQ/kubeclient#394 (comment)

KnVerey pushed a commit to Shopify/krane that referenced this pull request Apr 30, 2019

Removes special treatment of GCP authentication

54f7cd2

ManageIQ/kubeclient#394 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcp automatic default credentials #394

gcp automatic default credentials #394

timothysmith0609 commented Feb 19, 2019 •

edited

Loading

tsontario commented Feb 20, 2019

cben commented Feb 20, 2019

timothysmith0609 left a comment

timothysmith0609 Feb 20, 2019

cben commented Feb 21, 2019 •

edited

Loading

cben commented Feb 21, 2019

cben commented Feb 21, 2019

cben Feb 21, 2019

jeremywadsack Feb 21, 2019 •

edited

Loading

jeremywadsack Feb 21, 2019 •

edited

Loading

cben Feb 21, 2019

jeremywadsack Feb 21, 2019

timothysmith0609 Feb 22, 2019

jeremywadsack commented Feb 21, 2019

timothysmith0609 commented Feb 22, 2019

timothysmith0609 commented Feb 22, 2019

jeremywadsack commented Feb 22, 2019

cben commented Feb 24, 2019

cben commented Feb 24, 2019

cben commented Mar 3, 2019

gcp automatic default credentials #394

gcp automatic default credentials #394

Conversation

timothysmith0609 commented Feb 19, 2019 • edited Loading

tsontario commented Feb 20, 2019

cben commented Feb 20, 2019

timothysmith0609 left a comment

Choose a reason for hiding this comment

timothysmith0609 Feb 20, 2019

Choose a reason for hiding this comment

cben commented Feb 21, 2019 • edited Loading

cben commented Feb 21, 2019

cben commented Feb 21, 2019

cben Feb 21, 2019

Choose a reason for hiding this comment

jeremywadsack Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

jeremywadsack Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

cben Feb 21, 2019

Choose a reason for hiding this comment

jeremywadsack Feb 21, 2019

Choose a reason for hiding this comment

timothysmith0609 Feb 22, 2019

Choose a reason for hiding this comment

jeremywadsack commented Feb 21, 2019

timothysmith0609 commented Feb 22, 2019

timothysmith0609 commented Feb 22, 2019

jeremywadsack commented Feb 22, 2019

cben commented Feb 24, 2019

cben commented Feb 24, 2019

cben commented Mar 3, 2019

timothysmith0609 commented Feb 19, 2019 •

edited

Loading

cben commented Feb 21, 2019 •

edited

Loading

jeremywadsack Feb 21, 2019 •

edited

Loading

jeremywadsack Feb 21, 2019 •

edited

Loading