-
Notifications
You must be signed in to change notification settings - Fork 366
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 support for short-circuiting in AntreaProxy #4815
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hongliangl I think you should first have a clear description to explain what it tries to do for better understanding. The term short-circuiting is too obscure. Even a few weeks ago we had totally different understandings about it, and I'm not sure if we reach a consensus yet before I take a deep look at the code, not to mention other reviewers.
988c0a5
to
e6b33de
Compare
e6b33de
to
29a1a29
Compare
29a1a29
to
8675f47
Compare
@hongliangl : the commit is "Add support for ExternalIP in AntreaProxy"? Have you pushed the right commit? |
8675f47
to
8af63ce
Compare
I pushed wrong git commit. I just updated it. |
1326571
to
0f3fc2b
Compare
@hongliangl I would suggest to remove |
194f5db
to
133d264
Compare
@hongliangl I think NodePort should respect ExternalTrafficPolicy according to https://github.com/kubernetes/kubernetes/blob/122a459dcbf7b2317ac5bd3793ed94a400bd4a77/staging/src/k8s.io/api/core/v1/types.go#L4746-L4749 |
Since it has the definition in API comment, we should implement proxy as it describes. |
133d264
to
34f157d
Compare
cff9001
to
3f0a45c
Compare
The dependency has been merged, could you rebase? |
Sure, I'll rebase it right now. |
3f0a45c
to
4ac17e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
4ac17e1
to
29a8b10
Compare
3184f14
to
6eaf6b0
Compare
6eaf6b0
to
fff3c66
Compare
|
||
clientIP, err := probeClientIPFromPod(data, pod, busyboxContainerName, url) | ||
require.NoError(t, err, errMsg) | ||
require.Equal(t, clientIP, expectedClientIPs[idx]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the clientIP expected to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For non-host-network Endpoint, the clientIP should be always the same (source Pod IP), but for host-network Endpoint, if the Endpoint is on local host-network, the clientIP should be the source Pod IP; if the Endpoint is on a remote Node, the clientIP will be the transparent interface IP. In this test, we have two host-network Endpoint, and both of them can be selected even when ExternalTrafficPolicy is Local for client from Pod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's transparent interface IP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be Node IP. For example, if the Endpoint is a host network on another Node, the packet will be forwarded to local Node host network, then will be SNATed to remote Node. The SNATed IP should be the local Node IP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it seems the SNAT is not necessary but I understand it's not related to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, assuming that there are two Nodes:
Node A: 192.168.77.100, Pod CIDR 10.10.0.0/24
Node B: 192.168.77.101, Pod CIDR 10.10.1.0/24
Service: 10.96.0.9:8080, two Endpoints, 192.168.77.100:8080, 192.168.77.101:8080
Pod on Node A: 10.10.0.10
Previously, if externalTrafficPolicy is Local, for Pod -> Service, only Endpoint 192.168.77.100:8080 will be selected, and the clientIP is 10.10.0.10. Now, Endpoint 192.168.77.101:8080 could be also selected. If so:
- on Pod network: 10.10.0.10 -> 10.96.0.9
- on host network: 10.10.0.10 -> 192.168.77.101
- to make sure that the reply packets can be returned correctly, on network between Nodes: 192.168.77.100 -> 192.168.77.101, then the clientIP is 192.168.77.100. As a result, the clientIP is not certain. It could be 10.10.0.10 or 192.168.77.100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
Short-circuiting is used to ensure that the traffic from Pod/Node clients to external addresses behaves the same way as the traffic from external clients to external addresses. External clients do not need to consider which Nodes have local Endpoints, as the load balancer handles this for them. However, for Pod/Node clients, when the externalTrafficPolicy of the Service is set to "Local", it will not work on Nodes without an Endpoint. With this PR, even when the externalTrafficPolicy is set to "Local", Pod/Node clients without local Endpoints can still work by selecting Endpoints from the cluster. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
fff3c66
to
8caa6ec
Compare
@tnqn only renamed a variable, no other changes. |
/test-all |
/test-windows-proxyall-e2e |
/test-windows-conformance |
Thanks for merging this PR. |
Short-circuiting is used to ensure that the traffic from Pod/Node clients to external addresses behaves the same way as the traffic from external clients to external addresses. External clients do not need to consider which Nodes have local Endpoints, as the load balancer handles this for them. However, for Pod/Node clients, when the externalTrafficPolicy of the Service is set to "Local", it will not work on Nodes without an Endpoint. With this PR, even when the externalTrafficPolicy is set to "Local", Pod/Node clients without local Endpoints can still work by selecting Endpoints from the cluster. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
Short-circuiting is used to ensure that the traffic from Pod/Node clients to
external addresses behaves the same way as the traffic from external clients to
external addresses.
External clients do not need to consider which Nodes have local Endpoints, as the
load balancer handles this for them. However, for Pod/Node clients, when the
externalTrafficPolicy of the Service is set to "Local", it will not work on Nodes
without an Endpoint. With this PR, even when the externalTrafficPolicy is set
to "Local", Pod/Node clients without local Endpoints can still work by selecting
Endpoints from the cluster.