Skip to content

Apply DNS server update only on changes #4693

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Apply DNS server update only on changes #4693

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9272227
Apply DNS server update only on changes
hurricanehrndz Oct 23, 2025
ccce298
fix: reduce complexity
hurricanehrndz Oct 23, 2025
2bb74d4
Add tests for DNSConfig comparison
hurricanehrndz Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 77 additions & 2 deletions client/internal/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ type Engine struct {

dnsServer dns.Server

// lastDNSConfig stores the last applied DNS configuration
lastDNSConfig *nbdns.Config

// checks are the client-applied posture checks that need to be evaluated on the client
checks []*mgmProto.Checks

Expand Down Expand Up @@ -1060,8 +1063,13 @@ func (e *Engine) updateNetworkMap(networkMap *mgmProto.NetworkMap) error {
protoDNSConfig = &mgmProto.DNSConfig{}
}

if err := e.dnsServer.UpdateDNSServer(serial, toDNSConfig(protoDNSConfig, e.wgInterface.Address().Network)); err != nil {
log.Errorf("failed to update dns server, err: %v", err)
newDNSConfig := toDNSConfig(protoDNSConfig, e.wgInterface.Address().Network)
if !dnsConfigsEqual(e.lastDNSConfig, &newDNSConfig) {
if err := e.dnsServer.UpdateDNSServer(serial, newDNSConfig); err != nil {
log.Errorf("failed to update dns server, err: %v", err)
} else {
e.lastDNSConfig = &newDNSConfig
}
}

// apply routes first, route related actions might depend on routing being enabled
Expand Down Expand Up @@ -1255,6 +1263,73 @@ func toDNSConfig(protoDNSConfig *mgmProto.DNSConfig, network netip.Prefix) nbdns
return dnsUpdate
}

func dnsConfigsEqual(a, b *nbdns.Config) bool {
if a == nil && b == nil {
return true
}
if a == nil || b == nil {
return false
}
if a.ServiceEnable != b.ServiceEnable {
return false
}
if len(a.CustomZones) != len(b.CustomZones) {
return false
}
if len(a.NameServerGroups) != len(b.NameServerGroups) {
return false
}

for i, zoneA := range a.CustomZones {
zoneB := b.CustomZones[i]
if zoneA.Domain != zoneB.Domain {
return false
}
if len(zoneA.Records) != len(zoneB.Records) {
return false
}
for j, recordA := range zoneA.Records {
recordB := zoneB.Records[j]
if recordA.Name != recordB.Name ||
recordA.Type != recordB.Type ||
recordA.Class != recordB.Class ||
recordA.TTL != recordB.TTL ||
recordA.RData != recordB.RData {
return false
}
}
}

for i, nsGroupA := range a.NameServerGroups {
nsGroupB := b.NameServerGroups[i]
if nsGroupA.Primary != nsGroupB.Primary ||
nsGroupA.SearchDomainsEnabled != nsGroupB.SearchDomainsEnabled {
return false
}
if len(nsGroupA.Domains) != len(nsGroupB.Domains) {
return false
}
for j, domainA := range nsGroupA.Domains {
if domainA != nsGroupB.Domains[j] {
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Direct indexing without bounds checking. While line 1309 checks length equality, consider using an explicit range check for defensive programming to prevent potential index out of bounds panics.

Suggested change
if domainA != nsGroupB.Domains[j] {
if j >= len(nsGroupB.Domains) || domainA != nsGroupB.Domains[j] {

Copilot uses AI. Check for mistakes.
return false
}
}
if len(nsGroupA.NameServers) != len(nsGroupB.NameServers) {
return false
}
for j, nsA := range nsGroupA.NameServers {
nsB := nsGroupB.NameServers[j]
Comment on lines +1335 to +1336
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Direct indexing without bounds checking. Although line 1317 verifies length equality, adding explicit bounds verification would make the code more defensive against potential runtime panics.

Copilot uses AI. Check for mistakes.
if nsA.IP != nsB.IP ||
nsA.NSType != nsB.NSType ||
nsA.Port != nsB.Port {
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Direct indexing without bounds checking could cause a panic. Although line 1276 checks length equality, if CustomZones order differs between configs with same length, this still represents unequal configs but the function may not detect it correctly. Consider using a map-based comparison or iterating with explicit bounds checks.

Suggested change
for i, zoneA := range a.CustomZones {
zoneB := b.CustomZones[i]
if zoneA.Domain != zoneB.Domain {
return false
}
if len(zoneA.Records) != len(zoneB.Records) {
return false
}
for j, recordA := range zoneA.Records {
recordB := zoneB.Records[j]
if recordA.Name != recordB.Name ||
recordA.Type != recordB.Type ||
recordA.Class != recordB.Class ||
recordA.TTL != recordB.TTL ||
recordA.RData != recordB.RData {
return false
}
}
}
for i, nsGroupA := range a.NameServerGroups {
nsGroupB := b.NameServerGroups[i]
if nsGroupA.Primary != nsGroupB.Primary ||
nsGroupA.SearchDomainsEnabled != nsGroupB.SearchDomainsEnabled {
return false
}
if len(nsGroupA.Domains) != len(nsGroupB.Domains) {
return false
}
for j, domainA := range nsGroupA.Domains {
if domainA != nsGroupB.Domains[j] {
return false
}
}
if len(nsGroupA.NameServers) != len(nsGroupB.NameServers) {
return false
}
for j, nsA := range nsGroupA.NameServers {
nsB := nsGroupB.NameServers[j]
if nsA.IP != nsB.IP ||
nsA.NSType != nsB.NSType ||
nsA.Port != nsB.Port {
// Compare CustomZones by domain, order-independent
zonesA := make(map[string]nbdns.CustomZone, len(a.CustomZones))
for _, zone := range a.CustomZones {
zonesA[zone.Domain] = zone
}
zonesB := make(map[string]nbdns.CustomZone, len(b.CustomZones))
for _, zone := range b.CustomZones {
zonesB[zone.Domain] = zone
}
if len(zonesA) != len(zonesB) {
return false
}
for domain, zoneA := range zonesA {
zoneB, ok := zonesB[domain]
if !ok {
return false
}
if len(zoneA.Records) != len(zoneB.Records) {
return false
}
// Compare records order-independently
type recordKey struct {
Name string
Type string
Class string
TTL uint32
RData string
}
recordsA := make(map[recordKey]struct{}, len(zoneA.Records))
for _, record := range zoneA.Records {
k := recordKey{
Name: record.Name,
Type: record.Type,
Class: record.Class,
TTL: record.TTL,
RData: record.RData,
}
recordsA[k] = struct{}{}
}
recordsB := make(map[recordKey]struct{}, len(zoneB.Records))
for _, record := range zoneB.Records {
k := recordKey{
Name: record.Name,
Type: record.Type,
Class: record.Class,
TTL: record.TTL,
RData: record.RData,
}
recordsB[k] = struct{}{}
}
if len(recordsA) != len(recordsB) {
return false
}
for k := range recordsA {
if _, ok := recordsB[k]; !ok {
return false
}
}
}
// Compare NameServerGroups order-independently by Primary and Domains
type nsGroupKey struct {
Primary bool
SearchDomainsEnabled bool
Domains string // concatenated domains
}
nsGroupsA := make(map[nsGroupKey]nbdns.NameServerGroup, len(a.NameServerGroups))
for _, nsGroup := range a.NameServerGroups {
domains := make([]string, len(nsGroup.Domains))
copy(domains, nsGroup.Domains)
sort.Strings(domains)
key := nsGroupKey{
Primary: nsGroup.Primary,
SearchDomainsEnabled: nsGroup.SearchDomainsEnabled,
Domains: strings.Join(domains, ","),
}
nsGroupsA[key] = nsGroup
}
nsGroupsB := make(map[nsGroupKey]nbdns.NameServerGroup, len(b.NameServerGroups))
for _, nsGroup := range b.NameServerGroups {
domains := make([]string, len(nsGroup.Domains))
copy(domains, nsGroup.Domains)
sort.Strings(domains)
key := nsGroupKey{
Primary: nsGroup.Primary,
SearchDomainsEnabled: nsGroup.SearchDomainsEnabled,
Domains: strings.Join(domains, ","),
}
nsGroupsB[key] = nsGroup
}
if len(nsGroupsA) != len(nsGroupsB) {
return false
}
for key, nsGroupA := range nsGroupsA {
nsGroupB, ok := nsGroupsB[key]
if !ok {
return false
}
// Compare NameServers order-independently
if len(nsGroupA.NameServers) != len(nsGroupB.NameServers) {
return false
}
type nsKey struct {
IP string
NSType string
Port uint32
}
nsA := make(map[nsKey]struct{}, len(nsGroupA.NameServers))
for _, ns := range nsGroupA.NameServers {
k := nsKey{
IP: ns.IP,
NSType: ns.NSType,
Port: ns.Port,
}
nsA[k] = struct{}{}
}
nsB := make(map[nsKey]struct{}, len(nsGroupB.NameServers))
for _, ns := range nsGroupB.NameServers {
k := nsKey{
IP: ns.IP,
NSType: ns.NSType,
Port: ns.Port,
}
nsB[k] = struct{}{}
}
if len(nsA) != len(nsB) {
return false
}
for k := range nsA {
if _, ok := nsB[k]; !ok {

Copilot uses AI. Check for mistakes.
return false
}
}
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Direct indexing assumes order matters for equality. If the same NameServerGroups exist in different orders, they would be considered unequal. This may be intentional, but could lead to unnecessary DNS server updates if the order changes but content remains the same. Consider whether order-independent comparison is needed.

Suggested change
for i, nsGroupA := range a.NameServerGroups {
nsGroupB := b.NameServerGroups[i]
if nsGroupA.Primary != nsGroupB.Primary ||
nsGroupA.SearchDomainsEnabled != nsGroupB.SearchDomainsEnabled {
return false
}
if len(nsGroupA.Domains) != len(nsGroupB.Domains) {
return false
}
for j, domainA := range nsGroupA.Domains {
if domainA != nsGroupB.Domains[j] {
return false
}
}
if len(nsGroupA.NameServers) != len(nsGroupB.NameServers) {
return false
}
for j, nsA := range nsGroupA.NameServers {
nsB := nsGroupB.NameServers[j]
if nsA.IP != nsB.IP ||
nsA.NSType != nsB.NSType ||
nsA.Port != nsB.Port {
return false
}
}
// Compare NameServerGroups in an order-independent way
if !nameServerGroupsEqual(a.NameServerGroups, b.NameServerGroups) {
return false

Copilot uses AI. Check for mistakes.
}

return true
}

func (e *Engine) updateOfflinePeers(offlinePeers []*mgmProto.RemotePeerConfig) {
replacement := make([]peer.State, len(offlinePeers))
for i, offlinePeer := range offlinePeers {
Expand Down
Loading