-
Notifications
You must be signed in to change notification settings - Fork 359
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
[Bugfix] Install Multicast related iptables rules only on IPv4 chains #6123
Conversation
073fe52
to
47585b4
Compare
/test-all |
/test-ipv6-e2e |
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.
The PR should close #6113 IMO. We can open another issue to track Multicast IPv6 support.
pkg/agent/agent.go
Outdated
@@ -130,6 +130,7 @@ type Initializer struct { | |||
enableL7FlowExporter bool | |||
connectUplinkToBridge bool | |||
enableAntreaProxy bool | |||
enableMulticast bool |
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.
what there an issue with doing the check in mcastController.Initialize
to avoid adding this field?
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 used to think about placing the check at the step before constructing multicastController instance, then some initial configurations may already be performed at that time, like OpenFlow, routes, cniserver, which seems obey the thought that block at the beginning when configurations is not correct.
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 agree with @antoninbas. Other than the validation performed in startup stage, other validation logic should be put in its own module to better encapsulate the specific logic, instead of spreading the logic among multiple places, each of which does a piece of the whole.
Generically, we should avoid making a "common" module include unrelated responsibilies. I'm not sure if AgentInitializer's responsibity is really clear today, but my understanding and expectation is that it is a module that initializes the bridge and the host network, not a validator for other modules.
Think about which is easier to maintain when doing something around Multicast:
- add the static config handling in option.go, add runtime config validation in AgentInitializer, then add initialization error handling in Multicast controller
- add the static config handling in option.go, add other handling in Multicast controller
And consider what if a config can only be validated after you have made some system calls or API calls, it would be bad to move such business specific system calls or API calls to something like AgentInitializer.
Lastly, it's less redundant to have the validation in multicast controller itself because it doesn't have to check again whether the feature is enabled when doing the validation (the validation doesn't need to run when it's disabled).
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.
Got it, I will update accordingly.
a09e2d0
to
7af7f6d
Compare
0ade0df
to
0cfb8ef
Compare
/test-all |
@@ -128,8 +128,6 @@ type Client struct { | |||
nodePortsIPv6 sync.Map | |||
// clusterNodeIPs stores the IPv4 of all other Nodes in the cluster | |||
clusterNodeIPs sync.Map | |||
// clusterNodeIP6s stores the IPv6 of all other Nodes in the cluster | |||
clusterNodeIP6s sync.Map |
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 we need clusterNodeIP6s when implementing Multicast for IPv6 in the future?
If yes, perhaps don't remove it, just remove its consumer code and add a comment that it's retained for future Multicast support to avoid back and forth 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.
We shall use it in the future after Multicast is supported with IPv6. Do you mean we can keep the publisher to add Node IPv6 address into this map, but not consume the items?
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.
Yes, just add the comment why it's maintained but not used currently.
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.
updated.
7f2a52d
to
850d2bc
Compare
/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.
only minor comments, otherwise LGTM
mockController.initMocks() | ||
} | ||
err := mockController.Initialize() | ||
require.Equal(t, err, tc.expErr) |
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.
should be assert
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.
update with assert.EqualError as suggested.
}) | ||
} | ||
} | ||
|
||
func (c *Controller) initialize(t *testing.T) 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.
The t
parameter is unused?
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.
yes, removed in the latest change.
if tc.expErr == "" { | ||
mockController.initMocks() | ||
} | ||
err := mockController.Initialize() |
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.
could we just call err := mockController.initialize()
unconditionally here?
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.
No, there are some mock functions inside mockController.initialize()
, which are called only when mockController.Initialize()
returns nil. If we call it (initialize not Initialize) unconditionally, the IPv6-only case would fail as those mock expects are missing.
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
@wenyingd Please backport when this is merged |
/test-multicast-e2e |
Sure, I would do it. |
pkg/agent/route/route_linux.go
Outdated
@@ -128,7 +128,8 @@ type Client struct { | |||
nodePortsIPv6 sync.Map | |||
// clusterNodeIPs stores the IPv4 of all other Nodes in the cluster | |||
clusterNodeIPs sync.Map | |||
// clusterNodeIP6s stores the IPv6 of all other Nodes in the cluster | |||
// clusterNodeIP6s stores the IPv6 address of all other Nodes in the cluster. It is maintained but not consumed | |||
// since Multicast supports IPv6. |
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.
// since Multicast supports IPv6. | |
// since Multicast doesn't support IPv6 yet. |
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 would update it as It is maintained but not consumed until Multicast supports IPv6.
It seems I missed this comment previously.
Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@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
…-io#6123) Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@vmware.com>
…-io#6123) Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@vmware.com>
…-io#6123) Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@vmware.com>
…-io#6123) Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@vmware.com>
…#6175) Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@vmware.com>
…#6174) Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@vmware.com>
…#6173) Add a pre-check on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains. Signed-off-by: Wenying Dong <wenyingd@vmware.com>
Add a precheck on the Multicast feature gate status with IPv6-only cluster settings in agent Initializer, and install the iptables rules only in the IPv4 related chains.
Fix: #6113