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

frr ipv6 disabled fixes #463

Merged
merged 6 commits into from
Sep 22, 2022

Conversation

jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Sep 18, 2022

  • Disable ipv6 ND when ipv6 is not configured (frr bug prevents this from getting applied)
  • Don't enable ipv6 forwarding when ipv6 is disabled
  • Explicitly disable ipv6 on interfaces without ipv6 enabled, under Linux

* Only create BGP unnumbered interface peers when ipv6 is enabled
* Explicitly disable ipv6 in Linux when not configured (to correctly remove ipv6 lla and avoid sending RAs)
Copy link
Owner

@ipspace ipspace left a comment

Choose a reason for hiding this comment

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

Good catch on the BGP conditional logic. However...

  • FRR IPv6 forwarding is (IIRC) enabled by default, so...
  • Changing FRR IPv6 forwarding does nothing to the "disable IPv6" system flag
  • Your implementation will disable unnumbered BGP sessions in IPv4-only lab unless we implement Set 'ipv6: True' on interfaces used for unnumbered IPv6 BGP sessions #460 by setting 'ipv6: True'
  • You copied my incomplete implementation (yeah, I know). The same stuff has to be done on VLAN interfaces and subinterfaces (that's why I hate it so much).

@jbemmel
Copy link
Collaborator Author

jbemmel commented Sep 19, 2022

I hit this during #458, when I used unnumbered: True instead of ipv4/ipv6: True
It was working with frr, but not with srlinux

For consistency, I figured it would be better to require the user to set ipv6: True for this use case, given that it requires ipv6 enabled on the link

@ipspace
Copy link
Owner

ipspace commented Sep 19, 2022

It was working with frr, but not with srlinux

As everyone is marketing that as EBGP sessions over unnumbered interfaces, and they forget to mention you need IPv6 LLA for that, I'd call that a SR Linux problem, not a FRR problem 😜 If SR Linux wants to claim feature parity with CL, then IPv6 LLA should be enabled the moment you configure EBGP session over an unnumbered IPv4 interface. #JustSaying

For consistency, I figured it would be better to require the user to set ipv6: True for this use case, given that it requires ipv6 enabled on the link

Regardless of what you think is right (and I might be agreeing with you from purely academic standpoint), everyone talks about EBGP sessions across unnumbered IPv4 interfaces, and now you come along saying "but that won't work unless you explicitly enable IPv6 in the lab topology file" while the vendor supporting this feature (Cumulus) gets this working by default (unless someone forgot to enable IPv6 in Dockerfile, which is what triggered the CL 4.4.0 SNAFU). That might not go down well with most of the audience.

@jbemmel
Copy link
Collaborator Author

jbemmel commented Sep 19, 2022

I'm only trying to be explicit and technically accurate/intuitive. If you think that the audience prefers/expects ipv4: True (and by association unnumbered: True) to enable ipv6 lla under the hood, then we can make it work like that too

@ipspace
Copy link
Owner

ipspace commented Sep 20, 2022

I'm only trying to be explicit and technically accurate/intuitive. If you think that the audience prefers/expects ipv4: True (and by association unnumbered: True) to enable ipv6 lla under the hood, then we can make it work like that too

The audience expects 'ipv4: True' to work with unnumbered EBGP sessions just like the vendor promised ;) It turns out that the vendor promises depend on the unmentioned fact that their distro of Linux happens to have IPv6 enabled on all interfaces 🤷‍♂️

Copy link
Owner

@ipspace ipspace left a comment

Choose a reason for hiding this comment

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

'ipv6: false' is removed from the interface somewhere in link transformation code, so there's no need to check for that. 'l.ipv6 is not defined' is good enough.

@ipspace ipspace merged commit c72605e into ipspace:dev Sep 22, 2022
@jbemmel jbemmel deleted the explicitly-disable-ipv6-when-absent branch September 22, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants