-
Notifications
You must be signed in to change notification settings - Fork 659
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
Clarify policy-forwarding and routing-policy if no rules are present #1223
Open
dplore
wants to merge
8
commits into
master
Choose a base branch
from
dplore/pf-match-clarification
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5a097d2
Clarify that if no rules are present, no packets will be matched.
dplore abd0e40
change to match all packets if no rules are specified.
dplore 3c0ecfe
Update openconfig-policy-forwarding.yang
dplore 552ac01
Merge branch 'master' into dplore/pf-match-clarification
dplore df6ab42
clarify all packets matched in routing-policy if no condition specified.
dplore def87eb
fix version statment
dplore 96a3514
Merge branch 'master' into dplore/pf-match-clarification
dplore 441428e
add description if action is missing
dplore File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why is this the right thing to do vs. having an explicit match-all type criteria?
The downside of this API design is that the absence of something means something implicitly -- it seems a better API design to have the presence of something indicate something, which would mean that we should rather have an explicit "match everything" (can be done with matching all src/dst IP addrs)?
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.
There is no right or wrong here. This proposal is made based on precedent as referenced in #1223 (comment) earlier in this PR. The industry precedent is no-condition = matches any and all. This was also discussed in the OpenConfig operator meeting on Nov 26, 2024 with some thoughtful conversation.
The precedent is pretty strong where 5 out of 5 implement that no "condition" means match all. (Not included in the gist of 4 examples is arista EOS, which also defines a route map with no match statements as match all)
I do like the idea of being explicit in general. A number of changes in OC in the last few years have been due to ambiguity. But what is better? Do something different in OC or go with the precedent? In this case, I think the precedent is so strong that it's better to go with no conditions means match all. This change makes this decision explicit via description, but not in the data model/API.