QoS: T7415: Fix tcp flags matching #4490
Conversation
Empty leaf nodes are cleaned, causing the tcp ack and syn flags to not match. These flags were moved to values of the tcp leafNode
|
👍 |
There was a problem hiding this comment.
I think we have two unrelated problems here that need two separate PRs to address.
The behavior of the QoS script is a bug that can be fixed by not deleting "empty" nodes that are actually significant. If the bug exist in 1.4, we will have to produce such a fix, maybe by adding some hardcoded special cases, because we cannot change the CLI in an LTS release.
Then there's the question of the CLI change. I agree with the general idea but there are caveats.
First, your change will break existing configs because currently that node is ip { tcp { ack } } and that will not load when ack is supposed to be a value of a leaf node. So any such change will need a migration script.
Second, your change makes it impossible to match SYN ACK packets, only either SYN or ACK. That's possible in the original CLI so we have to account for that. The simplest way to do that is to mark the node as <multi> but I wonder if there are use cases for matching SYN, ACK packets separately from either SYN or ACK ones.
The bug is present in 1.4.2 In this context, is "CLI" the ordering of nodes, tag nodes, and leaf nodes? Or just syntax? If it's the latter, the syntax would be identical to the user. But obviously, the underlying structure that is invisible to the user would be different.
I didn't realize that this would break existing configs, but I tested and it does. In my head, the config is stored as If your CLI concern is not syntax, but the structure of the node types, along with the issue of breaking existing configs, I agree with modifying the clean function instead. I was trying to avoid that initially since the simpler and cleaner fix seemed to be avoiding the cleaning. Here's my thoughts on the change. Is this along the lines of what you were thinking? From: if isinstance(conf, dict):
return {node: _clean_conf_dict(val) for node, val in conf.items() if val != {} and _clean_conf_dict(val) != {}}To: if isinstance(conf, dict):
preserve_empty_nodes = {'syn', 'ack'}
return {
node: _clean_conf_dict(val)
for node, val in conf.items()
if (val != {} and _clean_conf_dict(val) != {}) or node in preserve_empty_nodes
} |
There was a problem hiding this comment.
Have you tested if removing this _clean_conf_dict function works and also resolves this issue?
It was introduced as part of T4248, which seems to be an issue prior to QoS rewrite by @c-po. A quick glance at the new vyos.qos module seems to do sufficient checks on match criteria.
Tested without, issue persists from T4248.
I'm in agreement for easy backporting, that the exclusions in your comment are likely the best way forward.
Empty leaf nodes are cleaned, causing the tcp ack and syn flags to not match. These values are exempted from being cleaned.
|
CI integration ❌ failed! Details
|
sarthurdev
added
bp/sagitta
github-actions
bot
added
the
mirror-initiated
vyosbot
added
mirror-completed
and removed
mirror-initiated
Change summary
This fixes incorrect behavior in T7415. The qos.py conf-mode script cleans empty dictionaries. Since the
ackandsynentries were not values, but valueless leafnodes themselves, it caused the entries to not be applied.This moves the
synandackto values under thetcpleafnode.Types of changes
Related Task(s)
https://vyos.dev/T7415
Related PR(s)
How to test / Smoketest result
Configure QoS policy:
Verify counters with
show qos shaper:Verify
tc filter:Smoketest results:
Checklist: