fix: be more smart when merging DNS resolver config

Fixes #8690 Consider the following scenario (e.g. OpenStack): platform issues a correct list of DNS servers, which includes both IPv4 and IPv6 resolvers, and configures DHCPv4 on the interface. DHCPv4 returns a set of IPv4 resolvers (as it can't return IPv6 ones), and this list completely overrides the list from the platform, wiping out the IPv6 resolvers completely. With this change, the merge process is more smart, as it tries to preserve IPv6 resolvers for example if the next layer provides no resolvers for IPv6. Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
siderolabs · Jul 16, 2024 · c288ace · c288ace
1 parent d983e44
commit c288ace
Show file tree

Hide file tree

Showing 8 changed files with 112 additions and 53 deletions.
diff --git a/go.mod b/go.mod
@@ -62,7 +62,7 @@ require (
 	github.com/containernetworking/plugins v1.5.1
 	github.com/coredns/coredns v1.11.2
 	github.com/coreos/go-iptables v0.7.0
-	github.com/cosi-project/runtime v0.5.2
+	github.com/cosi-project/runtime v0.5.3
 	github.com/distribution/reference v0.6.0
 	github.com/docker/docker v27.0.3+incompatible
 	github.com/docker/go-connections v0.5.0

diff --git a/go.sum b/go.sum
@@ -187,8 +187,8 @@ github.com/coreos/go-semver v0.3.1 h1:yi21YpKnrx1gt5R+la8n5WgS0kCrsPp33dmEyHReZr
 github.com/coreos/go-semver v0.3.1/go.mod h1:irMmmIw/7yzSRPWryHsK7EYSg09caPQL03VsM8rvUec=
 github.com/coreos/go-systemd/v22 v22.5.0 h1:RrqgGjYQKalulkV8NGVIfkXQf6YYmOyiJKk8iXXhfZs=
 github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc=
-github.com/cosi-project/runtime v0.5.2 h1:3UlNiXF1/jexLYu2grd8jdTR8/FrACOUdN1mxRnCKZs=
-github.com/cosi-project/runtime v0.5.2/go.mod h1:m+bkfUzKYeUyoqYAQBxdce3bfgncG8BsqcbfKRbvJKs=
+github.com/cosi-project/runtime v0.5.3 h1:smWlrxr37qP+myY3NeEg94Q1UbQt3ayTZsFp8q/RW1I=
+github.com/cosi-project/runtime v0.5.3/go.mod h1:m+bkfUzKYeUyoqYAQBxdce3bfgncG8BsqcbfKRbvJKs=
 github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
 github.com/cpuguy83/go-md2man/v2 v2.0.4 h1:wfIWP927BUkWJb2NmU/kNDYIBTh/ziUX91+lVfRxZq4=
 github.com/cpuguy83/go-md2man/v2 v2.0.4/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=

diff --git a/internal/app/machined/pkg/controllers/network/link_spec.go b/internal/app/machined/pkg/controllers/network/link_spec.go
@@ -8,7 +8,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"sort"
 
 	"github.com/cosi-project/runtime/pkg/controller"
 	"github.com/cosi-project/runtime/pkg/resource"
@@ -145,37 +144,23 @@ func (ctrl *LinkSpecController) Run(ctx context.Context, r controller.Runtime, l
 
 // SortBonds sort resources in increasing order, except it places slave interfaces right after the bond
 // in proper order.
-func SortBonds(items LinkSpecs) {
-	sort.Sort(&struct {
-		LinkSpecs
-		lessLinkSpecs
-	}{items, lessLinkSpecs{items}})
-}
-
-type lessLinkSpecs struct{ LinkSpecs }
-
-func (ls lessLinkSpecs) Less(i, j int) bool {
-	left := ls.Get(i).TypedSpec()
-	right := ls.Get(j).TypedSpec()
-
-	l := ordered.MakeTriple(left.Name, 0, "")
-	if left.BondSlave.MasterName != "" {
-		l = ordered.MakeTriple(left.BondSlave.MasterName, left.BondSlave.SlaveIndex, left.Name)
-	}
-
-	r := ordered.MakeTriple(right.Name, 0, "")
-	if right.BondSlave.MasterName != "" {
-		r = ordered.MakeTriple(right.BondSlave.MasterName, right.BondSlave.SlaveIndex, right.Name)
-	}
+func SortBonds(items *safe.List[*network.LinkSpec]) {
+	items.SortFunc(func(ll, rr *network.LinkSpec) int {
+		left := ll.TypedSpec()
+		right := rr.TypedSpec()
+
+		l := ordered.MakeTriple(left.Name, 0, "")
+		if left.BondSlave.MasterName != "" {
+			l = ordered.MakeTriple(left.BondSlave.MasterName, left.BondSlave.SlaveIndex, left.Name)
+		}
 
-	return l.LessThan(r)
-}
+		r := ordered.MakeTriple(right.Name, 0, "")
+		if right.BondSlave.MasterName != "" {
+			r = ordered.MakeTriple(right.BondSlave.MasterName, right.BondSlave.SlaveIndex, right.Name)
+		}
 
-// LinkSpecs is a sortable collection of network.LinkSpec.
-type LinkSpecs interface {
-	Len() int
-	Swap(i, j int)
-	Get(i int) *network.LinkSpec
+		return l.Compare(r)
+	})
 }
 
 func findLink(links []rtnetlink.LinkMessage, name string) *rtnetlink.LinkMessage {

diff --git a/internal/app/machined/pkg/controllers/network/resolver_merge.go b/internal/app/machined/pkg/controllers/network/resolver_merge.go
@@ -3,16 +3,18 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 // Package network provides controllers which manage network resources.
-//
-//nolint:dupl
 package network
 
 import (
+	"cmp"
 	"context"
 	"fmt"
+	"net/netip"
+	"slices"
 
 	"github.com/cosi-project/runtime/pkg/controller"
 	"github.com/cosi-project/runtime/pkg/resource"
+	"github.com/cosi-project/runtime/pkg/safe"
 	"github.com/cosi-project/runtime/pkg/state"
 	"go.uber.org/zap"
 
@@ -65,28 +67,29 @@ func (ctrl *ResolverMergeController) Run(ctx context.Context, r controller.Runti
 		}
 
 		// list source network configuration resources
-		list, err := r.List(ctx, resource.NewMetadata(network.ConfigNamespaceName, network.ResolverSpecType, "", resource.VersionUndefined))
+		list, err := safe.ReaderList[*network.ResolverSpec](ctx, r, resource.NewMetadata(network.ConfigNamespaceName, network.ResolverSpecType, "", resource.VersionUndefined))
 		if err != nil {
 			return fmt.Errorf("error listing source network addresses: %w", err)
 		}
 
+		// sort by config layer
+		list.SortFunc(func(l, r *network.ResolverSpec) int {
+			return cmp.Compare(l.TypedSpec().ConfigLayer, r.TypedSpec().ConfigLayer)
+		})
+
 		// simply merge by layers, overriding with the next configuration layer
 		var final network.ResolverSpecSpec
 
-		for _, res := range list.Items {
-			spec := res.(*network.ResolverSpec) //nolint:errcheck,forcetypeassert
-
-			if final.DNSServers != nil && spec.TypedSpec().ConfigLayer < final.ConfigLayer {
-				// skip this spec, as existing one is higher layer
-				continue
-			}
+		for iter := list.Iterator(); iter.Next(); {
+			spec := iter.Value().TypedSpec()
 
-			if spec.TypedSpec().ConfigLayer == final.ConfigLayer {
-				// merge server lists on the same level
-				final.DNSServers = append(final.DNSServers, spec.TypedSpec().DNSServers...)
+			if spec.ConfigLayer == final.ConfigLayer {
+				// simply append server lists on the same layer
+				final.DNSServers = append(final.DNSServers, spec.DNSServers...)
 			} else {
-				// otherwise, replace the lists
-				final = *spec.TypedSpec()
+				// otherwise, do a smart merge across IPv4/IPv6
+				final.ConfigLayer = spec.ConfigLayer
+				mergeDNSServers(&final.DNSServers, spec.DNSServers)
 			}
 		}
 
@@ -130,3 +133,39 @@ func (ctrl *ResolverMergeController) Run(ctx context.Context, r controller.Runti
 		r.ResetRestartBackoff()
 	}
 }
+
+func mergeDNSServers(dst *[]netip.Addr, src []netip.Addr) {
+	if *dst == nil {
+		*dst = src
+
+		return
+	}
+
+	srcHasV4 := len(filterIPFamily(src, true)) > 0
+	srcHasV6 := len(filterIPFamily(src, false)) > 0
+	dstHasV4 := len(filterIPFamily(*dst, true)) > 0
+	dstHasV6 := len(filterIPFamily(*dst, false)) > 0
+
+	// if old set has IPv4, and new one doesn't, preserve IPv4
+	// and same vice versa for IPv6
+	switch {
+	case dstHasV4 && !srcHasV4:
+		*dst = append(slices.Clone(src), filterIPFamily(*dst, true)...)
+	case dstHasV6 && !srcHasV6:
+		*dst = append(slices.Clone(src), filterIPFamily(*dst, false)...)
+	default:
+		*dst = src
+	}
+}
+
+func filterIPFamily(src []netip.Addr, isIPv4 bool) []netip.Addr {
+	var dst []netip.Addr
+
+	for _, addr := range src {
+		if addr.Is4() == isIPv4 {
+			dst = append(dst, addr)
+		}
+	}
+
+	return dst
+}
diff --git a/internal/app/machined/pkg/controllers/network/resolver_merge_test.go b/internal/app/machined/pkg/controllers/network/resolver_merge_test.go
@@ -7,6 +7,7 @@ package network_test
 
 import (
 	"context"
+	"fmt"
 	"net/netip"
 	"sync"
 	"testing"
@@ -118,6 +119,42 @@ func (suite *ResolverMergeSuite) TestMerge() {
 	)
 }
 
+func (suite *ResolverMergeSuite) TestMergeIPv46() {
+	def := network.NewResolverSpec(network.ConfigNamespaceName, "default/resolvers")
+	*def.TypedSpec() = network.ResolverSpecSpec{
+		DNSServers: []netip.Addr{
+			netip.MustParseAddr(constants.DefaultPrimaryResolver),
+			netip.MustParseAddr(constants.DefaultSecondaryResolver),
+		},
+		ConfigLayer: network.ConfigDefault,
+	}
+
+	platform := network.NewResolverSpec(network.ConfigNamespaceName, "platform/resolvers")
+	*platform.TypedSpec() = network.ResolverSpecSpec{
+		DNSServers:  []netip.Addr{netip.MustParseAddr("1.1.2.0"), netip.MustParseAddr("fe80::1")},
+		ConfigLayer: network.ConfigPlatform,
+	}
+
+	dhcp := network.NewResolverSpec(network.ConfigNamespaceName, "dhcp/eth1")
+	*dhcp.TypedSpec() = network.ResolverSpecSpec{
+		DNSServers:  []netip.Addr{netip.MustParseAddr("1.1.2.1")},
+		ConfigLayer: network.ConfigOperator,
+	}
+
+	for _, res := range []resource.Resource{def, platform, dhcp} {
+		suite.Require().NoError(suite.state.Create(suite.ctx, res), "%v", res.Spec())
+	}
+
+	suite.assertResolvers(
+		[]string{
+			"resolvers",
+		}, func(r *network.ResolverSpec, asrt *assert.Assertions) {
+			asrt.Equal(network.ConfigOperator, r.TypedSpec().ConfigLayer)
+			asrt.Equal(`["1.1.2.1" "fe80::1"]`, fmt.Sprintf("%q", r.TypedSpec().DNSServers))
+		},
+	)
+}
+
 func (suite *ResolverMergeSuite) TearDownTest() {
 	suite.T().Log("tear down")
 

diff --git a/internal/app/machined/pkg/controllers/network/timeserver_merge.go b/internal/app/machined/pkg/controllers/network/timeserver_merge.go
@@ -3,8 +3,6 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 // Package network provides controllers which manage network resources.
-//
-//nolint:dupl
 package network
 
 import (

diff --git a/pkg/machinery/go.mod b/pkg/machinery/go.mod
@@ -9,7 +9,7 @@ replace gopkg.in/yaml.v3 => github.com/unix4ever/yaml v0.0.0-20220527175918-f17b
 require (
 	github.com/blang/semver/v4 v4.0.0
 	github.com/containerd/go-cni v1.1.10
-	github.com/cosi-project/runtime v0.5.1
+	github.com/cosi-project/runtime v0.5.3
 	github.com/dustin/go-humanize v1.0.1
 	github.com/emicklei/dot v1.6.2
 	github.com/evanphx/json-patch v5.9.0+incompatible

diff --git a/pkg/machinery/go.sum b/pkg/machinery/go.sum
@@ -21,8 +21,8 @@ github.com/containerd/go-cni v1.1.10 h1:c2U73nld7spSWfiJwSh/8W9DK+/qQwYM2rngIhCy
 github.com/containerd/go-cni v1.1.10/go.mod h1:/Y/sL8yqYQn1ZG1om1OncJB1W4zN3YmjfP/ShCzG/OY=
 github.com/containernetworking/cni v1.2.2 h1:9IbP6KJQQxVKo4hhnm8r50YcVKrJbJu3Dqw+Rbt1vYk=
 github.com/containernetworking/cni v1.2.2/go.mod h1:DuLgF+aPd3DzcTQTtp/Nvl1Kim23oFKdm2okJzBQA5M=
-github.com/cosi-project/runtime v0.5.1 h1:07r4lk8sgiyhLdRuqZidB6qV3jFpKYhGccWdhTYHYcc=
-github.com/cosi-project/runtime v0.5.1/go.mod h1:m+bkfUzKYeUyoqYAQBxdce3bfgncG8BsqcbfKRbvJKs=
+github.com/cosi-project/runtime v0.5.3 h1:smWlrxr37qP+myY3NeEg94Q1UbQt3ayTZsFp8q/RW1I=
+github.com/cosi-project/runtime v0.5.3/go.mod h1:m+bkfUzKYeUyoqYAQBxdce3bfgncG8BsqcbfKRbvJKs=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM=