[management,client] Make DNS ForwarderPort Configurable & Change Well Known Port #4479
Conversation
…ns in computeForwarderPort
hakansa
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR makes the DNS forwarder port configurable in the management and client components, while changing the well-known port from 5454 to 22054. The change includes version-aware port assignment to ensure backward compatibility.
- Adds a configurable
ForwarderPortfield to the DNS configuration protocol - Implements version-based port computation that returns the new port (22054) only when all peers support version 0.58.0 or newer
- Updates the client to dynamically restart the DNS forwarder when the port changes
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| shared/management/proto/management.proto | Adds ForwarderPort field to DNSConfig message |
| shared/management/proto/management.pb.go | Generated protobuf code with ForwarderPort field and getter method |
| management/server/dns.go | Implements port computation logic based on peer versions |
| management/server/dns_test.go | Adds comprehensive tests for the port computation functionality |
| management/server/grpcserver.go | Updates toSyncResponse function to pass all peers for port computation |
| management/server/peer.go | Updates calls to toSyncResponse with additional peers parameter |
| management/server/peer_test.go | Updates test call to toSyncResponse with empty peers slice |
| client/internal/engine.go | Implements DNS forwarder restart logic when port changes |
| client/internal/dnsfwd/manager.go | Updates manager to accept configurable port parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client/internal/engine.go
Outdated
| func (e *Engine) restartDnsFwd(fwdEntries []*dnsfwd.ForwarderEntry, forwarderPort int) { | ||
| log.Infof("updating domain router service port from %d to %d", e.dnsFwdPort, forwarderPort) | ||
| // stop and start the forwarder to apply the new port | ||
| if err := e.dnsForwardMgr.Stop(context.Background()); err != nil { | ||
| log.Errorf("failed to stop DNS forward: %v", err) | ||
| } | ||
| e.dnsForwardMgr = dnsfwd.NewManager(e.firewall, e.statusRecorder, forwarderPort) | ||
| if err := e.dnsForwardMgr.Start(fwdEntries); err != nil { | ||
| log.Errorf("failed to start DNS forward: %v", err) | ||
| e.dnsForwardMgr = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The function has a duplicate log message (line 1881 and 1882). The log message on line 1881 should be removed since it's identical to the one in the calling function and adds no additional value.
Copilot uses AI. Check for mistakes.
…pdate related calls
…alls Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>
client/internal/dnsfwd/manager.go
Outdated
| if listenPort > 0 { | ||
| ListenPort = listenPort | ||
| } | ||
| m.dnsForwarder = NewDNSForwarder(fmt.Sprintf(":%d", listenPort), dnsTTL, m.firewall, m.statusRecorder) |
There was a problem hiding this comment.
in case the port is 0 we do skip setting it globally. Why do we restart the forwarder with port 0. Should be capital L no?
client/internal/dnsfwd/manager.go
Outdated
| const ( | ||
| var ( | ||
| // ListenPort is the port that the DNS forwarder listens on. It has been used by the client peers also | ||
| ListenPort = 5353 |
There was a problem hiding this comment.
This var is not sufficiently protected against races (e.g. ServeDNS reads this outside of the engine's lock). Why do we need a global var accessed by the engine in the first place?
nit: Make this a uint16, then you don't need to convert everywhere else
There was a problem hiding this comment.
We use this listen port value on DNS Interceptor and somewhere in netflow logger. I've guarded this var with a method and mutex, should be fine now.
|
5 participants
Describe your changes
[management,client] Make DNS ForwarderPort Configurable & Change Well Known Port
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__