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

Deprecate azure and gcp in-tree auth plugins #102181

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

enj
Copy link
Member

@enj enj commented May 20, 2021

With the client-go credential plugin functionality going GA in 1.22,
it is now time to deprecate these legacy integrations.

Signed-off-by: Monis Khan mok@vmware.com

/kind cleanup
/kind deprecation
/sig auth
/priority important-soon
/triage accepted
/milestone v1.22
@kubernetes/sig-auth-pr-reviews
/assign @ritazh @mikedanese @cjcullen @ankeesler @liggitt

The in-tree azure and gcp auth plugins have been deprecated.  The https://github.com/Azure/kubelogin and gcloud commands serve as out-of-tree replacements via the kubectl/client-go credential plugin mechanism.
- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/541-external-credential-providers

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 20, 2021
@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label May 20, 2021
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 20, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 20, 2021
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. triage/accepted Indicates an issue or PR is ready to be actively worked on. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 20, 2021
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 20, 2021
@enj enj added this to Needs Triage PRs in SIG Auth Old May 20, 2021
@enj enj moved this from Needs Triage PRs to In Review (v1.22) in SIG Auth Old May 20, 2021
@@ -114,6 +114,9 @@ type gcpAuthProvider struct {
}

func newGCPAuthProvider(_ string, gcpConfig map[string]string, persister restclient.AuthProviderConfigPersister) (restclient.AuthProvider, error) {
klog.Warningf(`WARNING: in-tree gcp auth plugin is now deprecated.
Please use the gcloud kubectl/client-go credential plugin instead.`)
Copy link
Member

Choose a reason for hiding this comment

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

We should hold off on this until we have an alternative. This is not currently actionable. Can we add this to release notes instead?

Copy link
Member Author

@enj enj May 21, 2021

Choose a reason for hiding this comment

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

I would prefer to have the warning where an end-user would actually see it. Perhaps GCP can make a documentation page that we can link to? Then you can update the documentation page with details when it becomes actionable.

Also GCP has had years of runway to migrate away from this. This should be no surprise. It is time (technically in August).

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to a .V(1) to limit unactionable warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #103499 to track moving this back to a Warningf by 1.24 at the latest.

@enj enj force-pushed the enj/i/deprecate_gcp_azure branch from c2bf4dd to 6bc9c85 Compare May 21, 2021 14:23
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 21, 2021
Comment on lines 92 to 93
klog.Warningf(`WARNING: in-tree azure auth plugin is now deprecated.
Please use the https://github.com/Azure/kubelogin kubectl/client-go credential plugin instead.`)
Copy link
Member Author

Choose a reason for hiding this comment

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

@ritazh are you happy with this verbiage?

Comment on lines 122 to 123
klog.V(1).Infof(`WARNING: in-tree gcp auth plugin is now deprecated.
Please use the gcloud kubectl/client-go credential plugin instead.`)
Copy link
Member Author

Choose a reason for hiding this comment

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

@mikedanese are you happy with this verbiage?

Copy link
Member

Choose a reason for hiding this comment

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

I'll defer to mike on content, but I'd suggest a single line if possible

Copy link
Member

Choose a reason for hiding this comment

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

also, in other deprecation warnings, we've included the "from" version and the targeted removal version, e.g.

WARNING: the gcp auth plugin is deprecated in v1.22+, unavailable in v1.25+; use ___  instead

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll defer to mike on content, but I'd suggest a single line if possible

The docs link forced me to use multiple lines to keep the length sane.

Updated to include the from and target versions.

@enj
Copy link
Member Author

enj commented May 21, 2021

/hold

(until PR to make client-go exec plugin officially GA is merged)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2021
func newGCPAuthProvider(_ string, gcpConfig map[string]string, persister restclient.AuthProviderConfigPersister) (restclient.AuthProvider, error) {
// deprecated in v1.22, remove in v1.25
// this should be updated to use klog.Warningf in v1.24 to more actively warn consumers
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there going to be some kind of process in place to ensure this happens? Otherwise is there anything programatic we could do to query the version at runtime and update the error?

Also it seems slightly risky to explicitly communicate the exact version it is being disabled. What if there is a lot of pushback at depreciation and its replacement and we need a few extra release cycles to hammer out those concerns?

Copy link
Member

Choose a reason for hiding this comment

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

Is there going to be some kind of process in place to ensure this happens?

I'd expect a milestoned issue

Also it seems slightly risky to explicitly communicate the exact version it is being disabled. What if there is a lot of pushback at depreciation and its replacement and we need a few extra release cycles to hammer out those concerns?

This won't merge until the client-go exec plugin GA merges. That is the target for any issues or concerns. Once that is released as GA, setting a definite removal version for the other auth plugins is reasonable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #103498 to track removal in 1.25. #102890 is tracking GA of client-go credential plugins.

// deprecated in v1.22, remove in v1.25
warnOnce.Do(func() {
klog.Warningf(`WARNING: in-tree azure auth plugin is now deprecated.
Please use the https://github.com/Azure/kubelogin kubectl/client-go credential plugin instead.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

As a casual/novice Kubectl user kubectl/client-go doesn't seem very actionable to me (as opposed to Azure's login thing getting an entire URL). Is it a directory? Or package? Or Repo? I would prefer a more actionable link where I can learn how to use kubectl/client-go.

Copy link
Member

Choose a reason for hiding this comment

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

suggestions for alternative links are welcome

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a link to the credential plugin docs.

@enj
Copy link
Member Author

enj commented Jul 6, 2021

/retest

Comment on lines 92 to 95
klog.Warningf(`WARNING: the azure auth plugin is deprecated in v1.22+, unavailable in v1.25+; use https://github.com/Azure/kubelogin client-go credential plugin instead.

To learn more about this feature, consult the documentation available at:
https://kubernetes.io/docs/reference/access-authn-authz/authentication/#client-go-credential-plugins`)
Copy link
Member

Choose a reason for hiding this comment

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

can we shrink to two lines instead of four?

WARNING: the azure auth plugin is deprecated in v1.22+, unavailable in v1.25+; use https://github.com/Azure/kubelogin instead.
To learn more, consult https://kubernetes.io/docs/reference/access-authn-authz/authentication/#client-go-credential-plugins

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

func newAzureAuthProvider(_ string, cfg map[string]string, persister restclient.AuthProviderConfigPersister) (restclient.AuthProvider, error) {
// deprecated in v1.22, remove in v1.25
warnOnce.Do(func() {
klog.Warningf(`WARNING: the azure auth plugin is deprecated in v1.22+, unavailable in v1.25+; use https://github.com/Azure/kubelogin client-go credential plugin instead.
Copy link
Member

Choose a reason for hiding this comment

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

should this get the same V(1) treatment and move to a Warningf in 1.24?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on what @liggitt pointed out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Opened #103525 to track.

Copy link
Member

Choose a reason for hiding this comment

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

looks good.

@enj enj force-pushed the enj/i/deprecate_gcp_azure branch from 74e5ca1 to 0c72fef Compare July 6, 2021 19:15
@enj
Copy link
Member Author

enj commented Jul 6, 2021

/hold

(until PR to make client-go exec plugin officially GA is merged)

/hold cancel

#102890 has merged.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2021
With the client-go credential plugin functionality going GA in 1.22,
it is now time to deprecate these legacy integrations.

Signed-off-by: Monis Khan <mok@vmware.com>
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 6, 2021

@enj: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-aks-engine-azure-disk-vmas 6bc9c858e9b5d71c6c18c1e25808be5ca38bd081 link /test pull-kubernetes-e2e-aks-engine-azure-disk-vmas

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@enj
Copy link
Member Author

enj commented Jul 6, 2021

/retest

@liggitt
Copy link
Member

liggitt commented Jul 6, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, liggitt

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 Jul 6, 2021
@k8s-ci-robot k8s-ci-robot merged commit 60475ee into kubernetes:master Jul 7, 2021
SIG Auth Old automation moved this from In Review (v1.22) to Closed / Done Jul 7, 2021
@howardjohn
Copy link
Contributor

As a user reading the release notes, I am still not quite sure what I am expected to do to authenticate with GCP now. I hope it will not require the 1gb+ (and 10x slower in our tests) gcloud command just to connect to a GCP cluster?

@EronWright
Copy link

EronWright commented Jul 29, 2021

@howardjohn I observe that the (now deprecated) gcp provider has two modes, one of which doesn't depend on the gcloud tool (it directly uses a go library). I believe it varies based on whether the cmd-path option is set.

@enj
Copy link
Member Author

enj commented Aug 7, 2021

As a user reading the release notes, I am still not quite sure what I am expected to do to authenticate with GCP now. I hope it will not require the 1gb+ (and 10x slower in our tests) gcloud command just to connect to a GCP cluster?

I suggest reaching out to GKE support. GKE will need to provide some binary to fetch credentials. Whether that is the gcloud binary or some new binary is entirely up to them. The same is true for all Kubernetes distributions.

@howardjohn
Copy link
Contributor

So when I distribute my k8s controller (which needs to authenticate with all platforms) and stick it on a distroless image, it's not going to work anymore? I need to somehow find a magic binary for gke (and aks, eks,....), get them in the docker image, manage the versions, etc?

I very much hope I misunderstand this change - this seems like a very hostile change for users if not.

@enj
Copy link
Member Author

enj commented Aug 8, 2021

So when I distribute my k8s controller (which needs to authenticate with all platforms) and stick it on a distroless image, it's not going to work anymore? I need to somehow find a magic binary for gke (and aks, eks,....), get them in the docker image, manage the versions, etc?

No, your controller would run as a Kubernetes service account via the in cluster config credentials provided uniformally across all clusters by the Kubelet. This is the standard way all controllers obtain an identity on Kubernetes. The various controller frameworks do this automatically.

I very much hope I misunderstand this change - this seems like a very hostile change for users if not.

This change pertains to human users obtaining proprietary credentials that are specific to their Kubernetes distribution. We are replacing the use of a proprietary SDK with a proper extension point.

@zulh-civo
Copy link

@enj how can I hide this warning when using kubectl?

@enj
Copy link
Member Author

enj commented Jul 5, 2022

@enj how can I hide this warning when using kubectl?

Follow the directions to migrate to the replacement client-go credential plugin.

@zulh-civo
Copy link

Follow the directions to migrate to the replacement client-go credential plugin.

I did. But it's still showing. My GKE cluster is on 1.21.11. My kubectl is on 1.24.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
SIG Auth Old
Closed / Done
Development

Successfully merging this pull request may close these issues.

None yet