Preserve original src ip #28457
Merged
Preserve original src ip #28457
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
|
/test all |
2548489 to
223df9c
Compare
9e1e9dc to
9a5ceb6
Compare
|
/retest |
dgn
reviewed
Nov 2, 2020
...st-generate/data-snapshot/charts/istio-control/istio-discovery/files/injection-template.yaml
Outdated
Show resolved
Hide resolved
operator/cmd/mesh/testdata/manifest-generate/output/all_on.golden-show-in-gh-pull-request.yaml
Outdated
Show resolved
Hide resolved
howardjohn
reviewed
Nov 2, 2020
...st-generate/data-snapshot/charts/istio-control/istio-discovery/files/injection-template.yaml
Outdated
Show resolved
Hide resolved
b398574 to
e38dfa4
Compare
rlenglet
suggested changes
Nov 4, 2020
operator/cmd/mesh/testdata/manifest-generate/output/all_on.golden-show-in-gh-pull-request.yaml
Outdated
Show resolved
Hide resolved
|
/retest |
rlenglet
suggested changes
Nov 5, 2020
manifests/charts/istio-control/istio-discovery/files/gen-istio.yaml
Outdated
Show resolved
Hide resolved
howardjohn
reviewed
Nov 6, 2020
|
@howardjohn can you take a look |
howardjohn
requested changes
Dec 11, 2020
tools/istio-iptables/pkg/cmd/run.go
Outdated
| @@ -432,9 +413,10 @@ func (iptConfigurator *IptablesConfigurator) run() { | |||
| // Avoid infinite loops. Don't redirect Envoy traffic directly back to | |||
| // Envoy for non-loopback traffic. | |||
| iptConfigurator.iptables.AppendRuleV4(constants.ISTIOOUTPUT, constants.NAT, "-m", "owner", "--uid-owner", uid, "-j", constants.RETURN) | |||
| } | |||
| } else { | |||
| // TPROXY uses GID | |||
There was a problem hiding this comment.
I am not sure this is safe, we may use GID in iptables mode as well. Also, why do we not need split anymore, I think it can be a list?
cc @JimmyCYJ
There was a problem hiding this comment.
I had added GID matching in the first place because it was required for TPROXY. It's not used for REDIRECT AFAIK.
There was a problem hiding this comment.
I think https://www.hyrumslaw.com/ may be in play, I am fairly sure @JimmyCYJ is depend on this. Which its fine to say "don't do that" and I agree, but we should be explicit about doing it with release note, etc
| @@ -104,9 +104,6 @@ func TestHandleInboundIpv6RulesWithEmptyInboundPorts(t *testing.T) { | |||
| "ip6tables -t nat -A ISTIO_OUTPUT -o lo ! -d ::1/128 -m owner --uid-owner 1337 -j ISTIO_IN_REDIRECT", | |||
| "ip6tables -t nat -A ISTIO_OUTPUT -o lo -m owner ! --uid-owner 1337 -j RETURN", | |||
| "ip6tables -t nat -A ISTIO_OUTPUT -m owner --uid-owner 1337 -j RETURN", | |||
| "ip6tables -t nat -A ISTIO_OUTPUT -o lo ! -d ::1/128 -m owner --gid-owner 1337 -j ISTIO_IN_REDIRECT", | |||
There was a problem hiding this comment.
would prefer to not mess with iptables rules - just change TPROXY stuff here
| "iptables -t nat -A ISTIO_OUTPUT -m owner --gid-owner 2 -j RETURN", | ||
| "iptables -t nat -A ISTIO_OUTPUT -o lo ! -d 127.0.0.1/32 -m owner --uid-owner 3,4 -j ISTIO_IN_REDIRECT", | ||
| "iptables -t nat -A ISTIO_OUTPUT -o lo -m owner ! --uid-owner 3,4 -j RETURN", | ||
| "iptables -t nat -A ISTIO_OUTPUT -m owner --uid-owner 3,4 -j RETURN", |
There was a problem hiding this comment.
--uid-owner 3,4 is this valid syntax?
| ISTIOREDIRECT = "ISTIO_REDIRECT" | ||
| ISTIOINREDIRECT = "ISTIO_IN_REDIRECT" | ||
| ISTIOOUTPUT = "ISTIO_OUTPUT" | ||
| ISTIOPROXYOUTPUT = "ISTIO_PROXY_OUTPUT" |
There was a problem hiding this comment.
if this is for tproxy only should it be ISTIO_TPROXY_OUTPUT?
|
Agree that mess everything up is not very friendly to review. I will revert the iptables refactor and some other unrelated changes. Only keep thoses changes related to original src ip preserve. |
167129d to
4ec080b
Compare
|
/retest |
howardjohn
approved these changes
Dec 14, 2020
|
/retest |
|
Thanks everyone, this is a big improvement. |
|
@hzxuzhonghu No Istio VirtualService no gateway, no ingress, no egress Minimal TCP app Istio setup IP info enter pod namespace curl SVC IP of deployment (pod namespace) (k8s node namespace) envoy-init logs
istio-proxy logs without TCP Pod logs Curl output |
|
@hi-usui Can you check the route_localnetwork config? sysctl net/ipv4/conf/all/route_localnet |
|
sysctl net/ipv4/conf/all/route_localnet shows 1 My final step is to use proxy_protocol and original_src, but the listeners are not applying. The source IP address observed in application pod is ingress-nginx pod LoadBalancer internal IP, not IP address in PROXY TCP4 header. Without istioctl, is there an easy way to check which listeners are attached to a sidecar? Or see the reason for attachment failure? Cannot get working the proxy_protocol and original_src filters for EnvoyFilter on a sidecar. Is there requirement for something else? EnvoyFilter Miscellaneous Linux |
For the inbound traffic, it will go through 15006 virtualInbound listener, maybe this is the reason |
You are correct. Perhaps it is not possible with ingress-nginx LoadBalancer? ingress-nginx tries to send to TCP pod (10.0.0.99.34566 > 10.0.0.157.3333) but cannot receive reply (10.0.0.157.3333 > 10.0.0.99.34566), envoy-proxy always ends stream with |
|
Does the ingress-nginx send with proxy protocol?
|
Yes, for ingress-nginx accepting non-PROXY that adds PROXY and sends to istio-proxy: ingress-nginx logs but with no EnvoyFilter, TCP pod responds like: |
|
On side note, @jrajahalme can you confirm up to which version this PR needs to be backported? I think you saw issues with TPROXY in 1.6, right? |
|
@hzxuzhonghu Figured it out: Add Pretty cool! Hopefully applications can now drop support for Proxy Protocol. Is there an easy way to have sidecars exclude cluster traffic from the listener match? Would like to apply this to Postgres DB but make it so cluster traffic does not have to send Proxy header EDIT: I guess just use After: |
|
All traffic goes to the virtualInbound listener with the above filters enabled, so i donot think it is possible. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is based on #28363, and with other fixes: adjust listener filters order, route local net, added an integration test
Fix: #5679
For more details: #28363 (comment)
RFC: https://docs.google.com/document/d/1fd9XWe755XtwNJCxpQe19oU1K6VR_wECB8uerWyu1xQ/edit?usp=sharing