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

remove use of kubectl proxy for api calls #1

Closed

Conversation

juanvallejo
Copy link

Work in progress.
Main idea is to remove the use of kubectl proxy... and just to as much as possible through kubectl get --raw.

cc @fabianofranz

@fabianofranz
Copy link

fabianofranz commented Sep 22, 2017

@juanvallejo

Note that the call to kubectl get --raw must respect some context that may have been passed through global flags and won't be present in your exec call by default unless you pass it.

For example, an user called something like kubectl plugin foo --namespace myns -v 8. You may need to pass these global flags (available to the plugin through env vars) in your exec call. Other global flags might also apply.

@juanvallejo juanvallejo force-pushed the simonleung8/plugin-cmds branch from 9c351dd to a3bdc6f Compare September 22, 2017 19:04
@juanvallejo
Copy link
Author

juanvallejo commented Sep 22, 2017

@fabianofranz

Note that the call to kubectl get --raw must respect some context that may have been passed through global flags and won't be present in you exec call by default unless you pass it.

thanks for pointing this out, I have updated the kubectl call to include --namespace. Also patched plugins to receive a namespace value through the KUBECTL_PLUGINS_CURRENT_NAMESPACE env var, rather than a plugin argument.

Also, updated patch to honor -v.

Other global flags might also apply.

According to this section in the docs, I could retrieve any other global flag values through KUBECTL_PLUGINS_GLOBAL_FLAG_*. Do we have a list of any additional global flags
that a plugin should honor? Should the global flags that a plugin honors be the intersection of global flags offered between oc and kubectl? How should we handle flag name differences (such as --v vs. --loglevel)?

@juanvallejo juanvallejo force-pushed the simonleung8/plugin-cmds branch 3 times, most recently from 9766a97 to ca9808f Compare September 22, 2017 19:24
@fabianofranz
Copy link

Do we have a list of any additional global flags that a plugin should honor?

We don't and it would depend on each plugin's role, but most plugins should honor as much as possible, if not all. For example, if I make a call to a given plugin with --as, --as-group, --cache-dir, --cluster, --context, --match-server-version, and so on; I'd expect the underlying kubectl get call to respect all of them. The good news is that it's probably just a matter of adding all flags to the get call with the values you received through the env vars.

Should the global flags that a plugin honors be the intersection of global flags offered between oc and kubectl? How should we handle flag name differences (such as --v vs. --loglevel)?

The plugin framework makes no assumptions or validations about the set of flags it's supposed to support, it just blindly passes through an env var every global flag available in the given version of the caller. So if you want to make your plugin support both oc and kubectl (and even different versions of a same caller that might have added or removed flags) without having multiple distributions of your plugin, you have to handle it in the plugin. For example, check if the LOGLEVEL version of the env var was sent to you, otherwise fallback to V.

Of course there's another front we could think about proposing which is making flag names conform between the projects (or at least have aliases).

@juanvallejo
Copy link
Author

@fabianofranz Thanks for the response

We don't and it would depend on each plugin's role, but most plugins should honor as much as possible, if not all. For example, if I make a call to a given plugin with --as, --as-group, --cache-dir, --cluster, --context, --match-server-version, and so on; I'd expect the underlying kubectl get call to respect all of them. The good news is that it's probably just a matter of adding all flags to the get call with the values you received through the env vars.

Maybe we could, in the future, think about making a package of common utils that could be used by all plugins, to further abstract some common logic handling?

  • global flag handling
  • flag support between multiple versions of oc and kubectl
  • making --raw calls using the KUBECTL_PLUGINS_CALLER and handling the output
  • handling and printing potentially large outputs from stdout / stderr

@fabianofranz
Copy link

Maybe we could, in the future, think about making a package of common utils

Definitely, at least for Go and maybe a couple others (shell script, etc).

@juanvallejo juanvallejo force-pushed the simonleung8/plugin-cmds branch from ca9808f to 128389f Compare September 22, 2017 20:40
func KubeGet(endpoint string) (stdout, stderr io.ReadCloser, err error) {
v, logLevel := Loglevel()
caller := os.Getenv("KUBECTL_PLUGINS_CALLER")
kubeCmd := exec.Command(caller, "--namespace", Namespace(), v, logLevel, "get", "--raw", "/api/v1/"+endpoint)
Copy link
Author

Choose a reason for hiding this comment

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

@fabianofranz A shortcoming with this patch that I have recently noticed, is that for cases like this one, where we use get --raw to see if a namespace exists, the success of the command is entirely dependent on whether or not the currently logged in user in kubeconfig has permission to list all namespaces in the cluster

@juanvallejo
Copy link
Author

@fabianofranz I am now handling the following global options when obtaining a client config:

--as
--as-group
--certificate-authority
--client-certificate
--client-key
--cluster # not sure where to override this
--context # not sure where to override this
--user # not sure where to override this
--config
--kubeconfig
--namespace
--password
--request-timeout
--server
--token
--user

The following flags are not being handled as of the latest commit.
I am either not sure how to handle them, or if they apply to our
use-case with the service catalog:

--insecure-skip-tls-verify # the default value received from the caller is `true`
--log-flush-frequency
--alsologtostderr
--loglevel
--logspec
--log-backtrace-at
--logtostderr
--match-server-version
--stderrthreshold
--v
--vmodule

@jberkhahn
Copy link

Hey there @juanvallejo

I picked up Simon's work on the service catalogue plugins. Unfortunately I don't have access to this repo so I've forked it over at github.com/jberkhahn/service-catalog

I'm interested in your work, but it unfortunately seems like it needs to be rebased again due to code drift. If you'd be up for rebasing on top of my branch, that would be great. If not, I might try and cherry pick in some of the utils you've written.

Thanks,

@juanvallejo
Copy link
Author

Hi @jberkhahn, sure will open a PR against your branch shortly that replays these patches onto your changes

@juanvallejo
Copy link
Author

Closing in favor of jberkhahn#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants