MVP Plugins for Service-Catalog Interaction #1621

jberkhahn · 2017-12-15T22:08:36Z

This PR adds a basic suite of plugins for interacting with service-catlaog resources such as brokers and instances. It includes List and Get commands for bindings, brokers, classes, instances, and plans using the human-readable external names, with output in human-readable tables.

to compile the plugins:

make plugins

to use the plugins:

export KUBECTL_PLUGINS_PATH=$PWD/bin
kubectl plugin

Alternatively, compile the plugins and then move the plugin dirs from service-catalog/bin to your .kube/plugins directory.

Signed-off-by: Doug Davis <dug@us.ibm.com>

Signed-off-by: Jonathan Berkhahn <jaberkha@us.ibm.com>

duglin · 2017-12-15T23:18:34Z

Makefile

@@ -110,7 +110,8 @@ NON_VENDOR_DIRS = $(shell $(DOCKER_CMD) glide nv)
 #########################################################################
 build: .init .generate_files \
 	$(BINDIR)/service-catalog \
-	$(BINDIR)/user-broker
+	$(BINDIR)/user-broker \
+	   plugins


space issue

duglin · 2017-12-15T23:21:13Z

plugin/cmd/kubectl/binding/binding.go

+`
+
+const getUsage = `Usage:
+  kubectl plugin binding get NAMESPACE INSTANCENAME


