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

T6681: Add option for SLAAC to support suppress Interval Advertisement in RA Packets #4022

Merged
merged 2 commits into from
Sep 2, 2024

Conversation

rgwan
Copy link
Contributor

@rgwan rgwan commented Aug 27, 2024

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)

https://vyos.dev/T6681

Related PR(s)

Component(s) name

SLAAC, RADVD, IPv6

Proposed changes

MikroTik's IPv6 SLAAC client has a bug, when it received a RA packet with Interval advertisement field, it can't obtain an address correctly from the router.

The log on MikroTik shows like that:

 22:01:50 radvd,debug received Router Advertisement on ether1 from <some mac>
 22:01:50 radvd,debug   prefix: <some prefix> valid: 1800 preferred: 900
 22:01:50 radvd,debug   unsupported option 7

So I'm submitted a patch which allows VyOS don't send Interval advertisement in RA packet.

e.g. the command line will be:

set service router-advert interface <some int> no-send-interval

How to test

Run the command I described above, and use Wireshark to capture packet from router, and the 'Interval Advertisement' field in RA packet is gone.

Smoketest result

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

Copy link

github-actions bot commented Aug 27, 2024

👍
No issues in PR Title / Commit Title

@rgwan rgwan changed the title T6681: Add option for SLAAC to support suppress Interval Advertisement on RA Packet T6681: Add option for SLAAC to support suppress Interval Advertisement in RA Packets Aug 27, 2024
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.

Thank you for the contribution. This is a behavioral change to VyOS, as in the past both options got set with a single command set service router-advert interface <some int> no-send-advert.

To preserve existing behavior a migration script is mandatory which will set the new CLI node once it detects that an interface has no-send-advert defined. This will preserve old behavior and add support for users to individually toggle the fields.

@rgwan
Copy link
Contributor Author

rgwan commented Aug 28, 2024

Thank you for the contribution. This is a behavioral change to VyOS, as in the past both options got set with a single command set service router-advert interface <some int> no-send-advert.

To preserve existing behavior a migration script is mandatory which will set the new CLI node once it detects that an interface has no-send-advert defined. This will preserve old behavior and add support for users to individually toggle the fields.

In fact, when no-send-advert is set, the RADVD daemon will not send any router-advertise to the interface, so AdvIntervalOpt option will be omitted by RADVD daemon in this situation.

I don't think we need a migration script for it, it didn't changes old behavior of no-send-advert.

@rgwan rgwan requested a review from c-po August 28, 2024 01:04
@rgwan
Copy link
Contributor Author

rgwan commented Aug 30, 2024

@c-po So, do you have any further question about this PR? I'm glad to hear from you.

@c-po
Copy link
Member

c-po commented Aug 31, 2024

In fact, when no-send-advert is set, the RADVD daemon will not send any router-advertise to the interface, so AdvIntervalOpt option will be omitted by RADVD daemon in this situation.

I don't think we need a migration script for it, it didn't changes old behavior of no-send-advert.

Confirmed:

https://github.com/radvd-project/radvd/blob/f2de4764559e102fe8492520be328858af0f964d/send.c#L880-L885

RADVD code bails out early if no-send-advert is set, thos omitting any other option and not adding a need for a migrator.

@c-po
Copy link
Member

c-po commented Sep 1, 2024

@Mergifyio backport sagitta circinus

Copy link
Contributor

mergify bot commented Sep 1, 2024

backport sagitta circinus

✅ Backports have been created

@c-po
Copy link
Member

c-po commented Sep 1, 2024

Added smoketests for the new option

[email protected]:~$ /usr/libexec/vyos/tests/smoke/cli/test_service_router-advert.py
test_advsendadvert_advintervalopt (__main__.TestServiceRADVD.test_advsendadvert_advintervalopt) ... ok
test_common (__main__.TestServiceRADVD.test_common) ... ok
test_deprecate_prefix (__main__.TestServiceRADVD.test_deprecate_prefix) ... ok
test_dns (__main__.TestServiceRADVD.test_dns) ... ok
test_nat64prefix (__main__.TestServiceRADVD.test_nat64prefix) ... ok
test_rasrcaddress (__main__.TestServiceRADVD.test_rasrcaddress) ... ok
test_route (__main__.TestServiceRADVD.test_route) ... ok

----------------------------------------------------------------------
Ran 7 tests in 19.518s

OK

@c-po c-po merged commit 497863b into vyos:current Sep 2, 2024
13 of 15 checks passed
c-po added a commit that referenced this pull request Sep 2, 2024
T6681: Add option for SLAAC to support suppress Interval Advertisement in RA Packets (backport #4022)
c-po added a commit that referenced this pull request Sep 3, 2024
T6681: Add option for SLAAC to support suppress Interval Advertisement in RA Packets (backport #4022)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants