Fix that AntreaProxy could unintentionally delete conntrack entries in zone 0 #6193

hongliangl · 2024-04-07T11:22:38Z

This is a subsequent PR for #5112. As mentioned in #5112:

Due to the restriction of the go library 'netlink', there is no API to specify a
target zone. As a result, when deleting a stale conntrack entry with a
destination port (such as NodePort), not only will the conntrack entry whose
destination port is the port added by AntreaProxy be deleted, but also the
conntrack entry that is not added by AntreaProxy will be deleted. This behavior
is unexpected, as only the conntrack entries added by AntreaProxy should be
deleted.

This PR resolves the issue by integrating a CT zone filter, now available in
the latest Go library netlink. Leveraging this feature, AntreaProxy can
accurately delete stale UDP conntrack entries.

tnqn

LGTM

tnqn · 2024-04-11T03:09:07Z

Some tests failed, can you check?

hongliangl · 2024-04-11T03:24:55Z

Some tests failed, can you check?

Sure. Besides, I was wondering if we could promote the feature gate CleanupStaleUDPSvcConntrack to Beta stage?

tnqn · 2024-04-11T03:34:19Z

Some tests failed, can you check?

Sure. Besides, I was wondering if we could promote the feature gate CleanupStaleUDPSvcConntrack to Beta stage?

How about running it for a while in CI first? I want to ensure this doesn't cause unexpected behaviors, though it doesn't seem to. Promoting a feature shouldn't be in the same PR as a bugfix, the former needs to be more careful and the latter can potentially be backported (though not this case) while the former will never.

hongliangl · 2024-04-11T15:31:28Z

Some tests failed, can you check?

Sure. Besides, I was wondering if we could promote the feature gate CleanupStaleUDPSvcConntrack to Beta stage?

How about running it for a while in CI first? I want to ensure this doesn't cause unexpected behaviors, though it doesn't seem to. Promoting a feature shouldn't be in the same PR as a bugfix, the former needs to be more careful and the latter can potentially be backported (though not this case) while the former will never.

Got that. I have resolved the test failure of unit tests and integration tests, and I found that e2e test E2e tests on a Kind cluster on Linux got a failure. I'm not sure if it is caused by the bump of netlink. I try to run this test several times to see that if the failed tests can be reproduced every time.

hongliangl · 2024-04-12T11:28:10Z

@tnqn I found that failure of test E2e tests on a Kind cluster on Linux was caused by the test TestAntreaPolicy/TestGroupValidationWebhook/Case=CreateInvalidTier. The following is the output. I'm not sure if this is caused by the bump of netlink. Have we had such test failures before?

=== RUN   TestAntreaPolicy/TestGroupValidationWebhook/Case=CreateInvalidTier
I0411 08:28:10.128451   23360 k8s_util.go:758] Creating tier tier-prio-20
I0411 08:28:10.138881   23360 k8s_util.go:758] Creating tier another-tier-prio-20
E0411 08:28:10.146187   23360 antreapolicy_test.go:82] Tiers created with overlapping priorities
I0411 08:28:10.153528   23360 k8s_util.go:1175] Deleting test Namespace {x-essto3kz map[]}
I0411 08:28:16.164888   23360 framework.go:793] Deleting Namespace x-essto3kz took 6.011230475s
I0411 08:28:16.164939   23360 k8s_util.go:1175] Deleting test Namespace {y-essto3kz map[]}
I0411 08:28:22.173253   23360 framework.go:793] Deleting Namespace y-essto3kz took 6.008293191s
I0411 08:28:22.174063   23360 k8s_util.go:1175] Deleting test Namespace {z-essto3kz map[]}
I0411 08:28:28.184114   23360 framework.go:793] Deleting Namespace z-essto3kz took 6.009502881s
    antreapolicy_test.go:84: test failed: Tiers created with overlapping priorities

See L506-L516 in https://github.com/antrea-io/antrea/actions/runs/8643534991/job/23697191286?pr=6193 for more information.

Got the same failure at L505-515 in attempt 2 https://github.com/antrea-io/antrea/actions/runs/8643534991/job/23700838340?pr=6193

Got the same failure at L564-574 in attempt 4 https://github.com/antrea-io/antrea/actions/runs/8643534991/job/23715419360?pr=6193

hongliangl · 2024-04-12T11:51:27Z

/test-e2e

hongliangl · 2024-04-15T04:18:40Z

/test-e2e

gran-vmv · 2024-05-17T02:50:53Z

pkg/agent/route/route_linux.go

