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

Add ignored interfaces names when getting interface by IP #3219

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

wenyingd
Copy link
Contributor

This is to resolve an issue that caused by two interfaces configured
with the same IP address (different masks) on the host. The issue
occurred in AKE setup with NetworkPolicyOnly mode, and antrea-gw0 is
configured with Node's IP with 32bit mask.

The changes include,

  1. Provide a set of ignored interface names (e.g., antrea-gw0) when
    getting the Node's transport interface with Node's IP in
    agentInitializer.
  2. Use Node's transport interface name to get the accurate interface in
    Egress feature instead of Node's IP.

Fixes #3217

Signed-off-by: wenyingd [email protected]

@wenyingd wenyingd force-pushed the issue_3217 branch 2 times, most recently from 7eabe74 to aeed963 Compare January 21, 2022 07:33
@wenyingd wenyingd requested review from wenqiq, xliuxu and tnqn and removed request for wenqiq January 21, 2022 07:33
@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2022

Codecov Report

Merging #3219 (b25ba04) into main (f7f353a) will decrease coverage by 7.86%.
The diff coverage is 75.00%.

❗ Current head b25ba04 differs from pull request most recent head 8edfa59. Consider uploading reports for the commit 8edfa59 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3219      +/-   ##
==========================================
- Coverage   60.09%   52.23%   -7.87%     
==========================================
  Files         331      303      -28     
  Lines       28420    36323    +7903     
==========================================
+ Hits        17080    18973    +1893     
- Misses       9476    15552    +6076     
+ Partials     1864     1798      -66     
Flag Coverage Δ
e2e-tests 52.23% <75.00%> (?)
kind-e2e-tests ?
unit-tests ?

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

Impacted Files Coverage Δ
pkg/agent/agent_linux.go 5.02% <0.00%> (+1.40%) ⬆️
pkg/agent/ipassigner/ip_assigner_linux.go 57.69% <66.66%> (-1.29%) ⬇️
pkg/agent/util/net.go 31.11% <71.42%> (-9.71%) ⬇️
pkg/agent/agent.go 50.12% <100.00%> (-1.13%) ⬇️
pkg/agent/controller/egress/egress_controller.go 66.14% <100.00%> (+27.33%) ⬆️
...g/agent/controller/serviceexternalip/controller.go 78.42% <100.00%> (-2.44%) ⬇️
pkg/controller/egress/controller.go 0.00% <0.00%> (-88.45%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 4.58% <0.00%> (-86.85%) ⬇️
pkg/agent/util/iptables/lock.go 0.00% <0.00%> (-81.82%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 0.00% <0.00%> (-80.29%) ⬇️
... and 327 more

@@ -111,8 +111,11 @@ func dialUnix(address string) (net.Conn, error) {
return net.Dial("unix", address)
}

// GetIPNetDeviceFromIP returns local IPs/masks and associated device from IP.
func GetIPNetDeviceFromIP(localIPs *ip.DualStackIPs) (v4IPNet *net.IPNet, v6IPNet *net.IPNet, iface *net.Interface, err error) {
// GetIPNetDeviceFromIP returns local IPs/masks and associated device from IP, and ignore the interfaces which has a
Copy link
Contributor

Choose a reason for hiding this comment

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

ignore->ignores
has->have

Copy link
Contributor

@xliuxu xliuxu left a comment

Choose a reason for hiding this comment

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

LGTM overall, except for the lint error in ip_assigner_windows.go caused by unused import.

@wenyingd wenyingd changed the title Add ignored interfaces names when getting interface by IP [WIP] Add ignored interfaces names when getting interface by IP Jan 21, 2022
@wenyingd
Copy link
Contributor Author

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

@wenyingd wenyingd changed the title [WIP] Add ignored interfaces names when getting interface by IP Add ignored interfaces names when getting interface by IP Jan 24, 2022
@wenyingd
Copy link
Contributor Author

/test-e2e
/test-windows-e2e
/test-windows-networkpolicy
/test-ipv6-only-all

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM overall, have a suggestion about the readability

@@ -1114,3 +1115,7 @@ func (i *Initializer) patchNodeAnnotations(nodeName, key string, value interface
}
return nil
}

func (i *Initializer) getIgnoredHostInterfaces() sets.String {
Copy link
Member

Choose a reason for hiding this comment

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

hard to tell what's the method used for from the method name when looking at it without knowing its callers, but I cannot think of a more understandable name either.
How about creating a method and use it in the 3 callers, then comment why we need to exclude the device in single place:

func (i *Initializer) getNodeInterfaceFromIP(ips *ip.DualStackIPs) sets.String {
        // Exclude i.hostGateway to resolve an issue that two interfaces are configured with the same IPs in NetworkPolicyOnly mode. 
        return getIPNetDeviceFromIP(ips, sets.NewString(i.hostGateway))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

func GetIPNetDeviceFromIP(localIPs *ip.DualStackIPs) (v4IPNet *net.IPNet, v6IPNet *net.IPNet, iface *net.Interface, err error) {
// GetIPNetDeviceFromIP returns local IPs/masks and associated device from IP, and ignores the interfaces which have
// names in the ignoredInterfaces.
// Use ignoredInterfaces to resolve an issue that two interfaces are configured with the same IPs in NetworkPolicyOnly
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be on caller side as the util is generic, having no idea why some interfaces should be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move it to caller.

@wenyingd
Copy link
Contributor Author

/test-e2e
/test-windows-e2e
/test-windows-networkpolicy
/test-ipv6-only-all

@wenyingd wenyingd requested a review from tnqn January 26, 2022 03:45
@wenyingd
Copy link
Contributor Author

/test-windows-all
/test-networkpolicy
/test-ipv6-all

@wenyingd
Copy link
Contributor Author

/test-conformance

pkg/agent/agent.go Outdated Show resolved Hide resolved
@@ -1114,3 +1115,7 @@ func (i *Initializer) patchNodeAnnotations(nodeName, key string, value interface
}
return nil
}

func (i *Initializer) getIgnoredHostInterfaces() sets.String {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be removed?

This is to resolve an issue that caused by two interfaces configured
with the same IP address (different masks) on the host. The issue
occurred in AKE setup with NetworkPolicyOnly mode, and antrea-gw0 is
configured with Node's IP with 32bit mask.

The changes include,
1. Provide a set of ignored interface names (e.g., antrea-gw0) when
   getting the Node's transport interface with Node's IP in
   agentInitializer.
2. Use Node's transport interface name to get the accurate interface in
   Egress feature instead of Node's IP.

Signed-off-by: wenyingd <[email protected]>
@wenyingd
Copy link
Contributor Author

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

@wenyingd wenyingd requested a review from tnqn February 10, 2022 01:17
@wenyingd
Copy link
Contributor Author

/test-e2e
/test-integration
/test-windows-all
/test-ipv6-only-all

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyingd
Copy link
Contributor Author

/test-e2e
/test-ipv6-only-e2e
/test-windows-proxyall-e2e

@tnqn tnqn merged commit 397750d into antrea-io:main Feb 15, 2022
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Feb 15, 2022
bangqipropel pushed a commit to bangqipropel/antrea that referenced this pull request Mar 2, 2022
…3219)

This is to resolve an issue that caused by two interfaces configured
with the same IP address (different masks) on the host. The issue
occurred in AKE setup with NetworkPolicyOnly mode, and antrea-gw0 is
configured with Node's IP with 32bit mask.

The changes include,
1. Provide a set of ignored interface names (e.g., antrea-gw0) when
   getting the Node's transport interface with Node's IP in
   agentInitializer.
2. Use Node's transport interface name to get the accurate interface in
   Egress feature instead of Node's IP.

Signed-off-by: wenyingd <[email protected]>
@antoninbas
Copy link
Contributor

@wenyingd could we backport this patch? I see that someone else ran into the same issue, this time on EKS: #3446

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.

Antrea Agent crashed after restart in AKE with NetworkPolicyOnly mode
5 participants