Minor thing, but INSTANCENAMEwould be easier to read asINSTANCE_NAME`. Same for other 2-word thingies.

pmorie · 2017-12-15T23:22:30Z

@fabianofranz @jvallejo @jwforres @luksa

…

On Fri, Dec 15, 2017 at 6:18 PM, Doug Davis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Makefile <#1621 (comment)> : > @@ -110,7 +110,8 @@ NON_VENDOR_DIRS = $(shell $(DOCKER_CMD) glide nv) ######################################################################### build: .init .generate_files \ $(BINDIR)/service-catalog \ - $(BINDIR)/user-broker + $(BINDIR)/user-broker \ + plugins space issue — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1621 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWXmPXoVJe-O0HojDKqlOTczCWojxXdks5tAv5LgaJpZM4REDsm> .

duglin · 2017-12-15T23:27:32Z

plugin/cmd/kubectl/binding/binding.go

+		namespace := os.Args[2]
+		bindings, err := client.ListBindings(namespace)
+		if err != nil {
+			utils.Exit1(fmt.Sprintf("Unable to list bindings in namespace %s (%s)", namespace, err))


use %q for the first one so its quoted and its clear that the word isn't part of a sentence but rather something special.

duglin · 2017-12-16T15:00:34Z

plugin/pkg/kubectl/client/binding.go

+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+//GetBinding retrieves  a binding by external name in a given namespace


nit but please add a space between // and GetBinding

duglin · 2017-12-16T15:08:41Z

@jberkhahn eventually there is a very good possibility that we're going to have cluster and namespace specific versions of some of these resources (e.g. ClusterServiceClass vs ServiceClass), when we do how do you envision the cmd line looking? Will the scoping be done via the object type or via the presence/absence of a --namespace type of flag?

Aside from just trying to see what things will look like in the future, I'm wondering if the use of names like "Class" instead of "ServiceClass" (or even "ClusterServiceClass") is something we'll need to reconsider as we think about cluster vs ns versions. Even w/o that concern, for consistency I'm wondering if we should use the same terms as what people will see in the yaml - meaning ServiceClass instead of just Class - switching terms can be confusing (or annoying) for people who are trying to learn these concepts. But I'd like to hear what other's think.

duglin · 2017-12-16T15:13:33Z

plugin/pkg/kubectl/client/plugin_client.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


Not 100% sure this is the file for this, but... it might be good to add a comment about how some of this will be removed when your CLI PR is merged and we have a common library that we can leverage - and a URL to the PR too. Just so people can make the linkage and see where we're headed.

duglin · 2017-12-16T15:16:09Z

plugin/pkg/kubectl/client/plugin_client.go

+		kubeconfig = configFile
+	} else if len(kubeConfigFile) > 0 {
+		kubeconfig = kubeConfigFile
+	}


Wow - there are 3 different env vars that might point to the kubeconfig file? That's not confusing at all :-)
I know that's not your doing but do you know why?

duglin · 2017-12-16T15:21:25Z

plugin/pkg/kubectl/client/plugin_client.go

+}
+
+func applyGlobalOptionsToConfig(config *restclient.Config) error {
+	// impersonation config


Can you add more comments here to explain what this is about? It seems like this is a plugin specific feature to impersonate someone, but its not clear to me why this feature exists within plugins. To me (at least for our stuff) a plugin is just a helper utility for the normal kubectl operations. So adding the concept of impersonation feels more like something that would be at the kubectl level and not scoped to a plugin.

duglin · 2017-12-16T15:23:04Z

plugin/pkg/kubectl/client/plugin_client.go

+	}
+	// tls config
+
+	caFile := os.Getenv("KUBECTL_PLUGINS_GLOBAL_FLAG_CERTIFICATE_AUTHORITY")


I think I may need more education because (like the impersonation stuff), it seems odd to me that plugins have their own mechanism that appear to be duplicating what kubectl does - like this CA stuff. Why isn't the plugin logic just using the same data that kubectl uses?

To this and the above - I'm not 100% sure. These flags appear to be something passed from the kubectl plugin framework if it wants to execute a plugin as a different user, or in a different context than the calling environment. As to why someone would want to do that - I don't know.

duglin · 2017-12-16T15:25:50Z

plugin/pkg/kubectl/utils/utils.go

+	// resolve kubeconfig location, prioritizing the --config global flag,
+	// then the value of the KUBECONFIG env var (if any), and defaulting
+	// to ~/.kube/config as a last resort.
+	home := os.Getenv("HOME")


Does this work on windows? I remember Docker having to go thru a ton of hoops to get this info across various platforms. Additionally, we should check to make sure we can use "/" instead of os.PathSeparator

check out https://github.com/mitchellh/go-homedir. my head is full of other things right now so I don't recall if it works on windows, but it avoids cgo and that's a start

duglin · 2017-12-16T15:28:43Z

plugin/pkg/kubectl/utils/ui.go

+}
+
+// Ok will print "OK" in green
+func Ok() {


Where are these color funcs used? I couldn't find Ok() being called. But if they are used, can we make them controlled by some flag since there are times when those color escape code play havoc with people trying to grep for stuff so there has to be some way to turn them off - or even better, only default to 'on' when there's a tty.

fabianofranz · 2017-12-18T20:02:26Z

What's the reasoning behind adding a plugin that does list and get at the resource level, which is exactly what kubectl get and kubectl describe do, instead of trying to make the existing commands work properly for these resources? This is redoing a lot of things that kubectl already does, while changing the VERB NOUN (e.g. get|describe binding) convention we try to follow to NOUN VERB (binding get|list).

I'm definitely +1 about having a plugin for the service catalog, but the MVP would be basically for

commands we don't support today without having to write yaml/json files (e.g. kubectl create servicebroker|serviceinstance|servicebinding|etc)
get/list commands, but providing a higher level interaction over the service catalog in comparison to what browsing API resources give you, targeted specifically at end users. E.g. a very high-level command like kubectl catalog or kubectl servicecatalog that gives you an overview of the services available, with plan and provider information, etc.

fabianofranz · 2017-12-18T20:04:25Z

plugin/cmd/kubectl/binding/binding.go

@@ -0,0 +1,86 @@
+/*
+Copyright 2016 The Kubernetes Authors.


fabianofranz · 2017-12-18T20:06:51Z

plugin/cmd/kubectl/binding/binding.go

+`
+
+const getUsage = `Usage:
+  kubectl plugin binding get NAMESPACE INSTANCENAME


Why not -n, --namespace= instead of an arg?

I think it depends on whether its a required arg or not. As of now its required. I know some other kubectl commands use --arg for required fields, but personally, I find that very odd.

Having said all of that, I think we do need to look ahead and think about how we're going to deal with cluster and NS scoped resources. That's when an optional --namespace might come into play.

Even if it's required for the processing, it should not be required for users to provide. In kubectl every command that is namespaced defaults to the current "in use" namespace if you don't provide it explicitly, and plugins should do the same for consistency.

The KUBECTL_PLUGINS_CURRENT_NAMESPACE will give you the namespace you must consider in the call, ready to use, with all the processing related to config file and env var precedence processed.

Ah good point - I forgot that kubectl has the notion of the current/default namespace. I would,however, prefer to use kubectl's and not create a new plugin-specific current/default namespace. To me plugins are extending kubectl, not an alternative to it, so whatever defaults kubectl has should apply to plugins as well.

Exactly! So just use the value in KUBECTL_PLUGINS_CURRENT_NAMESPACE and you get what kubectl considered as the namespace for the current call, with defaults, flags and env vars already processed.

I'm confused. Isn't KUBECTL_PLUGINS_CURRENT_NAMESPACE a plugin thing instead of the default kubectl namespace? Shouldn't we grab it from the current context or something like that?

It is what plugins must consider as the namespace to use in the current call. If you need a namespace, just use the value set in that env var as is and you're good.

kubectl does all its namespace processing (take the default namespace, kubeconfig, what user provided through -n, etc), processes the precedence logic, and sets the result in that env var. This way plugin writers don't have to duplicate all this logic.

I'm not suggesting we duplicate it, we should just use it. Part of what @jberkhahn is doing is creating a shared library kind of thing and I would think the getting the current NS would be a util in there that both kubectl and plugins can leverage. IOW, the "current namespace" should yield the same answer for kubectl as it does for the plugin.

fabianofranz · 2017-12-18T20:11:28Z

plugin/pkg/kubectl/client/plugin_client.go

+	home := os.Getenv("HOME")
+	kubeconfig := home + "/.kube/config"
+
+	kubeconfigEnv := os.Getenv("KUBECONFIG")


This logic was already processed in kubectl, you should just rely on the plugin-specific env vars (KUBECTL_PLUGINS_*) you get.

I was actually unaware of that, thanks for the heads up. I'd like to try to limit discussion on the plugin client to my PR here as the client here in service-catalog is strictly temprorary.. Once the PR to kubernetes/kubectl gets merged I'll be switching this plugin to use the client over there.

fabianofranz · 2017-12-18T20:13:09Z

plugin/pkg/kubectl/client/plugin_client.go

+	return &pluginClient, nil
+}
+
+func clientFromConfig(path string) (*restclient.Config, string, error) {


All of this is already done in kubectl. We should make it reusable (likely part of client-go).

fabianofranz · 2017-12-18T20:13:51Z

plugin/pkg/kubectl/client/plugin_client.go

+	return config, namespace, nil
+}
+
+func applyGlobalOptionsToConfig(config *restclient.Config) error {


fabianofranz · 2017-12-18T20:14:22Z

plugin/pkg/kubectl/utils/ui.go

+
+// Green will print the specified string in green text
+func Green(str string) string {
+	return fmt.Sprintf("\x1b[32;1m%s\x1b[0m", str)


Doesn't work on Windows.

There are several packages for printing colorized output. For example: https://godoc.org/github.com/fatih/color

Can you use one of those instead of rolling your own here

arschles

Repeating what I said in our weekly meeting so it's written down - I don't like the UX for plugins and I think we shouldn't commit to them right now. I've also heard some differing opinions on the future and intended use of plugins.

I am happy to write down my criteria for adopting plugins for service catalog if folks would like.

duglin · 2017-12-19T17:25:08Z

Yes I'd be interested in seeing those

pmorie · 2017-12-20T01:04:39Z

@arschles

I am happy to write down my criteria for adopting plugins for service catalog if folks would like.

Please do, I am very interested in seeing them as soon as possible. There is likely to be flux in the kubectl plugin story and there are tons of unsolved problems. We should be good citizens here and ensure that we help to nudge plugins in the direction we want them to go.

fabianofranz · 2017-12-21T16:08:52Z

@arschles I'm hearing this just now, have you expressed your concerns to SIG CLI? Where/when?

jwforres · 2018-01-04T19:02:11Z

bumping the discussion now that folks are back from the holidays - @arschles what are your requirements for adopting plugins / what are the problems in the current proposal of plugins. These issues really need to be addressed with SIG CLI as soon as possible

cc @deads2k

arschles · 2018-01-05T19:26:40Z

plugin/pkg/kubectl/client/fakes/fake_servicecatalogv1beta1_interface.go

+	}
+	invocations      map[string][][]interface{}
+	invocationsMutex sync.RWMutex
+}


can you use the standard fakes in the generated svc-cat code instead of this?

arschles · 2018-01-05T19:30:56Z

plugin/pkg/kubectl/utils/ui.go

+
+// Green will print the specified string in green text
+func Green(str string) string {
+	return fmt.Sprintf("\x1b[32;1m%s\x1b[0m", str)


There are several packages for printing colorized output. For example: https://godoc.org/github.com/fatih/color

Can you use one of those instead of rolling your own here

arschles · 2018-01-05T19:32:56Z

plugin/pkg/kubectl/utils/utils.go

+	// resolve kubeconfig location, prioritizing the --config global flag,
+	// then the value of the KUBECONFIG env var (if any), and defaulting
+	// to ~/.kube/config as a last resort.
+	home := os.Getenv("HOME")


check out https://github.com/mitchellh/go-homedir. my head is full of other things right now so I don't recall if it works on windows, but it avoids cgo and that's a start

jberkhahn · 2018-01-19T21:54:53Z

Closing this in favor of #1664

Simon Leung and others added 18 commits December 15, 2017 12:27

kubectl plugin create-service-broker

d0f3881

kubectl plugin create-service-instance

2642f9d

kubectl plugin bind-service

833f379

Add plugins to Makefile and fix lint issues

807c7b3

Signed-off-by: Doug Davis <dug@us.ibm.com>

Fix some more "verify" issues

a7b5086

Signed-off-by: Doug Davis <dug@us.ibm.com>

rebase based on name change

bf4dedd

use standard build

8d1cf3e

remove hard coded test url

a81afa9

Update plugins to use v1beta1

8113214

Signed-off-by: Jonathan Berkhahn <jaberkha@us.ibm.com>

Add relist broker plugin

412c1b2

Signed-off-by: Jonathan Berkhahn <jaberkha@us.ibm.com>

remove use of kubectl proxy for api calls

74e55b4

acknowledge value of KUBECTL_PLUGINS_CURRENT_NAMESPACE

a84e54f

use KUBECONFIG to create clients

dd3eb7d

handle global config flags

ffbbf61

Refactor relist-service-broker to work with changes

2a41027

Fix bugs in plugin output

9d3e5bd

Add plugin for clusterservicebroker interaction

0586082

MVP plugins for service-catalog interaction

fe0b140

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 15, 2017

MHBauer requested review from arschles, duglin and MHBauer December 15, 2017 22:21

jberkhahn force-pushed the plugin-cmds branch 3 times, most recently from a79383e to f81ba29 Compare December 15, 2017 23:11

duglin reviewed Dec 15, 2017

View reviewed changes

duglin reviewed Dec 16, 2017

View reviewed changes

fabianofranz reviewed Dec 18, 2017

View reviewed changes

plugin/cmd/kubectl/binding/binding.go Outdated

@@ -0,0 +1,86 @@

/*

Copyright 2016 The Kubernetes Authors.

Copy link

fabianofranz Dec 18, 2017

Choose a reason for hiding this comment

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

2017

fabianofranz reviewed Dec 18, 2017

View reviewed changes

Formatting changes

3ca40a8

arschles reviewed Dec 19, 2017

View reviewed changes

pmorie mentioned this pull request Jan 4, 2018

Kubectl plugin for service catalog #840

Closed

arschles suggested changes Jan 5, 2018

View reviewed changes

jberkhahn added 3 commits January 15, 2018 17:02

update color utils to be platform independent

59c3bc4

refactor plugin tests to use generated fakes

fa9fbce

fix dependencies for color change

fba2ea0

jberkhahn force-pushed the plugin-cmds branch from 367007b to fba2ea0 Compare January 17, 2018 19:39

Find home dir in a cross-platform compatible way

961883c

jberkhahn closed this Jan 19, 2018

MVP Plugins for Service-Catalog Interaction #1621

MVP Plugins for Service-Catalog Interaction #1621

Conversation

jberkhahn commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Dec 15, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Dec 16, 2017

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

jberkhahn Dec 18, 2017 • edited

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 Dec 18, 2017

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

Choose a reason for hiding this comment

arschles left a comment

Choose a reason for hiding this comment

duglin commented Dec 19, 2017

pmorie commented Dec 20, 2017 • edited

fabianofranz commented Dec 21, 2017

jwforres commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Jan 19, 2018

jberkhahn Dec 18, 2017 •

edited

pmorie commented Dec 20, 2017 •

edited