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 the issue in Windows kube-proxy when processing unqualified name. This is for DNS client such as ping or iwr that validate name in response and original question. #45642

Merged
merged 3 commits into from
May 18, 2017

Conversation

JiangtianLi
Copy link
Contributor

What this PR does / why we need it:
This PR is an additional fix to #41618 and the corresponding commit. The DNS client such as nslookup does not validate name matching in response and original question. That works fine when we append DNS suffix to unqualified name in DNS query in Windows kube-proxy. However, for DNS client such as ping or Invoke-WebRequest that validates name in response and original question, the issue arises and the DNS query fails although the received DNS response has no error.

This PR fixes the additional issue by restoring the original question name in DNS response. Further, this PR refactors DNS message routines by using miekg's DNS library.

This PR affects the Windows kube-proxy only.

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

Special notes for your reviewer:

Release note:

Fix DNS suffix search list support in Windows kube-proxy.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 11, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @JiangtianLi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@JiangtianLi
Copy link
Contributor Author

This PR addresses the comment in #44872 since I closed #44872 due to my bad rebase.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 11, 2017
@JiangtianLi
Copy link
Contributor Author

One side note, with rebasing the latest master, building Windows kube-proxy is broken due to #45554.

@fejta
Copy link
Contributor

fejta commented May 12, 2017

Thanks for the fix!

Networking team: kubernetes is broken on windows, and our cross-build suite is failing. Will someone from sig-networking review?

@k8s-bot ok to test
@kubernetes/sig-network-pr-reviews

/assign @bowei @thockin @dcbw
/unassign @lavalamp

@k8s-ci-robot k8s-ci-robot assigned bowei, dcbw and thockin and unassigned lavalamp May 12, 2017
@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 12, 2017
Copy link
Contributor

@pires pires left a comment

Choose a reason for hiding this comment

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

LGTM but I didn't got a chance to test it manually on a Windows node.

@JiangtianLi
Copy link
Contributor Author

JiangtianLi commented May 15, 2017

Thanks all for the review. The change has been patched in my private branch for 1.6.0 and 1.6.2. Once cross-build issue is fixed, we can verify with the latest build.

@thockin
Copy link
Member

thockin commented May 17, 2017

I have NO IDEA what this code is doing - no other proxy mode has anything DNS related - can you explain what is happening here? Did you build a DNS server into kube-proxy?

@thockin
Copy link
Member

thockin commented May 17, 2017

I'm going to approve this to unblock, but I'd like to discuss what the heck is going on here.

@thockin
Copy link
Member

thockin commented May 17, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JiangtianLi, thockin

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2017
@JiangtianLi
Copy link
Contributor Author

JiangtianLi commented May 17, 2017

@thockin Sorry, I should have more context here. This PR is an additional fix to #41618 and the corresponding commit. There is no new DNS server built into kube-proxy.

The issue this PR and #41618 is trying to fix is that DNS suffix search list doesn't work in Windows container (--dns-search option from this link). Therefore lookup unqualified name such as kubernetes or any other unqualified service name doesn't work in Windows container. The users have to specify or change to full qualified name in their apps in Windows container to reach service endpoint.

To overcome that and until Windows releases container that fix DNS suffix search list issue, we have come up with a solution to fix in Windows kube-proxy. The idea is to have kubeproxy do what DNS client would do to search DNS suffix search list and append it to query, i.e., intercept DNS packet and append a list of common DNS suffix ("cluster.local", "svc.cluster.local", "default.svc.cluster.local", and host DNS suffix) to the name in DNS query. This has worked pretty well for most Windows user scenarios.

Hope that clarifies a bit. Please let me know if you have any question.

@bowei
Copy link
Member

bowei commented May 17, 2017

@JiangtianLi does that code belong in kube-proxy? It seems like something that could be a separate Windows only module that implements the *nix style resolv.conf behavior.

@JiangtianLi
Copy link
Contributor Author

@bowei Thanks for the suggestion. Yes, that code belongs to kube-proxy. The code is in the winuserspace part of the kube-proxy. So it is in Windows kube-proxy and separated from the *nix kube-proxy.

@thockin
Copy link
Member

thockin commented May 18, 2017

Yeah, I find this exceptionally weird, but I am willing to grant latitude since it seems a) temporary and b) contained. Before long we need to really decide if it is worthwhile for the Window kube-proxy to actually be part of kube-proxy, or a separate thing.

@thockin
Copy link
Member

thockin commented May 18, 2017

@k8s-bot gce etcd3 e2e test this

@JiangtianLi
Copy link
Contributor Author

@thockin Agreed that this fix is a workaround to get Windows scenario work. Windows's kube-proxy forked from kube-proxy and implemented a few Windows specific features. sig-windows folks should have more context. Thanks for the approval. I felt this fix should be with its precedent commit in the upstream rather than in my private branch separately.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/network Categorizes an issue or PR as relevant to SIG Network. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS resolving does not work on windows nodes