feat(dns): embedded DNS server instead of coreDNS #13124

lahabana · 2025-03-17T17:52:13Z

Motivation

Add an embedded DNS server that resolves mesh local hostnames.
This feature is disabled by default and needs to be enabled with kuma_dns_embedded_proxy_port on DPs in universal or simply with kuma_runtime_kubernetes_injector_builtin_dns_embedded=true on CP in Kubernetes (this leverages sidecar injection).

Implementation information

We leveraged the same system as MeshMetric. The DNS map is exposed in the _kuma:dynamicconfig listener, we poll this from the DP at a set frequency and reload the data.

We improved the whole configfetcher to make it reusable across different components. We also setup the endpoint so that it works with etag caching while avoids unnecessary reloading and processing, we can therefore refresh the configs more quickly as well.

For the moment I added a new matrix for e2e test. If this is stable I'll switch the default.

For DNS proxy we'd like to reuse the same configFetcher mechanism Split it out to its own package and define an api to add handlers Also add support for the handlers to handle etag to avoid reloading when the config doesn't change Signed-off-by: Charly Molter <[email protected]>

Also add support for etag caching Signed-off-by: Charly Molter <[email protected]>

Signed-off-by: Charly Molter <[email protected]>

lahabana · 2025-03-17T17:52:33Z

I've made a few separate commits as this is a bit of a mouthful

github-actions · 2025-03-17T17:56:28Z

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
IPv6 is taken into account (.e.g: no string concatenation of host port)
Tests (Unit test, E2E tests, manual test on universal and k8s)
- Don't forget ci/ labels to run additional/fewer tests
Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

Signed-off-by: Charly Molter <[email protected]>

.github/workflows/_test.yaml

app/kuma-dp/pkg/dataplane/configfetcher/api.go

app/kuma-dp/cmd/run.go

app/kuma-dp/pkg/dataplane/configfetcher/component.go

app/kuma-dp/pkg/dataplane/configfetcher/metrics.go

lobkovilya · 2025-03-18T15:28:09Z

app/kuma-dp/pkg/dataplane/configfetcher/metrics.go

