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

kubernetes: Improve GCE support #7585

Closed
wants to merge 2 commits into from

Conversation

petervo
Copy link
Contributor

@petervo petervo commented Aug 30, 2017

Make basic auth work with GCE 1.7.x

Allow tokens to be read from auth-provider sections of a user's kubeconfig file.

Some versions don't return a 401 with an empty basic
header.
Auth providers rely on the kube client library to
execute their commands and populate the kubeconfig file.

Execute kubectl version to trigger that behavior and
then read the kubeconfig file.
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

LGTM, just two questions.

token = provider.config['access-token'];
else if (provider.config['token'])
token = provider.config['token'];
}
Copy link
Member

Choose a reason for hiding this comment

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

With this the config-provided token will be preferred over user.token even if the latter exists. Is this intended? Could this cause some backwards compatibility issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A user shouldn't have both according the spec. But in cases that they do, this would match the kubectl behavior so i think it's correct.

.then(function() {
return runCommand(cmd);
}, function () {
return runCommand(cmd);
Copy link
Member

Choose a reason for hiding this comment

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

Seems a bit odd to expect kubectl to be present and working (for config) but be silent about kubectl version failing. Should the error be at least logged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the cluster is down or otherwise unreachable the kubectl version command will fail. If we log it we'll end up with double logs for the same error in those cases once when version tries to connect and once when we try.

@martinpitt martinpitt added the question Further information is requested label Sep 4, 2017
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications! Makes sense.

@martinpitt martinpitt closed this in 3f7a6af Sep 4, 2017
@martinpitt martinpitt removed the question Further information is requested label Sep 4, 2017
@petervo petervo deleted the gce-fixes branch September 8, 2017 23:41
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