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

Add inject flag for skipping outbound ports #38

Merged
merged 2 commits into from Dec 19, 2017

Conversation

adleong
Copy link
Member

@adleong adleong commented Dec 13, 2017

Add the --skip-outbound-ports flag to conduit inject. This flag accepts a comma separated list of ports. When the application sends traffic to one of these ports, the sidecar proxy will be bypassed and the traffic will be sent directly to the original destination. This is useful for when an application sends traffic that the Conduit proxy does not support such as HTTP/1.1 or raw TCP.

@adleong adleong self-assigned this Dec 13, 2017
@adleong
Copy link
Member Author

adleong commented Dec 14, 2017

I spoke to @klingerf about this in person and he brought up the very good point that multiple different downstreams may all use the same port number but speak different protocols. For example, you may have a downstream A that listens for gRPC on port 7777 and a downstream B that listens for HTTP/1.1 on port 7777. This change doesn't allow you to differentiate between the two and forces you to skip the outbound proxy for both or neither.

An alternative to this would be to allow skipping the outbound proxy based on IP ranges instead of by port. Unfortunately, for service-to-service traffic in scheduled environments like Kubernetes, there is no fixed IP address for a given downstream.

Given this, I think I'd like to move forward with this change as-is. This definitely doesn't address the issue where multiple downstreams may use the same port number and can't be individually excluded from the proxy, but this is at least potentially work-around-able by changing your services to use unique ports. A more complete solution will come later when the Conduit proxy is able to transparently proxy arbitrary TCP traffic.

Copy link
Member

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ Your writeup makes sense to me, and I'm good with preceding as written. I think it would be worthwhile to add a test for this in proxy-init/integration_test/iptables/http_test.go, since the rest of the configurations are tested there.

Copy link
Contributor

@pcalcado pcalcado left a comment

Choose a reason for hiding this comment

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

LGTM, but should we also reflect this in the integration tests?

@@ -42,7 +43,8 @@ func init() {
RootCmd.PersistentFlags().IntVarP(&outgoingProxyPort, "outgoing-proxy-port", "o", -1, "Port to redirect outgoing traffic")
RootCmd.PersistentFlags().BoolVar(&simulateOnly, "simulate", false, "Don't execute any command, just print what would be executed")
RootCmd.PersistentFlags().IntSliceVarP(&portsToRedirect, "ports-to-redirect", "r", make([]int, 0), "Port to redirect to proxy, if no port is specified then ALL ports are redirected")
RootCmd.PersistentFlags().IntSliceVarP(&portsToIgnore, "ports-to-ignore", "i", make([]int, 0), "Port to ignore and not redirect to proxy. This has higher precedence than any other parameters.")
RootCmd.PersistentFlags().IntSliceVar(&inboundPortsToIgnore, "inbound-ports-to-ignore", make([]int, 0), "Inound ports to ignore and not redirect to proxy. This has higher precedence than any other parameters.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo Inound

@adleong adleong assigned pcalcado and unassigned adleong Dec 15, 2017
@adleong
Copy link
Member Author

adleong commented Dec 15, 2017

I have added an integration test for this, but the test is not yet passing and I haven't figured out why.

@pcalcado
Copy link
Contributor

I've tested this branch like this:

$ minikube start
$ eval "$(minikube docker-env ) "
$ bash -x bin/docker-build-proxy-init latest
$ cd proxy-init/integration_test
$ ./run_tests.shq

It fails with all sorts of crazy errors.

Looking at the init-container log, I see that the iptables setup logic has failed but the container didn't abort:

$ kubectl logs pod-doesnt-redirect-blacklisted conduit-init
Error: unknown shorthand flag: 'i' in -i
Usage:
  proxy-init [flags]

Flags:
  -h, --help                                help for proxy-init
      --inbound-ports-to-ignore intSlice    Inbound ports to ignore and not redirect to proxy. This has higher precedence than any other parameters.
  -p, --incoming-proxy-port int             Port to redirect incoming traffic (default -1)
      --outbound-ports-to-ignore intSlice   Outbound ports to ignore and not redirect to proxy. This has higher precedence than any other parameters.
  -o, --outgoing-proxy-port int             Port to redirect outgoing traffic (default -1)
  -r, --ports-to-redirect intSlice          Port to redirect to proxy, if no port is specified then ALL ports are redirected
  -u, --proxy-uid int                       User ID that the proxy is running under. Any traffic coming from this user will be ignored to avoid infinite redirection loops. (default -1)
      --simulate                            Don't execute any command, just print what would be executed

I confirmed this locally:

 $ go run proxy-init/main.go  -p 80  -o 90 -i 7070 -u 1337 --simulate  ; echo $?
Error: unknown shorthand flag: 'i' in -i
Usage:
  proxy-init [flags]

Flags:
  -h, --help                                help for proxy-init
      --inbound-ports-to-ignore intSlice    Inbound ports to ignore and not redirect to proxy. This has higher precedence than any other parameters.
  -p, --incoming-proxy-port int             Port to redirect incoming traffic (default -1)
      --outbound-ports-to-ignore intSlice   Outbound ports to ignore and not redirect to proxy. This has higher precedence than any other parameters.
  -o, --outgoing-proxy-port int             Port to redirect outgoing traffic (default -1)
  -r, --ports-to-redirect intSlice          Port to redirect to proxy, if no port is specified then ALL ports are redirected
  -u, --proxy-uid int                       User ID that the proxy is running under. Any traffic coming from this user will be ignored to avoid infinite redirection loops. (default -1)
      --simulate                            Don't execute any command, just print what would be executed

0

So if the -i flag doesn't exist anymore, the CLI is exiting with 0 as its status. This looks similar to spf13/cobra#582 , which makes me thing it might be an issue with Cobra.

To get this moving for the release, I've changed the iptablestest-lab.ymnl file to use --inbound-ports-to-ignore instead of -i and the tests now pass. I've also manually inspected the output using the --simulate flag nd it looks ok.

So, unless we have any other requirements for this change, I'd 👍 it for now and merge.

Copy link
Member

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ Updates looks good to me.

@pcalcado pcalcado merged commit 772b43f into master Dec 19, 2017
@olix0r olix0r deleted the alex/skip-outbound-ports branch April 20, 2018 01:03
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this pull request Mar 5, 2019
This branch should not make any functional changes.

This branch makes two minor refactorings to the `client` module in 
`control::destination::background`:

 1. Remove the `AddOrigin` middleware and replace it with the 
    `tower-add-origin` crate from `tower-http`. These middlewares are
    functionally identical, but the Tower version has tests.
 2. Change `ClientService` from a type alias to a tuple struct. This
    means that some of the middleware that are used only in this module
    (`LogErrors` and `Backoff`) are no longer part of a publicly visible
    type and can be made private to the module.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants