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

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

Closed

Conversation

jdef
Copy link
Contributor

@jdef jdef commented Feb 19, 2016

there's a need for customizing kubectl config at runtime via plugins. kubectl flag parsing and overrides, config loading rules, etc. are complex and we need to keep the default case as clear as possible:

  • all kubectl plugins are registered, every time
  • KUBECTL_PLUGINS envvar may contain a comma-separated list of plugin names for kubectl to "activate" at runtime.
  • only activated plugins should provide options to kubectl
  • mesosphere/acs plugin as an example, injects custom HTTP headers into requests

if there's agreement that this approach is OK then I'll write up some unit tests to go along with it

/cc @cjcullen @roberthbailey @jlowdermilk

supersedes #21527

@jdef jdef added this to the v1.2-candidate milestone Feb 19, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 19, 2016
@jdef
Copy link
Contributor Author

jdef commented Feb 19, 2016

@k8s-bot test this issue: #21463

@k8s-bot
Copy link

k8s-bot commented Feb 19, 2016

GCE e2e test build/test passed for commit a9ef36c.

@bgrant0607
Copy link
Member

cc @smarterclayton @kubernetes/kubectl

return err
}
}
pluginEnv := os.Getenv("KUBECTL_PLUGINS")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why wouldn't we activate plugins from config or via presence of an on disk marker? Why environment instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implementing this outside of config gives a plugin the change to alter the config loading process/rules. specifically w/ respect to flag processing. if config plugin activation was driven by a config file ... it would seem to further complicate the loading rules for config processing. for example, if I wanted to add a --dcos-token flag override how would I do that? require another pass over the config file(s) after the plugins have been activated?

@bgrant0607
Copy link
Member

@jdef Examples of what you'd like to do with this?

Ref #13606

@bgrant0607
Copy link
Member

cc @kubernetes/sig-api-machinery

@jdef
Copy link
Contributor Author

jdef commented Feb 23, 2016

@bgrant0607

use case 1: mesosphere's DCOS uses a proprietary authentication scheme.
we're trying to avoid building a dcos-kubectl separate binary just to
support that. it's likely that the configuration scheme may change over
time, or that we may add additional authentication schemes.

use case 2: DCOS users don't want to manage kubecfg files directly, we want
to do that on their behalf. as such, it could be useful for a plugin to
dynamically modify the search path for kubecfg files prior to actually
loading the config.

use case 3: it's possible that DCOS tokens may time out and that upon
kubectl invocation the plugin needs to prompt the user for additional
credentials (this is has not been prototyped here).

the code sketch here uses DCOS_TOKEN as a method of consuming a token from
the environment (feels dirty, but seemed good enough for a code sketch).
Instead, I could have written up support for a flag and consumed the token
that way. This helps to explain the plugin calls I added before the actual
loading of the default configuration: plugins could manipulate the flags
structure to accept additional config parameters (and/or modify the loading
rules, as per use case 2).

I didn't see a way to add arbitrary metadata into a config file (perhaps
there is a way, but I missed it?)

from the conv on slack it appeared that @cjcullen was also interested in
support a custom authentication scheme.

On Mon, Feb 22, 2016 at 2:35 PM, Brian Grant notifications@github.com
wrote:

@jdef https://github.com/jdef Examples of what you'd like to do with
this?

Ref #13606 #13606


Reply to this email directly or view it on GitHub
#21553 (comment)
.

@cjcullen
Copy link
Member

@bgrant0607 Yeah, I'd like to be able to pass gcloud credentials in my Authorization header (see my comment in the commit).

I think this would be much cleaner as a config-driven plugin system. My thoughts were that we'd add a field in the user object of the kubecfg file. E.g.:

apiVersion: v1
clusters:
- cluster:
    certificate-authority: ~/clusterca.crt
    server: https://1.2.3.4
  name: my-cluster
contexts:
- context:
    cluster: my-cluster
    user: my-user
  name: my-context
users:
- name: my-user
  user:
    auth-plugin: dcos

The "dcos" plugin could then do whatever it needed to the transport (in the "google" case, we'd use the Google ApplicationDefaultCredentials library to get a valid bearer token).

@jdef This would work for your Use Case 1 and Use Case 3.
I don't understand your Use Case 2 yet. Why would a plugin need to use a different kubeconfig file? If that were the case, shouldn't that be out of the scope of what's in a kubeconfig file?

@jdef
Copy link
Contributor Author

jdef commented Feb 24, 2016

Agreed, use case #2 could be solved in other ways.

With respect to where you have "auth-plugin" in the above config, why is it
scoped by "user"? Wouldn't it make more sense to be part of the "cluster"
scope?

On Tue, Feb 23, 2016 at 6:50 PM, CJ Cullen notifications@github.com wrote:

@bgrant0607 https://github.com/bgrant0607 Yeah, I'd like to be able to
pass gcloud credentials in my Authorization header (see my comment
#21553 (comment)
in the commit).

I think this would be much cleaner as a config-driven plugin system. My
thoughts were that we'd add a field in the user object of the kubecfg file.
E.g.:

apiVersion: v1
clusters:

  • cluster:
    certificate-authority: ~/clusterca.crt
    server: https://1.2.3.4
    name: my-cluster
    contexts:
  • context:
    cluster: my-cluster
    user: my-user
    name: my-context
    users:
  • name: my-user
    user:
    auth-plugin: dcos

The "dcos" plugin could then do whatever it needed to the transport (in
the "google" case, we'd use the Google ApplicationDefaultCredentials
https://developers.google.com/identity/protocols/application-default-credentials
library to get a valid bearer token).

@jdef https://github.com/jdef This would work for your Use Case 1 and
Use Case 3.
I don't understand your Use Case 2 yet. Why would a plugin need to use a
different kubeconfig file? If that were the case, shouldn't that be out of
the scope of what's in a kubeconfig file.


Reply to this email directly or view it on GitHub
#21553 (comment)
.

@cjcullen
Copy link
Member

I leaned towards user-scoped because it felt like it paralleled the current setup. We don't prescribe that client-cert/token/password-auth is chosen per-cluster; we let a single cluster have multiple users that authenticate in different ways. On the server-side, we provide chained authentication plugins, so allowing different client-plugins for a single cluster would mirror that.

That said, it doesn't seem crazy to me to have it cluster-scoped. I think it comes down to whether we want multiple users of a cluster to be able to use different auth modes. Most use-cases would probably be fine with a single client-plugin per cluster for now, but it seems like we might be painting ourselves into a corner.

// NewFactory creates a factory with the default Kubernetes resources defined
// if optionalClientConfig is nil, then flags will be bound to a new clientcmd.ClientConfig.
// if optionalClientConfig is not nil, then this factory will make use of it.
func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory {
func NewFactory(optionalClientConfig clientcmd.ClientConfig, configOptions ...ConfigOption) *Factory {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is how we should be doing this. If this does become a recommended way to change the kubectl factory, I think the code is more coherent if a certain kind of plugin can provide a different factory implementation. This is actually why the factory exists: it is the way to specify behavior that is otherwise global to kubectl commands.

Trying to handle this as an argument that "flavors" the return value for "the one area I want to touch" effectively means passing this list to every bit down the chain of factory creation. This makes it easy for the one you're adding, but structurally harder for everyone following you.

@k8s-github-robot
Copy link

@jdef PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2016
@cjcullen
Copy link
Member

@jdef I put together #23066 to give an idea of how I thought this could be done less intrusively. I think what I did works for your use cases 1 & 3.

@jdef
Copy link
Contributor Author

jdef commented Mar 16, 2016

yep, just finished review. thanks!

On Wed, Mar 16, 2016 at 2:38 PM, CJ Cullen notifications@github.com wrote:

@jdef https://github.com/jdef I put together #23066
#23066 to give an idea of
how I thought this could be done less intrusively. I think what I did works
for your use cases 1 & 3.


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

@smarterclayton smarterclayton removed their assignment Apr 1, 2016
@bgrant0607 bgrant0607 assigned cjcullen and unassigned dchen1107 Jul 19, 2016
@bgrant0607
Copy link
Member

@cjcullen Can this be closed?

@jdef
Copy link
Contributor Author

jdef commented Jul 19, 2016

closing this out, needs a new PR based on #23066

@jdef jdef closed this Jul 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet