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

Kubeclient 4.2.2 #418

Merged
merged 11 commits into from
Feb 19, 2019
Merged

Kubeclient 4.2.2 #418

merged 11 commits into from
Feb 19, 2019

Conversation

timothysmith0609
Copy link
Contributor

What are you trying to accomplish with this PR?
Closes #406
Update Kubeclient to 4.2.2. This will let AWS users properly auth via their kubeconfig

How is this accomplished?

  • Bump Gem
  • Remove GoogleFriendlyConfig (still need a shim for GCP, but less involved)
  • Go through Changelog

Response to changelog

  • KubeException is deprecated (it already was, before). Replaced with Kubeclient::HttpError, which is just a subclass of KubeException.

What could go wrong?
New, strange, behaviour not covered by our tests

@timothysmith0609 timothysmith0609 changed the title Kubeclient 4 2 2 Kubeclient 4.2.2 Jan 28, 2019
@timothysmith0609
Copy link
Contributor Author

As @KnVerey pointed out, it looks like ActiveFailover makes a reference to the to-be-removed GoogleFriendlyConfig. It looks like all we need to do is change the reference from GoogleFriendlyConfig to KubernetesDeploy::KubeclientBuilder::Kubeconfig and their client should retain the correct behaviour.

I'm tempted to change the reference to just use the base KubeClient class (Kubeclient::Config) since it's passing the tests, but I think that will cause issues when it actually has to auth against GCP in production.

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

still need a shim for GCP, but less involved

Why do we still need one? kubeclient's readme makes me think we could replace auth_options: kube_context.auth_options in our KubeClient construction with auth_options: { bearer_token: Kubeclient::GoogleApplicationDefaultCredentials.token }. Does that not work?

I'm tempted to change the reference to just use the base KubeClient class (Kubeclient::Config) since it's passing the tests, but I think that will cause issues when it actually has to auth against GCP in production.

I bet you're right not to assume it has test coverage for this. We'll probably want to work with that team to test the changes locally before merging them. They have a "check kubectl works" script on that repo that should do the trick (and actually uses the same code as failovers--the repo has a neat modular task architecture).

lib/kubernetes-deploy/restart_task.rb Outdated Show resolved Hide resolved
@KnVerey
Copy link
Contributor

KnVerey commented Feb 6, 2019

Another note: we should definitely test this branch manually using both user credentials and application default credentials before merging. My test app has a Shipit task that can be used for the latter.

@timothysmith0609
Copy link
Contributor Author

auth_options: { bearer_token: Kubeclient::GoogleApplicationDefaultCredentials.token }. Does that not work?

This would work up until the point we're not targeting a GCP cluster, which due to the open-source nature of this gem is not a valid assumption to make. I'm wondering if it's worth patching kubeclient upstream. Our simple fix (checking for `auth-provider == "gcp") seems sufficient.

@timothysmith0609
Copy link
Contributor Author

Another note: we should definitely test this branch manually using both user credentials and application default credentials before merging. My test app has a Shipit task that can be used for the latter.

Running with user credentials locally is working fine. Is the Cumulus Cat Task the Shipit task you're talking about?

@timothysmith0609
Copy link
Contributor Author

Created upstream kubeclient issue for automatically loading default application credentials.

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

I don't need to review again, but please fix the Policial errors and 🎩 with the test app before merging.

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