From d2193cdfbb4a155231a06306b8e0795bd474a52e Mon Sep 17 00:00:00 2001 From: Dmitriy Matrenichev Date: Wed, 11 Sep 2024 22:50:49 +0300 Subject: [PATCH] chore: account for resource sorting in dns upstream resource `List` returns a sorted (by id) list of resources. This doesn't work when the order of dns upstreams is important. Because of that add an `Idx` field to the "DNSUpstreams.net.talos.dev" resource, so we can preserve order. Fixes #9274 Signed-off-by: Dmitriy Matrenichev --- .../controllers/network/dns_resolve_cache.go | 21 +++++--- .../network/dns_resolve_cache_test.go | 51 +++++++++++++++++++ .../pkg/controllers/network/dns_upstream.go | 13 ++++- .../pkg/controllers/network/resolver_merge.go | 8 ++- .../resources/network/dns_upstream.go | 6 +++ 5 files changed, 84 insertions(+), 15 deletions(-) diff --git a/internal/app/machined/pkg/controllers/network/dns_resolve_cache.go b/internal/app/machined/pkg/controllers/network/dns_resolve_cache.go index d93ccaaf483..4925944e07c 100644 --- a/internal/app/machined/pkg/controllers/network/dns_resolve_cache.go +++ b/internal/app/machined/pkg/controllers/network/dns_resolve_cache.go @@ -5,6 +5,7 @@ package network import ( + "cmp" "context" "errors" "fmt" @@ -160,14 +161,7 @@ func (ctrl *DNSResolveCacheController) Run(ctx context.Context, r controller.Run return fmt.Errorf("error getting resolver status: %w", err) } - addrs, prxs := make([]string, 0, upstreams.Len()), make([]*proxy.Proxy, 0, upstreams.Len()) - - for it := upstreams.Iterator(); it.Next(); { - prx := it.Value().TypedSpec().Value.Prx - - addrs = append(addrs, prx.Addr()) - prxs = append(prxs, prx.(*proxy.Proxy)) //nolint:forcetypeassert - } + prxs, addrs := SortedProxies(upstreams) if ctrl.handler.SetProxy(prxs) { ctrl.Logger.Info("updated dns server nameservers", zap.Strings("addrs", addrs)) @@ -179,6 +173,17 @@ func (ctrl *DNSResolveCacheController) Run(ctx context.Context, r controller.Run } } +// SortedProxies returns sorted list of proxies and their addresses. +func SortedProxies(upstreams safe.List[*network.DNSUpstream]) ([]*proxy.Proxy, []string) { + upstreams.SortFunc(func(a, b *network.DNSUpstream) int { + return cmp.Compare(a.TypedSpec().Value.Idx, b.TypedSpec().Value.Idx) + }) + + //nolint:forcetypeassert + return safe.ToSlice(upstreams, func(d *network.DNSUpstream) *proxy.Proxy { return d.TypedSpec().Value.Prx.(*proxy.Proxy) }), + safe.ToSlice(upstreams, func(d *network.DNSUpstream) string { return d.TypedSpec().Value.Prx.Addr() }) +} + func (ctrl *DNSResolveCacheController) writeDNSStatus(ctx context.Context, r controller.Runtime, config runnerConfig) error { return safe.WriterModify(ctx, r, network.NewDNSResolveCache(fmt.Sprintf("%s-%s", config.net, config.addr)), func(drc *network.DNSResolveCache) error { drc.TypedSpec().Status = "running" diff --git a/internal/app/machined/pkg/controllers/network/dns_resolve_cache_test.go b/internal/app/machined/pkg/controllers/network/dns_resolve_cache_test.go index 0c4a731dc6b..fac7e79630b 100644 --- a/internal/app/machined/pkg/controllers/network/dns_resolve_cache_test.go +++ b/internal/app/machined/pkg/controllers/network/dns_resolve_cache_test.go @@ -14,6 +14,7 @@ import ( "github.com/cosi-project/runtime/pkg/resource" "github.com/cosi-project/runtime/pkg/resource/rtestutils" + "github.com/cosi-project/runtime/pkg/safe" "github.com/miekg/dns" "github.com/siderolabs/gen/xslices" "github.com/siderolabs/gen/xtesting/must" @@ -284,3 +285,53 @@ func makeAddrs(port string) []netip.AddrPort { netip.MustParseAddrPort("127.0.0.53:" + port), } } + +type DNSUpstreams struct { + ctest.DefaultSuite +} + +func (suite *DNSUpstreams) TestOrder() { + port := must.Value(getDynamicPort())(suite.T()) + + cfg := network.NewHostDNSConfig(network.HostDNSConfigID) + cfg.TypedSpec().Enabled = true + cfg.TypedSpec().ListenAddresses = makeAddrs(port) + + suite.Require().NoError(suite.State().Create(suite.Ctx(), cfg)) + + resolverSpec := network.NewResolverStatus(network.NamespaceName, network.ResolverID) + + for i, dnsSlice := range [][]string{ + {"1.0.0.1", "8.8.8.8", "1.1.1.1"}, + {"1.1.1.1", "8.8.8.8", "1.0.0.1", "8.0.0.8"}, + {"192.168.0.1"}, + } { + resolverSpec.TypedSpec().DNSServers = xslices.Map(dnsSlice, netip.MustParseAddr) + + if i == 0 { + suite.Require().NoError(suite.State().Create(suite.Ctx(), resolverSpec)) + } else { + suite.Require().NoError(suite.State().Update(suite.Ctx(), resolverSpec)) + } + + rtestutils.AssertLength[*network.DNSUpstream](suite.Ctx(), suite.T(), suite.State(), len(dnsSlice)) + + upstreams, err := safe.ReaderListAll[*network.DNSUpstream](suite.Ctx(), suite.State()) + suite.Require().NoError(err) + + _, upstreamAddrs := netctrl.SortedProxies(upstreams) + + suite.Require().Equal(xslices.Map(dnsSlice, func(t string) string { return t + ":53" }), upstreamAddrs) + } +} + +func TestDNSUpstreams(t *testing.T) { + suite.Run(t, &DNSUpstreams{ + DefaultSuite: ctest.DefaultSuite{ + Timeout: 10 * time.Second, + AfterSetup: func(suite *ctest.DefaultSuite) { + suite.Require().NoError(suite.Runtime().RegisterController(&netctrl.DNSUpstreamController{})) + }, + }, + }) +} diff --git a/internal/app/machined/pkg/controllers/network/dns_upstream.go b/internal/app/machined/pkg/controllers/network/dns_upstream.go index 98312333066..8f6bd25266e 100644 --- a/internal/app/machined/pkg/controllers/network/dns_upstream.go +++ b/internal/app/machined/pkg/controllers/network/dns_upstream.go @@ -103,7 +103,7 @@ func (ctrl *DNSUpstreamController) run(ctx context.Context, r controller.Runtime return err } - for _, s := range rs.TypedSpec().DNSServers { + for i, s := range rs.TypedSpec().DNSServers { remoteAddr := s.String() if err = safe.WriterModify[*network.DNSUpstream]( @@ -114,6 +114,14 @@ func (ctrl *DNSUpstreamController) run(ctx context.Context, r controller.Runtime touchedIDs[u.Metadata().ID()] = struct{}{} if u.TypedSpec().Value.Prx != nil { + // Found upstream, update index + if u.TypedSpec().Value.Idx != i { + old := u.TypedSpec().Value.Idx + u.TypedSpec().Value.Idx = i + + l.Info("updated dns upstream idx", zap.String("addr", remoteAddr), zap.Int("was", old), zap.Int("now", i)) + } + return nil } @@ -122,8 +130,9 @@ func (ctrl *DNSUpstreamController) run(ctx context.Context, r controller.Runtime prx.Start(500 * time.Millisecond) u.TypedSpec().Value.Prx = prx + u.TypedSpec().Value.Idx = i - l.Info("created dns upstream", zap.String("addr", remoteAddr)) + l.Info("created dns upstream", zap.String("addr", remoteAddr), zap.Int("idx", i)) return nil }, diff --git a/internal/app/machined/pkg/controllers/network/resolver_merge.go b/internal/app/machined/pkg/controllers/network/resolver_merge.go index 8e3960083ca..bc0f8575970 100644 --- a/internal/app/machined/pkg/controllers/network/resolver_merge.go +++ b/internal/app/machined/pkg/controllers/network/resolver_merge.go @@ -94,9 +94,7 @@ func (ctrl *ResolverMergeController) Run(ctx context.Context, r controller.Runti } if final.DNSServers != nil { - if err = r.Modify(ctx, network.NewResolverSpec(network.NamespaceName, network.ResolverID), func(res resource.Resource) error { - spec := res.(*network.ResolverSpec) //nolint:errcheck,forcetypeassert - + if err = safe.WriterModify(ctx, r, network.NewResolverSpec(network.NamespaceName, network.ResolverID), func(spec *network.ResolverSpec) error { *spec.TypedSpec() = final return nil @@ -150,9 +148,9 @@ func mergeDNSServers(dst *[]netip.Addr, src []netip.Addr) { // and same vice versa for IPv6 switch { case dstHasV4 && !srcHasV4: - *dst = append(slices.Clone(src), filterIPFamily(*dst, true)...) + *dst = slices.Concat(src, filterIPFamily(*dst, true)) case dstHasV6 && !srcHasV6: - *dst = append(slices.Clone(src), filterIPFamily(*dst, false)...) + *dst = slices.Concat(src, filterIPFamily(*dst, false)) default: *dst = src } diff --git a/pkg/machinery/resources/network/dns_upstream.go b/pkg/machinery/resources/network/dns_upstream.go index f4237a41726..0a1913185a8 100644 --- a/pkg/machinery/resources/network/dns_upstream.go +++ b/pkg/machinery/resources/network/dns_upstream.go @@ -29,6 +29,7 @@ type DNSUpstreamSpecSpec struct { // We could use a generic struct here, but without generic aliases the usage would look ugly. // Once generic aliases are here, redo the type above as `type DNSUpstream[P Proxy] = typed.Resource[...]`. Prx Proxy + Idx int } // MarshalYAML implements yaml.Marshaler interface. @@ -38,6 +39,7 @@ func (d *DNSUpstreamSpecSpec) MarshalYAML() (any, error) { return map[string]string{ "healthy": strconv.FormatBool(d.Prx.Fails() == 0), "addr": d.Prx.Addr(), + "idx": strconv.Itoa(d.Idx), }, nil } @@ -67,6 +69,10 @@ func (DNSUpstreamExtension) ResourceDefinition() meta.ResourceDefinitionSpec { Name: "Address", JSONPath: "{.addr}", }, + { + Name: "Idx", + JSONPath: "{.idx}", + }, }, } }