Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support running a nodelocal dns cache #70555

Merged
merged 2 commits into from
Nov 14, 2018
Merged

Support running a nodelocal dns cache #70555

merged 2 commits into from
Nov 14, 2018

Conversation

prameshj
Copy link
Contributor

@prameshj prameshj commented Nov 1, 2018 •
edited
Loading

This change includes the yaml files and gce startup script changes
to run this addon. It is disabled by default, can be enabled by setting
KUBE_ENABLE_NODELOCAL_DNS=true
An ip address is required for the cache instance to listen for
requests on, default is a link local ip address of value 169.254.25.10

Cluster with nodelocaldns running can be created using:
KUBE_ENABLE_NODELOCAL_DNS=true go run hack/e2e.go -v --up

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Implementation of https://github.com/kubernetes/community/blob/master/keps/sig-network/0030-nodelocal-dns-cache.md
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Introduces support for running a nodelocal dns cache. It is disabled by default, can be enabled by setting KUBE_ENABLE_NODELOCAL_DNS=true
An ip address is required for the cache instance to listen for requests on, default is a link local ip address of value 169.254.20.10

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubeadm needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 1, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2018
@MrHohn
Copy link
Member

MrHohn commented Nov 1, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 1, 2018
@prameshj
Copy link
Contributor Author

prameshj commented Nov 2, 2018

/retest

@neolit123
Copy link
Member

/kind feature
/priority important-longterm
/sig network

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 2, 2018
@prameshj
Copy link
Contributor Author

prameshj commented Nov 2, 2018

/assign @bowei

@timothysc
Copy link
Member

/hold

@roberthbailey - google needs to make certain that these changes are more broadly disseminated and outlined as a default for every deployment.

@prameshj - is there a proposal on this?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2018
roberthbailey
roberthbailey reviewed Nov 13, 2018
View reviewed changes
Copy link
Contributor

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

cluster/centos/util.sh Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2018
mikedanese
mikedanese reviewed Nov 13, 2018
View reviewed changes
cluster/addons/dns/nodelocaldns/Makefile Outdated Show resolved Hide resolved
cluster/addons/dns/nodelocaldns/nodelocaldns.yaml.base Outdated Show resolved Hide resolved
cluster/addons/dns/nodelocaldns/nodelocaldns.yaml.in Outdated Show resolved Hide resolved
cluster/addons/dns/nodelocaldns/nodelocaldns.yaml.in Outdated Show resolved Hide resolved
cluster/addons/dns/nodelocaldns/nodelocaldns.yaml.in Outdated Show resolved Hide resolved
cluster/centos/deployAddons.sh Outdated Show resolved Hide resolved
hack/local-up-cluster.sh Outdated Show resolved Hide resolved
hack/local-up-cluster.sh Outdated Show resolved Hide resolved
hack/local-up-cluster.sh Outdated Show resolved Hide resolved
cluster/gce/gci/configure-helper.sh Outdated Show resolved Hide resolved
@mikedanese mikedanese removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2018
@mikedanese
Copy link
Member

My biggest concern is this #70555 (comment)

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 14, 2018
mikedanese
mikedanese reviewed Nov 14, 2018
View reviewed changes
Copy link
Member

@mikedanese mikedanese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you verify hack/local-up-cluster.sh with and without enabling local DNS and verify that GCE works with enabling local DNS?

Otherwise
lgtm
/approve

cluster/gce/gci/configure-helper.sh Outdated
local -r localdns_file="${dst_dir}/dns/nodelocaldns/nodelocaldns.yaml"
mv "${dst_dir}/dns/nodelocaldns/nodelocaldns.yaml.sed" "${localdns_file}"
# Replace the sed configurations with variable values.
sed -i -e "s/\\\$DNS_DOMAIN/${DNS_DOMAIN}/g" "${localdns_file}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, I'd recommend sticking with the DNS_DOMAIN format so you don't have to worry about escaping.

hack/local-up-cluster.sh
@@ -704,7 +706,11 @@ function start_kubelet {
mkdir -p "/var/lib/kubelet" &>/dev/null || sudo mkdir -p "/var/lib/kubelet"
# Enable dns
if [[ "${ENABLE_CLUSTER_DNS}" = true ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage of == vs = in this file is confusing but I see we do this elsewhere in this file.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, madhusudancs, mikedanese, prameshj, roberthbailey

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:

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

@prameshj
Copy link
Contributor Author

/retest

@prameshj
Removed .salt template , using .sed template
6d7c5e9 
Removed default config options from yaml.
Removed unused yaml files
@prameshj
Copy link
Contributor Author

@mikedanese Verified that gce cluster comes up with cache enabled.
hack/local-up-cluster.sh worked.
"KUBE_ENABLE_NODELOCAL_DNS=true hack/local-up-cluster.sh" failed with "disallowed by cluster policy" error since node-local-dns runs in privileged mode and the local kubelet does not get the --allow-privileged flag. So this is working as intended.

@mikedanese
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2018
@prameshj
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 79dab47 into kubernetes:master Nov 14, 2018
@prameshj prameshj mentioned this pull request Nov 20, 2018
Closed
@krmayankk
Copy link

does this work on GKE as well ? How are we solving the issue of connecting to local dnsmasq from the kubernetes pods ?

@prameshj
Copy link
Contributor Author

does this work on GKE as well ? How are we solving the issue of connecting to local dnsmasq from the kubernetes pods ?

Yes, this will be available on GKE once version 1.13 is released. Not sure i understand the second question - The pods will connect to the dns cache instance because that will be the configured server in /etc/resolv.conf. This is the same way it works in OpenSource.

@blakebarnett
Copy link

blakebarnett commented Nov 29, 2018 •
edited
Loading

@krmayankk It works on GKE because it redirects requests for the well-known DNS service IP (x.x.x.10) to the coredns process running locally as a daemonset using iptables. You can see the code for how it works here: kubernetes/dns#270

This was referenced Dec 4, 2018
Closed
Closed
@superseb superseb mentioned this pull request Dec 4, 2018
Closed
@cameronattard cameronattard mentioned this pull request Dec 18, 2018
Closed
This was referenced Feb 14, 2019
Merged
Closed
@shekhar-rajak shekhar-rajak mentioned this pull request Feb 7, 2021
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madhusudancs madhusudancs madhusudancs left review comments

@justinsb justinsb justinsb left review comments

@neolit123 neolit123 neolit123 left review comments

@bowei bowei bowei left review comments

@liggitt liggitt liggitt left review comments

@mikedanese mikedanese mikedanese left review comments

@thockin thockin thockin left review comments

@roberthbailey roberthbailey roberthbailey left review comments

@chuckha chuckha Awaiting requested review from chuckha

Assignees

@bowei bowei

@mikedanese mikedanese

@thockin thockin

@roberthbailey roberthbailey

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.13
Development

Successfully merging this pull request may close these issues.

None yet