WIP: Support GKE and OpenID Authentication #247
Conversation
|
@silasbw or whoever else, for OpenID support we're going to have to have this be async, what way would you like that to proceed? |
BREAKING: .fromKubeConfig: now is asynchronous and requires that you .then() or await it. This will break all uses of it.
|
OpenID support is implemented but... is async... 💀 💀 💀 Need to refactor this a bit as well. |
There was a problem hiding this comment.
This is awesome. ❤️
How often does refresh need to happen? If I have a long running controller, for example, that creates a client instance at startup, will that controller need to refreshAuth at some point? If so, another option might be to refresh on demand. E.g., fromKubeconfig stays sync, but the existing async methods (e.g., api.v1.namespaces.get()) do the check for expired auth and try to refresh it before executing the actual request.
Thoughts on the "on-demand" approach?
|
Oooooh that's a much better idea, I'll migrate it over to that. |
|
Cool. you probably figured it out, but i'd suggesting trying to add that logic in here: https://github.com/godaddy/kubernetes-client/blob/master/lib/request.js |
lib/auth-providers/cmd.js
Outdated
| try { | ||
| result = JSON.parse(output.stdout.toString('utf8')); | ||
| } catch (err) { | ||
| return reject('Failed to run cmd.'); |
There was a problem hiding this comment.
can we reject with an Error instance instead of the string?
There was a problem hiding this comment.
Oops, yeah will do.
lib/auth-providers/cmd.js
Outdated
|
|
||
| module.exports = { | ||
| refresh: function (config) { | ||
| return new Promise((resolv, reject) => { |
There was a problem hiding this comment.
is there any reason why it is not resolve but resolve?
There was a problem hiding this comment.
I've always been used to that style... then again haven't used non async/await in a longggg time. If this is really necessary I'll change it.
lib/auth-providers/openid.js
Outdated
| module.exports = { | ||
| refresh: function (config) { | ||
| return new Promise((resolv, reject) => { | ||
| Issuer.discover(config['idp-issuer-url']) // => Promise |
There was a problem hiding this comment.
do we need that Promise comment?
There was a problem hiding this comment.
Yeah it needs to be refactored, wrote this very quickly and it's going to be transformed into on-demand refresh in request.js. Thanks for the review!
lib/config.js
Outdated
| let auth; | ||
| if (config['token-key']) { | ||
| const token = getProperty(config['token-key'].replace(/[{}]+/g, ''), result); | ||
| auth = { |
There was a problem hiding this comment.
can we remove the whitespaces?
lib/config.js
Outdated
| } | ||
|
|
||
| const authProvider = user['auth-provider']; | ||
| if (authProvider) { |
There was a problem hiding this comment.
would it make sense to split this huge function into smaller ones?
|
@silasbw This is ready, LMK what changes are needed, otherwise LGTM! |
🎉
| // if we can't determine the type, just fail later (or don't refresh). | ||
| let type = null; | ||
| let token = null; | ||
| if (config['cmd-path']) { |
There was a problem hiding this comment.
Would this and https://github.com/godaddy/kubernetes-client/pull/247/files#diff-f7bf2b665273dfa66ffa4ad7c0a52bb9R111 ever both be true? if not, can you make that clear using else if below?
lib/request.js
Outdated
| return refreshAuth(auth.type, auth.config) | ||
| .then(newAuth => { | ||
| this.requestOptions.auth = newAuth; | ||
| return this.request(method, options, cb); |
There was a problem hiding this comment.
Can we call request directly (instead of the recursive call)? I worry about the case where the HTTP request keeps 401ing for some reason, and we end up in an infinite request loop / oom situation.
lib/auth-providers/openid.js
Outdated
| .then(tokenSet => { | ||
| return resolv(tokenSet.id_token); | ||
| }) | ||
| .catch(err => { |
|
OK, fixed as per your feedback @silasbw. Thanks!! |
|
Thanks again @jaredallard 🌶️ 🌶️ 🌶️ ! |
|
🎉🎉🎉 What's the timeline for this being pushed to npm? |
|
We'll do it today and post an update on #184 afterwards. |
Checklist
Saves new token to kube config(?)What does it do?
Implements a semi-standardized way of implementing
auth-providersand refreshes authentication on-demand (i.e 401)Fixes #184.