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

Prioritize L7NP flows over TrafficControl #5768

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Prioritize L7NP flows over TrafficControl #5768

merged 1 commit into from
Dec 14, 2023

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Dec 4, 2023 •
edited

When applying an L7NP to a Pod, there's a potential issue where creating a TrafficControl CR with a redirect action to the same Pod could bypass the L7 engine. This is due to the fact that both the ct mark L7NPRedirectCTMark for identifying L7NP packets and the reg mark TrafficControlRedirectRegMark for identifying TrafficControl redirect packets can be set together. In OutputTable, the priorities of flows to match the ct mark and the reg mark are the same. Without an additional condition to distinguish between them, packets with both the reg mark and ct mark may be matched by either flow with an equal chance. To rectify this and ensure proper L7NP enforcement, it is crucial to assign a higher priority to the flow that matches the ct mark L7NPRedirectCTMark.

This patch also adds an extra parameter priority to InstallTrafficControlMarkFlows, which is
used to set the priority of the related flows.

antoninbas
antoninbas reviewed Dec 4, 2023
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.

That looks fine to me, but that may not be sufficient for @tushartathgur's L7Visibility implementation. I think ideally we want a way to ensure that priority(L7NP) > priority(user-defined TC) > priority(L7Visibility). Some extra work may be needed to ensure the second > relationship.

@hongliangl
Copy link
Contributor Author

You are right, some extra work should be done to ensure the second > relationship. I think it's better to include these part of work in @tushartathgur 's PR. I can work on this part based on his PR.

@hongliangl hongliangl force-pushed the 20231204-tc branch 2 times, most recently from b6f5eb9 to 25c4721 Compare December 5, 2023 07:33
@hongliangl
Copy link
Contributor Author

That looks fine to me, but that may not be sufficient for @tushartathgur's L7Visibility implementation. I think ideally we want a way to ensure that priority(L7NP) > priority(user-defined TC) > priority(L7Visibility). Some extra work may be needed to ensure the second > relationship.

@antoninbas Done as you said. I have added an extra parameter to InstallTrafficControlMarkFlows to decide the priority of the related flows. Please see the latest commit I have pushed. Thanks.

@hongliangl hongliangl force-pushed the 20231204-tc branch 2 times, most recently from 5986a79 to 13d9482 Compare December 5, 2023 08:49
antoninbas
antoninbas reviewed Dec 5, 2023
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.

fine by me again, but @tnqn and @tushartathgur should take a look

I'll defer to Quan for approval

pkg/agent/openflow/client.go Outdated
Comment on lines 42 to 46
const (
TrafficControlFlowPriorityHigh = "high"
TrafficControlFlowPriorityMedium = "medium"
TrafficControlFlowPriorityLow = "low"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

you could define a dedicated type for this:

type TrafficControlFlowPriority string

const (
	TrafficControlFlowPriorityHigh   TrafficControlFlowPriority = "high"
	TrafficControlFlowPriorityMedium TrafficControlFlowPriority = "medium"
	TrafficControlFlowPriorityLow    TrafficControlFlowPriority = "low"
)

func (p TrafficControlFlowPriority) toOFPriority() uint16 {
	switch p {
	case TrafficControlFlowPriorityHigh:
		return priorityHigh
	case TrafficControlFlowPriorityMedium:
		return priorityNormal
	case TrafficControlFlowPriorityLow:
		return priorityLow
	default:
		return 0
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Thanks

@hongliangl
Copy link
Contributor Author

@tnqn @tushartathgur Could you help verify this? Thanks a lot.

tnqn
tnqn reviewed Dec 7, 2023
View reviewed changes
pkg/agent/openflow/client.go Outdated
Comment on lines 43 to 45
TrafficControlFlowPriorityHigh types.TrafficControlFlowPriority = "high"
TrafficControlFlowPriorityMedium types.TrafficControlFlowPriority = "medium"
TrafficControlFlowPriorityLow types.TrafficControlFlowPriority = "low"
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't they defined in the same file as the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid cycled-import in test code, I only moved the definition to another file. It's okay to move these definitions to the where TrafficControlFlowPriority is defined.

@hongliangl hongliangl force-pushed the 20231204-tc branch 2 times, most recently from e7eab89 to 1586f4a Compare December 7, 2023 07:14
tnqn
tnqn reviewed Dec 8, 2023
View reviewed changes
pkg/agent/openflow/network_policy.go Outdated
@@ -2239,7 +2239,7 @@ func (f *featureNetworkPolicy) l7NPTrafficControlFlows() []binding.Flow {
Done(),
// This generates the flow to forward the returned packets (with FromTCReturnRegMark) to stageOutput directly
// after loading output port number to reg1 in L2ForwardingCalcTable.
TrafficControlTable.ofTable.BuildFlow(priorityHigh).
TrafficControlTable.ofTable.BuildFlow(priorityHigh+1).
Copy link
Member

Choose a reason for hiding this comment

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

What's this change for? The flow has the same match and condition as the one in trafficControlCommonFlows, changing priority shouldn't change anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a necessary change. As you mentioned, this is the same flow with the one in trafficControlCommonFlows. When both L7NP and TrafficControl are enabled, the same flows will be added and it is a little weird, so I changed the priority. I think it is also okay to revert the change since we can handle duplicated flows when syncing flows to OVS.

pkg/agent/types/trafficcontrol.go Outdated
Comment on lines 20 to 22
TrafficControlFlowPriorityHigh TrafficControlFlowPriority = "high"
TrafficControlFlowPriorityMedium TrafficControlFlowPriority = "medium"
TrafficControlFlowPriorityLow TrafficControlFlowPriority = "low"
Copy link
Member

Choose a reason for hiding this comment

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

Better to comment which kind of flows should which priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@hongliangl hongliangl requested a review from tnqn December 13, 2023 06:24
@hongliangl
Prioritize L7NP flows over TrafficControl
45c091a 
When applying an L7NP to a Pod, there's a potential issue where creating a TrafficControl
CR with a redirect action to the same Pod could bypass the L7 engine. This is due to the
fact that both the ct mark `L7NPRedirectCTMark` for identifying L7NP packets and the reg mark
`TrafficControlRedirectRegMark` for identifying TrafficControl redirect packets can be set together.
In `OutputTable`, the priorities of flows to match the ct mark and the reg mark are the same.
Without an additional condition to distinguish between them, packets with both the reg mark and
ct mark may be matched by either flow with an equal chance. To rectify this and ensure proper L7NP
enforcement, it is crucial to assign a higher priority to the flow that matches the ct mark
L7NPRedirectCTMark.

This patch also adds an extra parameter `priority` to InstallTrafficControlMarkFlows, which is
used to set the priority of the related flows.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
tnqn
tnqn approved these changes Dec 13, 2023
View reviewed changes
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

@tnqn
Copy link
Member

tnqn commented Dec 13, 2023

/test-all

@hongliangl
Copy link
Contributor Author

/test-e2e

1 similar comment
@tnqn
Copy link
Member

tnqn commented Dec 14, 2023

/test-e2e

@tnqn tnqn merged commit 1403a31 into antrea-io:main Dec 14, 2023
47 of 52 checks passed
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Dec 14, 2023
@hongliangl hongliangl deleted the 20231204-tc branch December 14, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoninbas antoninbas antoninbas left review comments

@tnqn tnqn tnqn approved these changes

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hongliangl @tnqn @antoninbas