Skip to content

don't require a detailsVpnAddr in a HostUpdateNotification - #1472

Merged
JackDoan merged 3 commits into
masterfrom
cert-v2-hostupdate
Sep 29, 2025
Merged

don't require a detailsVpnAddr in a HostUpdateNotification#1472
JackDoan merged 3 commits into
masterfrom
cert-v2-hostupdate

Conversation

@JackDoan

@JackDoan JackDoan commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

If we don't use this information, don't put it on the wire, so we then do not have to check it. If this does indeed have a use, we can just remove the comment and close this

@nbrownus

Copy link
Copy Markdown
Collaborator

Places where we set Details.VpnAddr

  • handleHostQuery where we prep a lighthouse response to a non lighthouse host.
  • handleHostUpdateNotification this currently results in a NebulaMeta_HostUpdateNotificationAck back to the host that sent the update which is a noop.
  • innerQueryServer where we send NebulaMeta_HostQuery to lighthouses.
  • sendHostPunchNotification where we send NebulaMeta_HostPunchNotification to responder hosts after an initiator sent a NebulaMeta_HostQuery about them to a lighthouse. Line 1351 could be an interesting follow on to this PR.
  • SendUpdate where we send NebulaMeta_HostUpdateNotification to the lighthouses with our current known underlay addresses in it.
  • resetMeta which just zeroes the shared memory between lighthouse requests

Looks like all is well!

Side note, the unlockedSetV4 and unlockedSetV6 need to be updated to deal with the known vpnAddrs instead of just the current one being passed around to completely cover the allow lists cases.

Site side note, that cat is out of the bag and I only bring it up because of #1464, since it looks like the primary addr choice isn't entirely sufficient, we could change the proto definition for NebulaMetaDetails.VpnAddr to be a repeated field and have the lighthouse always inform the remote of all known vpn addrs for a host at that point in time. No doubt it will create other problems but it might also short circuit some of the issues around choosing a primary vpn addr.

@JackDoan

Copy link
Copy Markdown
Collaborator Author

Side note, the unlockedSetV4 and unlockedSetV6 need to be updated to deal with the known vpnAddrs instead of just the current one being passed around to completely cover the allow lists cases.

Site side note, that cat is out of the bag and I only bring it up because of #1464, since it looks like the primary addr choice isn't entirely sufficient, we could change the proto definition for NebulaMetaDetails.VpnAddr to be a repeated field and have the lighthouse always inform the remote of all known vpn addrs for a host at that point in time. No doubt it will create other problems but it might also short circuit some of the issues around choosing a primary vpn addr.

I kind of like this idea -- we could also send a whole hostQueryReply per IP on a query, but that sounds much worse I think?

@nbrownus

Copy link
Copy Markdown
Collaborator

Yeah sending individual replies per vpnAddr will confuse the situation more than anything I think, the client host has no way to correlate the answers into a single RemoteList.

Comment thread lighthouse.go Outdated
@JackDoan
JackDoan requested a review from wadey September 29, 2025 18:29

@wadey wadey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

approved, although anyone already using V2 certs will need to be careful to deploy this change to their lighthouses before updating their clients.

@JackDoan
JackDoan merged commit f1e992f into master Sep 29, 2025
9 checks passed
@JackDoan
JackDoan deleted the cert-v2-hostupdate branch September 29, 2025 18:43
@nbrownus nbrownus mentioned this pull request Nov 19, 2025
63 tasks
@wadey wadey added this to the v1.10.0 milestone Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants