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

Enable "gcp" authentication structure for bearer token in .kube/config #211

Closed
wants to merge 3 commits into from

Conversation

jeremywadsack
Copy link
Contributor

@jeremywadsack jeremywadsack commented Nov 14, 2016

This adds the ability to parse the bearer token from ~/.kube/config
stored by Google Cloud Platform. This token is only good for an hour
since it was last renewed with a kubectl command. So if a 401 error
is returned adds a note to the exception message when the token is
expired.

Also updates other auth failure tests that were bypassing exception
parsing in Kubeclient::ClientMixing#handle_exception.

Addresses #210

This adds the ability to parse the bearer token from ~/.kube/config
stored by Google Cloud Platform. This token is only good for an hour
since it was last renewed with a `kubectl` command. So if a 401 error
is returned adds a note to the exception message when the token is
expired.

Also updates other auth failure tests that were bypassing exception
parsing in Kubeclient::ClientMixing#handle_exception.
@simon3z
Copy link
Collaborator

simon3z commented Nov 15, 2016

@moolitayer @enoodle can you review?

@@ -477,5 +482,16 @@ def http_options(uri)

options.merge(@socket_options)
end

def expired_token_message(http_code, message)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the error returned from the server in this case? I would prefer to keep mirroring the returned exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moolitayer intended to include the original message. That's why it passes in message and carries that through. Maybe I misunderstood your comment.

Another option would be to have a TokenExpiredException (or something) that takes the original exception as an argument and adds the expiration message. I thought this was a simpler approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be happier if we can avoid the check. Are you worried here that the user will not understand what is going on?

Copy link
Contributor

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

Looks good, pending the comment from @moolitayer

@@ -477,5 +482,16 @@ def http_options(uri)

options.merge(@socket_options)
end

def expired_token_message(http_code, message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing the name of the method to something like check_token_expiration as it adds to the mesasge.

Copy link
Contributor Author

@jeremywadsack jeremywadsack Nov 15, 2016

Choose a reason for hiding this comment

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

Good suggestion. I wasn't happy with the original name. Fixed 22912ce

@auth_options.key?(:bearer_token_expiry) &&
@auth_options[:bearer_token_expiry] < Time.now
return message + '. Your token has expired. If you are using `kubectl`, running any '\
'command should renew your token.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I don't understand this but it does not seem like a flow that a normal Kubeclient user would use.
(use library to talk to api while using command line tool to get authentication token). Can we do the same thing kubectl does to renew the token? what is needed for the kubectl to give back a token (meaning do you have to be pre authenticated against www.googleapis.com/auth/cloud-platform ?)

This seems relevant: https://github.com/kubernetes/kubernetes/blob/3e51ca2e16479763bc832e893e1b9ffa34310b7e/plugin/pkg/client/auth/gcp/gcp.go#L56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moolitayer I agree. That was my reason for opening #210 before opening a PR. I'd much prefer that I not have to run kubectl to get the token.

That link and the docs here[1] describe getting the default token (at least with Go). I looked at google-auth-library-ruby but don't see anything for the token, just for default credential. Digging into this issue[2] I wonder if we can just use the default credentials and bypass the config file completely.

Are you Ok adding a dependency on google-auth-library-ruby if that works?

[1] https://developers.google.com/identity/protocols/application-default-credentials
[2] kubernetes/kubernetes#30617 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #213 which provides support for using Google's default application credentials. I think this is a better solution as long as you are fine with a dependency on googleauth gem.

@@ -115,6 +115,11 @@ def fetch_user_auth_options(user)
options = {}
if user.key?('token')
options[:bearer_token] = user['token']
elsif user.key?('auth-provider') &&
user['auth-provider'].key?('config') &&
user['auth-provider']['config'].key?('access-token')
Copy link
Collaborator

@moolitayer moolitayer Nov 20, 2016

Choose a reason for hiding this comment

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

do we need to check if the auth provider is gcp OR is this the same for all auth providers?

@jeremywadsack
Copy link
Contributor Author

Closing this as I don't think it's a reasonable solution. See #213 for a better approach to handling GCP authorizations.

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

4 participants