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

[24.11] Fix IPv6 autoconfiguration #1245

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

sysvinit
Copy link
Member

@sysvinit sysvinit commented Jan 22, 2025

Due to refactoring and merges, the network code in the platform did not correctly ensure that IPv6 autoconfiguration is disabled on all interfaces by default. This change fixes this regression, and introduces some tests to ensure that we catch unintentional changes to this behaviour again in the future.

Additionally, I've adjusted the network configuration code to allow the VM or hardware configuration profile to be selectable using a NixOS option. This allows the hardware profile to be used in Hydra test VMs so we can test EVPN-related network functions without having to rewrite all the config manually by hand, and I've also included a test fixture which will create a VM acting as a mock datacentre VXLAN switch.

PL-133360

@flyingcircusio/release-managers

Release process

  • Created changelog entry using ./changelog.sh

PR release workflow (internal)

  • PR has internal ticket
  • internal issue ID (PL-…) part of branch name
  • internal issue ID mentioned in PR description text
  • ticket is on Platform agile board
  • ticket state set to Pull request ready
  • if ticket is more urgent than within the next few days, directly contact a member of the Platform team

Design notes

  • Provide a feature toggle if the change might need to be adjusted/reverted quickly depending on context. Consider whether the default should be on or off. Example: rate limiting.
  • All customer-facing features and (NixOS) options need to be discoverable from documentation. Add or update relevant documentation such that hosted and guided customers can understand it as well.

Security implications

  • Security requirements defined? (WHERE)
    • IPv6 autoconfiguration should be disabled by default: this ensures that hosts will use only IP addresses explicitly assigned to them in the directory, and prevents traffic being stolen by rogue hosts sending router advertisements.
  • Security requirements tested? (EVIDENCE)
    • New Hydra tests to check that the desired configuration is really applied
    • Manually verified on both hardware and in VMs.

We need to be able to check that network configuration works correctly
in tests.

PL-133360
The NixOS test driver assigns each test VM an ID number which is used
for generating MAC addresses. As this number might diverge from the ID
we provide in our test framework for e.g. assigning IP addresses, we
should use the driver-provided ID for MAC addresses so that interfaces
are detected correctly.

PL-133360
Additionally, only reset the link-local address generation mode on
hardware links which may have had dhcpcd using them during
bootstrapping.

PL-133360
@sysvinit sysvinit requested a review from ctheune January 22, 2025 09:05
Copy link
Member

@ctheune ctheune left a comment

Choose a reason for hiding this comment

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

Looks good to me - I'm trusting the fix (I'm a bit hazy of what exactly was wrong here) as you mentioned that this has happened before, fixed up the learnings from the various incarnations and wrote tests.


with subtest("testing ipv6 autoconf configuration on ethsrv"):
for sysctl in sysctls:
machine.succeed(f"sysctl net.ipv6.conf.ethsrv.{sysctl} | grep 0")
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit frustrated by the switch from Python to Bash ... ;) Specifically the grep feels like a shotgun.

If we need to stay within bash then this would be a bit more exact: [ "$(sysctl net.ipv6.conf.ethsrv.accept_ra -n -b)" == "1" ] ... BUT ... it's terrible to read IMHO ...

I'll let you decide.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, the sysctls which handle temporary SLAAC-generated addresses take times in seconds, so this would match if there's a 0 digit in the number of seconds. I think a better solution might be to write a script which asserts that the sysctl is 0, and then invoke that in the test.

@sysvinit sysvinit force-pushed the PL-133360-fix-ipv6-autoconfig-again branch from 270cf82 to a552dc7 Compare January 23, 2025 08:49
@sysvinit
Copy link
Member Author

Looks good to me - I'm trusting the fix (I'm a bit hazy of what exactly was wrong here)

In the implementation of the network code we have a distinction between interfaces which are backed by some hardware network device, and those which are backed by a synthetic VXLAN kernel network device. The existing code only generates the systemd units which disable IPv6 autoconfig for the VXLAN-backed "virtual" links. This means that the physical links are left with the default behaviour (which is to enable IPv6 autoconfig, accept router advertisements, etc).

in
(map (link:
lib.nameValuePair (unitName link) (virtualLinkUnit link))
virtualLinks)
) ++

This is missing the creation of the systemd units also for ethernetLinks. This used to be there back when we were developing VXLAN support, however it seems that it got erroneously removed at some point during a refactoring.

@sysvinit sysvinit requested a review from ctheune January 23, 2025 09:04
@sysvinit sysvinit merged commit f944419 into fc-24.11-dev Jan 23, 2025
2 checks passed
@sysvinit sysvinit deleted the PL-133360-fix-ipv6-autoconfig-again branch January 23, 2025 09:35
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