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

Fix translation from kube-dns to CoreDNS Config to skip invalid values #75969

Merged
merged 1 commit into from Apr 2, 2019

Conversation

@rajansandeep
Copy link
Member

commented Apr 1, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:
Kube-dns now accepts service name for stubdomain and nameserver, while CoreDNS does not support this feature.

As a result, a user having service name in stubdomains/nameserver in their kube-dns ConfigMap while migrating to CoreDNS will result in an invalid CoreDNS Config, resulting in a broken k8s cluster.

The translation will now detect a service name and omit it while translating to the equivalent CoreDNS Config and additionally warn the user that it has been omitted during translation since it's not supported.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1473

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

 `StubDomains` and `Upstreamnameserver` which contains a service name will be omitted while translating to the equivalent CoreDNS config.
@rajansandeep

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

/priority important-soon
/cc @neolit123

@rajansandeep

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

@k8s-ci-robot k8s-ci-robot requested a review from chrisohaver Apr 1, 2019

@rajansandeep rajansandeep changed the title Fix translation to skip invalid values Fix translation from kube-dns to CoreDNS Config to skip invalid values Apr 1, 2019

}
}
if isInvalid {
continue

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Apr 1, 2019

Contributor

Looks like if one upstream server is not an IP, we omit the entire stub-domain.
Intentional? Should we instead omit only the hostname nameserver, and only drop the entire stubdomain definition if there are zero IP nameservers for it present?

fmt.Printf("[WARNING]: Your upstreamnameserver Config contains a hostname %v, it will be omitted from the translation.\n", upstreamProxyHost)
}
}
if index != -1 {

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Apr 1, 2019

Contributor

This will only omit the last instance of a hostname nameserver. IOW, If there are two, it would leave the first one in there.

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Apr 1, 2019

Contributor

Unrelated, but shouldn't this function be called ....

translateUpstreamNameServerOfKubeDNSToForwardCoreDNS instead of ...

translateUpstreamNameServerOfKubeDNSToUpstreamProxyCoreDNS

@chrisohaver
Copy link
Contributor

left a comment

lgtm

@neolit123
Copy link
Member

left a comment

thanks for the fix @rajansandeep
added one minor comment.

please squash the commits after and should be LGTM.
@kubernetes/sig-cluster-lifecycle-pr-reviews

}
parseIP := net.ParseIP(proxyHost)
if parseIP == nil {
fmt.Printf("[WARNING]: Your Config contains a hostname %v, it will be omitted from the translation.\n", proxyHost)

This comment has been minimized.

Copy link
@neolit123

neolit123 Apr 1, 2019

Member

can we please use something in these lines here:

klog.Warningf("your kube-dns configuration contains a hostname %v. It will be omitted in the translation to CoreDNS as hostnames are unsupported", proxyHost)

This comment has been minimized.

Copy link
@rajansandeep

rajansandeep Apr 1, 2019

Author Member

Done!

@rajansandeep rajansandeep force-pushed the rajansandeep:translationcheck branch from a9a567e to deadefd Apr 1, 2019

@neolit123
Copy link
Member

left a comment

/lgtm
/approve
thanks.

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 1, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, rajansandeep

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 c3cc317 into kubernetes:master Apr 2, 2019

17 checks passed

cla/linuxfoundation rajansandeep authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.