Merging iptables scripts #3562

nmittler · 2018-02-16T20:38:56Z

Fixes #3514

istio-merge-robot · 2018-02-16T20:39:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: nmittler

Assign the PR to them by writing /assign @nmittler in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pilot/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

rshriram · 2018-02-16T21:11:38Z

pilot/docker/prepare_proxy.sh

  echo '  -i: Comma separated list of IP ranges in CIDR form to redirect to envoy (optional)'
  echo ''
 }

 IP_RANGES_INCLUDE=""
+REDIRECT_INBOUND_TRAFFIC="yes" # Route all inbound traffic to Envoy by default


nit: TRAP_INBOUND_TRAFFIC and change the rest to reflect this statement.. In general, we want TRAP_INBOUND, TRAP_OUTBOUND, etc.. If at all possible, use a different flag than -g.
And this does not have much to do with gateway btw, as gateways will not have any IPTables stuff.

rshriram · 2018-02-16T21:12:33Z

pilot/docker/prepare_proxy.sh

@@ -28,6 +30,9 @@ while getopts ":p:u:e:i:h" opt; do
    i)
      IP_RANGES_INCLUDE=${OPTARG}
      ;;
+    g)
+      REDIRECT_INBOUND_TRAFFIC="no" # For gateways, do not redirect inbound traffic


Please remove gateway, as it implies that we need IPtables for gateways.. What you need here is a customization for the use case you point to (trap outbound but not inbound). That could apply to any normal pod as well.

rshriram · 2018-02-16T21:12:54Z

pilot/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-port ${ENVOY_PORT}  -m comment --comment "istio/redirect-to-envoy-port"

-# Redirect all inbound traffic to Envoy.
-iptables -t nat -A PREROUTING -j ISTIO_REDIRECT                               -m comment --comment "istio/install-istio-prerouting"
+# Redirect all inbound traffic to Envoy (if not configuring a gateway).


rshriram · 2018-02-16T21:16:51Z

A simpler and less riskier option would be to have the nginx ingress controller generate 127.0.0.1 as the upstream address for each entry.. You could then run both nginx and envoy as two processes without any IPtables interception business (this becomes especially useful when you install firewalls and other such stuff that may in turn have more IPtables stuff). But this is something left to the end user's appetite.

nmittler · 2018-02-16T23:26:10Z

FYI, I've backed off on the original change to instead focus on making the two scripts within istio identical. Renamed this PR accordingly.

rshriram

If you are going to be touching the core script, we need to test this thoroughly with all e2e tests.

I suggest you tweak the circleci file to add all the jobs to the “all” workflow (bookinfo and mixer). Make sure they all pass. Once they do, undo the change to circleci config, and then merge.

costinm

Looks good as code - but I agree with Shriram, this deserves testing with the full suite. Please check the prow tests first - they do run the bookinfo.

costinm · 2018-02-17T00:25:19Z

In particular, the state on the proxy_init in the circleci test:
State: Waiting
Reason: CrashLoopBackOff

costinm

Sorry, clicked the close accidentally.

Added 'request changes' so it doesn't get accidentally merged, will approve again once the tests pass.

ldemailly · 2018-02-17T02:33:39Z

/test e2e-suite-rbac-auth

ldemailly · 2018-02-17T02:35:38Z

now (beginning of 0.7 cycle) is the perfect time to get this left over from 0.2 cleanup as well as get the feature we do need. but yes it does mean all tests should be good

one thing to do is check / compare the output of iptables-save on a pod with this script and on a pod with the last release

ldemailly · 2018-03-14T02:27:00Z

tools/deb/istio-iptables.sh

@@ -30,19 +28,26 @@
 # After more testing, the goal is to replace and unify the script in K8S - by generating
 # the sidecar image using the .deb file created by proxy.

+set -o errexit


in short, this makes it fail because it's doing -D commands that are expected to fail; those commands should have || true

ldemailly · 2018-03-15T06:03:58Z

ping

ldemailly · 2018-03-22T21:53:35Z

@nmittler can we get this done for 0.8 ?

Fixes istio#3514

costinm · 2018-03-30T23:27:29Z

tools/deb/istio-iptables.sh

 fi

 # Remove the old chains, to generate new configs.
-iptables -t nat -D PREROUTING -p tcp -j ISTIO_INBOUND 2>/dev/null


This will result in an unexpected message the first time it is run. ( and next lines - the second time it is run ).

I think ignoring the err is useful and avoids confusion

@costinm that's fine .... doing this seems to negate the set -x at the top of the file, resulting in the echo for each statement also being redirected to /dev/null. Do you happen to know some bash magic that might make it work?

costinm

See my comment, also discussed offline about better docs on the env variables we support and using env variables consistently ( for VM and inject ), but can be done in separate PRs.

nmittler · 2018-03-30T23:35:53Z

/test istio-presubmit

nmittler · 2018-03-31T00:29:05Z

@xiaolanz The race test is also failing for this PR. See the output here: https://circleci.com/gh/istio/istio/62253?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

ldemailly · 2018-03-31T01:25:53Z

/test e2e-suite-rbac-auth
(the output above is pre readiness updates / old from 2/16)

nmittler · 2018-03-31T04:16:01Z

/test istio-pilot-e2e-v1alpha3

nmittler · 2018-03-31T05:07:31Z

@costinm looks like CI is happy .... can you lgtm?

ldemailly · 2018-03-31T09:08:59Z

/lgtm

istio-testing · 2018-03-31T09:09:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ldemailly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

istio-merge-robot · 2018-04-01T19:39:46Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2018-04-01T20:16:16Z

Automatic merge from submit-queue.

Automatic merge from submit-queue. Merging iptables scripts

nmittler requested review from costinm and a team February 16, 2018 20:38

googlebot added the cla: yes label Feb 16, 2018

rshriram reviewed Feb 16, 2018

View reviewed changes

nmittler force-pushed the ingress_sidecar branch from 5f0a5da to 4902439 Compare February 16, 2018 23:05

nmittler requested a review from ldemailly as a code owner February 16, 2018 23:05

nmittler changed the title ~~Add iptables option for configuring gateways~~ Making iptables scripts consistent. Feb 16, 2018

nmittler force-pushed the ingress_sidecar branch 2 times, most recently from 91a899b to 06a973b Compare February 16, 2018 23:18

rshriram reviewed Feb 17, 2018

View reviewed changes

costinm approved these changes Feb 17, 2018

View reviewed changes

costinm closed this Feb 17, 2018

costinm reopened this Feb 17, 2018

costinm requested changes Feb 17, 2018

View reviewed changes

nmittler force-pushed the ingress_sidecar branch from 4e900bd to 7e1a809 Compare February 19, 2018 04:14

nmittler requested review from a team February 19, 2018 04:14

nmittler force-pushed the ingress_sidecar branch 5 times, most recently from 23b773a to 344e853 Compare February 20, 2018 00:49

ldemailly reviewed Mar 14, 2018

View reviewed changes

ldemailly mentioned this pull request Mar 15, 2018

Remove outdated, unused prepare_proxy_test.sh #4279

Closed

ldemailly modified the milestones: Istio 0.7, Istio 0.8 Mar 22, 2018

ldemailly added the kind/blocking release label Mar 22, 2018

nmittler force-pushed the ingress_sidecar branch 3 times, most recently from 7b5964c to dcef0dc Compare March 30, 2018 22:12

Merging iptables scripts

539aad7

Fixes istio#3514

nmittler force-pushed the ingress_sidecar branch from dcef0dc to 539aad7 Compare March 30, 2018 22:26

costinm reviewed Mar 30, 2018

View reviewed changes

costinm approved these changes Mar 30, 2018

View reviewed changes

istio-testing assigned ldemailly Mar 31, 2018

istio-testing added the lgtm label Mar 31, 2018

istio-testing added the approved label Mar 31, 2018

nmittler removed the do-not-merge/hold Block automatic merging of a PR. label Apr 1, 2018

istio-merge-robot merged commit 391a510 into istio:master Apr 1, 2018

bianpengyuan pushed a commit to bianpengyuan/istio that referenced this pull request Apr 5, 2018

Merging iptables scripts (istio#3562)

7e99f7e

Automatic merge from submit-queue. Merging iptables scripts

hklai mentioned this pull request Apr 10, 2018

istio.io/istio/tests/e2e/tests/bookinfo TestVMExtendsIstio failed two days in a row #4794

Closed

Merging iptables scripts #3562

Merging iptables scripts #3562

Uh oh!

Conversation

nmittler commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-merge-robot commented Feb 16, 2018

Uh oh!

rshriram Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

rshriram Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

rshriram Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

rshriram commented Feb 16, 2018

Uh oh!

nmittler commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

costinm commented Feb 17, 2018

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 17, 2018

Uh oh!

ldemailly commented Feb 17, 2018

Uh oh!

ldemailly Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Mar 15, 2018

Uh oh!

ldemailly commented Mar 22, 2018

Uh oh!

costinm Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

nmittler Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

nmittler commented Mar 30, 2018

Uh oh!

nmittler commented Mar 31, 2018

Uh oh!

ldemailly commented Mar 31, 2018

Uh oh!

nmittler commented Mar 31, 2018

Uh oh!

nmittler commented Mar 31, 2018

Uh oh!

ldemailly commented Mar 31, 2018

Uh oh!

istio-testing commented Mar 31, 2018

Uh oh!

istio-merge-robot commented Apr 1, 2018

Uh oh!

istio-merge-robot commented Apr 1, 2018

Uh oh!

Uh oh!

nmittler commented Feb 16, 2018 •

edited

Loading

nmittler commented Feb 16, 2018 •

edited

Loading

ldemailly Mar 14, 2018 •

edited

Loading

nmittler Mar 30, 2018 •

edited

Loading