Command tree and exported env in kubectl plugins #45981

fabianofranz · 2017-05-17T18:39:51Z

This is part of kubectl plugins V1:

Adds support to several env vars passing context information to the plugin. Plugins can make use of them to connect to the REST API, access global flags, get the path of the plugin caller (so that kubectl can be invoked) and so on. Exported env vars include
- KUBECTL_PLUGINS_DESCRIPTOR_*: the plugin descriptor fields
- KUBECTL_PLUGINS_GLOBAL_FLAG_*: one for each global flag, useful to access namespace, context, etc
- ~~KUBECTL_PLUGINS_REST_CLIENT_CONFIG_*: one for most fields in rest.Config so that a REST client can be built.~~
- KUBECTL_PLUGINS_CALLER: path to kubectl
- KUBECTL_PLUGINS_CURRENT_NAMESPACE: namespace in use
Adds support for plugins as child of other plugins so that a tree of commands can be built (e.g. kubectl myplugin list, kubectl myplugin add, etc)

Release note:

Added support to a hierarchy of kubectl plugins (a tree of plugins as children of other plugins).

Added exported env vars to kubectl plugins so that plugin developers have access to global flags, namespace, the plugin descriptor and the full path to the caller binary.

@kubernetes/sig-cli-pr-reviews

fabianofranz · 2017-05-17T19:20:26Z

@k8s-bot bazel test this

fabianofranz · 2017-05-17T21:50:19Z

@monopole a couple pieces for V1 here.

monopole

nice progress

monopole · 2017-05-18T00:42:38Z

pkg/kubectl/plugins/runner.go

+	env = append(env, FieldToEnv("ShortDesc", p.Plugin.ShortDesc, prefix))
+	env = append(env, FieldToEnv("LongDesc", p.Plugin.LongDesc, prefix))
+	env = append(env, FieldToEnv("Example", p.Plugin.Example, prefix))
+	env = append(env, FieldToEnv("Command", p.Plugin.Command, prefix))


Given that one starts from a literal string, might as well use the final result here rather than have all the reflection-y camelcase and period interpretation, i.e.

env = env.append(env, makePair(prefix + "NAME", p.Plugin.Name)) env = env.append(env, makePair(prefix + "SHORT_DESC", p.Plugin.ShortDesc))

that way one can find this code with a code search on SHORT_DESC.

As it is the code has the complexity of reflection without actually reading from variables.

Plus, if FieldToEnvName doesn't exist, it doesn't need test coverage.

monopole · 2017-05-18T01:05:42Z

pkg/kubectl/plugins/loader.go

+				setSource(path, fileInfo, child)
+			}
+		}
+		setSource(path, fileInfo, plugin)


well, this looks clever! if you want to add code for subtrees, please also add test coverage.

command trees and env var propagation are orthogonal. RPs that don't mix orthogonal things are easier to review, right? :)

I have subtrees covered in test-cmd, will also add some unit tests.

I have them in 2 separate commits here, but I'll remember that for the next ones. ;)

monopole · 2017-05-18T01:20:49Z

pkg/kubectl/cmd/plugin.go

+	env = append(env, plugins.FieldToEnv("APIPath", p.cfg.APIPath, prefix))
+	env = append(env, plugins.FieldToEnv("Prefix", p.cfg.Prefix, prefix))
+	env = append(env, plugins.FieldToEnv("Username", p.cfg.Username, prefix))
+	env = append(env, plugins.FieldToEnv("Password", p.cfg.Password, prefix))


This enables a world of perl scripts passing around passwords, certs etc, building libraries that attempt to make restful calls without the benefit of apimachinery client libs.

Wouldn't it be better to fire up a proxy within kubectl, then make that available to the plugin? Pass the (local) port via an env var, and simplify the the job of the plugin author. Maybe hold off on this particular provider?

I'd like to provide plugin authors a way to access the api (other that invoking kubectl through the KUBECTL_PLUGINS_CALLER env we are introducing here), so both ideas work for me. I agree with the concerns of these sensitive data passing around and the proxy is definitely easier for plugin authors, but do you think it would be easier to get approved? If so, that was part of my original plugins R&D and should not be too hard to adjust/improve be added here or in a follow-up PR.

@pwittrock might weigh in. Personally I'd rather approve a PR that set up the proxy for the plugin and fed the plugin what it needed to know about the proxy via env vars, than the above code which is simple but enables bad behavior. A proxy would encourage simpler plugins that followed a particular recognizable pattern.

as you say - you did a bunch of research on the proxy idea already... just dust it off in a separate PR.

monopole · 2017-05-18T02:11:22Z

pkg/kubectl/cmd/plugin.go

+		return []string{}, err
+	}
+	return []string{fmt.Sprintf("%s=%s", "KUBECTL_PLUGINS_CURRENT_NAMESPACE", cmdNamespace)}, nil
+}


Nice job on the env provider interface and various impls! Very clear.

One nit: at this level it makes sense to create an array of pair struct {n, v string} and push the

fmt.Sprintf("%s=%s",

(which occurs several times here) down into runner.Run, converting to the format exec.Command seems to need just before it is invoked. That odd detail shouldn't leak up this high.

I'm +1 about a struct pair, the caveat is that os.Environ() already returns []string and it's used in one of the env providers. I'd need to iterate over it and split to make the pairs that would be returned in the provider list. Do you think it's worth?

Sigh - didn't see that. Up to you. Seems a bit silly to unpack os.Environ.
OTHO, is osEnviron needed? I'd think a plugin running in subprocess has access to exported vars present in the parent process.

Setting os.Environ is needed, the subprocess only takes what you set there.

fabianofranz · 2017-05-19T20:29:58Z

@monopole Thanks for the review, PR updated. Addressing comments, more tests added, and the config env provider was removed. An additional way to access the API (likely through proxy) will be in a separate PR.

monopole · 2017-05-19T22:24:48Z

/lgtm

Thanks!

k8s-github-robot · 2017-05-19T22:39:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, monopole

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [fabianofranz]
~~pkg/kubectl/OWNERS~~ [fabianofranz]
~~test/fixtures/pkg/kubectl/OWNERS~~ [fabianofranz]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fabianofranz · 2017-05-19T23:36:05Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

fabianofranz · 2017-05-20T00:43:55Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-ci-robot · 2017-05-20T01:13:01Z

@fabianofranz: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`18cb56b`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

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.

k8s-github-robot · 2017-05-20T06:29:31Z

Automatic merge from submit-queue (batch tested with PRs 46033, 46122, 46053, 46018, 45981)

Automatic merge from submit-queue (batch tested with PRs 46033, 46122, 46053, 46018, 45981) Command tree and exported env in kubectl plugins This is part of `kubectl` plugins V1: - Adds support to several env vars passing context information to the plugin. Plugins can make use of them to connect to the REST API, access global flags, get the path of the plugin caller (so that `kubectl` can be invoked) and so on. Exported env vars include - `KUBECTL_PLUGINS_DESCRIPTOR_*`: the plugin descriptor fields - `KUBECTL_PLUGINS_GLOBAL_FLAG_*`: one for each global flag, useful to access namespace, context, etc - ~`KUBECTL_PLUGINS_REST_CLIENT_CONFIG_*`: one for most fields in `rest.Config` so that a REST client can be built.~ - `KUBECTL_PLUGINS_CALLER`: path to `kubectl` - `KUBECTL_PLUGINS_CURRENT_NAMESPACE`: namespace in use - Adds support for plugins as child of other plugins so that a tree of commands can be built (e.g. `kubectl myplugin list`, `kubectl myplugin add`, etc) **Release note**: ```release-note Added support to a hierarchy of kubectl plugins (a tree of plugins as children of other plugins). Added exported env vars to kubectl plugins so that plugin developers have access to global flags, namespace, the plugin descriptor and the full path to the caller binary. ``` @kubernetes/sig-cli-pr-reviews

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 17, 2017

fabianofranz force-pushed the kubectl_plugins_v1_part1 branch from f7f3da6 to 6f638d2 Compare May 17, 2017 18:40

k8s-github-robot assigned derekwaynecarr and dims May 17, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 17, 2017

fabianofranz assigned monopole and unassigned derekwaynecarr May 17, 2017

fabianofranz requested review from monopole and soltysh May 17, 2017 18:40

fabianofranz force-pushed the kubectl_plugins_v1_part1 branch 2 times, most recently from 357be18 to a9e790a Compare May 17, 2017 21:18

monopole suggested changes May 18, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2017

fabianofranz force-pushed the kubectl_plugins_v1_part1 branch from a9e790a to 782c46c Compare May 19, 2017 20:24

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2017

Adds support to a tree hierarchy of kubectl plugins

da85262

fabianofranz force-pushed the kubectl_plugins_v1_part1 branch from 782c46c to b718375 Compare May 19, 2017 21:12

kubectl plugins have access config, global flags and environment

18cb56b

fabianofranz force-pushed the kubectl_plugins_v1_part1 branch from b718375 to 18cb56b Compare May 19, 2017 22:17

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

k8s-github-robot merged commit 8fe818b into kubernetes:master May 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command tree and exported env in kubectl plugins #45981

Command tree and exported env in kubectl plugins #45981

fabianofranz commented May 17, 2017 •

edited

Loading

fabianofranz commented May 17, 2017

fabianofranz commented May 17, 2017

monopole left a comment

monopole May 18, 2017

monopole May 18, 2017

fabianofranz May 18, 2017

monopole May 18, 2017

fabianofranz May 19, 2017

monopole May 18, 2017

fabianofranz May 18, 2017

monopole May 18, 2017

monopole May 18, 2017

monopole May 18, 2017

fabianofranz May 18, 2017

monopole May 18, 2017

fabianofranz May 19, 2017

fabianofranz commented May 19, 2017

monopole commented May 19, 2017

k8s-github-robot commented May 19, 2017

fabianofranz commented May 19, 2017

fabianofranz commented May 20, 2017

k8s-ci-robot commented May 20, 2017

k8s-github-robot commented May 20, 2017

Command tree and exported env in kubectl plugins #45981

Command tree and exported env in kubectl plugins #45981

Conversation

fabianofranz commented May 17, 2017 • edited Loading

fabianofranz commented May 17, 2017

fabianofranz commented May 17, 2017

monopole left a 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

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

fabianofranz commented May 19, 2017

monopole commented May 19, 2017

k8s-github-robot commented May 19, 2017

fabianofranz commented May 19, 2017

fabianofranz commented May 20, 2017

k8s-ci-robot commented May 20, 2017

k8s-github-robot commented May 20, 2017

fabianofranz commented May 17, 2017 •

edited

Loading