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.

Sign up for GitHub

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

Jump to bottom

🏃 Add auth providers support to clusterctl #2684

Merged
merged 1 commit into from
Mar 23, 2020
Merged

🏃 Add auth providers support to clusterctl #2684

merged 1 commit into from
Mar 23, 2020

Conversation

dippynark
Copy link
Contributor

@dippynark dippynark commented Mar 15, 2020 •
edited
Loading

What this PR does / why we need it:

Running clusterctl against GKE clusters gives:

$ clusterctl init --infrastructure kubernetes
Error: failed to connect to the management cluster: no Auth Provider found for name "gcp"

This PR adds support for the GCP auth provider.
kubernetes/client-go#242 (comment)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 15, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @dippynark. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 15, 2020
@dippynark dippynark changed the title :bug Add GCP auth provider support 🐛Add GCP auth provider support Mar 15, 2020
@dippynark dippynark changed the title 🐛Add GCP auth provider support 🐛 Add GCP auth provider support Mar 15, 2020
@dippynark dippynark force-pushed the add-gcp-auth-provider-support branch from 03cea2e to 5df16c0 Compare March 15, 2020 11:55
@dippynark
Copy link
Contributor Author

/assign @vincepri

@dippynark dippynark changed the title 🐛 Add GCP auth provider support 🐛 Add GCP auth provider support to clusterctl Mar 15, 2020
JoelSpeed
JoelSpeed reviewed Mar 16, 2020
View reviewed changes
cmd/clusterctl/main.go Outdated
@@ -17,6 +17,7 @@ limitations under the License.
package main

import (
_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the interest of improving UX, would it be better to import all of the auth providers provider in the client-go auth package rather than just GCP? (This may cause issues with dependencies for the Azure provider, dep regularly had problems resolving this in the past, not sure if this is solved with go mod)

Suggested change
_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
_ "k8s.io/client-go/plugin/pkg/client/auth"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, seem to have had more luck with go mod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah actually, spoke too soon - CI seems to have the issue you described

@vincepri
Copy link
Member

vincepri commented Mar 16, 2020 •
edited
Loading

In the past we've imported all auth providers rather than just one, +1 on @JoelSpeed's suggestion

@vincepri
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 16, 2020
@vincepri
Copy link
Member

/milestone v0.3.2

@dippynark might need to run make modules to update the go.mod files

@k8s-ci-robot k8s-ci-robot added this to the v0.3.2 milestone Mar 17, 2020
@vincepri
Copy link
Member

/retitle 🏃 Add auth providers support to clusterctl

@k8s-ci-robot k8s-ci-robot changed the title 🐛 Add GCP auth provider support to clusterctl 🏃 Add auth providers support to clusterctl Mar 17, 2020
@vincepri
Copy link
Member

/assign @fabriziopandini

@vincepri
Copy link
Member

@dippynark do you have time to run make modules and squash?

@fabriziopandini
Copy link
Member

LGTM pending squash & make modules as suggested by @vincepri

@vincepri
Copy link
Member

/milestone v0.3.x

@k8s-ci-robot k8s-ci-robot modified the milestones: v0.3.2, v0.3.x Mar 19, 2020
@dippynark dippynark force-pushed the add-gcp-auth-provider-support branch 2 times, most recently from 0b1be88 to e5b0ffd Compare March 21, 2020 07:12
@dippynark
Copy link
Contributor Author

@vincepri sorry for the slow response - have done

@dippynark dippynark force-pushed the add-gcp-auth-provider-support branch from e5b0ffd to 7058fa6 Compare March 21, 2020 18:23
@JoelSpeed
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2020
vincepri
vincepri approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dippynark, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2020
@k8s-ci-robot k8s-ci-robot merged commit d1c2d70 into kubernetes-sigs:master Mar 23, 2020
@dippynark dippynark deleted the add-gcp-auth-provider-support branch March 24, 2020 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@vincepri vincepri vincepri approved these changes

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@vincepri vincepri

@fabriziopandini fabriziopandini

@JoelSpeed JoelSpeed

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v0.3
Development

Successfully merging this pull request may close these issues.
5 participants
@dippynark @k8s-ci-robot @vincepri @fabriziopandini @JoelSpeed