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

Support automatically port forwarding to flux instances in a Kubernetes cluster. #1212

Merged
merged 2 commits into from Jul 13, 2018

Conversation

@justinbarrick
Copy link
Collaborator

justinbarrick commented Jul 8, 2018

Simplify accessing flux instances in a Kubernetes cluster by automatically creating
port forwards, just like Helm does.

This is based on the port forward implementations from kubectl and Helm:

The flow is:

  • If neither --flux-namespace nor FLUX_NAMESPACE are set, then this code will do nothing
    and everything will work as before.
  • If it is, it will instantiate a Kubernetes client and search for pods in FLUX_NAMESPACE
    that have the name=flux labels set.
  • If the number of pods with name=flux in the namespace are not exactly 1, it will
    return an error.
  • It will find an empty port (by first binding to port 0 and then closing the port) and
    start a port forward on that port that forwards to the flux pod on port 3030.
  • It will set the flux url to http://127.0.0.1:$port/api/flux.
  • The port forward goroutine will get cleaned up automatically when fluxctl terminates.

Use-cases:

  • Easier use of standalone flux instances.
  • Discourage exposing flux apis to the internet (hopefully nobody does this).
  • More easily manage large numbers of flux instances.
return err
}

go pf.ForwardPorts()

This comment has been minimized.

Copy link
@justinbarrick

justinbarrick Jul 8, 2018

Author Collaborator

Errors from ForwardPorts() should be handled somehow.

@justinbarrick

This comment has been minimized.

Copy link
Collaborator Author

justinbarrick commented Jul 8, 2018

Fission also has a port forward implementation: https://github.com/fission/fission/blob/master/fission/portforward/portforward.go

This seems like something useful to a lot of projects, so I think it would be better for me to factor this out into a separate library and pull that in here.

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Jul 9, 2018

This is a great idea! If you don't mind a bit of back and forth, I have some suggestions.

For fluxd we tend to prefix flags that pertain to Kubernetes machinery with --k8s-. Hitherto this hasn't come up with fluxctl, but it'd be nice to be consistent, if we can do so without the flag name(s) getting ridiculous (ever a danger).

There's an additional thing that might need a flag: Weave Cloud installations of fluxd use the name weave-flux-agent rather than flux in the deployment.

I can think of a couple of alternatives that cover these off (given with defaults):

  • --k8s-forward-namespace=<taken from kubeconfig> and --k8s-forward-name=flux; if either of these are mentioned, FLUX_URL and --url are ignored in favour of using a port forward. To use port forwarding with the defaults, you can use the boolean flag --k8s-forward (which could have a shorthand). The environment variables FLUX_FORWARD_NAMESPACE, FLUX_FORWARD_NAME, and FLUX_FORWARD act as equivalents.

Upside: accounts for both name and namespace. Downside: getting a little on the verbose side.

  • --k8s-forward-resource=default:deploy/flux switches port forwarding on, to the resource so identified (using flux's resource ID syntax). To forward to the default resource, use the boolean flag --k8s-forward. If either is set, FLUX_URL and --url are ignored.

Upside: you can supply the namespace and name in one go. Downside: uses a non-standard syntax for the ID; you have to supply both namespace and name if you want to supply either.

I'm not totally convinced by either of these, to be quite honest. But maybe we can come up with a synthesis (or you can argue that your formulation is better than both!)

This seems like something useful to a lot of projects, so I think it would be better for me to factor this out into a separate library and pull that in here.

That sounds sensible. I'd happily have it vendored here, then perhaps others could follow suit.

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Jul 9, 2018

Another thing that occurred to me: it'd be wonderful if this were the default mode of operation, so people have no reason to be tempted to expose the API. This is a bit tricky though, since we'd have to account for people that have already made shell scripts, etc. Though I am not adverse to a small amount of breakage in the interest of encouraging people to use port-forwarding instead of e.g., a NodePort, when either would work.

@justinbarrick

This comment has been minimized.

Copy link
Collaborator Author

justinbarrick commented Jul 9, 2018

I also saw name=fluxd on some old documentation on your website just now, too. So I’ll have it search for a couple labels if you don’t specify one.

I’m not sure which command line I prefer. The first one is nice because if we are searching by label hopefully all you need to change is the namespace so it’s less verbose. But the second one keeps in line with the rest of flux and is more flexible since it lets you pass in a resource name (especially if you have more than one flux in a namespace).

I wasn’t sure about making it the default since it looks like weavcloud connect through the Weave Cloud API. Is that not the case?.

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Jul 9, 2018

I’m not sure which command line I prefer. The first one is nice because if we are searching by label hopefully all you need to change is the namespace so it’s less verbose. But the second one keeps in line with the rest of flux and is more flexible since it lets you pass in a resource name (especially if you have more than one flux in a namespace).

On balance I think I like the first one, since it's progressive -- you can just flip the switch, or you can supply a namespace, or you can supply the whole lot if you need to.

I wasn’t sure about making it the default since it looks like weavcloud connect through the Weave Cloud API. Is that not the case?.

That is the case, yup. You have to supply an auth token as well (either with --token,-t or FLUX_SERVICE_TOKEN or WEAVE_CLOUD_TOKEN), so we can in principle detect when people have it set up that way. We would get a false positive in cases that people have set up an ingress or NodePort or the like; those ones I think I'd be prepared to break, temporarily, for the greater good. We can deal with changing the default after this PR, though -- I don't want to pile more requirements on.

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Jul 9, 2018

I also saw name=fluxd on some old documentation on your website just now, too. So I’ll have it search for a couple labels if you don’t specify one.

I think I have conflated .metadata.name=flux and selection by label -- does that change anything?

@justinbarrick justinbarrick force-pushed the justinbarrick:master branch from 3262bee to 88cfecd Jul 12, 2018
@justinbarrick

This comment has been minimized.

Copy link
Collaborator Author

justinbarrick commented Jul 12, 2018

Okay, I switched to the vendored library. That means the last thing here (I think) is deciding on the command line interface that we want and modifying it to allow either of the common label sets.

--k8s-forward-namespace= and --k8s-forward-name=flux; if either of these are mentioned, FLUX_URL and --url are ignored in favour of using a port forward. To use port forwarding with the defaults, you can use the boolean flag --k8s-forward (which could have a shorthand). The environment variables FLUX_FORWARD_NAMESPACE, FLUX_FORWARD_NAME, and FLUX_FORWARD act as equivalents.

Currently, the port forwarding works by looking for a pod with the labels name=flux like so: https://github.com/weaveworks/flux/blob/master/deploy/flux-deployment.yaml#L13

For command line simplicity, I don't think we should allow people to specify the pod name or the labels and just have a fixed set of labels that we check for - it's easy enough to change your Fluxd if you have a non-standard set of labels.

That makes the expectation for this that you can pass a namespace to it and it will be able to find the single flux instance in that namespace.

  • So the first question is, what should the default namespace be? default or kube-system?

Next, we need to decide how the feature itself is turned on and off. It sounds like we want it to be on by default, unless a token is specified (in which case, we set the URL to weave cloud) or a URL has been manually specified on the command line.

So I think the logic we will have is:

  • No flags: search for Flux in the default namespace and attempt a port forward.
  • Token is set: set the URL to weave cloud.
  • URL is set: use the URL instead of a port forward.
@justinbarrick justinbarrick force-pushed the justinbarrick:master branch from 88cfecd to a0fd9c1 Jul 12, 2018
@justinbarrick

This comment has been minimized.

Copy link
Collaborator Author

justinbarrick commented Jul 12, 2018

Though I am not adverse to a small amount of breakage in the interest of encouraging people to use port-forwarding instead of e.g., a NodePort, when either would work.

Thinking about this more, I don't actually think we'll break anything at all here! fluxctl with no arguments was never a valid configuration - you always had to either pass a token or a URL in order for fluxctl to work. Now, if you pass neither of those, it will attempt a port forward instead.

@justinbarrick justinbarrick force-pushed the justinbarrick:master branch from a0fd9c1 to cea5d22 Jul 12, 2018
@justinbarrick

This comment has been minimized.

Copy link
Collaborator Author

justinbarrick commented Jul 12, 2018

Okay, I've implemented everything I described above:

  • The command line interface logic.
  • We search for pods that match the label selector name in (flux, fluxd, weave-flux-agent)

This is good to go from my perspective!

@justinbarrick

This comment has been minimized.

Copy link
Collaborator Author

justinbarrick commented Jul 12, 2018

Demo:

Default port forward fails, since my flux is in kube-system:

➜  flux git:(master) ✗ ./fluxctl list-controllers
Error: Could not find pod for selector: labels "name in (flux,fluxd,weave-flux-agent)"
Run 'fluxctl list-controllers --help' for usage.

Search for flux in kube-system and create a port forward:

➜  flux git:(master) ✗ ./fluxctl -f kube-system list-controllers
CONTROLLER                                              CONTAINER                      IMAGE                                                                  RELEASE  POLICY
default:daemonset/elasticsearch-operator-sysctl         sysctl-conf                    busybox:1.26.2                                                         ready                                                                                                                    

Try to connect to weave cloud with fake token:

➜  flux git:(master) ✗ ./fluxctl -t mytoken list-controllers
== Error ==

The request failed authentication

This most likely means you have a missing or incorrect token. Please
make sure you supply a service token, either by setting the
environment variable FLUX_SERVICE_TOKEN, or using the argument --token
with fluxctl.

Set a URL and connect directly:

➜  flux git:(master) ✗ ./fluxctl -u http://127.0.0.1:3030/ list-controllers
Error: Get http://127.0.0.1:3030/v6/services?namespace=default: dial tcp 127.0.0.1:3030: connect: connection refused
Run 'fluxctl list-controllers --help' for usage.
…es cluster.

Simplify accessing flux instances in a Kubernetes cluster by automatically creating
port forwards, just like Helm does.

This is based on the port forward implementations from kubectl and Helm:

* https://github.com/kubernetes/helm/blob/master/pkg/kube/tunnel.go
* https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/portforward.go

The flow is:

* If `--url` is set, then fluxctl will connect directly to the specified URL.
* If `--token` is set, then fluxctl will connect to Weave cloud with the token.
* Otherwise, fluxctl will look for a pod in `--k8s-forward-namespace` (`default` by
  default) with the `name in (flux, fluxd, weave-flux-agent)` labels.
* If the number of pods that match the labels in the namespace are not exactly 1, it will
  return an error.
* It will find an empty port (by first binding to port 0 and then closing the port) and
  start a port forward on that port that forwards to the flux pod on port 3030.
* It will set the flux url to `http://127.0.0.1:$port/api/flux`.
* The port forward goroutine will get cleaned up automatically when fluxctl terminates.

Use-cases:

* Easier use of standalone flux instances.
* Discourage exposing flux apis to the internet (hopefully nobody does this).
* More easily manage large numbers of flux instances.
@justinbarrick justinbarrick force-pushed the justinbarrick:master branch from cea5d22 to 14c070a Jul 12, 2018
Copy link
Member

squaremo left a comment

The only showstopper really is that this will break our own development envs (see the comment on tokens and URLs). See what you think, otherwise ..

opts.URL = getFromEnvIfNotSet(cmd.Flags(), "url", opts.URL, envVariableURL)

if opts.Token != "" {

This comment has been minimized.

Copy link
@squaremo

squaremo Jul 12, 2018

Member

We use fluxctl against development environments of Weave Cloud, and for this we need to be able to set both the URL and the auth token. I think it should be (in pseudo-code): if either URL or token are set, use the URL and token arguments (with defaults as they are now); otherwise, use port forwarding.

use set outcome
OSS install - uses port forwarding to default namespace
OSS install k8s-forward-namespace=foo port forward to namespace "foo"
weave cloud token=abc uses default URL (https://cloud.weave.works/api/flux) and token
weave cloud dev token=abc, url=http://$(minikube ip):$port/api/flux uses URL and token

This comment has been minimized.

Copy link
@justinbarrick

justinbarrick Jul 12, 2018

Author Collaborator

Good catch, fixed!

@@ -74,11 +82,38 @@ func (opts *rootOpts) Command() *cobra.Command {
}

func (opts *rootOpts) PersistentPreRunE(cmd *cobra.Command, _ []string) error {
opts.Namespace = getFromEnvIfNotSet(cmd.Flags(), "k8s-forward-namespace", opts.Namespace, envVariableNamespace)

This comment has been minimized.

Copy link
@squaremo

squaremo Jul 12, 2018

Member

There's a difference between the default namespace (which is determined by your kubeconfig), and the namespace "default". Some people use contexts in kubeconfig -- or different kubeconfigs entirely -- when operating in different namespaces. Since the port forwarding library is implicitly getting things from a kubeconfig anyway, to be able to connect at all, could it also pick up the default namespace?

This comment has been minimized.

Copy link
@justinbarrick

justinbarrick Jul 12, 2018

Author Collaborator

Good point! I'm still working on trying to figure out how to fetch the default namespace from the kube config (it's conspiciously missing from https://godoc.org/k8s.io/client-go/rest#Config).

This comment has been minimized.

Copy link
@squaremo

squaremo Jul 13, 2018

Member

I think https://godoc.org/k8s.io/client-go/tools/clientcmd has the bits needed. k8s.io/client-go really is labyrinthine isn't it :-/

@@ -49,7 +54,10 @@ func (opts *rootOpts) Command() *cobra.Command {
SilenceErrors: true,
PersistentPreRunE: opts.PersistentPreRunE,
}
cmd.PersistentFlags().StringVarP(&opts.URL, "url", "u", "https://cloud.weave.works/api/flux",

cmd.PersistentFlags().StringVarP(&opts.Namespace, "k8s-forward-namespace", "f", "default",

This comment has been minimized.

Copy link
@squaremo

squaremo Jul 12, 2018

Member

Not sure how I feel about having a shorthand for this -- and -f is usually used for file (albeit we don't have any commands that accept a file; and, we are stuck with -u and -t for connection flags, which I wouldn't have chosen ..).

We can shorten the long name, it is a little long: --k8s-fwd-namespace or even --k8s-fwd-ns. wdyt?

This comment has been minimized.

Copy link
@justinbarrick

justinbarrick Jul 12, 2018

Author Collaborator

Makes sense to me, changed.

insecure channel. Do not do this _unless_ you are operating Flux
entirely locally; and arguably, only to try it out.
If you are not able to use the port forward to connect, you can connect by URL with
`--url` or `FLUX_URL`:

This comment has been minimized.

Copy link
@squaremo

squaremo Jul 12, 2018

Member

This is misleading, since there are no preceding instructions for setting up a NodePort or equivalent. I'm happy to remove those, but we need to be clear that the example given won't just work.

This comment has been minimized.

Copy link
@justinbarrick

justinbarrick Jul 12, 2018

Author Collaborator

Updated it to indicate that you need to setup some way of connecting to it.

Copy link
Member

squaremo left a comment

LGTM, and works seamlessly. Thank you for this -- it'll make using fluxctl so much more pleasant.

@squaremo squaremo merged commit 5656d6b into fluxcd:master Jul 13, 2018
@justinbarrick

This comment has been minimized.

Copy link
Collaborator Author

justinbarrick commented Jul 13, 2018

Thanks for the review and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.