feat: support marking cluster domains as FQDNs, and change the default to FQDN

[dervoeti]

Description

Fixes stackabletech/issues#656

The current regex for domain names does not permit a trailing dot. Analogous to the logic in Kubernetes, I decided to remove the trailing dot before validation (if the dot is present) instead of adjusting the regex.
After validation was successful, a trailing dot is now always appended to make the domain name a FQDN. This improves DNS performance, since for FQDNs the "search" domains in resolv.conf are not considered.

How to test this

General test setup

Local Kind cluster, add the line log (after errors) to the CoreDNS ConfigMap to make CoreDNS log all DNS queries, scale CoreDNS Deployment to 1 so we can get all the logs from one CoreDNS Pod, restart the CoreDNS Deployment to reload the config.

Install some operators:

stackablectl operator install commons secret listener zookeeper

Setup a Zookeeper cluster and a ZNode, wait for all pods to start, check CoreDNS logs

I grepped through the CoreDNS logs for svc.cluster.local.cluster.local, which indicates a "search" domain was appended to the DNS query (which degrades performance).

Using the current dev release, ndots=5 (default value)

Create a ZK cluster with 3 replicas:

apiVersion: zookeeper.stackable.tech/v1alpha1
kind: ZookeeperCluster
metadata:
  name: simple-zk
  namespace: default
spec:
  image:
    productVersion: 3.9.2
  servers:
    roleGroups:
      default:
        replicas: 3

Create a dummy ZNode:

apiVersion: zookeeper.stackable.tech/v1alpha1
kind: ZookeeperZnode
metadata:
  name: dummy-znode
spec:
  clusterRef:
    name: simple-zk

Lines like this should appear in the CoreDNS log:

[INFO] 10.244.0.45:35325 - 7328 "A IN simple-zk.default.svc.cluster.local.cluster.local. udp 67 false 512" NXDOMAIN qr,aa,rd 160 0.000192228s
[INFO] 10.244.0.45:35325 - 31385 "AAAA IN simple-zk.default.svc.cluster.local.cluster.local. udp 67 false 512" NXDOMAIN qr,aa,rd 160 0.000258389s
[INFO] 10.244.0.45:33465 - 56111 "A IN simple-zk.default.svc.cluster.local.cluster.local. udp 67 false 512" NXDOMAIN qr,aa,rd 160 0.000028455s
[INFO] 10.244.0.45:33465 - 5921 "AAAA IN simple-zk.default.svc.cluster.local.cluster.local. udp 67 false 512" NXDOMAIN qr,aa,rd 160 0.00008136s

The lines around these matches show how all search domains are tried and only the final query (for simple-zk.default.svc.cluster.local.) succeeds.

Using the current dev release, PodOverrides with ndots=3

Patch the Zookeeper Operator deployment:

kubectl patch deployment -n stackable-operators zookeeper-operator-deployment -p '{"spec":{"template":{"spec":{"dnsConfig":{"options":[{"name":"ndots","value":"3"}]}}}}}'

Clean up old ZK resources:

kubectl delete zookeeperclusters.zookeeper.stackable.tech simple-zk
kubectl delete pvc data-simple-zk-server-default-{0,1,2}

Restart CoreDNS deployment to clear the logs:

kubectl rollout restart deployment -n kube-system coredns

Recreate ZK cluster with ndots=3:

apiVersion: zookeeper.stackable.tech/v1alpha1
kind: ZookeeperCluster
metadata:
  name: simple-zk
  namespace: default
spec:
  image:
    productVersion: 3.9.2
  servers:
    roleGroups:
      default:
        podOverrides:
          spec:
            dnsConfig:
              options:
                - name: ndots
                  value: "3"
        replicas: 3

Wait for Pods to start, check CoreDNS log. svc.cluster.local.cluster.local should not be present in the log.

Using this fix, no PodOverrides

Clean up old ZK resources:

kubectl delete zookeeperclusters.zookeeper.stackable.tech simple-zk
kubectl delete pvc data-simple-zk-server-default-{0,1,2}

Uninstall Zookeeper operator:

stackablectl operator uninstall zookeeper

Install the Zookeeper operator with the fix from this PR (make run-dev).

Restart CoreDNS deployment to clear the logs:

kubectl rollout restart deployment -n kube-system coredns

Create a normal ZK cluster without the ndots override:

apiVersion: zookeeper.stackable.tech/v1alpha1
kind: ZookeeperCluster
metadata:
  name: simple-zk
  namespace: default
spec:
  image:
    productVersion: 3.9.2
  servers:
    roleGroups:
      default:
        replicas: 3

Wait for Pods to start, check CoreDNS log. svc.cluster.local.cluster.local should not be present in the log.

I additionally tested the fix with ndots set to 7, it still works even though the FQDN contains only 5 dots. I think that's because the search domains are never used when you make a DNS query for an FQDN (with a trailing dot).

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

Preview Give feedback

1. Changes are OpenShift compatible
2. CRD changes approved
3. Integration tests passed (for non trivial changes)

Reviewer

Preview Give feedback

1. Code contains useful comments
2. (Integration-)Test cases added
3. Documentation added or updated
4. Changelog updated
5. Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

Preview Give feedback

1. Feature Tracker has been updated
2. Proper release label has been added

[maltesander]

Tested and works!

Will wait with approval for now until:

• we decided on where to add the changes e.g. DomainName vs FqdnDomainName etc.
• we tested with operator PRs, integrationtests and demos

[maltesander]

LGTM overall!
Changelog entry is missing.
Do we want to mention anything about DNS/ndots etc. here https://docs.stackable.tech/home/stable/kubernetes/#_configuring_the_cluster_domain (or the sublinks)?

[sbernauer]

Thanks for working on this!

Actually I now have enough from the DomainName using the RFC_1123_SUBDOMAIN_REGEX :( I would suggest to add a new FQDN_REGEX allowing a trailing dot (as foo. is a valid domain name!) and using that, WDYT?

Regarding the user input I would prefer to not fiddle with the input the user gave us, but document and warn accordingly. Maybe our dotting strategy isn't perfect and the dot actually hurts something? Currently we would have no way of disabling it. But no strong opinion.

I spiked both things in main...feat/dns-performance-fqdns-review, what are your opinions on this?

Do we want to mention anything about DNS/ndots etc. here docs.stackable.tech/home/stable/kubernetes#_configuring_the_cluster_domain (or the sublinks)?

In case we go with my suggestion we should definitively update the docs to recommend a trailing dot

[dervoeti]

@sbernauer I like the idea of removing the logic from the new() function of KubernetesClusterInfo and printing a warning if a trailing dot is not present. That would basically reduce this PR to just appending a dot to KUBERNETES_CLUSTER_DOMAIN_DEFAULT 🫠
But I think this way it's less error prone and we have a way for users to opt out of this behavior: just set KUBERNETES_CLUSTER_DOMAIN to cluster.local.

Regarding the FQDN regex: We also thought about this, I don't think it's a bad idea but we decided to do it differently to be in line with the way Kubernetes does it: https://github.com/kubernetes/kubernetes/blob/30de989fb57fb5921a7ae3e3203cf7ecac9cf3f0/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L100
But I'm fine with your solution as well. WDYT @maltesander?

[sbernauer]

Regarding the FQDN regex

Works for me, we can use your existing logic.

But I think this way it's less error prone and we have a way for users to opt out of this behavior: just set KUBERNETES_CLUSTER_DOMAIN to cluster.local.

I don't understand how they can opt out to be honest. I think currently they are unable to set it to cluster.local, correct?

[maltesander]

To summarize, we can:

1. Just "pass-through" the cluster domain, do not append anything
   • users can opt out of the dot suffix
   • default behavior is the same
   • no default performance increase
2. Just "pass-through" the cluster domain, default = "cluster.local."
   • users can opt out of the dot suffix
   • default behavior changes
   • default performance increase
3. Suffix cluster domain (and default) with dot
   • performance increase by default
   • users cannot opt out

Is that correct? I honestly think option 1 is my favorite but this comes at the cost of not having the performance increase by default. WDYT?

Edit: We decided for option 2, using another FQDN regex for DomainName and defaulting to "cluster.local.". Users can opt out by e.g. setting "cluster.local" explicitly.

[sbernauer]

Thanks! Code looks good, we have a opt out as well :)

Please add a changelog and make some changes to the https://docs.stackable.tech/home/stable/guides/kubernetes-cluster-domain/ guire

[dervoeti]

@maltesander @sbernauer

Updates:

• Changelog adapted
• Docs written
• Added explicit tests for a trailing dot in FQDNs
• I tested different scenarios with Zookeeper and HDFS, including specifying a custom cluster domain (with and without trailing dot). Everything worked as expected, it was possible to opt out via setting KUBERNETES_CLUSTER_DOMAIN to cluster.local (a warning was printed in that case).

[maltesander]

Nice thanks! Checked docs as well.
LGTM. Lets wait for @sbernauer though.

[sbernauer]

LGTM, thanks!

[sbernauer]

Code still looks good, thanks for the review @nightkr
However, I think the title should also mention that we are changing the default cluster domain, because this is probably the biggest user-facing change