Add docker image for TPROXY by costinm · Pull Request #6873 · istio/istio

costinm · 2018-07-06T02:26:25Z

This complements the inject time version. Values based on the build time SHA of the proxy and version.

istio-testing · 2018-07-06T02:26:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [costinm]

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

rshriram · 2018-07-06T02:28:31Z

I just modified my other PR to add the version to docker images

codecov · 2018-07-06T03:58:06Z

Codecov Report

Merging #6873 into release-1.0 will increase coverage by 1%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release-1.0   #6873    +/-   ##
============================================
+ Coverage           71%     71%    +1%     
============================================
  Files              360     360            
  Lines            31273   31128   -145     
============================================
- Hits             22123   22082    -41     
+ Misses            8275    8179    -96     
+ Partials           875     867     -8

Impacted Files	Coverage Δ
mixer/adapter/cloudwatch/cloudwatch.go	`58% <0%> (-14%)`	⬇️
pilot/pkg/config/memory/monitor.go	`82% <0%> (-9%)`	⬇️
mixer/adapter/signalfx/metrics.go	`41% <0%> (-2%)`	⬇️
mixer/adapter/circonus/circonus.go	`69% <0%> (-2%)`	⬇️
mixer/adapter/redisquota/redisquota.go	`88% <0%> (-2%)`	⬇️
pilot/pkg/serviceregistry/kube/controller.go	`67% <0%> (-1%)`	⬇️
mixer/adapter/statsd/statsd.go	`96% <0%> (-1%)`	⬇️
mixer/adapter/servicecontrol/reportbuilder.go	`88% <0%> (ø)`	⬇️
mixer/adapter/stdio/stdio.go	`100% <0%> (ø)`	⬆️
mixer/adapter/memquota/dedup.go	`100% <0%> (ø)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 426b63e...34417a4. Read the comment docs.

rshriram · 2018-07-06T13:30:56Z

@rlenglet please take a look. I think we need to wire this up for sure.

rlenglet

LGTM.
I would also modify the Helm template to default to this image if the mode is known to be TPROXY at Helm configuration time.

rlenglet · 2018-07-06T15:52:03Z

pilot/docker/Dockerfile.proxytproxy

+
+ENV ISTIO_META_ISTIO_PROXY_SHA $proxy_version
+ENV ISTIO_META_ISTIO_VERSION $istio_version
+ENV ISTIO_META_INTERCEPTION_MODE TPROXY


That’s not necessary here, since that will be set by the injector.

Actually, this should NOT be set to TPROXY if the other conditions are not met (run the sidecar with CAP_NET_ADMIN, in particular), otherwise Envoy will just keep crashing trying to set the IP_TRANSPARENT socket option on the listener.
So you really should remove this ENV from the Dockerfile.

My plan is to make the init container customizable and identical with the sidecar container, and pass the same ENV on both. For 1.1 we may provide dynamic changes to the iptable capture.

The other 'conditions' should go away - if you specify tproxy sidecar (and init) image, it will take care of both and no extra customizations needed. This will also allow to clean up the iptable initializer, which is a mess right now - we'll have a script for tproxy and one for redirect.

Short term (1.0) - users will need to specify both image and the old annotation. Better then not working at all..

if you specify tproxy sidecar (and init) image, it will take care of both and no extra customizations needed

That's not enough. You need to run istio-proxy with CAP_NET_ADMIN for TPROXY to work. And that can only be done by modifying the pod spec. If you don't want that, you'll have to always run istio-proxy with CAP_NET_ADMIN. I'm fine with that, but I got push back when I had proposed that.

The other way would be to set envoy setuid root in this Dockerfile here. That will make sure that it gets CAP_NET_ADMIN. I'm fine with that too.

Yes, I know - but requirements change. There are plans to have more flexibility in how we capture, which will need NET_ADMIN anyways ( proxy-agent may change iptables at runtime ). We still want to start proxy-agent as root
and exec envoy as 1337 - I still don't think it's ok to run envoy as root (as it happens in tproxy mode).

rlenglet · 2018-07-06T16:06:28Z

I would also recommend that you just build the tproxy image as derived from the proxyv2 image. You only need to RUN the chgrp/chmod commands on top of it.
That should make the maintenance trivial.

… missing

costinm · 2018-07-06T18:56:41Z

It looks like this version works - even on a kernel without -m socket support ( I think matching existing socket is an optimization to skip extra rules - I've used tproxy for UDP without socket and also works).

The performance with small number of connections seem to still lag - haven't tested yet with large number, where tproxy is supposed to be better.

There are further fixes and polish needed - in particular having pilot-agent run as root and exec
envoy as UID/GID 1337 - we want this for all cases, because pilot-agent will be involved in dynamic
iptables changes. So most of the tproxy-specific hacks will go away - hopefully by the time envoy
adds UDP support.

costinm · 2018-07-06T18:59:06Z

Note the I sneaked in a small env variable that if set will allow the 'split port' mode.
Once Andra makes the changes in pilot to have 2 chains, combined with Shriram PR to detect proxy 1.0,
and with Piotr envoy change - we can implement the other capture option ( this is scheduled for 1.1).
Even if we move ahead with pilot-agent doing dynamic iptables, configured by pilot - we still want to
have separate chains for in/out capture - it is cleaner and the stats will look better.

costinm · 2018-07-06T19:21:37Z

As an update - unlike 0.8, in 1.0 tproxy appears to be faster (as I expected).
At 500qps, 3ms 50% and 6ms for 90% - versus 4ms and 8ms with REDIRECT.
Would be good if someone repeats the testing later - have to move to other things, but should be good enough for 1.0.

costinm · 2018-07-07T14:59:55Z

/test istio-pilot-e2e-envoyv2-v1alpha3

rshriram · 2018-07-09T15:58:43Z

pilot/docker/Dockerfile.proxytproxy

+COPY envoy_bootstrap_v2.json /var/lib/istio/envoy/envoy_bootstrap_tmpl.json
+
+ENV ISTIO_META_ISTIO_PROXY_SHA $proxy_version
+ENV ISTIO_META_ISTIO_VERSION $istio_version


These are outdated? ISTIO_META_ISTIO_PROXY_VERSION is what we read in Pilot IIRC

Those are additional variables.

rshriram · 2018-07-09T15:59:55Z

tools/deb/istio-iptables.sh


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


shouldn't this be guarded? if TPROXY, then set the mangle?

rshriram

missing istio_meta_istio_proxy_version

costinm · 2018-07-09T20:03:21Z

/test e2e-mixer-no_auth

zenvdeluca · 2019-02-06T04:14:06Z

As an update - unlike 0.8, in 1.0 tproxy appears to be faster (as I expected).
At 500qps, 3ms 50% and 6ms for 90% - versus 4ms and 8ms with REDIRECT.
Would be good if someone repeats the testing later - have to move to other things, but should be good enough for 1.0.

interesting in knowing how can I reproduce the tests on my lab. any tooling to suggest?

ldemailly · 2019-02-07T09:50:52Z

interesting in knowing how can I reproduce the tests on my lab. any tooling to suggest?

you can use https://fortio.org/
sample setup scripts are described in https://github.com/istio/istio/blob/master/tools/README.md

Add proxy sha and istio version to docker meta

6fb9d18

costinm requested a review from rshriram July 6, 2018 02:26

istio-testing requested review from ldemailly and vadimeisenbergibm July 6, 2018 02:26

googlebot added the cla: yes label Jul 6, 2018

istio-testing added the approved label Jul 6, 2018

costinm mentioned this pull request Jul 6, 2018

Add proxy version info to envoy metadata #6854

Merged

Fix the build args, and add a tproxy variant

2f2b168

rshriram changed the title ~~Add proxy sha and istio version to docker meta~~ Add docker image for TPROXY Jul 6, 2018

rlenglet approved these changes Jul 6, 2018

View reviewed changes

costinm added 3 commits July 6, 2018 10:27

tproxy failing in route, may be due to restart

b4be980

More small fixes to capture. Tproxy seems to work even if m socket is…

c986071

… missing

Merge branch 'release-1.0' of github.com:istio/istio into 10-install

d6c7e66

Bad cut&paste

a3a8944

rshriram reviewed Jul 9, 2018

View reviewed changes

rshriram approved these changes Jul 9, 2018

View reviewed changes

costinm added 2 commits July 9, 2018 11:47

Merge branch 'release-1.0' of github.com:istio/istio into 10-install

f5f1556

Add missing env based on review

34417a4

costinm merged commit eee51c9 into istio:release-1.0 Jul 9, 2018

rlenglet mentioned this pull request Aug 21, 2018

Port 15001 not opened when trying to use TPROXY mode #8059

Closed

Conversation

costinm commented Jul 6, 2018

Uh oh!

istio-testing commented Jul 6, 2018

Uh oh!

rshriram commented Jul 6, 2018

Uh oh!

codecov bot commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rshriram commented Jul 6, 2018

Uh oh!

rlenglet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlenglet commented Jul 6, 2018

Uh oh!

costinm commented Jul 6, 2018

Uh oh!

costinm commented Jul 6, 2018

Uh oh!

costinm commented Jul 6, 2018

Uh oh!

costinm commented Jul 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

costinm commented Jul 9, 2018

Uh oh!

zenvdeluca commented Feb 6, 2019

Uh oh!

ldemailly commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Jul 6, 2018 •

edited

Loading

ldemailly commented Feb 7, 2019 •

edited

Loading