-
Notifications
You must be signed in to change notification settings - Fork 362
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
Support configurable interfaces for Pod traffic across Node #2370
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2370 +/- ##
==========================================
+ Coverage 59.86% 64.88% +5.02%
==========================================
Files 284 284
Lines 22178 25457 +3279
==========================================
+ Hits 13277 16518 +3241
+ Misses 7480 7384 -96
- Partials 1421 1555 +134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
fc73f8f
to
a652c83
Compare
/test-windows-all |
/test-windows-conformance |
92298e6
to
c2ea073
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.
In the commit message: "SNAT is performing by" -> "SNAT is performed by".
44745b2
to
d27871e
Compare
pkg/agent/util/net_others.go
Outdated
|
||
// GetVirtualInterfaceByName is not implemented. An error returned when this function is called, which means the required | ||
// interface is not found. | ||
func GetVirtualInterfaceByName(ifaceName, ovsBridgeName string) (*net.Interface, error) { |
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.
I understand this cause compiliation issue on MAC but this is not the only missing function, can we rename net_linux to net_other instead?
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.
In my mind, some dependent libraries in current net_linux (possibly is netlink) is not supported by MAC, that's why we used to rename it from net_other to net_linux.
pkg/agent/util/net_windows.go
Outdated
func CreateNetNatOnHost(subnetCIDR *net.IPNet) error { | ||
netNatName := "antrea-nat" | ||
cmd := fmt.Sprintf(`Remove-NetNat -Name %s -Confirm:$false`, netNatName) | ||
if _, err := ps.RunCommand(cmd); err != nil { |
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.
Can we do "create if not found" instead of "remove and create" to be less disruptive?
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.
Actually, I assumed the PodCIDR is possibly changed on the Windows host, that's why I use "remove and create". But if this assumption doesn't exist, I would 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.
To be safe, it can compare whether the value is expected first, then decide whether to remove and create.
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.
Get-NetNat doesn't support using "InternalIPInterfaceAddressPrefix" as filter. I would try to compare with the result fields
hi @wenyingd how are you compiling the executable here? when I ran this antrea-agent.exe, I got a "not a valid windows executable" error message. To test:
|
I usually compile the binary on Windows host. I have installed cygwin on Windows host, and run |
e995e7f
to
40fb21f
Compare
/test-windows-networkpolicy |
2 similar comments
/test-windows-networkpolicy |
/test-windows-networkpolicy |
I assume this works for Linux too. Could you remove |
/test-all |
# The name of the interface on Node which is used for tunneling or routing the traffic across Nodes. | ||
# If there are multiple IP addresses configured on the interface, the first one is used. | ||
# The interface configured with Node IP is used if this parameter is not set. | ||
#transportInterface: |
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.
Sometimes there have different interface name on each node, which will be used to inter-host communication.
Maybe need to use special cidr range for inter-host communication, such as antrea-agent --iface-cidr 172.16.100.0/24
, could we add a param transportCIDRRange
for this scenarios? @wenyingd @tnqn
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.
Do you mean on each Node, we look up the interface by IP which should fall into the transportCIDRRange?
But it is also possible Nodes' interfaces are in different CIDRs. I feel not easy to cover all cases, but do you already have a use case that Nodes use different interfaces?
/test-integration |
/test-integration |
/test-integration |
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. two minor comments left
1. SNAT is performed by Windows host for Pod-to-external traffic, not by OVS 2. Antrea Agent uses the configurable interface for Pod traffic. The interface which is configured with Node IP is chosen, if user doesn't configure the in-band traffic interface. Signed-off-by: wenyingd <wenyingd@vmware.com> Co-authored-by: Quan Tian <qtian@vmware.com>
/test-all |
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, thanks
/test-windows-conformance All comments have been addressed except #2370 (comment), which we are tracking in #2473. I will merge it after the remaining tests pass. |
@wenyingd could you check if the failures of jenkins-windows-conformance and jenkins-windows-networkpolicy are related? |
No, it is CI testbed issue. I have passed Windows conformance and networkpolicy tests in my setup many times. |
hi folks, cool, do we have any releases that has this fix ? |
Hi @jayunit100 - this was just merged last week. It is not a bug fix and will not be backported. It will be included in Antrea v1.3 (end of August). |
thanks ! anyone have an antrea.exe binary aroujnd i can use? |
@jayunit100 hello, currently there's no place to download an exe with this patch but you can compile yourself. How did you do it? From my side, I usually |
which is configured with Node IP is chosen, if user doesn't configure the
in-band traffic interface.
Signed-off-by: wenyingd wenyingd@vmware.com
Fixes #2344