Skip to content

Conversation

42wim
Copy link
Contributor

@42wim 42wim commented Apr 9, 2025

Description

Fixes #25627 by adding an extra alloc_advertise_ipv6 option similar to the AdvertiseIPv6Addr with the docker driver config. Seems the simplest way to fix this?

I'll finish the contributor checklist if I get a green light about this PR.

Links

See #25627

Contributor Checklist

  • Changelog Entry If this PR changes user-facing behavior, please generate and add a
    changelog entry using the make cl command.
  • Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
    ensure regressions will be caught.
  • Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
    and job configuration, please update the Nomad website documentation to reflect this. Refer to
    the website README for docs guidelines. Please also consider whether the
    change requires notes within the upgrade guide.

Reviewer Checklist

  • Backport Labels Please add the correct backport labels as described by the internal
    backporting document.
  • Commit Type Ensure the correct merge method is selected which should be "squash and merge"
    in the majority of situations. The main exceptions are long-lived feature branches or merges where
    history should be preserved.
  • Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
    within the public repository.

@42wim 42wim requested review from a team as code owners April 9, 2025 18:20
@tgross tgross moved this from Needs Triage to Triaging in Nomad - Community Issues Triage May 5, 2025
@tgross tgross moved this from Triaging to Needs Triage in Nomad - Community Issues Triage Jun 6, 2025
@tgross tgross self-requested a review June 9, 2025 20:10
@tgross tgross moved this from Needs Triage to In Progress in Nomad - Community Issues Triage Jun 9, 2025
@tgross tgross moved this from In Progress to Triaging in Nomad - Community Issues Triage Jun 9, 2025
@tgross tgross self-assigned this Jun 9, 2025
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @42wim! It looks like if we pull in the fix in #25997 we only ever need this for dual-stack network interfaces. For IPv6-only we'll automatically advertise the IPv6 address.

We were having a discussion about whether it might make sense to have better default behavior in the dual-stack case and default to IPv6 there. But it doesn't look like there's a good way to do that in a backwards-compatible way.

In any case, I'm 👍 on this general approach. You'll want to include a changelog entry (make cl), tests, and documentation updates as well.

@@ -134,7 +134,7 @@ func GetAddress(

return driverNet.IP, port, nil

case structs.AddressModeAlloc:
case structs.AddressModeAlloc, structs.AddressModeAllocAdvertiseIPv6:
Copy link
Member

Choose a reason for hiding this comment

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

The existing error messages below all refer to address_mode = "alloc" and we're adding a bunch of conditionals here for the IPv6 mode. I think these want to be different cases and maybe there's some helper functions we can factor out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a helper function getAddressPort

@42wim 42wim requested a review from a team as a code owner August 8, 2025 15:26
@42wim
Copy link
Contributor Author

42wim commented Aug 8, 2025

I've added the changelog, tests and documentation update.
Also added the helper function

@42wim 42wim changed the title Add AllocAdvertiseIPv6 option to allow IPv6 address being used for service registration Add AllocIPv6 option to allow IPv6 address being used for service registration Aug 8, 2025
@aimeeu aimeeu added the theme/docs Documentation issues and enhancements label Aug 8, 2025
Copy link
Contributor

@aimeeu aimeeu left a comment

Choose a reason for hiding this comment

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

Thanks for updating the docs. I left a tiny suggestion so that the alloc_ipv6 definitions match.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgross tgross added backport/ent/1.9.x+ent Changes are backported to 1.9.x+ent backport/1.10.x backport to 1.10.x release line labels Aug 8, 2025
@tgross tgross merged commit f712d5d into hashicorp:main Aug 8, 2025
54 of 57 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Nomad - Community Issues Triage Aug 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.9.x+ent Changes are backported to 1.9.x+ent backport/1.10.x backport to 1.10.x release line theme/docs Documentation issues and enhancements theme/ipv6 theme/service-discovery type/bug
Projects
Development

Successfully merging this pull request may close these issues.

PR #23959 breaks consul ipv6 registration address
3 participants