Kubernetes manifest need rework

[Quentin-M]

Hi,

I was evaluating using CoreDNS rather than the deprecated kube-dns addon, as now suggested by Kubernetes. However, I found a few issues in the provided manifests:

• It is not clear at first glance which manifest should be used: coredns.yaml.sed, coredns-1.6.yaml.sed? We are now on Kubernetes 1.8 and soon 1.9. In my understanding, coredns.yaml.sed should be renamed coredns-1.5.yaml.sed and coredns-1.6.yaml.sed should simply be coredns.yaml.sed, or even better: the RBAC resource should be separated (like most projects do).
• coredns-1.6.yaml.sed has an invalid ConfigMap (cidrs)
• The kubernetes.io/cluster-service label has been deprecated, and was only useful to the Addon Manager
• scheduler.alpha.kubernetes.io/tolerations has been replaced by an actual spec element (see bel0w_.
• Tolerations should contain the standardized master taint / label (see below)
• For stability and security purposes (especially with the Node Authorizer admission controller), CoreDNS should only run on master nodes (and should probably have at least 2 replicas by default).
• The deployment should have a RollingUpdate strategy for availability purposes.
• Using the latest image is generally considered bad practice.
• Exposing the metrics port on the service can arguably be considered a threat (especially if Network Policies are being used).

The Kubernetes documentation deprecated most of the cluster addons (even the well-known kubedns), and now lists a very few addons, including CoreDNS (pointing to that repository). However:

• The manifest does not seem to be strongly maintained
• The main repository's README file mentions: If you do want to use CoreDNS in production, please let us know.
  This is a little bit worrying considering thousands users refer to that documentation as a reference, including numerous companies, which sometimes run hundreds/thousands clusters.

Tolerating and selecting the master nodes

      tolerations:
        - key: node-role.kubernetes.io/master
          effect: NoSchedule
        - key: "CriticalAddonsOnly"
          operator: "Exists"
      nodeSelector:
        node-role.kubernetes.io/master: ""

Rolling-Update strategy

  strategy:
    type: RollingUpdate
    rollingUpdate:
      maxUnavailable: 1

Thank you.

[johnbelamaric]

Thanks for your thorough review. You are right this is not really up-to-date, and we will get on that ASAP (PRs are welcome too!).

You may also be interested in kubernetes/kubernetes#52501 and kubernetes/minikube#1995.

@rajansandeep PTAL

[chrisohaver]

Thanks again for the review. Summarizing what we have fixed so far...

• Removed the pre 1.6 version, leaving only one manifest.
• Corrected the deprecated "cidrs" in the ConfigMap.
• Moved tolerations to spec element
• Added master NoSchedule taint toleration
• Use current release, not "latest".
• main repo README updated

not addressed yet:

• CoreDNS master affinity - kube-dns does not have master node affinity. Actually we may want to use antiAffinity to make sure replicas don't land all on one node.
• Exposed metrics port - kube-dns appears to also expose the metrics port in its default configuration... I found this issue Improving the Default Security Posture Through Defense in Depth kubernetes/kubernetes#52184 scroll to "7. Exposed /metrics APIs Allow for Pod/Svc Enumeration" ... the risk seems low enough to be an OK default. As they are now, CoreDNS metrics are aggregations (e.g. you cant enumerate services or pods based on them).
• RollingUpdate strategy - I think 2 replicas + maxUnavailable: 1 is a sensible minimal default. @johnbelamaric, what do you think?

[ae6rt]

Nice work.

I'd be somewhat sorry to see the non-RBAC manifests removed. Not every cluster uses RBAC yet, and if not, experimenting with CoreDNS with the RBAC manifest would be challenging.

[johnbelamaric]

@chrisohaver yes on the RollingUpdate strategy. Also update to 0.9.10 and get rid of the "CIDR needs to be on a /8 boundary" comment. What about kubernetes.io/cluster-service? Should we remove it here?

[chrisohaver]

For stability and security purposes ... CoreDNS should only run on master nodes ...

Most every thing here is in the deployment now, except for the above. In fact, we have recently merged a change that does the exact opposite of this, never allowing coredns to be scheduled on the master node. See discussion #50.