Skip to content

Conversation

@talmakion
Copy link
Contributor

Change summary

NOTE: Potentially hijacking an old ticket with a similar problem.

T5816 improved 1.4/1.5's strict validation to allow full regex on a single large-community tuple. However, it is common to match multiple components of a large-community string via regex in earlier VyOS versions (not to mention Cisco IOS). T5816 is too strict about the form of the regex pattern, does not allow the common-separator wildcard '_' and whitespace, breaking old 1.3 configs and preventing simple pattern logic being used to match multiple communities.

To resolve T5069's concerns and my own, this patch attempts to:

  • Perform basic, easy sanity checks that should always pass:
    • Contain at least one well-formed tuple
    • Permit only specific characters valid in community strings and POSIX regex
    • Rejecting obviously incorrect regular expressions, treating Python regex as a superset of POSIX
  • Run a final "sanity check" regex across the top, issue a warning if the overall pattern is not considered well formed

"Well formed" in this case would be one or more community segments roughly fitting the original check, separated by proper whitespace. "1:1:12:2:2" might be a mistake for "1:1:1 2:2:2", but there are still instances of useful patterns not following this form.

Since there's no mechanism for providing warning feedback from a validator and validator output is thrown away unless an error, I've wrapped this in the console_warning() function and it just prints directly to the TTY. If this seems like overkill, if it could be more generally useful, or if there's a suitable mechanism I missed - I imagine validation success/warning/failure feedback could be useful at API level, not just CLI - let me know and I'll update it to match.

If we retain the warning, I'll likely issue a followup patch for the documentation explaining the specific validation logic and what to check when the warning pops up.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

How to test / Smoketest result

# Example tests via CLI:
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1"

  
  
  
  Malformed large-community-list
  Value validation failed
  Set failed

[edit]
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1:1"
[edit]
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1:12:2:2"
WARNING: Regex does not follow expected form and may not match as expected.
[edit]
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1:1 2:2:2"
[edit]
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1:1_.*_2:2:2"
[edit]
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1:1_.*_2:2:2:"
WARNING: Regex does not follow expected form and may not match as expected.
[edit]
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1:1_.*(_2:2:2)"
[edit]
vyos@TEST-VYOS-LEFT# set policy large-community-list TEST rule 20 regex "1:1:1_.*(_2:2:2))"

  
  
  
  Malformed large-community-list
  Value validation failed
  Set failed

[edit]

I've checked that these compound matches are accepted by FRR and work in a production BGP environment as the older versions of FRR in 1.3 did.

Smoketests, test_policy.py:

Ran 28 tests in 375.194s

OK

test_protocols_bgp.py:

Ran 30 tests in 438.052s

OK

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@github-actions
Copy link

github-actions bot commented Apr 29, 2025

👍
No issues in PR Title / Commit Title

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

The fix looks sensible but please use the standard warning mechanism instead of a custom alternative.

@talmakion talmakion force-pushed the bugfix/T5069/permit-compound-regex branch from 098ae39 to d9455b2 Compare May 4, 2025 10:10
@talmakion talmakion requested a review from dmbaturin May 4, 2025 10:18
@sever-sever sever-sever requested a review from Copilot May 5, 2025 10:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses T5069 by relaxing the regex validator for large-community lists in BGP, allowing whitespace and the common-separator wildcard '_' while still enforcing basic sanity checks and warning on potentially malformed patterns.

  • Updated the regex pattern and allowed characters definition to accept underscores and whitespace.
  • Added a warning mechanism using Warning() to alert users when the provided regex deviates from the expected form.
  • Updated the copyright statement.

@talmakion talmakion closed this May 8, 2025
@talmakion talmakion force-pushed the bugfix/T5069/permit-compound-regex branch from d9455b2 to af2ddd5 Compare May 8, 2025 05:46
@talmakion talmakion reopened this May 8, 2025
@talmakion talmakion force-pushed the bugfix/T5069/permit-compound-regex branch from 86827d4 to d49ecca Compare May 8, 2025 15:09
@talmakion
Copy link
Contributor Author

talmakion commented May 8, 2025

Unsure why the smoketest failed - it is UserMessage/Warning-related, but I can't get it to fail in local. Comparing a vanilla VyOS with an updated one, manual smoketest runs and direct config sets work as expected.

I did muff the previous force-push so I've just tweaked minor Ruff lint fails and some naming that didn't track changes in vyos.base to trigger another smoketest run.

Edit: It was a vyos-configd interaction, need to use runtime values of sys.stdout.

@talmakion talmakion force-pushed the bugfix/T5069/permit-compound-regex branch from d49ecca to 427cd3f Compare May 9, 2025 02:18
…espace

* Re-introduce the whitespace/pattern matches ' ' and '_' as allowed
* Perform a general Python regex validity check (not 100% 1003.2, but in combination
  with allowedChars, pretty close)
* Introduce a warning against potentially malformed or over-complex patterns,
  but leave it up to the user to resolve - there are plenty of useful
  expressions we cannot validate easily
@talmakion talmakion force-pushed the bugfix/T5069/permit-compound-regex branch from 427cd3f to 7c9f908 Compare May 9, 2025 08:29
@talmakion
Copy link
Contributor Author

Updated version: split off the warning-logic into conf verify and dropped output changes for validators.

Re-ran smoketests for policy OK.

@github-actions
Copy link

github-actions bot commented May 9, 2025

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests (interfaces only) ❌ failed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests 👍 passed

Copy link
Member

@sever-sever sever-sever left a comment

Choose a reason for hiding this comment

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

Fixes BGP large-community regex
Smoketest passed. Approved.

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

The warning is now in the config scripts. No objections against the logic.

@dmbaturin dmbaturin added bp/sagitta Create automatic backport for sagitta LTS version bp/circinus Create automatic backport for circinus labels May 20, 2025
@dmbaturin dmbaturin merged commit 532a2ef into vyos:current May 20, 2025
14 of 16 checks passed
@github-actions github-actions bot added the mirror-initiated This PR initiated for mirror sync workflow label May 20, 2025
@vyosbot vyosbot added mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow labels May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bp/circinus Create automatic backport for circinus bp/sagitta Create automatic backport for sagitta LTS version current mirror-completed

Development

Successfully merging this pull request may close these issues.

4 participants