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

load cloud provider plugins when using tkctl #756

Merged
merged 5 commits into from
Aug 28, 2019

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Aug 12, 2019

Signed-off-by: yeya24 [email protected]
I' m sorry that I don't have a gcp environment. Can you make a test for this PR? @aylei 😄

What problem does this PR solve?

#721

What is changed and how does it work?

Check List

Tests

  • Unit test
  • E2E test
  • Stability test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has Helm charts change
  • Has Go code change
  • Has CI related scripts change
  • Has documents change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Does this PR introduce a user-facing change?:


@sre-bot
Copy link
Contributor

sre-bot commented Aug 12, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Aug 12, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@aylei
Copy link
Contributor

aylei commented Aug 12, 2019

/ok-to-test

@aylei
Copy link
Contributor

aylei commented Aug 12, 2019

@yeya24 IMHO, it is better to load all the auth plugins

@yeya24
Copy link
Contributor Author

yeya24 commented Aug 12, 2019

OK. Let me recheck it. Do you mean gcp, aws and alicloud? Do I need to add other plugins?

@aylei
Copy link
Contributor

aylei commented Aug 12, 2019

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

Importing all auth plugins is sufficient I suppose

@yeya24
Copy link
Contributor Author

yeya24 commented Aug 13, 2019

Thanks for pointing out this. @aylei Updated, PTAL.

@yeya24 yeya24 changed the title load gcp plugin when using tkctl load cloud provider plugins when using tkctl Aug 13, 2019
@aylei
Copy link
Contributor

aylei commented Aug 13, 2019

@yeya24
Try to compile tkctl by executing make cli but met the following errors:

# k8s.io/client-go/plugin/pkg/client/auth/azure
../../../../pkg/mod/k8s.io/[email protected]+incompatible/plugin/pkg/client/auth/azure/azure.go:246:4: cannot use expiresIn (type string) as type json.Number in field value
../../../../pkg/mod/k8s.io/[email protected]+incompatible/plugin/pkg/client/auth/azure/azure.go:247:4: cannot use expiresOn (type string) as type json.Number in field value
../../../../pkg/mod/k8s.io/[email protected]+incompatible/plugin/pkg/client/auth/azure/azure.go:248:4: cannot use expiresOn (type string) as type json.Number in field value
../../../../pkg/mod/k8s.io/[email protected]+incompatible/plugin/pkg/client/auth/azure/azure.go:265:23: cannot use token.token.ExpiresIn (type json.Number) as type string in assignment
../../../../pkg/mod/k8s.io/[email protected]+incompatible/plugin/pkg/client/auth/azure/azure.go:266:23: cannot use token.token.ExpiresOn (type json.Number) as type string in assignment
make: *** [cli] Error 2

"github.com/pingcap/tidb-operator/pkg/tkctl/config"

Copy link
Member

Choose a reason for hiding this comment

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

Remove this empty line.

@yeya24
Copy link
Contributor Author

yeya24 commented Aug 13, 2019

Seems in the latest client-go, the code is OK. https://github.com/kubernetes/client-go/blob/4f902818859a531e018f805b2d3597f14bf2c494/plugin/pkg/client/auth/azure/azure.go#L254

	return &azureToken{
		token: adal.Token{
			AccessToken:  accessToken,
			RefreshToken: refreshToken,
			ExpiresIn:    json.Number(expiresIn),
			ExpiresOn:    json.Number(expiresOn),
			NotBefore:    json.Number(expiresOn),

@yeya24
Copy link
Contributor Author

yeya24 commented Aug 13, 2019

Seems in the latest client-go, the code is OK. https://github.com/kubernetes/client-go/blob/4f902818859a531e018f805b2d3597f14bf2c494/plugin/pkg/client/auth/azure/azure.go#L254

	return &azureToken{
		token: adal.Token{
			AccessToken:  accessToken,
			RefreshToken: refreshToken,
			ExpiresIn:    json.Number(expiresIn),
			ExpiresOn:    json.Number(expiresOn),
			NotBefore:    json.Number(expiresOn),

After digging into this I found out that this code was fixed in client-go v10.0.0, which is compatible for k8s 1.13+.
If I change go.mod to client-go v10.0.0, it will produce error

build command-line-arguments: cannot load k8s.io/api/admissionregistration/v1alpha1: cannot find module providing package k8s.io/api/admissionregistration/v1alpha1

And the error above can be fixed if we update k8s related package version to 1.13+. See issue kubernetes/client-go#551

Currently we are still using 1.12.5, do we have any plan to update it recently? If not, I think it is better to not add azure auth plugin now.

@aylei
Copy link
Contributor

aylei commented Aug 13, 2019

@yeya24 I'm ok with not importing Azure auth plugin

@yeya24
Copy link
Contributor Author

yeya24 commented Aug 20, 2019

Need review for this PR.

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@aylei
Copy link
Contributor

aylei commented Aug 27, 2019

/run-e2e-tests

@aylei
Copy link
Contributor

aylei commented Aug 27, 2019

/run-e2e-in-kind

@aylei
Copy link
Contributor

aylei commented Aug 27, 2019

/run-e2e-in-kind

Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

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

LGTM

@aylei
Copy link
Contributor

aylei commented Aug 27, 2019

/run-e2e-in-kind

@aylei
Copy link
Contributor

aylei commented Aug 28, 2019

@yeya24 Thanks!

@aylei aylei merged commit c51b0d4 into pingcap:master Aug 28, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

cherry pick to release-1.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants