prepare_proxy: add support for excluding external traffic #573

ayj · 2017-04-24T07:42:45Z

(Note for reviewers: prepare_proxy.sh is ready for review. prepare_proxy_test.sh is functional with some hand edits required for the configuration. It could probably be rewritten in golang to make it more readable.)

Short term solution to #515 for external traffic.

Add generic support for including and excluding outbound traffic based
on CIDR blocks. Default behavior is opt-out where all outbound traffic
is redirected to proxy unless explicitly opted-opt with "-e"
option. When the "-i" option is used the behavior changes to opt-in
and only outbound traffic explicitly specified with "-i" is redirected
to the proxy. The "-i" option takes precedence over "-e" the option if
both are specified.

As part of this change the iptable rules were refactored to make it
easier differentiate redirect from bypass cases, e.g. '-j
ISTIO_REDIRECT' vs. '-j RETURN'.

Add generic support for including and excluding outbound traffic based on CIDR blocks. Default behavior is opt-out where all outbound traffic is redirected to proxy unless explicitly opted-opt with "-e" option. When the "-i" option is used the behavior changes to opt-in and only outbound traffic explicitly specified with "-i" is redirected to the proxy. The "-i" option takes precedence over "-e" the option if both are specified. As part of this change the iptable rules were refactored to make it easier differentiate redirect from bypass cases, e.g. '-j ISTIO_REDIRECT' vs. '-j RETURN'.

rshriram · 2017-04-24T15:43:14Z

Can we reduce the confusion on opt-out and opt-in and just have one? Like a set of -i flags. With no -i, all traffic goes through proxy. With -i, operators could restrict traffic capture to specific CIDR blocks.

rshriram

Overall, looks pretty well done. Thanks for the extensive comments. Few minor issues in the PR.

rshriram · 2017-04-24T15:45:34Z

docker/prepare_proxy.sh

-# (i.e. proxy aware) or not from the proxy itself is redirected proxy port.
-iptables -t nat -A OUTPUT -p tcp -j REDIRECT ! -s 127.0.0.1/32 \
-  --to-port ${ISTIO_PROXY_PORT} -m owner '!' --uid-owner ${ISTIO_PROXY_UID}
+iptables -t nat -F


Why do we need to flush nat?

We don't. It was useful for unit testing but can be removed since prepare_proxy_test.sh restarts the pods for each test iteration.

rshriram · 2017-04-24T15:46:57Z

docker/prepare_proxy.sh

+# the common Istio proxy port.
+iptables -t nat -N ISTIO_REDIRECT                                                   -m comment --comment "istio/redirect-common-chain"
+iptables -t nat -A ISTIO_REDIRECT -p tcp -j REDIRECT --to-ports ${ISTIO_PROXY_PORT} -m comment --comment "istio/redirect-to-proxy-port"
+


Nicely done! Thanks for these comments and clean documentation.

Change istio proxy to Envoy everywhere to align with rest of documentation

rshriram · 2017-04-24T15:47:52Z

docker/prepare_proxy.sh

+iptables -t nat -N ISTIO_REDIRECT                                                   -m comment --comment "istio/redirect-common-chain"
+iptables -t nat -A ISTIO_REDIRECT -p tcp -j REDIRECT --to-ports ${ISTIO_PROXY_PORT} -m comment --comment "istio/redirect-to-proxy-port"
+
+# Redirect all inbound traffic to the proxy.


Inbound traffic into the pod

Can you add -m comment to all commands?

rshriram · 2017-04-24T15:54:15Z

docker/prepare_proxy.sh

+# istio and attach it the OUTPUT rule for all tcp traffic. '-j RETURN'
+# bypasses the proxy port; '-j ISTIO_REDIRECT' redirects to the proxy
+# port.
+iptables -t nat -N ISTIO_OUTPUT


And what is -j ISTIO_OUTPUT ? (Missing comments).

The preceding comment was intended to cover lines 68 and 69 (i.e. create chain and attach to OUTPUT rule). I'll split this up to make it more clear.

rshriram · 2017-04-24T16:18:26Z

docker/prepare_proxy.sh

+# Locally routed traffic is not captured by PREROUTING chain. Redirect
+# loopback back traffic when the DST_IP is not explicitly the loopback
+# address to handles appN => proxy (client) => proxy (server) => appN.
+iptables -t nat -A ISTIO_OUTPUT -o lo ! -d 127.0.0.1/32 -j ISTIO_REDIRECT           -m comment --comment "istio/implicit-loopback"


Probably best to describe the use case instead of the data flow. E.g., when kubelet accesses the app for health checks (??), or app calls itself via http://podIP/..

And shouldn't this be restricted to tcp?

We only jump to ISTIO_OUTPUT for tcp protocol traffic (line 69) so the -p tcp seemed redundant. I can add it back if that makes things more clear though.

rshriram · 2017-04-24T16:18:58Z

docker/prepare_proxy.sh

+# container-to-container traffic which explicitly use localhost.
+iptables -t nat -A ISTIO_OUTPUT -d 127.0.0.1/32 -j RETURN                           -m comment --comment "istio/explicit-loopback"
+
+# The default outobund redirection policy is opt-out. IP ranges may be


Typo. Outbound

rshriram · 2017-04-24T16:25:44Z

docker/prepare_proxy.sh

+    for cidr in ${ISTIO_IP_RANGES_INCLUDE}; do
+        iptables -t nat -A ISTIO_OUTPUT -d ${cidr} -j ISTIO_REDIRECT                -m comment --comment "istio/include-cidr-block-${cidr}"
+    done
+    iptables -t nat -A ISTIO_OUTPUT -j RETURN                                       -m comment --comment "istio/default-opt-out-redirect"


If we follow the suggestion I made earlier (just have -i cidr), then this else block moves to the top doesn't it? We would no longer require the default catch all policy either.

rshriram · 2017-04-24T16:27:09Z

docker/prepare_proxy_test.sh

+
+if false; then
+    # exclude specific external traffic (opt-out)
+    EXCLUDE_CIDR=$(host httpbin.org | cut -f4 -d' ' | sed -e 's|$|/32|' | paste -sd ",")


What's the point of this if block ?

It creates a comma-separated list of IP ranges for httpbin.org to be excluded from redirection.

$ host httpbin.org | cut -f4 -d' ' | sed -e 's|$|/32|' | paste -sd "," 107.21.206.81/32,50.19.93.247/32,54.225.109.133/32,54.243.85.55/32,23.23.128.123/32,23.23.223.197/32,23.21.77.86/32,23.23.117.228/32 ```

No, I meant the if false block. This is never going to be executed is it?

Ah, this was for manual testing. I'll added a flag at the top of control this as part of the refactoring to remove the EXCLUDE option.

rshriram · 2017-04-24T16:27:59Z

docker/prepare_proxy_test.sh

+    INCLUDE_CIDR=$(gcloud container clusters describe ${GCLOUD_CLUSTER_NAME} --zone=${GCLOUD_CLUSTER_ZONE} |
+		       grep -e clusterIpv4Cidr -e servicesIpv4Cidr |
+		       cut -f2 -d' ' | paste -sd ",")
+


Could you perhaps abstract this function out so that people could test this on say minikube or their local k8s clusters?

1) remove exlude option 2) rename proxy to Envoy throughout 3) add some additional comments

…raffic-bypass-flag

rshriram · 2017-04-24T20:26:09Z

docker/prepare_proxy.sh

-# redirection with ISTIO_IP_RANGES_INCLUDE.
+# Only redirect outbound traffic to selective IP ranges when
+# ISTIO_IP_RANGES_INCLUDE is non-empty. Otherwise redirect all
+# outbound traffic not already handled by the previous rules to Envoy.


I would reuse a previous version of this comment as it looked better.
All outbound traffic will be redirected to Envoy by default. If ISTIO_IP_RANGES_INCLUDE is non-empty, only traffic bound for the destinations specified in this list will be captured.

…raffic-bypass-flag

…ager into add-outbound-traffic-bypass-flag

ayj · 2017-04-24T20:32:31Z

Fixes #515 for alpha.

rshriram · 2017-04-24T20:27:50Z

docker/prepare_proxy_test.sh

 CLIENT_PORT=81
 OTHER_PORT=82
+TEST_IP_RANGE_INCLUDE=0



add a variable: PLATFORM="gcloud"

rshriram · 2017-04-24T20:30:11Z

docker/prepare_proxy_test.sh

+# list, e.g. 10.0.0.1/32,10.2.0.1/16. This function is GKE specific
+# and must modified for other providers, e.g. minikube.
+function k8sClusterAndServiceIPRange() {
+    echo $(gcloud container clusters describe ${GCLOUD_CLUSTER_NAME} --zone=${GCLOUD_CLUSTER_ZONE} |


case $platform in
gcloud)
echo..
*)
echo ""
esac

rshriram

couple of comments in the test script

kyessenov · 2017-04-24T22:42:05Z

docker/prepare_proxy.sh

  echo '  -u: Specify the UID of the user for which the redirection is not'
  echo '      applied. Typically, this is the UID of the proxy container'
+  echo '  -i: Comma separated list of IP ranges in CIDR form to redirection to envoy (optional)'


"for redirection"?

kyessenov · 2017-04-24T22:42:37Z

docker/prepare_proxy.sh

      ;;
    h)
-      usage
+      usagey


kyessenov · 2017-04-24T22:43:17Z

docker/prepare_proxy.sh

+      ENVOY_UID=${OPTARG}
+      ;;
+    i)
+      ISTIO_IP_RANGES_INCLUDE=${OPTARG}


why rename ISTIO_PROXY_PORT but keep this ISTIO_?

there's not much specific to envoy in this script btw

kyessenov · 2017-04-24T22:45:03Z

docker/prepare_proxy.sh

+# Create a new chain for redirecting inbound and outbound traffic to
+# the common Envoy port.
+iptables -t nat -N ISTIO_REDIRECT                                             -m comment --comment "istio/redirect-common-chain"
+iptables -t nat -A ISTIO_REDIRECT -p tcp -j REDIRECT --to-ports ${ENVOY_PORT} -m comment --comment "istio/redirect-to-envoy-port"


"to-ports"?

Either will work, but changed to "to-port" to avoid confusion.

kyessenov · 2017-04-24T22:46:36Z

docker/prepare_proxy_test.sh

@@ -0,0 +1,238 @@
+#!/bin/sh


I'd really prefer not have these long bash test scripts. They are close to unmaintainable...

kyessenov · 2017-04-24T22:48:12Z

docker/prepare_proxy.sh

+# Skip redirection for Envoy-aware applications and
+# container-to-container traffic both of which explicitly use
+# localhost.
+iptables -t nat -A ISTIO_OUTPUT -d 127.0.0.1/32 -j RETURN                     -m comment --comment "istio/bypass-explicit-loopback"


I have difficulty understanding all implications of these rules, so I trust @rshriram carefully reviewed these

Agreed. Most of the setup/teardown code can be shared with the other integration tests. Maybe after release we can split that out into its own test package and have individual tests consume that that?

…raffic-bypass-flag

…ager into add-outbound-traffic-bypass-flag

istio-testing · 2017-04-25T00:04:09Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-25T00:08:28Z

Jenkins job manager/e2e-smoketest passed

istio-testing · 2017-04-25T00:40:18Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-25T00:47:43Z

Jenkins job manager/e2e-smoketest passed

kyessenov · 2017-04-25T00:58:43Z

docker/BUILD

-docker_build(
-    name = "init",
-    base = "@docker_ubuntu//:xenial",
+manager_docker_build(


please put those back in jenkinsfile if you need them.

ayj requested review from kyessenov and rshriram April 24, 2017 07:42

rshriram suggested changes Apr 24, 2017

View reviewed changes

ayj added 4 commits April 24, 2017 10:16

address review comments

3aeefb7

1) remove exlude option 2) rename proxy to Envoy throughout 3) add some additional comments

Merge branch 'master' of github.com:istio/manager into add-outbound-t…

1651516

…raffic-bypass-flag

refactor test to run both with and without "-i" option

059ad9d

Merge branch 'master' into add-outbound-traffic-bypass-flag

c9b3c56

rshriram reviewed Apr 24, 2017

View reviewed changes

ayj added 3 commits April 24, 2017 13:27

update comment

b2b6ac3

Merge branch 'master' of github.com:istio/manager into add-outbound-t…

900551f

…raffic-bypass-flag

Merge branch 'add-outbound-traffic-bypass-flag' of github.com:ayj/man…

f3fdb32

…ager into add-outbound-traffic-bypass-flag

rshriram reviewed Apr 24, 2017

View reviewed changes

add PLATFORM variable

13779a5

rshriram approved these changes Apr 24, 2017

View reviewed changes

Merge branch 'master' into add-outbound-traffic-bypass-flag

e76a2ab

kyessenov reviewed Apr 24, 2017

View reviewed changes

ayj added 4 commits April 24, 2017 16:13

more review comments

4bb5e1d

Merge branch 'master' of github.com:istio/manager into add-outbound-t…

61d128b

…raffic-bypass-flag

Merge branch 'add-outbound-traffic-bypass-flag' of github.com:ayj/man…

9755e99

…ager into add-outbound-traffic-bypass-flag

fix linter error

5da5af6

Merge branch 'master' into add-outbound-traffic-bypass-flag

17ef78c

ayj merged commit 204a14f into istio:master Apr 25, 2017

kyessenov reviewed Apr 25, 2017

View reviewed changes

ayj deleted the add-outbound-traffic-bypass-flag branch April 28, 2017 16:37

ayj mentioned this pull request May 3, 2017

Allow external traffic from Istio pods #515

Closed

prepare_proxy: add support for excluding external traffic #573

prepare_proxy: add support for excluding external traffic #573

Conversation

ayj commented Apr 24, 2017

rshriram commented Apr 24, 2017

rshriram left a 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

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

ayj commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram left a 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

istio-testing commented Apr 25, 2017

istio-testing commented Apr 25, 2017

istio-testing commented Apr 25, 2017

istio-testing commented Apr 25, 2017

Choose a reason for hiding this comment