NodeLocalDNS operator #59

somtochiama · 2020-06-04T15:08:50Z

This is for the localnodedns operator

Boilerplate
Ensure it runs out of the cluster
Ensure it runs in-cluster
Golden file tests

k8s-ci-robot · 2020-06-04T15:08:55Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dholbach · 2020-06-05T07:46:28Z

/uncc
/cc @stealthybox

somtochiama · 2020-06-11T22:44:43Z

/assign @stealthybox

rajansandeep

This is a really good start!
I have added some general queries in the review

rajansandeep · 2020-06-12T19:24:10Z

localnodedns/controllers/localnodedns_controller.go

+			o.Spec.DNSIP = "169.254.20.10"
+		}
+
+		if o.Spec.ClusterIP == "" {


Here, we can try to fetch the Cluster IP from the kube-dns service.
kubectl get svc kube-dns -n kube-system -o jsonpath={.spec.clusterIP}

If we are unable to determine, then we can use the default value.

There are similar functions in the coredns-operators for finding the ClusterIP and DNS Domain for a cluster. Maybe we could reuse it? Wdyt?

Yes, we should start to create a shared library of these functions. I think probably in kubebuilder-declarative-pattern, because if e.g. the DNS team takes on ownership of this operator this repo may eventually end up pretty empty :-)

I think we can do this in a follow on PR, if you'd like. (You can add a TODO here so we don't forget)

rajansandeep · 2020-06-12T19:27:59Z

localnodedns/Dockerfile

+COPY channels/ channels/
+USER nonroot:nonroot
+
+ENTRYPOINT ["/manager"]


Nit: newline

This file is auto-generated I believe - is this a bug in our kubebuilder plugin?

I doubt. I edit the file somewhere along the line. But I will generate a new operator and check, to be sure

rajansandeep · 2020-06-12T19:57:30Z

localnodedns/controllers/localnodedns_controller_test.go

@@ -0,0 +1 @@
+package controllers


The test file seems empty. How are the golden test files generated?

Sorry, That was a mistake

rajansandeep · 2020-06-12T20:13:56Z

localnodedns/controllers/tests/simple.out.yaml

+        reload
+        loop
+        bind 169.254.20.10 10.96.0.10
+        forward . __PILLAR__CLUSTER__DNS__ {


~~The __PILLAR__CLUSTER__DNS__ placeholder values should probably be generated properly~~

Nvm, looks like the automatically updated Corefile values reflected only in the logs as per https://github.com/kubernetes/dns/blob/9903307a652efd77d5de40fdab3bfcd468fd1c74/cmd/node-cache/app/configmap.go#L67

rajansandeep · 2020-06-12T20:14:30Z

localnodedns/controllers/tests/simple.out.yaml

+        reload
+        loop
+        bind 169.254.20.10 10.96.0.10
+        forward . __PILLAR__UPSTREAM__SERVERS__ {


~~The __PILLAR__UPSTREAM__SERVERS__ placeholder values should probably be generated properly~~

Nvm, looks like the automatically updated Corefile values reflected only in the logs as per https://github.com/kubernetes/dns/blob/9903307a652efd77d5de40fdab3bfcd468fd1c74/cmd/node-cache/app/configmap.go#L67

rajansandeep · 2020-06-12T20:19:02Z

localnodedns/channels/packages/localnodedns/1.15.0/manifest.yaml

+              path: Corefile.base
+---
+apiVersion: app.k8s.io/v1beta1
+kind: Application


Does node-local-cache need/use an application?

I actually thought all addons needed an application. What qualifies an addon to use an application?

We don't require an application.

I think it's a good idea to have one, it means we have a consistent object that groups the addon contents. Some UIs (for example GKE's) will display Applications nicely.

I know sig-apps is having some debates about Application, but until we have something better, I think we want something, so let's use an Application. If something better comes along, we can then easily replace Applications with new-thing!

Aah, makes sense. I should add one for the coredns operator then.

justinsb · 2020-06-19T12:48:05Z

localnodedns/Dockerfile

+COPY channels/ channels/
+USER nonroot:nonroot
+
+ENTRYPOINT ["/manager"]


This file is auto-generated I believe - is this a bug in our kubebuilder plugin?

justinsb · 2020-06-19T12:50:50Z

localnodedns/channels/packages/localnodedns/1.15.0/manifest.yaml

+              path: Corefile.base
+---
+apiVersion: app.k8s.io/v1beta1
+kind: Application


We don't require an application.

I think it's a good idea to have one, it means we have a consistent object that groups the addon contents. Some UIs (for example GKE's) will display Applications nicely.

I know sig-apps is having some debates about Application, but until we have something better, I think we want something, so let's use an Application. If something better comes along, we can then easily replace Applications with new-thing!

justinsb · 2020-06-19T12:57:59Z

localnodedns/channels/packages/localnodedns/1.15.0/manifest.yaml

+        operator: "Exists"
+      containers:
+      - name: node-cache
+        image: k8s.gcr.io/k8s-dns-node-cache:1.15.13


This is going to be annoying feedback, so I'm sorry in advance...

Should we name it "LocalNodeDNS" or "NodeLocalDNS" or "DNSNodeCache"? We want to be consistent with other names.

I've always personally called in NodeLocalDNS. We should probably pick whatever name is most commonly used (with luck, that will be LocalNodeDNS!).

The names inside the code don't matter as much as the CRD name - it's the CRD name that users will interact with.

I think the "official" name is NodeLocalDNS (I'm using https://github.com/kubernetes/kubernetes/tree/release-1.18/cluster/addons/dns/nodelocaldns as reference), so I think we should follow that.

I think NodeLocalDNS is the one that is most commonly use. Should I manually change all occurrences of the name(I think this is error-prone) or generate a new operator with the new name.

justinsb · 2020-06-19T13:01:14Z

localnodedns/controllers/localnodedns_controller.go

+		o := object.(*api.LocalNodeDNS)
+		kubeProxyMode, err := findKubeProxyMode(ctx, mgr.GetClient())
+		if err != nil {
+			fmt.Println("Error determining kube-proxy mode: Defaulting to iptables")


In general, use logging functions over stdout / stderr. So either klog.Warningf("error determining kube-proxy mode...) or log.Log("error determining kube-proxy mode....

You also should include the error, otherwise it's really hard for someone that sees the log to know what went wrong; for example:

klog.Warningf("error determining kube-proxy mode, defaulting to iptables: %v", err)

I think log has a dedicated helper:

log.Error(err, "error determining kube-proxy mode, defaulting to iptables")

Logging in kubernetes is generally in flux right now; we're moving to structured logging (log) but doing so by adding it to klog (I think) https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1602-structured-logging

justinsb · 2020-06-19T13:06:35Z

localnodedns/controllers/localnodedns_controller.go

+			o.Spec.DNSIP = "169.254.20.10"
+		}
+
+		if o.Spec.ClusterIP == "" {


Yes, we should start to create a shared library of these functions. I think probably in kubebuilder-declarative-pattern, because if e.g. the DNS team takes on ownership of this operator this repo may eventually end up pretty empty :-)

I think we can do this in a follow on PR, if you'd like. (You can add a TODO here so we don't forget)

justinsb · 2020-06-19T13:10:59Z

This looks good @somtochiama (and thanks @rajansandeep for the reviews!).

I apologize for this, but we should figure out what name we want to use. I think the most common terms were LocalNodeDNS or LocalNodeDNSCache based on some basic google searching; but if we can justify the existing name then that's fine too!

justinsb · 2020-06-29T13:53:05Z

Thanks for renaming - for other tweaks (e.g. sharing functions that you've now put into the kubebuilder-declarative-pattern library :-) ) I think we can merge + iterate

/aprpove
/lgtm

justinsb · 2020-06-29T13:53:09Z

/approve

k8s-ci-robot · 2020-06-29T13:53:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, SomtochiAma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Initial Scaffolding

c5c9283

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 4, 2020

k8s-ci-robot requested review from dholbach and justinsb June 4, 2020 15:09

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 4, 2020

somtochiama added 2 commits June 4, 2020 16:16

Replaces placeholders in the manifest.yaml

09e25a0

Uses correct default

dd7748c

somtochiama force-pushed the localnode branch from 0dc55ef to dd7748c Compare June 4, 2020 15:18

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 4, 2020

somtochiama changed the title ~~Local-node-dns-cache operator~~ [WIP] Local-node-dns-cache operator Jun 4, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2020

k8s-ci-robot requested review from stealthybox and removed request for dholbach June 5, 2020 07:46

somtochiama added 3 commits June 5, 2020 10:21

Sets placeholders based on kubeproxy mode

673aaa3

Adds golden file test

129b27b

Makes operator run in-cluster

69c9107

somtochiama changed the title ~~[WIP] Local-node-dns-cache operator~~ Local-node-dns-cache operator Jun 11, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2020

k8s-ci-robot assigned stealthybox Jun 11, 2020

rajansandeep reviewed Jun 12, 2020

View reviewed changes

Writes the controller test files

7c483a5

somtochiama requested a review from rajansandeep June 13, 2020 09:41

Adds newline

d0fa09a

justinsb reviewed Jun 19, 2020

View reviewed changes

Removes fmt and uses klog

b198c9b

Renames localnodedns to nodelocaldns

10f2654

somtochiama changed the title ~~Local-node-dns-cache operator~~ NodeLocalDNS operator Jun 28, 2020

k8s-ci-robot assigned justinsb Jun 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2020

k8s-ci-robot merged commit 7353066 into kubernetes-sigs:master Jun 29, 2020

somtochiama mentioned this pull request Aug 14, 2020

REQUEST: New membership for SomtochiAma kubernetes/org#2107

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NodeLocalDNS operator #59

NodeLocalDNS operator #59

somtochiama commented Jun 4, 2020 •

edited

Loading

k8s-ci-robot commented Jun 4, 2020

dholbach commented Jun 5, 2020

somtochiama commented Jun 11, 2020

rajansandeep left a comment

rajansandeep Jun 12, 2020

somtochiama Jun 13, 2020

justinsb Jun 19, 2020

rajansandeep Jun 12, 2020

justinsb Jun 19, 2020

somtochiama Jun 20, 2020

rajansandeep Jun 12, 2020

somtochiama Jun 13, 2020

rajansandeep Jun 12, 2020 •

edited

Loading

rajansandeep Jun 12, 2020 •

edited

Loading

rajansandeep Jun 12, 2020

somtochiama Jun 13, 2020

justinsb Jun 19, 2020 •

edited

Loading

rajansandeep Jun 19, 2020

justinsb Jun 19, 2020

justinsb Jun 19, 2020 •

edited

Loading

justinsb Jun 19, 2020

rajansandeep Jun 19, 2020

somtochiama Jun 20, 2020

justinsb Jun 19, 2020

justinsb Jun 19, 2020

justinsb commented Jun 19, 2020

justinsb commented Jun 29, 2020

justinsb commented Jun 29, 2020

k8s-ci-robot commented Jun 29, 2020

NodeLocalDNS operator #59

NodeLocalDNS operator #59

Conversation

somtochiama commented Jun 4, 2020 • edited Loading

k8s-ci-robot commented Jun 4, 2020

dholbach commented Jun 5, 2020

somtochiama commented Jun 11, 2020

rajansandeep 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

rajansandeep Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

rajansandeep Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Jun 19, 2020 • edited Loading

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

justinsb commented Jun 19, 2020

justinsb commented Jun 29, 2020

justinsb commented Jun 29, 2020

k8s-ci-robot commented Jun 29, 2020

somtochiama commented Jun 4, 2020 •

edited

Loading

rajansandeep Jun 12, 2020 •

edited

Loading

rajansandeep Jun 12, 2020 •

edited

Loading

justinsb Jun 19, 2020 •

edited

Loading

justinsb Jun 19, 2020 •

edited

Loading