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.

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 incorrect FlowMod message passing in openflow client modifyFlows #5125

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Jun 13, 2023

In the openflow client, the modifyFlows function incorrectly flipped the add and mod type of FlowMod messages that it passes to the lower level BundleOps operation. This could lead to unexpected openflow state for the caller.

@Dyanngg Dyanngg added kind/bug Categorizes issue or PR as related to a bug. action/backport Indicates a PR that requires backports. labels Jun 13, 2023
@@ -450,10 +450,10 @@ func (c *client) modifyFlows(cache *flowCategoryCache, flowCacheKey string, flow
var msg *openflow15.FlowMod
if _, ok := oldFlowCache[matchString]; ok {
msg = getFlowModMessage(flow, binding.ModifyMessage)
adds = append(adds, msg)
mods = append(mods, msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

pkg/agent/openflow/client_test.go Show resolved Hide resolved
@antoninbas
Copy link
Contributor

Looks like it was introduced by #4495

@Dyanngg I can't tell if this is a serious bug, do we need to backport to v1.12?

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

Dyanngg commented Jun 14, 2023

Looks like it was introduced by #4495

Yes it seems to be the case.

@Dyanngg I can't tell if this is a serious bug, do we need to backport to v1.12?

In my own dev testing (for a feature that's unrelated to released ones), I found out that because of this bug, OVS will fail to add specific flows explicitly passed to the client in update cases, and throw no openflow errors. I added the backport tag because it seems the function is used in installPodFlows and installNodeFlows in addition to multiple MC flow syncs, and could easily lead to openflow rules in an unexpected state which will be difficult to root cause IMO.

@Dyanngg Dyanngg requested a review from hongliangl June 14, 2023 17:36
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for catching and fixing the issue.

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jun 16, 2023

/test-all

Copy link
Contributor

@luolanzone luolanzone 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 tnqn added the action/release-note Indicates a PR that should be included in release notes. label Jun 16, 2023
@tnqn tnqn merged commit e3b9629 into antrea-io:main Jun 16, 2023
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants