Skip to content

Conversation

@natali-rs1985
Copy link
Contributor

In some cases, only a numeric port or port range must be specified; service names (for example, "ssh") are not allowed

Change summary

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

vyos@vyos# set vpp acl ip tag-name test rule 10 source  port ssh

  Error: ssh is not a valid port or port range



  Invalid value
  Value validation failed
  Set failed

[edit]

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 Nov 21, 2025

👍
No issues in PR Title / Commit Title

@natali-rs1985 natali-rs1985 added the bp/circinus Create automatic backport for circinus label Nov 21, 2025
@sever-sever sever-sever requested a review from Copilot November 21, 2025 14:29
Copilot finished reviewing on behalf of sever-sever November 21, 2025 14:31
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 adds a --only-numeric argument to the port-range validator to restrict input to numeric ports and port ranges only, rejecting service names like "ssh". This is required for VPP ACL configurations where service names are not supported.

Key Changes:

  • Added argparse support to the port-range validator with a new --only-numeric flag
  • Modified validation logic to reject non-numeric inputs when the flag is set
  • Updated VPP ACL port range XML configuration to use the new validator argument

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/validators/port-range Added argparse with --only-numeric flag and conditional logic to reject service names when flag is enabled
interface-definitions/include/vpp/acl_port_range.xml.i Updated validator call to include --only-numeric argument for VPP ACL port ranges

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

In some cases, only a numeric port or port range must be specified; service names (for example, "ssh") are not allowed
@natali-rs1985 natali-rs1985 changed the title T8016: Add --only-numeric argument to port-range validator T8016: Add --numeric-only argument to port-range validator Nov 21, 2025
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.

Extended port-range validator for numeric-only ports
As VPP wants to see integers and not ssh, for example

@github-actions
Copy link

CI integration 👍 passed!

Details

CI logs

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

Copy link
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

In my opinion we should not support port name literals at all and remove code which allows for it. As VPP is new we should drop this.

@natali-rs1985
Copy link
Contributor Author

In my opinion we should not support port name literals at all and remove code which allows for it. As VPP is new we should drop this.

VPP is the one that does not support literals. That's why this change is needed. Do not allow port name to avoid errors in VPP. But I think some other services use port names.

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 current

Development

Successfully merging this pull request may close these issues.

3 participants