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.

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

kubeadm: Create control plane with ClusterFirstWithHostNet dns policy #68890

Merged
merged 1 commit into from
Sep 27, 2018

Conversation

andrewrynhard
Copy link
Contributor

@andrewrynhard andrewrynhard commented Sep 20, 2018

What this PR does / why we need it:
Currently, the static pods created by kubeadm use the ClusterFirst dns policy. This prevents doing things like setting the oidc-issuer-url flag of the API Server to a Kubernetes service. By using the ClusterFirstWithHostNet policy the API Server will be able to resolve the service DNS.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Special notes for your reviewer:

Release note:

kubeadm: create control plane with ClusterFirstWithHostNet DNS policy

/cc sig-cluster-lifecycle

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 20, 2018
@andrewrynhard
Copy link
Contributor Author

/test pull-kubernetes-integration

@andrewrynhard
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@andrewrynhard
Copy link
Contributor Author

/assign @fabriziopandini

@andrewrynhard
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@neolit123
Copy link
Member

@andrewrynhard
thanks for the PR!

/kind feature

this might not get reviews for a while due to the 1.12 release closing by.

for reference:
https://godoc.org/k8s.io/kubernetes/pkg/apis/core#DNSPolicy

the addition makes sense, except that we need to verify that there are no unwanted sides effects.

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/cc @rajansandeep @chrisohaver
for feedback as well.

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 20, 2018
@k8s-ci-robot
Copy link
Contributor

@neolit123: GitHub didn't allow me to request PR reviews from the following users: chrisohaver.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@andrewrynhard
thanks for the PR!

/kind feature

this might not get reviews for a while due to the 1.12 release closing by.

for reference:
https://godoc.org/k8s.io/kubernetes/pkg/apis/core#DNSPolicy

the addition makes sense, except that we need to verify that there are no unwanted sides effects.

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/cc @rajansandeep @chrisohaver
for feedback as well.

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.

@k8s-ci-robot k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Sep 20, 2018
@neolit123
Copy link
Member

@andrewrynhard
please add a release note in the lines of:

kubeadm: create control plane with ClusterFirstWithHostNet DNS policy

thanks

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 20, 2018
@fabriziopandini
Copy link
Member

@andrewrynhard thanks for this PR! I think this is a valuable change
/approve

Being this change on all the control plane components, I left lgtm pending for a second opinion.
/cc @detiber @chuckha @timothysc

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2018
@andrewrynhard
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@stealthybox
Copy link
Member

I'm a bit confused on what the current behavior is and why it fails:

    // DNSClusterFirst indicates that the pod should use cluster DNS
    // first unless hostNetwork is true, if it is available, then
    // fall back on the default (as determined by kubelet) DNS settings.
    DNSClusterFirst DNSPolicy = "ClusterFirst"

Even if kube-dns is not up yet, won't the default kubelet settings just get used?

Is there an issue that describes this in more detail?

@andrewrynhard
Copy link
Contributor Author

andrewrynhard commented Sep 23, 2018

@stealthybox it is a bit confusing. And I'm not sure I understand either. I just know that service DNS did not resolve until I switched to self-hosted and that uses ClusterFirstWithHostNet. The description in https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/ says

For Pods running with hostNetwork, you should explicitly set its DNS policy

EDIT:
On second thought, I think I do understand this :). The snippet you posted says

unless hostNetwork is true

which is true in the case of the control plane. The default policy in this case is describe here. ClusterFirstWithHostNet will allow ClusterFirst functionality for pods with host net.

@stealthybox
Copy link
Member

Thanks for figuring that out @andrewrynhard

Looks like this modification is sufficient to cover the etcd, apiserver, KCM, and scheduler:
https://sourcegraph.com/github.com/kubernetes/kubernetes@8f6ec989e006f53c98c107bca17a1d2aeaa82bfa/-/blob/cmd/kubeadm/app/util/staticpod/utils.go#L52:6&tab=references

+1
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewrynhard, fabriziopandini, stealthybox

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

@k8s-ci-robot k8s-ci-robot merged commit e9fe3f7 into kubernetes:master Sep 27, 2018
@andrewrynhard andrewrynhard deleted the dnspolicy branch September 27, 2018 03:52
@BenTheElder
Copy link
Member

The actual functionality for these options is here:

case v1.DNSClusterFirstWithHostNet:
return podDNSCluster, nil
case v1.DNSClusterFirst:
if !kubecontainer.IsHostNetworkPod(pod) {
return podDNSCluster, nil
}
// Fallback to DNSDefault for pod on hostnetowrk.
fallthrough
case v1.DNSDefault:
return podDNSHost, nil
}

I think this broke my funky docker-in-docker local cluster setup, trying to figure out why :^)

@BenTheElder
Copy link
Member

BenTheElder commented Sep 27, 2018

AFAICT this is basically equivalent to setting these pods to only use the in-cluster DNS:

case podDNSCluster:
if len(c.clusterDNS) != 0 {
// For a pod with DNSClusterFirst policy, the cluster DNS server is
// the only nameserver configured for the pod. The cluster DNS server
// itself will forward queries to other nameservers that is configured
// to use, in case the cluster DNS server cannot resolve the DNS query
// itself.
dnsConfig.Servers = []string{}
for _, ip := range c.clusterDNS {
dnsConfig.Servers = append(dnsConfig.Servers, ip.String())
}
dnsConfig.Searches = c.generateSearchesForDNSClusterFirst(dnsConfig.Searches, pod)
dnsConfig.Options = defaultDNSOptions
break
}

Not sure how this is supposed to work for bootstrapping?
Looks like maybe the API server cannot talk to itself via localhost (not the IP) because the DNS is not up yet.. cc @MrHohn

@MrHohn
Copy link
Member

MrHohn commented Sep 27, 2018

the API Server will be able to resolve the service DNS

Does kubeadm always deploy kube-proxy on master? To be able to send DNS query to kube-dns (coredns), kube-dns's service VIP need to be routable beforehand.

@neolit123
Copy link
Member

Does kubeadm always deploy kube-proxy on master?

yes, it does.

the CI tests for master are green (it runs nodes over GCE):
https://k8s-testgrid.appspot.com/sig-cluster-lifecycle-all#kubeadm-gce-master

but Ben is seeing local failures:
https://gist.github.com/BenTheElder/d1dccf01fa194bbc683fd93a2d6c0d62#file-apiserver-logs-txt

@BenTheElder
Copy link
Member

I don't think we start kube-proxy during control plane bringup.

@BenTheElder
Copy link
Member

Issues are attributable back to #69195 meaning we cannot resolve localhost without DNS in some cases. 😅

@mauilion
Copy link

The apiserver can't use kube-dns as it's only resolver as it's a circular dependency. The apiserver will come up before the kube-dns pods are present and needs to resolve things like etcd to be able to work.

@BenTheElder
Copy link
Member

Right. With #69195 in some cases we fixed /etc/hosts not being respected as a fallback, which was sufficient for my case. For others this might still be problematic.

@mauilion
Copy link

mauilion commented Nov 13, 2018

the resolv.conf for when ClusterFirstWithHostNet is set:

bash-4.3# cat /etc/resolv.conf 
nameserver 10.96.0.10
search default.svc.cluster.local svc.cluster.local cluster.local cisco.com
options ndots:5

This is likely green because the apiserver manifest has the following set:

--kubelet-preferred-address-types=InternalIP,ExternalIP,Hostname
--etcd-servers=https://127.0.0.1:2379

With these settings we would probably make the tests green cause kube-apiserver will be able to resolve what it needs to by using the ip addresses.

If the user has an external etcd cluster this will break.
or if the kubelet-preferred-address-types is the default it will break. Probably how it hit you @BenTheElder

@BenTheElder
Copy link
Member

I don't have external etcd, just have some fun networking (https://sigs.k8s.io/kind), but I suspect this would break external etcd unless the nodes had a hosts entry. cc @MrHohn

@timothysc
Copy link
Member

@mauilion Could you file an issue in the kubeadm repo and we'll assign @chuckha

priority would be critical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm 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. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants