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

Jump to bottom

bridge: T3137: Let VLAN aware bridge approach the behavior of professional equipment #677

Merged
merged 8 commits into from Jan 16, 2021
Merged

bridge: T3137: Let VLAN aware bridge approach the behavior of professional equipment #677

merged 8 commits into from Jan 16, 2021

Conversation

jack9603301
Copy link
Contributor

@jack9603301 jack9603301 commented Jan 11, 2021 •
edited

Change Summary

Let VLAN aware bridge approach the behavior of professional equipment

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)

https://phabricator.vyos.net/T3137

Component(s) name

bridge

Proposed changes

According to the consensus, the specific behavior of a VLAN aware bridge should conform
to the behavior of professional equipment. This commit makes a significant change to the
behavior of VLAN aware bridge, and has the following behaviors:

  1. Disable vif 1 configuration
  2. When the VLAN aware bridge is enabled, the parent interface is always VLAN 1
  3. When native-vlan is not configured, the default behavior of the device is native-vlan 1
  4. The VLAN ids forwarded by the bridge are determined by vif
  5. It has an enable-vlan node to enable VLAN awareness
  6. VLAN configuration is allowed only when VLAN aware bridge is activated

How to test

See smoke test for details

Checklist:

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

vyos/vyos-documentation#424

@jack9603301
Copy link
Contributor Author

Due to the migration command line, you need to configure the migration device, please handle it at the same time vyos/vyatta-cfg-system#134

@jack9603301 jack9603301 mentioned this pull request Jan 11, 2021
Closed
@jack9603301 jack9603301 marked this pull request as draft January 11, 2021 12:41
@jack9603301 jack9603301 marked this pull request as ready for review January 11, 2021 13:26
@jack9603301
Copy link
Contributor Author

Local test passed

c-po
c-po requested changes Jan 12, 2021
View reviewed changes
python/vyos/ifconfig/bridge.py Outdated Show resolved Hide resolved
python/vyos/ifconfig/bridge.py Outdated Show resolved Hide resolved
python/vyos/ifconfig/bridge.py Outdated Show resolved Hide resolved
python/vyos/ifconfig/interface.py Outdated Show resolved Hide resolved
src/conf_mode/interfaces-bridge.py Outdated Show resolved Hide resolved
@jack9603301 jack9603301 requested a review from c-po January 13, 2021 07:31
c-po
c-po requested changes Jan 13, 2021
View reviewed changes
python/vyos/ifconfig/interface.py Outdated Show resolved Hide resolved
smoketest/scripts/cli/test_interfaces_bridge.py Show resolved Hide resolved
@jack9603301 jack9603301 requested a review from c-po January 14, 2021 10:58
@c-po
Copy link
Member

c-po commented Jan 14, 2021

As this feature has never been released to any LTS or stable version, we can skip the migrator part.

@jack9603301 jack9603301 requested a review from c-po January 15, 2021 05:09
@jack9603301
bridge: T3137: Let VLAN aware bridge approach the behavior of profess…
38566b8 
…ional equipment

According to the consensus, the specific behavior of a VLAN aware bridge should conform
to the behavior of professional equipment. This commit makes a significant change to the
behavior of VLAN aware bridge, and has the following behaviors:

1. Disable `vif 1` configuration
2. When the VLAN aware bridge is enabled, the parent interface is always VLAN 1
3. When `native-vlan` is not configured, the default behavior of the device is `native-vlan 1`
4. The VLAN ids forwarded by the bridge are determined by `vif`
5. It has an `enable-vlan` node to enable VLAN awareness
6. VLAN configuration is allowed only when VLAN aware bridge is activated
@jack9603301
bridge: T3137: Better implementation of VLAN aware Bridge
fddf7da
@jack9603301
bridge: op: T3137: Operation mode output VLAN uses color output
6ad6908
@jack9603301
bridge: T3137: Delete blank lines
ac86489
@jack9603301
Revert "bridge: op: T3137: Operation mode output VLAN uses color output"
8623a8b 
This reverts commit 0f9f275.
@jack9603301
Copy link
Contributor Author

The PR has been updated, the Migrator has been deleted, the smoke test has been modified, and the smoke test has passed

c-po
c-po requested changes Jan 15, 2021
View reviewed changes
src/conf_mode/interfaces-bridge.py Outdated Show resolved Hide resolved
src/conf_mode/interfaces-bridge.py Outdated Show resolved Hide resolved
@c-po
Copy link
Member

c-po commented Jan 15, 2021 •
edited

Please also add a new custom validator for the allowed-vlan CLI node at https://github.com/vyos/vyos-1x/blob/current/interface-definitions/interfaces-bridge.xml.in#L116-L128

The custom validator (see https://github.com/vyos/vyos-1x/tree/current/src/validators as reference) should validate the passed string if it is within a given range. The range should be an argument to the validator itself. Maybe the numeric validator can be re-used with some small bash shim which splits the input value if a sting in the form of "10-30" is passed?

Besides this change the rest looks ok to me.

@c-po c-po merged commit 3315af9 into vyos:current Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c-po c-po c-po requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@jack9603301 @c-po