Client auth provider plugin framework #23066

cjcullen · 2016-03-16T18:33:13Z

Allows client plugins to modify the underlying transport to, for example, add custom authorization headers.

cjcullen · 2016-03-16T18:35:47Z

@jdef @jlowdermilk @deads2k @smarterclayton @erictune I'd love to hear your thoughts.

k8s-bot · 2016-03-16T19:07:03Z

GCE e2e build/test passed for commit 759f65de42125a402f759a8080b0100e8cd7cc9e.

jdef · 2016-03-16T20:26:20Z

pkg/client/transport/plugin.go

+	pluginsLock.Lock()
+	defer pluginsLock.Unlock()
+	if _, found := plugins[name]; found {
+		glog.Fatalf("Round tripper plugin %q was registered twice", name)


dislike fatalf, complicates unit testing. can this be an error instead?

Sure. I wasn't sure what the right way to do this would be in a client binary. Would Errorf and then overwrite be reasonable?

could registration be made more explicit? as in the kubectl process invokes some "RegisterAuthPlugins()" func vs. using init? RegisterAuthPlugins() would basically do what init does now, except that it could return an error if registration for any plugin fails.

so .. if we do that, then you could return fmt.Errorf(...) here and NOT overwrite.

jdef · 2016-03-16T20:32:27Z

nits, otherwise lgtm. thanks for putting this together!

jdef · 2016-03-16T20:32:49Z

any chance this would get "backported" to a future 1.2.x release?

deads2k · 2016-03-17T12:18:34Z

pkg/client/restclient/config.go

@@ -62,6 +62,9 @@ type Config struct {
 	// TODO: demonstrate an OAuth2 compatible client.
 	BearerToken string

+	// Server requires plugin-specified authentication.
+	AuthPlugin string


I'm not sure this ends up being enough structure for what we're likely to need. It seems like the next likely step is needing to have a piece of information I need to pass through to my plugin. Internally we wouldn't make it ahead of time, but you're plumbing this through a serialized file so I'm more concerned.

My initial (self-serving) thought was that other plugins could handle config themselves. E.g. @jdef's DCOS plugin could read an environment variable, another plugin could have its own conf file.

That said, I am very open to changes in structure/naming/etc.

My initial (self-serving) thought was that other plugins could handle config themselves. E.g. @jdef's DCOS plugin could read an environment variable, another plugin could have its own conf file.

I think that we should try to keep configuration in one location, so that the behavior of a kubeconfig file relatively predictable without needing to set particular env vars. During the debugging process, I find "show me the content of file X" easier than, "what was your env when you ran that command".

deads2k · 2016-03-17T12:21:27Z

any chance this would get "backported" to a future 1.2.x release?

It's a neat feature, but would we then end up considering backports for all the plugins we'd want for it too? I don't know that I'd make that tradeoff.

smarterclayton · 2016-03-17T17:20:39Z

pkg/client/unversioned/clientcmd/api/types.go

@@ -92,6 +92,8 @@ type AuthInfo struct {
 	Username string `json:"username,omitempty"`
 	// Password is the password for basic authentication to the kubernetes cluster.
 	Password string `json:"password,omitempty"`
+	// AuthPlugin is a custom plugin used to authenticate to the kubernetes cluster.
+	AuthPlugin string `json:"auth-plugin,omitempty"`


Should this be called "AuthProvider"? Or CustomAuthProvider? If this is specified, does it take precedence over the other fields in this map?

I would kind of expect to be able to have a custom auth plugin that uses username/password that overrides the default basic auth flow.

So AuthProvider would take precedence over any other field, but its implementation would be provided those fields? I think that is reasonable.

It does change the meaning of the fields slightly. Instead of just being "* for authentication to the kubernetes cluster", they would now be "* for authentication to the k8s cluster or to a custom intermediary".

Other than figuring out naming, is this something we can punt on until we have a concrete use case? Or does the fact that this gets serialized mean we need to figure it out up front?

Other than figuring out naming, is this something we can punt on until we have a concrete use case? Or does the fact that this gets serialized mean we need to figure it out up front?

I think we should decide which fields get plumbed through to the plugins before deciding on a serialization. Passing the information from the AuthInfo doesn't seem unreasonable. It also doesn't seem unreasonable to allow plumb through a map[string]string or an object registered in the scheme down through either.

Okay. How about something like:

type AuthProviderConfig struct { Name string Config json.RawMessage }

Plugins would then be provided with the specified Config when instantiated. Simple plugins (like mine) can specify a Name, and leave Config blank. More complex plugins could define and parse their own config, and it would all look nice and pretty in the kubeconfig file.

This sounds pretty flexible, and at first glace I really like this. Upon
further reflection I wonder if a configmap might be more appropriate? Or
would that be too restrictive? Configmap is an API standard after all..

On Mon, Mar 21, 2016 at 7:27 PM, CJ Cullen notifications@github.com wrote:

In pkg/client/unversioned/clientcmd/api/types.go
#23066 (comment)
:

@@ -92,6 +92,8 @@ type AuthInfo struct {
Username string json:"username,omitempty"
// Password is the password for basic authentication to the kubernetes cluster.
Password string json:"password,omitempty"

// AuthPlugin is a custom plugin used to authenticate to the kubernetes cluster.

AuthPlugin string json:"auth-plugin,omitempty"

Okay. How about something like:

type AuthProviderConfig struct {
Name string
Config json.RawMessage
}

Plugins would then be provided with the specified Config when
instantiated. Simple plugins (like mine) can specify a Name, and leave
Config blank. More complex plugins could define and parse their own config,
and it would all look nice and pretty in the kubeconfig file.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/23066/files/759f65de42125a402f759a8080b0100e8cd7cc9e#r56915022

ConfigMap would be less flexible for plugin builders (string->string map instead of generic JSON blob) without providing much of a benefit.

I need to look into whether it'd be better to use something like runtime.RawExtension & runtime.Object. I'm hoping @smarterclayton or @krousey can kick in some feedback on that.

cjcullen · 2016-03-22T06:55:35Z

Reworked this WIP after the first round of comments:

Changed AuthPlugin string to AuthProvider *AuthProviderConfig (which has a *json.RawMessage in it).
Made RegisterPlugin return an error. RegisterPlugin is still called from the init() func of the plugin (let's the structs stay non-exported), but at least the function will be testable.
Reused the WrapTransport field of the transport config, instead of plumbing knowledge of auth plugins into the generic client transport.
Moved where the plugins are imported from. I didn't previously consider that clients are created from other places than just cmd/kubectl.

PTAL

k8s-bot · 2016-03-22T07:30:19Z

GCE e2e build/test failed for commit 6513f0d96e0e699f117afee7b1f59838044980e4.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

deads2k · 2016-03-22T14:09:03Z

pkg/client/unversioned/clientcmd/api/types.go

+// AuthProviderConfig
+type AuthProviderConfig struct {
+	Name   string           `json:"name"`
+	Config *json.RawMessage `json:"config"`


I'm more used to seeing runtime.RawExtension, and runtime.Object. It's what we we use downstream and it works reasonably well.

I do like this a lot better than just a name.

I noticed that there is already a runtime.RawExtension, in the v1 clientcmd types, and a corresponding runtime.Object in the unversioned types. Can I just reuse that? Or am I better off defining a new field?

I'm still looking for where (if) the extensions are actually registered/used...

I'm still looking for where (if) the extensions are actually registered/used...

They were there so that deep integrators like openshift would be able to supply custom configuration values and the kube decoding/encoding path wouldn't strip them out. They aren't natively used in kube.

Since the core kube code will actually be interpreting these values, the structure you've described here is more suitable. If you were going to say, "we aren't going to provide this, build your own "gcp" on top", then you'd point them to the existing Extensions field and let them do their thing.

cjcullen · 2016-03-25T20:50:19Z

Talked w/ @erictune about this offline. He recommended that we punt on the config format until we have a concrete use case. So, for now, AuthProviderConfig would just have a name:

type AuthProviderConfig struct {
    Name string `json:"name"`
}

So, the serialized version would look like:

users:
- name: cjcullen
  user:
    auth-provider:
      name: gcp
...

This would still be valid if we later added a config: field or something else to the AuthProviderConfig.

Thoughts?

jdef · 2016-03-26T21:53:03Z

fine w/ me

On Fri, Mar 25, 2016 at 4:51 PM, CJ Cullen notifications@github.com wrote:

Talked w/ @erictune https://github.com/erictune about this offline. He
recommended that we punt on the config format until we have a concrete use
case. So, for now, AuthProviderConfig would just have a name:

type AuthProviderConfig struct {
Name string json:"name"
}

So, the serialized version would look like:

users:

name: cjcullen
user:
auth-provider:
name: gcp
...

This would still be valid if we later added a config: field or something
else to the AuthProviderConfig.

Thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23066 (comment)

deads2k · 2016-03-28T12:53:34Z

Thoughts?

That's fine. Are you still ok with making it a slice to allow multiple participants? That would change serialization.

cjcullen · 2016-03-28T22:39:21Z

Yeah, I'll make it a slice. A little awkward, but probably outweighed by the serialization BWC benefits.

cjcullen · 2016-03-29T02:39:16Z

Made AuthProviders a slice, and added a test for chaining several AuthProviders together.

PTAL

k8s-bot · 2016-03-29T03:09:30Z

GCE e2e build/test passed for commit 4b5ba3e3d31159bfd17d0a5ec4b10c3bfa65b89d.

jdef · 2016-03-29T03:40:36Z

👍

cjcullen · 2016-03-30T01:24:34Z

Removing the "WIP" as I think we've settled on the general direction for this.

k8s-bot · 2016-04-15T02:33:29Z

GCE e2e build/test failed for commit 03f9b5a.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

cjcullen · 2016-04-15T16:54:00Z

@k8s-bot e2e test this issue: #23545

k8s-bot · 2016-04-15T17:29:07Z

GCE e2e build/test passed for commit 03f9b5a.

erictune · 2016-04-20T22:45:58Z

LGTM

k8s-bot · 2016-04-21T00:38:46Z

GCE e2e build/test passed for commit 03f9b5a.

k8s-bot · 2016-04-21T03:16:16Z

GCE e2e build/test passed for commit 03f9b5a.

k8s-github-robot · 2016-04-21T15:59:25Z

@cjcullen
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

cjcullen · 2016-04-21T16:02:01Z

@k8s-bot unit test this issue: #24603

cjcullen · 2016-04-21T17:39:17Z

Re-adding LGTM.

k8s-bot · 2016-04-21T20:20:46Z

GCE e2e build/test passed for commit 03f9b5a.

k8s-bot · 2016-04-22T00:30:10Z

GCE e2e build/test passed for commit 03f9b5a.

k8s-github-robot · 2016-04-22T00:30:27Z

Automatic merge from submit-queue

@bobbyrullo

Automatic merge from submit-queue Support persisting config from kubecfg AuthProvider plugins Plumbs through an interface to the plugin that can persist a `map[string]string` config for just that plugin. Also adds `config` to the AuthProvider serialization type, and `Login()` to the AuthProvider plugin interface. Modified the gcp AuthProvider to cache short-term access tokens in the kubecfg file. Builds on #23066 @bobbyrullo @deads2k @jlowdermilk @erictune

@cjcullen

…als-auth-providers Automatic merge from submit-queue kubectl config set-crentials: add arguments for auth providers This PR adds `--auth-provider` and `--auth-provider-arg` flags to the `kubectl config set-credentials` sub-command. There's currently no way of interacting with the new auth provider framework added in #23066 through kubectl. You have to render a custom kubeconfig to use them. Additionally `kubectl config set` just sort of craps out when attempting to interact with authentication info objects (#29312). This is a minimal implementation of allowing `kubect config set-credentials` to set fields for client auth providers. cc @cjcullen @kubernetes/kubectl

googlebot added the cla: yes label Mar 16, 2016

cjcullen force-pushed the clientplugin branch from 8a8b757 to 759f65d Compare March 16, 2016 18:33

k8s-github-robot assigned smarterclayton Mar 16, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 16, 2016

cjcullen mentioned this pull request Mar 16, 2016

initial config plugin framework for kubectl and mesosphere/acs impl #21553

Closed

jdef reviewed Mar 16, 2016
View reviewed changes

jdef mentioned this pull request Mar 16, 2016

kubectl needs support for dcos EE authentication mesosphere/kubernetes-mesos#792

Open

deads2k reviewed Mar 17, 2016
View reviewed changes

smarterclayton reviewed Mar 17, 2016
View reviewed changes

cjcullen force-pushed the clientplugin branch from 759f65d to 6513f0d Compare March 22, 2016 06:42

deads2k reviewed Mar 22, 2016
View reviewed changes

cjcullen force-pushed the clientplugin branch from 4b5ba3e to 446ffae Compare March 30, 2016 01:23

cjcullen changed the title ~~WIP: Client auth plugin framework~~ Client auth provider plugin framework Mar 30, 2016

cjcullen removed the release-note-label-needed label Apr 13, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2016

cjcullen mentioned this pull request Apr 15, 2016

Support persisting config from kubecfg AuthProvider plugins #24304

Merged

Add client auth plugin framework for kubectl with GCP auth plugin.

03f9b5a

cjcullen force-pushed the clientplugin branch from 2bed767 to 03f9b5a Compare April 15, 2016 00:49

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2016

erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2016

cjcullen mentioned this pull request Apr 21, 2016

Petset unit test failure #24603

Closed

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2016

cjcullen added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2016

k8s-github-robot merged commit f4beccf into kubernetes:master Apr 22, 2016

bobbyrullo mentioned this pull request May 5, 2016

Refresh Tokens #18549

Closed

This was referenced Jul 18, 2016

Kubectl login subcommand kubernetes/enhancements#32

Closed

"kubectl config set" hangs and panics when trying to set user fields #29312

Closed

ericchiang mentioned this pull request Aug 3, 2016

kubectl config set-crentials: add arguments for auth providers #30007

Merged

jheiss mentioned this pull request Oct 12, 2016

Bedrock auth sources #2202

Closed

Client auth provider plugin framework #23066

Client auth provider plugin framework #23066

Conversation

cjcullen commented Mar 16, 2016

cjcullen commented Mar 16, 2016

k8s-bot commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdef commented Mar 16, 2016

jdef commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcullen commented Mar 22, 2016

k8s-bot commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcullen commented Mar 25, 2016

jdef commented Mar 26, 2016

deads2k commented Mar 28, 2016

cjcullen commented Mar 28, 2016

cjcullen commented Mar 29, 2016

k8s-bot commented Mar 29, 2016

jdef commented Mar 29, 2016

cjcullen commented Mar 30, 2016

k8s-bot commented Apr 15, 2016

cjcullen commented Apr 15, 2016

k8s-bot commented Apr 15, 2016

erictune commented Apr 20, 2016

k8s-bot commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

k8s-github-robot commented Apr 21, 2016

cjcullen commented Apr 21, 2016

cjcullen commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

k8s-bot commented Apr 22, 2016

k8s-github-robot commented Apr 22, 2016