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

auth: reregister auth providers #60334

Merged
merged 1 commit into from
Feb 27, 2018
Merged

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Feb 24, 2018

Alternatively we can register this in the mains of most (or all) components.

Fixes GKE e2es
Fixes: 5d721bf

That PR unregistered auth providers for kubectl and probably elsewhere.

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 24, 2018
@mikedanese mikedanese added the kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. label Feb 24, 2018
@mikedanese
Copy link
Member Author

/retest

@@ -37,6 +37,9 @@ import (
schedulinginternalversion "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/scheduling/internalversion"
settingsinternalversion "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/settings/internalversion"
storageinternalversion "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/storage/internalversion"

// Import solely to initialize client auth plugins.
_ "k8s.io/client-go/plugin/pkg/client/auth"
Copy link
Member

Choose a reason for hiding this comment

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

Import where it's needed, not here.

Copy link
Member

Choose a reason for hiding this comment

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

So, yeah, in the mains of the components that need it.

Copy link
Member

Choose a reason for hiding this comment

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

Or from the code that constructs auth providers. Not sure we want non-uniform handling of auth providers from loaded kubeconfig files using client-go

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 24, 2018
@mikedanese
Copy link
Member Author

Our options are in the mains like we have here or in "k8s.io/client-go/rest" which actually retrieves the transport.

@liggitt
Copy link
Member

liggitt commented Feb 24, 2018

Where we build the auth providers from the config would make the most sense to me.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 24, 2018
@mikedanese
Copy link
Member Author

@liggitt in "k8s.io/client-go/tools/clientcmd"?

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 24, 2018
@mikedanese
Copy link
Member Author

@liggitt wants consistent handling of kubeconfig so he's recommending registering auth providers in clientcmd. Binaries can still construct raw rest.Configs and not import the auth providers.

@mikedanese
Copy link
Member Author

/test pull-kubernetes-e2e-gke

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 24, 2018
@mikedanese
Copy link
Member Author

mikedanese commented Feb 24, 2018

@liggitt moving this to clientcmd adds a ton of dependencies to staging repos. Is this better than registering auth providers in mains?

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2018
@liggitt
Copy link
Member

liggitt commented Feb 24, 2018

All of those dependencies already exist in client-go, and were pulled down on clone, right? The listing in godep files is annoying, but I don't actually think it bothers me that much. @lavalamp and @deads2k can weigh in.

I keep coming back to when I would want kubeconfig loading via clientcmd to behave inconsistently, and I can't think of a time I'd want that.

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

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@deads2k
Copy link
Contributor

deads2k commented Feb 26, 2018

All of those dependencies already exist in client-go, and were pulled down on clone, right? The listing in godep files is annoying, but I don't actually think it bothers me that much. @lavalamp and @deads2k can weigh in.

That depends on how your vendoring tool worked. Ones like glide walk packages back to find repo dependencies. As an example, before this pull, depending on clientcmd would not pull in azure. Now it would. Pulling in azure brings with it the risk of conflicting levels of dependencies. Because we (and many other libraries) leak dependent packages through their interfaces, you have to vendor with "strip-vendor". Doing that with conflicting levels of Azure (or its transitives) produces a vendoring mess that can make it impossible to use the library.

The short version is that golang dependency management is a mess and this makes it noticeably worse. I don't like any of the options. Does anyone want to make a case for why someone who didn't care about the plugin should be forced to resolve conflicting level problems?

@liggitt
Copy link
Member

liggitt commented Feb 26, 2018

The short version is that golang dependency management is a mess and this makes it noticeably worse.

Ok. We can just register in kubectl for now. Long-term, the externalized exec auth provider will be recommended, and could conceivably be added by default, since it likely has a much smaller dependency impact.

That PR unregistered auth providers for kubectl and probably elsewhere.

only kubectl packages imported that client, so I'd only expect that command to have been affected

Fixes: 5d721bf

That PR unregistered auth providers for kubectl and probably elsewhere.
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 26, 2018
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018
@liggitt
Copy link
Member

liggitt commented Feb 26, 2018

/lgtm
/hold for gke test

Unhold at will

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese, soltysh

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

@mikedanese
Copy link
Member Author

/test pull-kubernetes-e2e-gke

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@mikedanese
Copy link
Member Author

/retest

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 39891ba into kubernetes:master Feb 27, 2018
@mikedanese mikedanese deleted the fix-gke branch February 27, 2018 01:49
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/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants