ori-edge / k8s_gateway

A CoreDNS plugin to resolve all types of external Kubernetes resources
Apache License 2.0
295 stars 63 forks source link

Missing SOA for '.' zone when using zoneFile #155

Closed qlonik closed 1 year ago

qlonik commented 1 year ago

When I'm attempting to use zoneFile feature (passed as values to helm configuration), I get an error saying plugin/file: file "/etc/coredns/example.db" has no SOA record for origin .. I think the point to note here is that there is a period ('.') in the error message, that doesn't appear to be a part of the sentence.

Reading through coredns docs for file, there is a syntax passage:

file DBFILE [ZONES...]

  • DBFILE the database file to read and parse. If the path is relative, the path from the root plugin will be prepended to it.
  • ZONES zones it should be authoritative for. If empty, the zones from the configuration block are used.

It appears that ZONES defaults to '.' zone since that is what is written at the top of Corefile: https://github.com/ori-edge/k8s_gateway/blob/79e977fc53b8239955252673173d1df69f5dd129/charts/k8s-gateway/templates/configmap.yaml#L9

Is it possible to add a feature where we can specify the list of zones while specifying zoneFile configuration option?

networkop commented 1 year ago

I remember it was working a few months ago https://github.com/networkop/nmos-kubernetes/blob/main/dns-values-http.yml

Are you able to manually test to confirm that adding zones fixes that error?

qlonik commented 1 year ago

Yes, once I add . IN SOA ns.${SECRET_DOMAIN}. root.${SECRET_DOMAIN}. ( 2 2h 1h 24h 1h ), the error goes away and coredns can properly start. (btw, the ${SECRET_DOMAIN} gets properly resolved to my domain, so that is not an issue)

While this lets coredns+k8s_gateway start, it does not look like this configuration is correct, since my DNS queries return some unexpected values. I think the correctness of this SOA entry for '.' zone is outside of the scope of this ticket.

networkop commented 1 year ago

ok, so what you've changed is the content of the zone file, which is a user-supplied input. Do you think that helm charts need to be changed?

qlonik commented 1 year ago

Oh, sorry, I haven't realized that by saying 'zones' in the previous message you meant zones in corefile. I tried it and it seems that coredns still requires SOA record for the zone passed in file configuration setting. However, I set it to the same value that k8s-gateway sets (k8s-gateway.networking.domain.com. and hostmaster.k8s-gateway.networking.domain.com.), and it seems to be working pretty well that way.

I think adding '.' into zone files is a way to bypass the error, but I don't think it is a proper fix, since it doesn't quite make sense why this DNS server has to be an authoritative server for '.' zone. If the DNS on the main network is configured to point to this service, without the use of split-horizon DNS setup, this service will act as authoritative server and will respond with empty queries to all other domains like google.com, github.com and so on.

Because of the above reasons, I would think that allowing to specify which zones the particular zonefile applies to would be a more desirable solution. With this setup, this would also open up a feature of letting coredns act as a split-horizon DNS.

EDIT: I think it should be a pretty small change on this line https://github.com/ori-edge/k8s_gateway/blob/79e977fc53b8239955252673173d1df69f5dd129/charts/k8s-gateway/templates/configmap.yaml#L39

Only this needs to be added at the end:

{{ if .Values.zoneFile.domains }} {{ .Values.zoneFile.domains }}{{ end }}

I can also open a PR if you want me to.

networkop commented 1 year ago

yep, this makes sense. I think what you're trying to achieve can be done using the extraZonePlugins. So, we could drop that Line39 completely and instead you would do something like this:

extraZonePlugins:
  - name: file
    parameters: /etc/coredns/myzone.db k8s-gateway.networking.domain.com

zoneFile:
  filename: myzone.db
  contents: "your content"

The above should render the following configmap:

data:
  Corefile: |-
    .:1053 {
        k8s_gateway foo.com {
          apex release-name-k8s-gateway.default
          ttl 300
        }
        file /etc/coredns/myzone.db k8s-gateway.networking.domain.com
    }
  myzone.db:     your content

This is how the coredns's helm charts are designed (if I'm not mistaken). wdyt @qlonik ?

networkop commented 1 year ago

just as I clicked send, I've found this https://github.com/coredns/helm/blob/13550344baed72aa720497df8b8b61051f5461b2/stable/coredns/templates/configmap.yaml#L44-L50 For some reason, I can't see the following piece in the latest coredns helm charts anymore.

      {{- range .Values.zoneFiles }}
        file /etc/coredns/{{ .filename }} {{ .domain }}
      {{- end }}

will try to do some more digging

networkop commented 1 year ago

looks like here's where that block has disappeared https://github.com/coredns/helm/commit/c73b5d0c2d7b9b41a0574672efec6569c4413991

networkop commented 1 year ago

looks like this was unintentional. The zoneFiles.domain value is not even used now in coredns's helm charts. so I'm fine either way, whatever you think is easier. option A:

      {{- range .Values.zoneFiles }}
        file /etc/coredns/{{ .filename }} {{ .domain }}
      {{- end }}

option B:

extraZonePlugins:
  - name: file
    parameters: /etc/coredns/myzone.db k8s-gateway.networking.domain.com

feel free to submit a PR for this

qlonik commented 1 year ago

I think if the line 39 is removed from the k8s_gateway/charts/k8s-gateway/templates/configmap.yaml file, it will be a breaking change to expect to add extraZonePlugins configuration (as shown in option B). I also finally managed to pass extraZonePlugins as helm values, and I ended up with a following bunch of default settings removed.

    log
    errors
    health {
        lameduck 5s
    }
    ready
    prometheus 0.0.0.0:9153
    forward . /etc/resolv.conf
    loop
    reload
    loadbalance

So, I think the option A will be a nicer one. I'll create a PR for that today. :+1:

qlonik commented 1 year ago

Ah shoot. Yesterday after debugging for a few hours, I figured that coredns with file plugin doesn't quite match my use case. I still think the improvement proposed here is a good one and should be kept.

But for a future reference, here is a note on what I found My split-horizon DNS setup has the majority of names setup via cloudflare and seen from outside, but I want to overwrite a very small subset of names to resolve to local IPs. When I'm using file plugin, coredns becomes authoritative for the domain name and doesn't consult with upstream service for names that are missing in its own records. It simply returns `NXDOMAIN` for missing domain name. Realistically, what is necessary is to allow fallthrough for those queries. Unfortunately, it turned out that file plugin doesn't provide fallthrough. However, I found some other approaches using other plugins that support fallthrough. There is one [using hosts plugin](https://www.reddit.com/r/selfhosted/comments/jk9g61/coredns_for_local_dns_resolution_and_forwarding/) ([webarchive](https://web.archive.org/web/20201031113717/https://www.reddit.com/r/selfhosted/comments/jk9g61/coredns_for_local_dns_resolution_and_forwarding/)) and another one [using external-dns and etcd plugin](https://www.technowizardry.net/2022/06/split-horizon-dns-with-external-dns-and-cert-manager-for-kubernetes/) ([webarchive](https://web.archive.org/web/20221006224537/https://www.technowizardry.net/2022/06/split-horizon-dns-with-external-dns-and-cert-manager-for-kubernetes/)). It seems it could also be possible to synchronize DNS by using secondary plugin or by using transfer plugin with cloudflare. However those features are only supported on enterprise accounts, which renders this approach not realistic. Another way might be to use [alternate](https://github.com/coredns/alternate) external plugin for coredns, but I'm not very familiar with compiling coredns for k8s to include the plugin, so I didn't attempt it. It might also be possible to use [rewrite](https://github.com/coredns/coredns/tree/master/plugin/rewrite) plugin to overwrite some of the queries to hardcoded IPs. One more way that might work is to have Dnsmasq, PiHole or Adguard sit in front of coredns and resolve those overwrites, before passing queries those don't know about to coredns (with this plugin) and coredns will forward remaining queries it doesn't know about to upstream like cloudflare or google DNS. Yet another way might be to support `ExternalName` by k8s_gateway as mentioned in https://github.com/ori-edge/k8s_gateway/issues/1#issuecomment-816022215. This way, since k8s_gateway supports fallthrough, all unknown queries will end up going to upstream as well.

Given the use of other plugins that require files present in the container (e.g. hosts requires hostfile to be given to it), maybe it would also be a good idea to provide a way to inject other files so they can be referenced within extraZonePlugins block? That could be covered by a different issue though.

networkop commented 1 year ago

I see. I think hosts plugin may be the easiest solution for you in this case. ExternalName would require a resolvable CNAME record.

qlonik commented 1 year ago

Thanks for the suggestion.

I think we can close this issue, since we resolved and merged the zoneFiles feature