@@ -33,6 +33,7 @@ import (
 	utilnet "k8s.io/utils/net"

 	"antrea.io/antrea/pkg/agent/config"
+	"antrea.io/antrea/pkg/agent/openflow"
 	"antrea.io/antrea/pkg/agent/servicecidr"
 	"antrea.io/antrea/pkg/agent/types"
 	"antrea.io/antrea/pkg/agent/util/ipset"


Also need to fix if r.Dst == nil in this file?

tnqn

LGTM

hongliangl · 2024-05-28T07:58:37Z

/test-latest-conformance

hongliangl · 2024-05-28T09:09:36Z

/test-latest-conformance

tnqn

LGTM

tnqn · 2024-05-28T14:47:38Z

/skip-all

In antrea-io#5112, due to the limitations of the Go netlink library, AntreaProxy would unconditionally delete conntrack entries added by kube-proxy in conntrack zone 0. AntreaProxy was supposed to only delete its own entries in conntrack zones 65520 or 65521. To address this, a feature was added to isolate the relevant code. After the merge of antrea-io#6193, the netlink library was updated, allowing AntreaProxy to precisely delete conntrack entries in zones 65520 or 65521. It is now safe to enable the corresponding code by default. Signed-off-by: Hongliang Liu <[email protected]>

gran-vmv · 2024-06-05T03:31:24Z

@hongliangl Do we need to backport this PR? My PR #6321 rely on this and it cannot be backported without this PR.

hongliangl · 2024-06-05T03:33:52Z

You could back them together.

hongliangl · 2024-06-05T03:34:27Z

I meant multiple PRs could be backported together in a PR.

…n zone 0 (antrea-io#6193) This is a subsequent PR for antrea-io#5112. As mentioned in antrea-io#5112: > Due to the restriction of the go library 'netlink', there is no API to specify a target zone. As a result, when deleting a stale conntrack entry with a destination port (such as NodePort), not only will the conntrack entry whose destination port is the port added by AntreaProxy be deleted, but also the conntrack entry that is not added by AntreaProxy will be deleted. This behavior is unexpected, as only the conntrack entries added by AntreaProxy should be deleted. This PR resolves the issue by integrating a CT zone filter, now available in the latest Go library `netlink`. Leveraging this feature, AntreaProxy can accurately delete stale UDP conntrack entries. Signed-off-by: Hongliang Liu <[email protected]>

…n zone 0 (#6193) (#6406) This is a subsequent PR for #5112. As mentioned in #5112: > Due to the restriction of the go library 'netlink', there is no API to specify a target zone. As a result, when deleting a stale conntrack entry with a destination port (such as NodePort), not only will the conntrack entry whose destination port is the port added by AntreaProxy be deleted, but also the conntrack entry that is not added by AntreaProxy will be deleted. This behavior is unexpected, as only the conntrack entries added by AntreaProxy should be deleted. This PR resolves the issue by integrating a CT zone filter, now available in the latest Go library `netlink`. Leveraging this feature, AntreaProxy can accurately delete stale UDP conntrack entries. Signed-off-by: Hongliang Liu <[email protected]>

…n zone 0 (#6193) (#6404) This is a subsequent PR for #5112. As mentioned in #5112: > Due to the restriction of the go library 'netlink', there is no API to specify a target zone. As a result, when deleting a stale conntrack entry with a destination port (such as NodePort), not only will the conntrack entry whose destination port is the port added by AntreaProxy be deleted, but also the conntrack entry that is not added by AntreaProxy will be deleted. This behavior is unexpected, as only the conntrack entries added by AntreaProxy should be deleted. This PR resolves the issue by integrating a CT zone filter, now available in the latest Go library `netlink`. Leveraging this feature, AntreaProxy can accurately delete stale UDP conntrack entries. Signed-off-by: Hongliang Liu <[email protected]>

…n zone 0 (#6193) (#6405) This is a subsequent PR for #5112. As mentioned in #5112: > Due to the restriction of the go library 'netlink', there is no API to specify a target zone. As a result, when deleting a stale conntrack entry with a destination port (such as NodePort), not only will the conntrack entry whose destination port is the port added by AntreaProxy be deleted, but also the conntrack entry that is not added by AntreaProxy will be deleted. This behavior is unexpected, as only the conntrack entries added by AntreaProxy should be deleted. This PR resolves the issue by integrating a CT zone filter, now available in the latest Go library `netlink`. Leveraging this feature, AntreaProxy can accurately delete stale UDP conntrack entries. Signed-off-by: Hongliang Liu <[email protected]>

In antrea-io#5112, due to the limitations of the Go netlink library, AntreaProxy would unconditionally delete conntrack entries added by kube-proxy in conntrack zone 0. AntreaProxy was supposed to only delete its own entries in conntrack zones 65520 or 65521. To address this, a feature was added to isolate the relevant code. After the merge of antrea-io#6193, the netlink library was updated, allowing AntreaProxy to precisely delete conntrack entries in zones 65520 or 65521. It is now safe to enable the corresponding code by default. Signed-off-by: Hongliang Liu <[email protected]>

In #5112, due to the limitations of the Go netlink library, AntreaProxy would unconditionally delete conntrack entries added by kube-proxy in conntrack zone 0. AntreaProxy was supposed to only delete its own entries in conntrack zones 65520 or 65521. To address this, a feature was added to isolate the relevant code. After the merge of #6193, the netlink library was updated, allowing AntreaProxy to precisely delete conntrack entries in zones 65520 or 65521. It is now safe to enable the corresponding code by default. Signed-off-by: Hongliang Liu <[email protected]>

hongliangl marked this pull request as draft April 7, 2024 11:22

hongliangl marked this pull request as ready for review April 11, 2024 01:20

hongliangl force-pushed the 20240407-stale-udp-connection-delete branch from 4dbda3d to 6fd4aec Compare April 11, 2024 01:28

hongliangl added the area/proxy Issues or PRs related to proxy functions in Antrea label Apr 11, 2024

hongliangl requested a review from tnqn April 11, 2024 01:29

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Apr 11, 2024

tnqn previously approved these changes Apr 11, 2024

View reviewed changes

hongliangl changed the title ~~Fix that AntreaProxy could unintentionally delete conntrack entries in zone 0~~ Promote feature gate CleanupStaleUDPSvcConntrack to Beta Apr 11, 2024

hongliangl dismissed tnqn’s stale review via bb6ba90 April 11, 2024 03:23

hongliangl force-pushed the 20240407-stale-udp-connection-delete branch from 6fd4aec to bb6ba90 Compare April 11, 2024 03:23

hongliangl force-pushed the 20240407-stale-udp-connection-delete branch 2 times, most recently from 09305f1 to fc64dcc Compare April 11, 2024 03:29

hongliangl force-pushed the 20240407-stale-udp-connection-delete branch from fc64dcc to 233faac Compare April 11, 2024 05:30

hongliangl changed the title ~~Promote feature gate CleanupStaleUDPSvcConntrack to Beta~~ Fix that AntreaProxy could unintentionally delete conntrack entries in zone 0 Apr 11, 2024

hongliangl added the action/backport Indicates a PR that requires backports. label Apr 11, 2024

hongliangl force-pushed the 20240407-stale-udp-connection-delete branch 2 times, most recently from 17bb6f3 to 68e1082 Compare April 11, 2024 08:09

hongliangl force-pushed the 20240407-stale-udp-connection-delete branch from 68e1082 to 3f5be8c Compare April 24, 2024 07:40

gran-vmv reviewed May 17, 2024

View reviewed changes

gran-vmv mentioned this pull request May 17, 2024

Remove unexpected AltName after rename interface #6321

Merged

hongliangl force-pushed the 20240407-stale-udp-connection-delete branch 2 times, most recently from bd253ea to d4928cd Compare May 23, 2024 09:38

tnqn previously approved these changes May 23, 2024

View reviewed changes

tnqn approved these changes May 28, 2024

View reviewed changes

tnqn merged commit df1655c into antrea-io:main May 28, 2024
55 of 58 checks passed

hongliangl mentioned this pull request May 28, 2024

Promote feature CleanupStaleUDPSvcConntrack from Alpha to Beta #6372

Merged

hongliangl deleted the 20240407-stale-udp-connection-delete branch May 28, 2024 16:02

gran-vmv mentioned this pull request Jun 5, 2024

Automated cherry pick of #6321: Remove unexpected AltName after rename interface (#6321) #6401

Merged

hongliangl mentioned this pull request Jun 5, 2024

Automated cherry pick of #6193: Fix that AntreaProxy could unintentionally delete conntrack #6404

Merged

hongliangl mentioned this pull request Jun 5, 2024

Automated cherry pick of #6193: Fix that AntreaProxy could unintentionally delete conntrack #6405

Merged

hongliangl mentioned this pull request Jun 5, 2024

Automated cherry pick of #6193: Fix that AntreaProxy could unintentionally delete conntrack #6406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix that AntreaProxy could unintentionally delete conntrack entries in zone 0 #6193

Fix that AntreaProxy could unintentionally delete conntrack entries in zone 0 #6193

hongliangl commented Apr 7, 2024 •

edited

Loading

tnqn left a comment

tnqn commented Apr 11, 2024

hongliangl commented Apr 11, 2024

tnqn commented Apr 11, 2024

hongliangl commented Apr 11, 2024 •

edited

Loading

hongliangl commented Apr 12, 2024

hongliangl commented Apr 12, 2024

hongliangl commented Apr 15, 2024

gran-vmv May 17, 2024

hongliangl May 23, 2024

tnqn left a comment

hongliangl commented May 28, 2024

hongliangl commented May 28, 2024

tnqn left a comment

tnqn commented May 28, 2024

gran-vmv commented Jun 5, 2024

hongliangl commented Jun 5, 2024

hongliangl commented Jun 5, 2024

Fix that AntreaProxy could unintentionally delete conntrack entries in zone 0 #6193

Fix that AntreaProxy could unintentionally delete conntrack entries in zone 0 #6193

Conversation

hongliangl commented Apr 7, 2024 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 11, 2024

hongliangl commented Apr 11, 2024

tnqn commented Apr 11, 2024

hongliangl commented Apr 11, 2024 • edited Loading

hongliangl commented Apr 12, 2024

hongliangl commented Apr 12, 2024

hongliangl commented Apr 15, 2024

gran-vmv May 17, 2024

Choose a reason for hiding this comment

hongliangl May 23, 2024

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

hongliangl commented May 28, 2024

hongliangl commented May 28, 2024

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented May 28, 2024

gran-vmv commented Jun 5, 2024

hongliangl commented Jun 5, 2024

hongliangl commented Jun 5, 2024

hongliangl commented Apr 7, 2024 •

edited

Loading

hongliangl commented Apr 11, 2024 •

edited

Loading