Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

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

Merged
merged 2 commits into from
Jul 13, 2018

Conversation

oliviabarrick
Copy link
Contributor

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 was marked as abuse.

@oliviabarrick
Copy link
Contributor Author

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
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
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.

@oliviabarrick
Copy link
Contributor Author

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
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
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?

@oliviabarrick
Copy link
Contributor Author

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.

@oliviabarrick
Copy link
Contributor Author

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.

@oliviabarrick
Copy link
Contributor Author

oliviabarrick 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!

@oliviabarrick
Copy link
Contributor Author

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.
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

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 was marked as abuse.

This comment was marked as abuse.

@@ -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 was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@@ -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 was marked as abuse.

This comment was marked as abuse.

site/using.md Outdated
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 was marked as abuse.

This comment was marked as abuse.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

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
@oliviabarrick
Copy link
Contributor Author

Thanks for the review and merge!

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

Successfully merging this pull request may close these issues.

2 participants