[PR #2291] [CLOSED] Apply host DNS settings on peer state change #14929

Open
opened 2026-08-05 03:06:37 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/2291
Author: @hurricanehrndz
Created: 7/19/2024
Status: Closed

Base: mainHead: poc_DNS_on_peer_state_change


📝 Commits (10+)

  • ec290ba Trigger additional DNS probes on peer conn status
  • 65fbdb2 Fix lint issues
  • a632acc Disable upstream if ns private and 0 peers
  • 1157e13 Better comment for trigger DNS probe on connstatus
  • 81aed5d Set handler to disable when nsgroup disabled
  • 25e47ff Drop atomic bool when probing on peer change
  • 814369b Update when aPeerConnStatusChanged is closed
  • 8db48a1 Reduce unnecessary complexity
  • 571885c Refactor make more readable and thread safe
  • 5dc65bc Log if nsgroup is enabled

📊 Changes

10 files changed (+474 additions, -149 deletions)

View changed files

📝 client/internal/dns/host.go (+2 -1)
📝 client/internal/dns/server.go (+37 -17)
📝 client/internal/dns/server_test.go (+22 -1)
📝 client/internal/dns/upstream.go (+118 -101)
client/internal/dns/upstream_probe.go (+9 -0)
client/internal/dns/upstream_probe_darwin.go (+163 -0)
📝 client/internal/dns/upstream_test.go (+16 -12)
📝 client/internal/peer/status.go (+100 -17)
📝 client/internal/peer/status_test.go (+6 -0)
📝 client/server/server.go (+1 -0)

📄 Description

Describe your changes

This is a PoC and a potential fix #2002. This patch is not full fleshed out and their are some unattended side effects, but I wanted to get your thoughts and opinions @mlsmaycon and @pascal-fischer and @lixmal.

More Info:

This essential tries to ensure that host dns changes aren't applied til at least one routing peer is connected and DNS servers are reachable. It does so by tying deactivation and reactivation of upstream servers to Peerlist state changes in the status recorder.

As it stands, now status -d doesn't accurately reflect the status on first connection. That is because I haven't figure out an effective method to deactivate the servers without more intense changes. Primarily because of how the deactivate and reactivate callbacks are structure and their dependence of the removeIndex. One potential workaround is to modify deactivate to skip applying host config, so that we can fix this side effect.

Thoughts and discussion please

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/2291 **Author:** [@hurricanehrndz](https://github.com/hurricanehrndz) **Created:** 7/19/2024 **Status:** ❌ Closed **Base:** `main` ← **Head:** `poc_DNS_on_peer_state_change` --- ### 📝 Commits (10+) - [`ec290ba`](https://github.com/netbirdio/netbird/commit/ec290ba2dc9a13e5ee3e8aff8150da0116e4beed) Trigger additional DNS probes on peer conn status - [`65fbdb2`](https://github.com/netbirdio/netbird/commit/65fbdb2dd484cd0e99de121fd278d88a00de2b1b) Fix lint issues - [`a632acc`](https://github.com/netbirdio/netbird/commit/a632accb6268be45e3d67d15fe60c4401c9af856) Disable upstream if ns private and 0 peers - [`1157e13`](https://github.com/netbirdio/netbird/commit/1157e131d8aa1214cdc4c97476b74d6c1c0e7cff) Better comment for trigger DNS probe on connstatus - [`81aed5d`](https://github.com/netbirdio/netbird/commit/81aed5de80f015a34667e1abd687f14171d6052c) Set handler to disable when nsgroup disabled - [`25e47ff`](https://github.com/netbirdio/netbird/commit/25e47ff0d67f01f6c0096563a0cd8f0336fb965a) Drop atomic bool when probing on peer change - [`814369b`](https://github.com/netbirdio/netbird/commit/814369b2f8b952ca896be71a59c7afa855af4ec3) Update when aPeerConnStatusChanged is closed - [`8db48a1`](https://github.com/netbirdio/netbird/commit/8db48a1388e0a1f46273ae05cb9991470e055b63) Reduce unnecessary complexity - [`571885c`](https://github.com/netbirdio/netbird/commit/571885ca836caad5702d05006c2fb6a1456f663c) Refactor make more readable and thread safe - [`5dc65bc`](https://github.com/netbirdio/netbird/commit/5dc65bc471588ccb750d0a5d619b60960db30b82) Log if nsgroup is enabled ### 📊 Changes **10 files changed** (+474 additions, -149 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/dns/host.go` (+2 -1) 📝 `client/internal/dns/server.go` (+37 -17) 📝 `client/internal/dns/server_test.go` (+22 -1) 📝 `client/internal/dns/upstream.go` (+118 -101) ➕ `client/internal/dns/upstream_probe.go` (+9 -0) ➕ `client/internal/dns/upstream_probe_darwin.go` (+163 -0) 📝 `client/internal/dns/upstream_test.go` (+16 -12) 📝 `client/internal/peer/status.go` (+100 -17) 📝 `client/internal/peer/status_test.go` (+6 -0) 📝 `client/server/server.go` (+1 -0) </details> ### 📄 Description ## Describe your changes This is a PoC and a potential fix #2002. This patch is not full fleshed out and their are some unattended side effects, but I wanted to get your thoughts and opinions @mlsmaycon and @pascal-fischer and @lixmal. More Info: This essential tries to ensure that host dns changes aren't applied til at least one routing peer is connected and DNS servers are reachable. It does so by tying deactivation and reactivation of upstream servers to Peerlist state changes in the status recorder. As it stands, now `status -d` doesn't accurately reflect the status on first connection. That is because I haven't figure out an effective method to deactivate the servers without more intense changes. Primarily because of how the deactivate and reactivate callbacks are structure and their dependence of the removeIndex. One potential workaround is to modify deactivate to skip applying host config, so that we can fix this side effect. Thoughts and discussion please ## Issue ticket number and link ### Checklist - [ ] Is it a bug fix - [ ] Is a typo/documentation fix - [x] Is a feature enhancement - [x] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] Extended the README / documentation, if necessary --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 03:06:37 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#14929