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.

Sign up for GitHub

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

Jump to bottom

[Multicast] Add precheck on the encryption mode configurations #5920

Merged
merged 1 commit into from
Jan 30, 2024
Merged

[Multicast] Add precheck on the encryption mode configurations #5920

merged 1 commit into from
Jan 30, 2024

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Jan 25, 2024 •
edited

Mutlicast feature can't work with WireGuard or IPSec configurations with encap mode. It will return error when Multicast feature gate is enabled and either Wireguard or IPSec is configured in the initial validations.

Fix: #5916

@wenyingd wenyingd force-pushed the issue_5916 branch 4 times, most recently from a5fa0a8 to e15be5a Compare January 25, 2024 06:37
@wenyingd
Copy link
Contributor Author

/test-all

antoninbas
antoninbas reviewed Jan 25, 2024
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

I understand why it doesn't work with WireGuard. What's the reason why it doesn't work with IPsec?

@wenyingd
Copy link
Contributor Author

I understand why it doesn't work with WireGuard. What's the reason why it doesn't work with IPsec?

@antoninbas For ipsec mode, antrea doesn't use flow based tunnel port (antrea-tun0), instead, antrea-agent would create one tunnel port per peer Node. For multicast, we have only design the flows to support antrea-tun0 by setting tun_dst in OpenFlow actions with encap mode. We have no logic to find the corresponding ipsec tunnel port with peer PodCIDR.

@antoninbas
Copy link
Contributor

I understand why it doesn't work with WireGuard. What's the reason why it doesn't work with IPsec?

@antoninbas For ipsec mode, antrea doesn't use flow based tunnel port (antrea-tun0), instead, antrea-agent would create one tunnel port per peer Node. For multicast, we have only design the flows to support antrea-tun0 by setting tun_dst in OpenFlow actions with encap mode. We have no logic to find the corresponding ipsec tunnel port with peer PodCIDR.

That's right, thanks for reminding me.

antoninbas
antoninbas previously approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas 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

@wenyingd wenyingd force-pushed the issue_5916 branch 2 times, most recently from d038817 to 44fde80 Compare January 26, 2024 08:25
@wenyingd
Copy link
Contributor Author

/test-all

@wenyingd
Copy link
Contributor Author

@antoninbas I updated antrea-ipsec.yml with its generator by disabling Multicast feature gate by default, otherwise the previous manifest may not work because of the added check. Would you review it again?

antoninbas
antoninbas reviewed Jan 26, 2024
View reviewed changes
cmd/antrea-agent/options_test.go Outdated
Comment on lines 270 to 272
assert.Equal(t, tt.expectedErr, err)
assert.Equal(t, tt.expectedVersions, o.igmpQueryVersions)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of this, we usually use require to verify the error. This way the test stops early if the err value is not what we expect, and we "skip" subsequent assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the test, we have designed both happy path and bad paths. This check is valid only with happy path (err is nil ). If we use assert.Require, it may only work with the happy cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I think I wasn't clear, I was just suggesting replacing assert.Equal(t, tt.expectedErr, err) with require.Equal(t, tt.expectedErr, err)

cmd/antrea-agent/options.go Outdated Show resolved Hide resolved
cmd/antrea-agent/options.go Outdated
@@ -320,9 +320,12 @@ func (o *Options) validateFlowExporterConfig() error {
return nil
}

func (o *Options) validateMulticastConfig() error {
func (o *Options) validateMulticastConfig(encryptionMode config.TrafficEncryptionModeType) error {
if features.DefaultFeatureGate.Enabled(features.Multicast) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at your latest code, we should actually add o.config.Multicast.Enable to the condition. Otherwise, this change is not user friendly at all: when installing Antrea (either using the manifest or the Helm chart), users will need to explicitly disable the Multicast FeatureGate. That should not be required. Instead, we should only return validation errors if WireGuard is enabled AND Multicast was explicitly enabled using the boolean toggle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, will update.

@wenyingd wenyingd force-pushed the issue_5916 branch 2 times, most recently from 30b7b51 to 5f87266 Compare January 29, 2024 08:21
@wenyingd
Copy link
Contributor Author

/test-all

antoninbas
antoninbas reviewed Jan 29, 2024
View reviewed changes
build/yamls/chart-values/antrea-ipsec.yml Outdated Show resolved Hide resolved
cmd/antrea-agent/options_linux_test.go Outdated Show resolved Hide resolved
cmd/antrea-agent/options.go Outdated Show resolved Hide resolved
cmd/antrea-agent/options_test.go Outdated
Comment on lines 270 to 272
assert.Equal(t, tt.expectedErr, err)
assert.Equal(t, tt.expectedVersions, o.igmpQueryVersions)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I think I wasn't clear, I was just suggesting replacing assert.Equal(t, tt.expectedErr, err) with require.Equal(t, tt.expectedErr, err)

hack/generate-manifest.sh Outdated Show resolved Hide resolved
test/e2e/wireguard_test.go
@@ -45,16 +45,20 @@ func TestWireGuard(t *testing.T) {
skipIfMissingKernelModule(t, data, node.name, []string{"wireguard"})
}
var previousTrafficEncryptionMode string
var previousMulticastEnabledState bool
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto: changes to this file can be reverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

We may still need this change, otherwise, "E2e tests on a Kind cluster on Linux with all features enabled" may fail as Multicast was enabled before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. It would have been nice to have a comment here explaining that enabling WireGuard requires disabling Multicast, but no big deal, I see that CI jobs have passed already

@wenyingd
Copy link
Contributor Author

/test-all

@wenyingd
[Multicast] Add precheck on the encryption mode configurations
87a311e 
Mutlicast feature can't work with WireGuard or IPSec configurations with encap
mode. It will return error when Multicast feature gate is enabled and either
Wireguard or IPSec is configured in the initial validations.

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

/test-all

2 similar comments
@wenyingd
Copy link
Contributor Author

/test-all

@hjiajing
Copy link
Contributor

/test-all

@XinShuYang
Copy link
Contributor

/test-all

@antoninbas antoninbas merged commit e6a9612 into antrea-io:main Jan 30, 2024
51 of 54 checks passed
@wenyingd wenyingd deleted the issue_5916 branch April 3, 2024 03:28
luolanzone added a commit to luolanzone/antrea that referenced this pull request Apr 26, 2024
@luolanzone
Skip IPSec/WireGuard e2e test when the Multicast feature is enabled
bac8086 
This is an e2e fix because of PR antrea-io#5920.

Signed-off-by: Lan Luo <luola@vmware.com>
@luolanzone luolanzone mentioned this pull request Apr 26, 2024
Merged
luolanzone added a commit to luolanzone/antrea that referenced this pull request Apr 28, 2024
@luolanzone
Skip IPSec/WireGuard e2e test when the Multicast feature is enabled
1563807 
This is an e2e fix because of PR antrea-io#5920.

Signed-off-by: Lan Luo <luola@vmware.com>
tnqn pushed a commit that referenced this pull request May 10, 2024
@luolanzone
Skip IPSec/WireGuard e2e test when the Multicast feature is enabled (#…
0f6fcdd 
…6264)

This is an e2e fix because of PR #5920.

Signed-off-by: Lan Luo <luola@vmware.com>
luolanzone added a commit to luolanzone/antrea that referenced this pull request Jun 17, 2024
@luolanzone
Skip IPSec/WireGuard e2e test when the Multicast feature is enabled (a…
269ce81 
…ntrea-io#6264)

This is an e2e fix because of PR antrea-io#5920.

Signed-off-by: Lan Luo <luola@vmware.com>
tnqn pushed a commit that referenced this pull request Jun 17, 2024
@luolanzone
Skip IPSec/WireGuard e2e test when the Multicast feature is enabled (#…
0571e4b 
…6264) (#6451)

This is an e2e fix because of PR #5920.

Signed-off-by: Lan Luo <luola@vmware.com>
luolanzone added a commit to luolanzone/antrea that referenced this pull request Jun 20, 2024
@luolanzone
Skip IPSec/WireGuard e2e test when the Multicast feature is enabled (a…
17f1b12 
…ntrea-io#6264)

This is an e2e fix because of PR antrea-io#5920.

Signed-off-by: Lan Luo <luola@vmware.com>
tnqn pushed a commit that referenced this pull request Jun 20, 2024
@luolanzone
Skip IPSec/WireGuard e2e test when the Multicast feature is enabled (#…
bce5c2e 
…6264) (#6466)

This is an e2e fix because of PR #5920.

Signed-off-by: Lan Luo <luola@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoninbas antoninbas antoninbas approved these changes

@ceclinux ceclinux Awaiting requested review from ceclinux

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Antrea Agent should fail when enabling Multicast with WireGuard
4 participants
@wenyingd @antoninbas @hjiajing @XinShuYang