Skip to content
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

[Windows] Use uplink MAC as source if packet is output to uplink #3516

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

wenyingd
Copy link
Contributor

With noEncap mode the Pod packet to remote Pod/Node is output to the
uplink interface directly. This change modifies the source MAC with the
uplink interface's MAC, so that it doesn't require hybrid configurations
on the host interface.

Fixes #3505

Signed-off-by: wenyingd wenyingd@vmware.com

With noEncap mode the Pod packet to remote Pod/Node is output to the
uplink interface directly. This change modifies the source MAC with the
uplink interface's MAC, so that it doesn't require hybrid configurations
on the host interface.

Signed-off-by: wenyingd <wenyingd@vmware.com>
@wenyingd
Copy link
Contributor Author

/test-windows-all
/test-all
/skip-ipv6-all
/skip-ipv6-only-all

@XinShuYang
Copy link
Contributor

/test-windows-e2e

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2022

Codecov Report

Merging #3516 (483c3f3) into main (2c7d486) will decrease coverage by 9.78%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3516      +/-   ##
==========================================
- Coverage   65.58%   55.79%   -9.79%     
==========================================
  Files         277      391     +114     
  Lines       27264    52463   +25199     
==========================================
+ Hits        17881    29272   +11391     
- Misses       7483    20756   +13273     
- Partials     1900     2435     +535     
Flag Coverage Δ
e2e-tests 54.23% <0.00%> (?)
integration-tests 38.18% <ø> (?)
kind-e2e-tests 55.74% <0.00%> (-0.20%) ⬇️
unit-tests 43.08% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/openflow/pipeline.go 75.30% <0.00%> (-1.64%) ⬇️
pkg/agent/cniserver/pod_configuration_linux.go 26.31% <0.00%> (-40.36%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 48.71% <0.00%> (-31.57%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 61.46% <0.00%> (-29.97%) ⬇️
pkg/controller/egress/controller.go 62.19% <0.00%> (-26.26%) ⬇️
pkg/controller/ipam/validate.go 57.95% <0.00%> (-24.31%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go 56.66% <0.00%> (-23.34%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go 57.24% <0.00%> (-22.55%) ⬇️
pkg/agent/util/iptables/lock.go 60.00% <0.00%> (-21.82%) ⬇️
pkg/controller/egress/store/egressgroup.go 37.93% <0.00%> (-21.80%) ⬇️
... and 354 more

Copy link
Contributor

@hongliangl hongliangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@XinShuYang
Copy link
Contributor

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-windows-conformance
/test-e2e

Copy link
Contributor

@XinShuYang XinShuYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, CI tests passed on windows testbed without promiscuous mode support. I will update all windows testbeds setup once this pr is merged. @tnqn @wenyingd

@tnqn tnqn merged commit b283f2d into antrea-io:main Mar 25, 2022
@tnqn tnqn added this to the Antrea v1.6 release milestone Mar 25, 2022
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 25, 2022
@tnqn
Copy link
Member

tnqn commented Mar 26, 2022

LGTM, CI tests passed on windows testbed without promiscuous mode support. I will update all windows testbeds setup once this pr is merged. @tnqn @wenyingd

@XinShuYang not only promiscuous mode, could you check whether it can work without enabling "MAC Address Changes" and "Forged transmits"? I think this patch is irrelavant to promiscuous mode, which is not required at all from the begining.

@tnqn
Copy link
Member

tnqn commented Mar 26, 2022

@wenyingd without this patch, vsphere and cloud platforms need to disable MAC check to work around, could you backport it to previous releases starting from 1.2 as well?

@tnqn tnqn added the action/backport Indicates a PR that requires backports. label Mar 26, 2022
@tnqn tnqn mentioned this pull request Mar 26, 2022
@XinShuYang
Copy link
Contributor

LGTM, CI tests passed on windows testbed without promiscuous mode support. I will update all windows testbeds setup once this pr is merged. @tnqn @wenyingd

@XinShuYang not only promiscuous mode, could you check whether it can work without enabling "MAC Address Changes" and "Forged transmits"? I think this patch is irrelavant to promiscuous mode, which is not required at all from the begining.

@tnqn I understand we need to disable "Forged transmits" to drop packets with noneffective MAC address, could you explain why it is necessary to disable "MAC Address Changes" for this PR?

@tnqn
Copy link
Member

tnqn commented Mar 28, 2022

@XinShuYang Not only for this PR, I think in theory we don't require any of these options to be configured but all of them were required to be set to specific values due to either misunderstanding or implementation bug. My point is to ensure everything works as expected, then we can remove these restrictions from document (if there is any) to clarify.

@XinShuYang
Copy link
Contributor

@tnqn Thanks for the explanation, I will retest it with new setup.

@XinShuYang
Copy link
Contributor

/test-windows-all

@XinShuYang
Copy link
Contributor

/test-windows-e2e

@wenyingd wenyingd deleted the issue_3505 branch August 15, 2022 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Received duplicate echo reply packets when pinging Windows hosts
6 participants