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.

Sign up for GitHub

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

T5518: Add basic MLD support #2179

Merged
merged 1 commit into from
Sep 9, 2023
Merged

T5518: Add basic MLD support #2179

merged 1 commit into from
Sep 9, 2023

Conversation

vfreex
Copy link
Contributor

@vfreex vfreex commented Aug 27, 2023 •
edited
Loading

Change Summary

Currently VyOS has protocol igmp option to enable IGMP querier and reports through FRR's pimd.

I would like to add support for IPv6 as well since FRR's IPv6 multicast functionality has significantly improved.

Enabling both MLD and IGMP on a VyOS router will allow us to turn on multicast snooping on layer-3 switches in dual-stack networks.

Example commands:

// Enable on interface eth0
set protocols pim6 interface eth0

// Explicitly join multicast group ff18::1234 on interface eth1
set protocols pim6 interface eth1 mld join ff18::1234

// Explicitly join source-specific multicast group ff38::5678 with source address 2001:db8::1 on interface eth1
set protocols pim6 interface eth1 mld join ff38::5678 source 2001:db8::1

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)

Component(s) name

Proposed changes

How to test

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

@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team August 27, 2023 23:57
sever-sever
sever-sever requested changes Aug 28, 2023
View reviewed changes
interface-definitions/protocols-mld.xml.in Outdated Show resolved Hide resolved
src/conf_mode/protocols_mld.py Outdated Show resolved Hide resolved
src/conf_mode/protocols_mld.py Outdated Show resolved Hide resolved
@c-po
Copy link
Member

c-po commented Aug 29, 2023

Any reason to not make the daemon always run as we do with bgpd, ospfd, ospf6d?

@vfreex
Copy link
Contributor Author

vfreex commented Aug 29, 2023

Any reason to not make the daemon always run as we do with bgpd, ospfd, ospf6d?

This PR mostly follows what protocols igmp does. I don't know if there is a reason actually. If we believe it is better to make the daemon always run I can make a change.

c-po
c-po requested changes Aug 31, 2023
View reviewed changes
src/conf_mode/protocols_mld.py Outdated
airbag.enable()

# Required to use the full path to pim6d, in another case daemon will not be started
pim6d_cmd = f'/usr/lib/frr/pim6d -d -F traditional --daemon -A 127.0.0.1'
Copy link
Member

Choose a reason for hiding this comment

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

This is for IPv6, correct? Or should pim for IPv4 be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is for IPv6. Only pim6d is needed for MLD.

@c-po
Copy link
Member

c-po commented Aug 31, 2023 •
edited
Loading

Any reason to not make the daemon always run as we do with bgpd, ospfd, ospf6d?

Okay, this was me thinking it'sIPv4 PIM where we can only run pimd or igmp-proxy - but this is IPv6.

Can we make it set protocols pim6 instead? So I see no reason to not include support for this in future versions.

@c-po
Copy link
Member

c-po commented Sep 1, 2023

pim6d was enabled in the base image vyos/vyos-build@1e10c34

Please use frr-reload like we do in other protocols (ospf, ospfv3 etc)

@vfreex
Copy link
Contributor Author

vfreex commented Sep 1, 2023 •
edited
Loading

@c-po

Can we make it set protocols pim6 instead? So I see no reason to not include support for this in future versions.

Yes. This PR makes it in set protocols mld because we have set protocols igmp for IPv4 (I wanted to follow the same practice). Also this PR only adds MLD query and report support and doesn't include complete pim6 features in FRR.

pim6d was enabled in the base image vyos/vyos-build@1e10c34
Please use frr-reload like we do in other protocols (ospf, ospfv3 etc)

Thanks. I will make a change to use frr-reload.

@vfreex vfreex force-pushed the add-mld branch 3 times, most recently from 1654b28 to 60270f7 Compare September 2, 2023 22:32
@vfreex vfreex changed the title T5518: Add MLD protocol support T5518: Add basic MLD support Sep 2, 2023
@vfreex
Copy link
Contributor Author

vfreex commented Sep 2, 2023

@c-po I've updated this PR and added smoketests for it. However I don't have an environment to run smoketests. Let me know if there are other any issues.

sever-sever
sever-sever requested changes Sep 3, 2023
View reviewed changes
data/templates/frr/daemons.frr.tmpl Outdated Show resolved Hide resolved
@vfreex vfreex force-pushed the add-mld branch 2 times, most recently from d6f8965 to 8407ad9 Compare September 3, 2023 08:52
c-po
c-po requested changes Sep 8, 2023
View reviewed changes
interface-definitions/protocols-pimv6.xml.in Outdated Show resolved Hide resolved
interface-definitions/protocols-pimv6.xml.in Outdated Show resolved Hide resolved
interface-definitions/protocols-pimv6.xml.in Outdated Show resolved Hide resolved
interface-definitions/protocols-pimv6.xml.in Outdated Show resolved Hide resolved
src/conf_mode/protocols_pimv6.py Outdated Show resolved Hide resolved
@vfreex
T5518: Add basic MLD support
99ed6c9 
Currently VyOS has `protocol igmp` option to enable IGMP querier and reports through FRR's pimd.

I would like to add support for IPv6 as well since FRR's IPv6 multicast functionality has significantly improved.

Enabling both MLD and IGMP on a VyOS router will allow us to turn on multicast snooping on layer-3 switches in dual-stack networks.

Example commands:

```
// Enable on interface eth0
set protocols pim6 interface eth0

// Explicitly join multicast group ff18::1234 on interface eth1
set protocols pim6 interface eth1 mld join ff18::1234

// Explicitly join source-specific multicast group ff38::5678 with source address 2001:db8::1 on interface eth1
set protocols pim6 interface eth1 mld join ff38::5678 source 2001:db8::1
```
@c-po c-po merged commit 312370c into vyos:current Sep 9, 2023
6 of 7 checks passed
@vfreex vfreex mentioned this pull request Sep 10, 2023
Merged
@vfreex
Copy link
Contributor Author

vfreex commented Sep 10, 2023

vyos/vyos-documentation#1077 for documentation.
#2233 fixes the smoketest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sever-sever sever-sever sever-sever requested changes

@c-po c-po c-po approved these changes

@dmbaturin dmbaturin dmbaturin approved these changes

@sarthurdev sarthurdev Awaiting requested review from sarthurdev sarthurdev is a code owner automatically assigned from vyos/reviewers

@zdc zdc Awaiting requested review from zdc zdc is a code owner automatically assigned from vyos/reviewers

@jestabro jestabro Awaiting requested review from jestabro jestabro is a code owner automatically assigned from vyos/reviewers

Assignees

@vfreex vfreex

Labels
current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vfreex @c-po @dmbaturin @sever-sever