+	})
+	prometheus.MustRegister(handlerTickCount)
+	handlerTickDuration := prometheus.NewSummary(prometheus.SummaryOpts{
+		Name:        "kuma_dp_envoyconfigfetcher_handler_call_duration_seconds",


why in seconds? aren't these calls usually less than a second? also I think in other places we mostly use milliseconds

I'm not sure it's always milliseconds:

kuma/pkg/plugins/resources/postgres/pgx_store.go

Line 496 in 4fa5da8

Help: "Time Blocked waiting for new connection in seconds",

kuma/pkg/api-server/resource_endpoints_test.go

Line 89 in a543cf8

Expect(test_metrics.FindMetric(metrics, "api_server_http_request_duration_seconds")).ToNot(BeNil())

Look at the prom API it takes float64 and it's meant to be seconds (that's also why Duration.Seconds() returns a float64)

I see, I just saw we have lots of .Observe(float64(time.Since(start).Milliseconds()))

❯ rg Observe\(.*Milliseconds\(\)' . ./pkg/dns/vips_allocator.go 68: d.metrics.VipGenerations.Observe(float64(core.Now().Sub(start).Milliseconds())) ./pkg/insights/resyncer.go 365: r.idleTime.Observe(float64(startProcessingTime.Sub(start).Milliseconds())) 366: r.timeToProcessItem.Observe(float64(startProcessingTime.Sub(event.time).Milliseconds())) 401: r.itemProcessingTime.WithLabelValues(reason, result).Observe(float64(time.Since(startProcessingTime).Milliseconds())) ./pkg/metrics/store/counter.go 74: s.metric.Observe(float64(core.Now().Sub(start).Milliseconds())) ...

but apparently, we haven't named some metrics properly, i.e. it's called vip_generation but actually reports milliseconds. I'll open an issue to track this

It's not dramatic because the metric doesn't contain a unit:

kuma/pkg/dns/metrics/metrics.go

Line 16 in aae7e5e

Name: "vip_generation",

But prometheus generally recommends using seconds: https://prometheus.io/docs/practices/naming/#base-units though it also recommends using the unit name in the metric name to help with ambiguity

Automaat · 2025-03-19T09:13:06Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+	response := &dns.Msg{}
+	// In case it was never loaded
+	s.dnsMap.CompareAndSwap(nil, &dnsMap{
+		ARecords:    make(map[string]*dnsEntry),


shouldn't we also handle SRV records like envoy do?

What would we do with SRV records? While Envoy handles SRV record the map we configure is empty so Envoy would return NXDOMAIN and fallback to proxying

slonka

In general looks promising, a bunch of questions and nitpicks. Also would love to see a simple test checking integration with a DNS client.

Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
IPv6 is taken into account (.e.g: no string concatenation of host port)
Tests (Unit test, E2E tests, manual test on universal and k8s)
- Don't forget ci/ labels to run additional/fewer tests
Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

.github/workflows/_e2e.yaml

slonka · 2025-03-19T09:20:18Z

app/kuma-dp/cmd/run.go

+				time.Second*5,
+				time.Minute,


we usually make this configurable or use:

rt.Config().General.ResilientComponentBaseBackoff.Duration, rt.Config().General.ResilientComponentMaxBackoff.Duration,

Is that useful to be configurable though?

We'd have to dig the history to see why this was first introduced - https://github.com/kumahq/kuma/pull/9892/files - looks like for resiliency e2e test 🤷 - maybe @Automaat can chip in. Basically I wasn't sure if we should follow how the others are set up or not.

This is for the CP though right? Here we're talking about the DP

Made a config for it

slonka · 2025-03-19T09:43:09Z

app/kuma-dp/pkg/dataplane/configfetcher/component.go

+func (cf *ConfigFetcher) stepForHandler(h *handlerInfo) (bool, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), cf.perHandlerTimeout)
+	defer cancel()
+	r, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("http://localhost%s", h.handler.Path()), nil)


is localhost a valid unix domain socket address? Wasn't there a comment or something in the MeshMetric code?

Host is just the host: header when doing http over unix domain socket because there's no TCP.

slonka · 2025-03-19T09:48:06Z

app/kuma-dp/pkg/dataplane/configfetcher/metrics.go

+	})
+	prometheus.MustRegister(handlerTickCount)
+	handlerTickDuration := prometheus.NewSummary(prometheus.SummaryOpts{
+		Name:        "kuma_dp_envoyconfigfetcher_handler_call_duration_seconds",


I'm not sure it's always milliseconds:

kuma/pkg/plugins/resources/postgres/pgx_store.go

Line 496 in 4fa5da8

Help: "Time Blocked waiting for new connection in seconds",

kuma/pkg/api-server/resource_endpoints_test.go

Line 89 in a543cf8

Expect(test_metrics.FindMetric(metrics, "api_server_http_request_duration_seconds")).ToNot(BeNil())

slonka · 2025-03-19T09:51:39Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+		AAAARecords: make(map[string]*dnsEntry),
+	})
+	var dnsEntry *dnsEntry
+	if len(req.Question) > 0 { // Apparently most DNS doesn't support multiple questions so let's just support the first one


should we log if it's > 1?

slonka · 2025-03-19T10:02:07Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+				continue
+			}
+			switch {
+			case strings.Contains(ipStr, "."):


I guess this will work but isn't it safer to just call:

case ip.To4() != nil:

?

as per docs:

// To4 converts the IPv4 address ip to a 4-byte representation. // If ip is not an IPv4 address, To4 returns nil.

It doesn't matter, I found the govalidator repo does the same way.
https://github.com/asaskevich/govalidator/blob/e11347878e2323a0777b40d33eeffd37a185e31b/validator.go#L818-L828

slonka · 2025-03-19T10:04:06Z

app/kuma-dp/pkg/dataplane/dnsproxy/metrics.go

+	})
+	prometheus.MustRegister(upstreamRequestDuration)
+	requestDuration := prometheus.NewSummary(prometheus.SummaryOpts{
+		Name: "kuma_dp_embeddeddns_proxy_request_duration_seconds",


why does it have proxy in the name?

slonka · 2025-03-19T10:07:16Z

pkg/config/plugins/runtime/k8s/config.go

@@ -389,6 +389,8 @@ type BuiltinDNS struct {
 	Port uint32 `json:"port,omitempty" envconfig:"kuma_runtime_kubernetes_injector_builtin_dns_port"`
 	// Turn on query logging for DNS
 	Logging bool `json:"logging,omitempty" envconfig:"kuma_runtime_kubernetes_injector_builtin_dns_logging"`
+	// Use the embedded DNS instead (This is an experimental feature)
+	UseEmbedded bool `json:"embedded,omitempty" envconfig:"kuma_runtime_kubernetes_injector_builtin_dns_embedded"`


isn't builtin and embedded the same thing?

Nope builtin the container embedded in the binary. In any case these 2 are not meant to be present together for too long

slonka · 2025-03-19T10:13:25Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+	"github.com/kumahq/kuma/pkg/dns/dpapi"
+)
+
+var log = core.Log.WithName("embeededdns")


nitpick:

Suggested change

var log = core.Log.WithName("embeededdns")

var log = core.Log.WithName("embeddeddns")

slonka · 2025-03-19T10:14:59Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+	upstreamHostPort string
+}
+
+func NewServer(address string, port uint32) *Server {


do we have a test for this? just starting the server, seeding it and having a client query it and seeing if the results match?

Signed-off-by: Charly Molter <[email protected]>

lahabana · 2025-03-19T10:23:09Z

@lobkovilya pushed a new commit which simplifies the handler API as it doesn't deal with shutdowns anymore (that's what components are for)

Icarus9913 · 2025-03-20T08:15:24Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+		if err != nil {
+			s.metrics.UpstreamRequestFailureCount.Inc()
+			log.Error(err, "Failed to write message to upstream")
+			response = &dns.Msg{}


It's already empty when initial

Suggested change

response = &dns.Msg{}

Icarus9913 · 2025-03-21T02:44:14Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+		if ctx.Err() != nil {
+			return ctx.Err()
+		}


Please add some comments here to explain why we put the ctx check conditional here. (ctx operation overtime)

Icarus9913 · 2025-03-21T08:17:41Z

app/kuma-dp/pkg/dataplane/dnsproxy/component.go

+				continue
+			}
+			switch {
+			case strings.Contains(ipStr, "."):


It doesn't matter, I found the govalidator repo does the same way.
https://github.com/asaskevich/govalidator/blob/e11347878e2323a0777b40d33eeffd37a185e31b/validator.go#L818-L828

Signed-off-by: Charly Molter <[email protected]>

lahabana added 3 commits March 17, 2025 18:51

Add new XDS to be able to add multiple config json

14b39e9

Also add support for etag caching Signed-off-by: Charly Molter <[email protected]>

add DNS embedded proxy

4543aad

Signed-off-by: Charly Molter <[email protected]>

lahabana added the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Mar 17, 2025

fix golangci-lint

17f1e04

Signed-off-by: Charly Molter <[email protected]>

lahabana force-pushed the experimentDNS branch from 37795b6 to 17f1e04 Compare March 17, 2025 19:51

lahabana added 3 commits March 18, 2025 09:10

fix package location

3abda26

Signed-off-by: Charly Molter <[email protected]>

fix typo

915a787

Signed-off-by: Charly Molter <[email protected]>

Merge branch 'master' into experimentDNS

abaa0dd

lahabana changed the title ~~feat(dns): running embedded DNS server instead of coreDNS~~ feat(dns): embedded DNS server instead of coreDNS Mar 18, 2025

lahabana marked this pull request as ready for review March 18, 2025 12:49

lahabana requested a review from a team as a code owner March 18, 2025 12:49

lahabana requested review from slonka and lobkovilya March 18, 2025 12:49

lahabana self-assigned this Mar 18, 2025

lukidzi reviewed Mar 18, 2025

View reviewed changes

.github/workflows/_test.yaml Outdated Show resolved Hide resolved

lobkovilya reviewed Mar 18, 2025

View reviewed changes

bartsmykla self-requested a review March 19, 2025 07:04

Merge branch 'master' into experimentDNS

901ece5

Automaat reviewed Mar 19, 2025

View reviewed changes

slonka reviewed Mar 19, 2025

View reviewed changes

make meshmetrics a component and don't do shutdown in configFetcher

2ed4431

Signed-off-by: Charly Molter <[email protected]>

Icarus9913 reviewed Mar 21, 2025

View reviewed changes

change params name

c964498

Signed-off-by: Charly Molter <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dns): embedded DNS server instead of coreDNS #13124

feat(dns): embedded DNS server instead of coreDNS #13124

lahabana commented Mar 17, 2025 •

edited

Loading

lahabana commented Mar 17, 2025

github-actions bot commented Mar 17, 2025

lobkovilya Mar 18, 2025

slonka Mar 19, 2025

lahabana Mar 19, 2025

lobkovilya Mar 19, 2025

lahabana Mar 20, 2025

Automaat Mar 19, 2025

lahabana Mar 19, 2025

slonka left a comment

slonka Mar 19, 2025

lahabana Mar 19, 2025

slonka Mar 20, 2025

lahabana Mar 20, 2025

lahabana Mar 20, 2025

slonka Mar 19, 2025

lahabana Mar 20, 2025

slonka Mar 19, 2025

slonka Mar 19, 2025

slonka Mar 19, 2025

Icarus9913 Mar 21, 2025

slonka Mar 19, 2025

slonka Mar 19, 2025

lahabana Mar 19, 2025

slonka Mar 19, 2025

slonka Mar 19, 2025

lahabana commented Mar 19, 2025

Icarus9913 Mar 20, 2025 •

edited

Loading

Icarus9913 Mar 21, 2025

Icarus9913 Mar 21, 2025

	var log = core.Log.WithName("embeededdns")
	var log = core.Log.WithName("embeddeddns")

feat(dns): embedded DNS server instead of coreDNS #13124

Are you sure you want to change the base?

feat(dns): embedded DNS server instead of coreDNS #13124

Conversation

lahabana commented Mar 17, 2025 • edited Loading

Motivation

Implementation information

lahabana commented Mar 17, 2025

github-actions bot commented Mar 17, 2025

Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slonka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahabana commented Mar 19, 2025

Icarus9913 Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahabana commented Mar 17, 2025 •

edited

Loading

Icarus9913 Mar 20, 2025 •

edited

Loading