Support persisting config from kubecfg AuthProvider plugins #24304

cjcullen · 2016-04-15T00:33:46Z

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 · 2016-04-15T00:39:49Z

pkg/client/unversioned/clientcmd/client_config.go

+	contextName               string
+	overrides                 *ConfigOverrides
+	fallbackReader            io.Reader
+	persistAuthProviderConfig PersistAuthProviderConfigForUser


Copied over comments from old commit:

@deads2k: I kind of want methods to give me back persisters of a given shape.

@cjcullen: What do you mean by this?

@deads2k you mean strongly-typed persisters? How would one do that in Go, given the lack of generics, templating, etc.

@deads2k you mean strongly-typed persisters? How would one do that in Go, given the lack of generics, templating, etc.

Yeah, that's what I meant, but without generics or templating I can see that the effort would be too great to be worth it.

My main fear is trying to put the burden of building the persistAuthProviderConfigForUser on the caller to a new config. I don't think most callers will pass anything other than nil and that will produce problems later.

Any caller that has the ability to persist should be able to handle providing a persister without too much trouble. A caller that doesn't have a notion of persisting the config may not be able to provide a persister, in which case the plugin should handle a nil persister gracefully.

Is that a reasonable assertion for now?

deads2k · 2016-04-22T12:19:09Z

pkg/client/restclient/plugin.go

@@ -30,9 +30,16 @@ type AuthProvider interface {
 	// WrapTransport allows the plugin to create a modified RoundTripper that
 	// attaches authorization headers (or other info) to requests.
 	WrapTransport(http.RoundTripper) http.RoundTripper
+	// Login is expected to maybe require user interaction; it won't be called
+	// in environments where a user is not present
+	Login() error


I think we'll want to provide people the option for two different methods. Something like InteractiveAuthentication(io.Reader, io.Writer, io.Writer) error and NonInteractiveAuthentication() error. I'm pretty sure that @bobbyrullo's flow doesn't require user interaction in refresh cases (no matter where the refresh token actually lives).

With that split, how would the caller know which one to call?

I'm guessing that the standard case for Interactive would be Stdin/Stdout. The provider plugin can create its own Reader/Writer if it needs them.

I know that we pass through os.Stdin as the "fallbackReader" in the interactive basic-auth case. Are their expected cases where we think Stdin/Stdout won't be the right place for an interactive login?

@deads2k: For the refresh case I don't think we need to add anything to the interface: The RoundTripper that is returned by WrapTransport should be able to handle that as necessary (eg, when short term creds are about to expire/have expired), as long as it can get a Persister.

So I think we only need a method for getting the initial credentials, which I would think it's safe to assume, will always require some sort of user interaction.

@deads2k: Ping on this question. Do we need to disambiguate the flows at the plugin level? Can't we just defer to letting the plugin modify its config however it likes (either interactively or non-interactively)?

@deads2k: Ping on this question. Do we need to disambiguate the flows at the plugin level? Can't we just defer to letting the plugin modify its config however it likes (either interactively or non-interactively)?

I feel like I'm missing something. If this requires user interaction, they'll need an io.Reader, right? I could live with a nil reader meaning, "non-interactive", but they'll still need one, right?

I see. You're thinking we may want the option to interactively login through the command line (w/ Readers/Writers provided by kubectl)?

Neither @bobbyrullo's use case or mine would need this right now. Can we defer on adding this to the plugin until we have a user?

Neither @bobbyrullo's use case or mine would need this right now. Can we defer on adding this to the plugin until we have a user?

If you update the comment to say that it "must not require user interaction", then ok.

I'm expecting that it might require user interaction, just not through the command line (passing an io.Reader). I'll change the comment to try to reflect this. I'm mostly going to defer its usage (and any necessary changes) to @bobbyrullo, because it isn't currently used anywhere.

yes that's true - for my case, we'll spawn a browser.

bobbyrullo · 2016-04-22T22:00:27Z

@cjcullen : This all looks good for my case.

deads2k · 2016-05-03T12:12:42Z

pkg/client/unversioned/clientcmd/config.go

+}
+
+func PersistAuthConfigForUser(loadingRules *ClientConfigLoadingRules) func(string) restclient.AuthProviderConfigPersister {
+	configAccess := NewDefaultPathOptions()


This doesn't look right. Seems like you should be building the ConfigAccess from the LoadingRules since they're giving (I think) the same information about where you're loading the config from.

It might even be appropriate to have the ClientConfig interface have a method for getting its ConfigAccess.

Ok. I've tried to only pass around a ConfigAccess (which contains LoadingRules).

deads2k · 2016-05-04T11:58:35Z

pkg/client/unversioned/clientcmd/client_config.go

@@ -246,6 +248,11 @@ func (config *DirectClientConfig) Namespace() (string, bool, error) {
 	return configContext.Namespace, overridden, nil
 }

+// Namespace implements KubeConfig


bad comment?

Yup. Fixed.

deads2k · 2016-05-04T12:14:43Z

pkg/client/unversioned/clientcmd/config.go

+)
+
+// ConfigAccess is used by subcommands and methods in this package to load and modify the appropriate config files
+type ConfigAccess interface {


Can't ClientConfigLoadingRules implement this interface?

GetLoadingPrecedence -> return Precedence GetStartingConfig() -> call through to NewNonInteractiveDeferredLoadingClientConfig with no overrides? GetDefaultFilename -> explicit if you have one, otherwise first existing from precedence, otherwise first from precedence IsExplicitFilename -> len(explicitpath) GetExplicitFile -> explicitPath

Seems like that about covers it and would eliminate the duplicate information.

Well that makes everything a heck of a lot cleaner. Good catch.

deads2k · 2016-05-04T12:15:53Z

That fell out pretty well. I think you can actually get the existing struct to conform to the interface to simplify a little more.

cjcullen · 2016-05-04T16:11:04Z

Cleaned up the redundancy. If it looks good, I'd like to squash that last commit before merge.

deads2k · 2016-05-04T17:04:19Z

pkg/client/unversioned/clientcmd/loader.go

@@ -229,6 +229,54 @@ func (rules *ClientConfigLoadingRules) Migrate() error {
 	return nil
 }

+// GetLoadingPrecedence implements ClientConfig


ConfigAccess here and elsewhere in this file?

deads2k · 2016-05-04T17:06:58Z

lgtm

cjcullen · 2016-05-04T17:21:46Z

Squashed & pushed.

bobbyrullo · 2016-05-04T20:05:21Z

pkg/client/restclient/plugin.go

+//  config is the inital configuration for this plugin.
+//  persister allows the plugin to save updated configuration.
+type Factory func(clusterAddress string, config map[string]string, persister AuthProviderConfigPersister) (AuthProvider, error)
+


Would it make sense for the config to be a map[string]interface so that it could support lists of strings as well? I just ran into this case in the work I am doing; I could just as well have a comma-separated list of strings but it's not ideal.

Sorry for saying this so late in the game but it didn't occur to me until I started working with it.

I'd eventually (post 1.3) be interested in making the config use runtime.RawExtension/runtime.Object so that plugins could have strongly typed config. Would you be okay w/ map[string]string until we move to that? If not, I'd be fine with you making the changes in a following PR.

Would it make sense for the config to be a map[string]interface so that it could support lists of strings as well?

map[string]interface{} won't work well because we're trying to serialize the data out into json.

map[string]string works well, map[string][]string also works fine. I don't have any objection to tweaking the type to map[string][]string in this pull or in a very close followup. The datatype change would be a breaking serialization change for kubeconfig files, so we'd have to change it before anyone gets a chance to use it.

Hmmm. I'd rather keep it as map[string]string then.

@bobbyrullo, how bad would it be for your case to just use strings.Split for a comma-separated list of strings?

strings.Split is just fine.

cjcullen · 2016-05-08T01:21:55Z

PR didn't actually change, I just rebased and repushed to try to unstick the pr-builder.

k8s-bot · 2016-05-08T10:20:29Z

GCE e2e build/test passed for commit 13a7d92.

k8s-github-robot · 2016-05-08T19:51:48Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-08T20:27:57Z

GCE e2e build/test passed for commit 13a7d92.

k8s-github-robot · 2016-05-08T20:27:57Z

Automatic merge from submit-queue

googlebot added the cla: yes label Apr 15, 2016

k8s-github-robot assigned deads2k Apr 15, 2016

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Apr 15, 2016

cjcullen reviewed Apr 15, 2016
View reviewed changes

cjcullen force-pushed the cacheauth branch from e6271b9 to 6162ba9 Compare April 15, 2016 00:52

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

cjcullen force-pushed the cacheauth branch from 6162ba9 to ce435fc Compare April 22, 2016 06:46

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 22, 2016

deads2k reviewed Apr 22, 2016
View reviewed changes

cjcullen force-pushed the cacheauth branch from ce435fc to bec9e44 Compare April 22, 2016 19:53

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

cjcullen force-pushed the cacheauth branch from bec9e44 to 4252982 Compare May 2, 2016 21:58

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

cjcullen force-pushed the cacheauth branch from 4252982 to d65c4e8 Compare May 3, 2016 01:07

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 3, 2016

cjcullen force-pushed the cacheauth branch from d65c4e8 to 2c79841 Compare May 3, 2016 04:56

cjcullen mentioned this pull request May 3, 2016

e2e flake: Kubernetes e2e suite.Kubectl client Kubectl apply should apply a new configuration to an existing RC #23389

Closed

cjcullen force-pushed the cacheauth branch from 2c79841 to 5d075d2 Compare May 3, 2016 05:37

cjcullen changed the title ~~WIP: Support persisting config from kubecfg AuthProvider plugins~~ Support persisting config from kubecfg AuthProvider plugins May 3, 2016

deads2k reviewed May 3, 2016
View reviewed changes

cjcullen added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 4, 2016

deads2k reviewed May 4, 2016
View reviewed changes

cjcullen force-pushed the cacheauth branch from f8c81fe to 32501b2 Compare May 4, 2016 16:08

deads2k reviewed May 4, 2016
View reviewed changes

cjcullen force-pushed the cacheauth branch 2 times, most recently from 4969774 to beb8515 Compare May 4, 2016 17:16

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2016

bobbyrullo reviewed May 4, 2016
View reviewed changes

bobbyrullo mentioned this pull request May 6, 2016

Implement OIDC client AuthProvider #25270

Merged

k8s-github-robot assigned janetkuo and deads2k and unassigned deads2k and janetkuo May 6, 2016

cjcullen added 2 commits May 7, 2016 18:15

Move ModifyConfig (and some related functions) into the clientcmd pkg.

8629381

Add a ConfigPersister for AuthProvider plugins in kubectl/clients.

13a7d92

cjcullen force-pushed the cacheauth branch from beb8515 to 13a7d92 Compare May 8, 2016 01:15

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

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

k8s-github-robot merged commit f40fe71 into kubernetes:master May 8, 2016

Support persisting config from kubecfg AuthProvider plugins #24304

Support persisting config from kubecfg AuthProvider plugins #24304

Conversation

cjcullen commented Apr 15, 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

cjcullen Apr 22, 2016 • edited Loading

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

Choose a reason for hiding this comment

bobbyrullo commented Apr 22, 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

deads2k commented May 4, 2016

cjcullen commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 4, 2016

cjcullen commented May 4, 2016

Choose a reason for hiding this comment

cjcullen May 4, 2016 • edited Loading

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 May 8, 2016

k8s-bot commented May 8, 2016

k8s-github-robot commented May 8, 2016

k8s-bot commented May 8, 2016

k8s-github-robot commented May 8, 2016

cjcullen Apr 22, 2016 •

edited

Loading

cjcullen May 4, 2016 •

edited

Loading