-
Notifications
You must be signed in to change notification settings - Fork 389
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
Fix that Antrea L7NetworkPolicies do not handle Service traffic correctly #6941
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2719,7 +2719,7 @@ func Test_client_ReplayFlows(t *testing.T) { | |
|
||
expectedFlows := append(pipelineDefaultFlows(true /* egressTrafficShapingEnabled */, false /* externalNodeEnabled */, true /* isEncap */, true /* isIPv4 */), egressInitFlows(true)...) | ||
expectedFlows = append(expectedFlows, multicastInitFlows(true)...) | ||
expectedFlows = append(expectedFlows, networkPolicyInitFlows(true, false, false)...) | ||
expectedFlows = append(expectedFlows, networkPolicyInitFlows(true, false)...) | ||
expectedFlows = append(expectedFlows, podConnectivityInitFlows(config.TrafficEncapModeEncap, config.TrafficEncryptionModeNone, false, true, true, true)...) | ||
expectedFlows = append(expectedFlows, serviceInitFlows(true, true, false, false)...) | ||
|
||
|
@@ -2917,3 +2917,30 @@ func TestSubscribeOFPortStatusMessage(t *testing.T) { | |
bridge.EXPECT().SubscribePortStatusConsumer(ch).Times(1) | ||
c.SubscribeOFPortStatusMessage(ch) | ||
} | ||
|
||
func Test_client_InstallL7NetworkPolicyFlows(t *testing.T) { | ||
hongliangl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ctrl := gomock.NewController(t) | ||
m := opstest.NewMockOFEntryOperations(ctrl) | ||
|
||
fc := newFakeClient(m, true, false, config.K8sNode, config.TrafficEncapModeEncap, enableL7NetworkPolicy) | ||
defer resetPipelines() | ||
|
||
expectedFlows := []string{ | ||
"cookie=0x1020000000000, table=Classifier, priority=200,in_port=11,vlan_tci=0x1000/0x1000 actions=pop_vlan,set_field:0x7/0xf->reg0,goto_table:UnSNAT", | ||
"cookie=0x1020000000000, table=ConntrackZone, priority=212,ip,reg0=0x0/0x800000 actions=set_field:0x800000/0x800000->reg0,ct(table=ConntrackZone,zone=65520)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know you said this would affect all packets, causing a performance degradation, which is why we only install the flow if at least one L7NP is applied locally on the Node. It feels like we could go further, and only apply this flow if the source / destination IP matches a Pod to which a L7NP is actually applied? I am not saying that this is necessarily a good idea, as it would make the implementation more complex, but I wanted to check my understanding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a great idea! If so, we can reduce the performance degradation to only the packets from/to the Pods enforced by the L7NP, rather than all packets. This would make the implementation a little more complex. At least, I think we need a conjunction here. Could we enhance the implementation in another PR? @antoninbas cc @tnqn There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's see what Quan thinks. I think we can consider it as a possible future enhancement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed with Hongliang offline, the proposal makes sense to me, and there seems a way that is not too complex. Addressing it in a separate PR sounds good to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After an offline discussion with @tnqn, I made a draft desgin.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason why I put the new table before
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will you open a follow up issue for this improvement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do later. |
||
"cookie=0x1020000000000, table=ConntrackZone, priority=210,ct_state=+rpl+trk,ct_mark=0x80/0x80,ip actions=goto_table:Output", | ||
"cookie=0x1020000000000, table=ConntrackZone, priority=211,ct_state=+rpl+trk,ip,reg0=0x7/0xf actions=ct(table=L3Forwarding,zone=65520,nat)", | ||
"cookie=0x1020000000000, table=ConntrackZone, priority=211,ct_state=-rpl+trk,ip,reg0=0x7/0xf actions=goto_table:L3Forwarding", | ||
"cookie=0x1020000000000, table=ConntrackZone, priority=210,ct_state=-rpl+trk,ct_mark=0x80/0x80,ip actions=ct(table=ConntrackState,zone=65520,nat)", | ||
"cookie=0x1020000000000, table=TrafficControl, priority=210,reg0=0x7/0xf actions=goto_table:Output", | ||
"cookie=0x1020000000000, table=Output, priority=213,reg0=0x7/0xf actions=output:NXM_NX_REG1[]", | ||
"cookie=0x1020000000000, table=Output, priority=212,ct_mark=0x80/0x80 actions=push_vlan:0x8100,move:NXM_NX_CT_LABEL[64..75]->OXM_OF_VLAN_VID[0..11],output:10", | ||
} | ||
|
||
m.EXPECT().AddAll(gomock.Any()).Return(nil).Times(1) | ||
cacheKey := "l7_np_flows" | ||
require.NoError(t, fc.InstallL7NetworkPolicyFlows()) | ||
fCacheI, ok := fc.featureNetworkPolicy.cachedFlows.Load(cacheKey) | ||
require.True(t, ok) | ||
assert.ElementsMatch(t, expectedFlows, getFlowStrings(fCacheI)) | ||
} |
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 we just replace the
l7Reconciler
parameter with afunc() error
parameter?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.
l7Reconciler
is used only for starting Suricata with methodStartSuricataOnce
. I removel7Reconciler
here and add a global variablestartSuricataOnceFn
which can be overridden for testing.