Removes PQDNs from conformance tests and places them in their own tests by nagiesek · Pull Request #70156 · kubernetes/kubernetes

nagiesek · 2018-10-24T02:16:26Z

What type of PR is this?

/kind failing-test

What this PR does / why we need it:
Windows assumes any DNS query PQDN with a "." in it to be fully qualified. It's possible to fix this, but it is likely that doing so would break expectations of applications on Windows. After consulting with SIG-Network, we came to the conclusion that conformance tests shouldn't break this assumption nor be tied to the behavioral nuances of a DNS resolver of a specific OS. We propose to separate the current DNS conformance tests as follows:"

Keep existing test assertions that test PQDN DNS queries not containing ".".
Keep existing test assertions that perform FQDN DNS queries.
For any test assertions that use PQDN DNS queries containing ".":
- Replace those test assertions in the conformance test with an equivalent FQDN DNS query.
- Move those test assertions into a new (non-conformance) test.

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:
/sig testing
/area conformance

Does this PR introduce a user-facing change?:
NONE

k8s-ci-robot · 2018-10-24T02:16:27Z

@nagiesek: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

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 · 2018-10-24T02:16:38Z

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 email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Details

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.

nagiesek · 2018-10-24T02:17:11Z

@daschott @dineshgovindasamy

dineshgovindasamy · 2018-10-24T16:13:05Z

/sig windows

dineshgovindasamy · 2018-10-24T16:13:24Z

@nagiesek can you please sign the CLA?

dineshgovindasamy · 2018-10-24T16:13:40Z

/ok-to-test

dineshgovindasamy · 2018-10-24T16:32:10Z

/sig network

nagiesek · 2018-10-24T20:36:34Z

Should be verified now?

dineshgovindasamy · 2018-10-25T23:39:44Z

@thockin Based on SIG-NETWORK feedback, we have split these tests into conformance and non-conformance tests. Could you please review and provide feedback?

spiffxp · 2018-10-30T21:11:42Z

/retest

daschott · 2018-11-12T23:57:17Z

Even with this fix-up, the 2 DNS tests will not pass because they use the dig cmdlet, which isn't honoring DNS SearchList RegKeys in Windows containers. That being said, we still rely on this getting in (otherwise we would be forced to change the behavior of the DNS resolver itself on Windows).

In a nutshell, to pass the tests on Windows after getting this in, we further need to do one of either alternatives:

If possible, fix up the implementation of the dig tool on Windows to not rely on Linux-specific files (/etc/hosts, see #70189)
(Better) For Windows, decouple the test from the dig cmdlet that isn't an accurate representation of default Windows DNS behavior, and migrate to a tool representative of the user experience such as resolve-dnsname while still capturing the intended test requirement.

thockin · 2018-11-14T08:58:24Z

I'm really torn about this behavior. Almost every example in the world that deals with service resolution in Kubernetes is using shortened examples like kubernetes.default. Without that ability we almost certainly need to be able to plumb the cluster suffix through downward-API.

What is the expected breakage if we actually fixed this in the Windows side?

daschott · 2018-11-14T18:19:40Z

@thockin The DNS resolver is a shared component that is used by applications running on Windows OS in general, so any changes need to be carefully examined. I talked to the Windows DNS team and we filed a work-item for the next Windows release to add a configuration parameter to make the behavior consistent with Linux. I'm not 100% confident this behavior may change by default for fear of possibly breaking networking for existing applications, but there should be some configuration we can set for containers at least.

In the meanwhile, can we make [SIG-Windows] tests that are outside of conformance and use Test-NetConnection or resolve-dnsname Windows cmdlets instead of changing the official conformance tests? The alternative is that we hack around with the dig binary to make it work correctly on Windows, which probably isn't as close of a representation as possible of the use-cases for the typical Windows application :)

thockin · 2019-01-04T21:36:26Z

I guess I can cope with this. We should probably add a downwardAPI for cluster suffix.

thockin · 2019-01-04T21:38:27Z

/lgtm
/approve

PatrickLang · 2019-01-23T01:19:43Z

/milestone v1.14

claudiubelu · 2019-01-29T12:49:17Z

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-integration

claudiubelu · 2019-01-29T13:45:05Z

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-kops-aws

claudiubelu · 2019-01-29T14:26:47Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-godeps

claudiubelu · 2019-01-29T14:28:28Z

/test pull-kubernetes-godeps

claudiubelu · 2019-01-30T12:34:13Z

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-kops-aws

k8s-ci-robot · 2019-01-30T13:43:37Z

@nagiesek: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`1a7e07b`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

claudiubelu · 2019-01-31T12:03:46Z

test/e2e/network/dns.go

+		namesToResolve := []string{
+			"kubernetes.default",
+			"kubernetes.default.svc",
+			"kubernetes.default.svc.cluster",


kubernetes.default.svc.cluster is not supposed to be resolvable. That would mean that local would also be a DNS suffix, which is not. Also, as you can see, you've removed only kubernetes.default and kubernetes.default.svc from should provide DNS for the cluster while you're adding 3 here.

This is causing the test to fail.

k8s-ci-robot · 2019-02-01T21:40:00Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/e2e/network/OWNERS~~ [thockin]

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

dims · 2019-02-05T13:36:29Z

/release-note-none

dims · 2019-02-05T13:37:33Z

re-applying lgtm from @thockin

/lgtm

k8s-ci-robot requested review from MrHohn and nicksardo October 24, 2018 02:16

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 24, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 24, 2018

k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 24, 2018

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Oct 24, 2018

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 Oct 24, 2018

thockin self-assigned this Nov 1, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2018

fturib mentioned this pull request Nov 8, 2018

e2e tests specific Network/DNS related : add Ipv6 capability #59894

Closed

nagiesek added 2 commits November 13, 2018 13:59

Removes PQDNs from conformance tests and places them in their own tests

ee73270

Rebase, fixes format error, removes a host name from confomance test

1a7e07b

daschott mentioned this pull request Nov 27, 2018

Windows networking tests #71468

Merged

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 4, 2019

k8s-ci-robot added this to the v1.14 milestone Jan 23, 2019

claudiubelu reviewed Jan 31, 2019

View reviewed changes

removes incorrect dns name to resolve in dns test

689db1b

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2019

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 5, 2019

k8s-ci-robot assigned dims Feb 5, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2019

k8s-ci-robot merged commit ffec911 into kubernetes:master Feb 5, 2019

claudiubelu mentioned this pull request Feb 20, 2019

REQUEST: New membership for bclau kubernetes/org#509

Closed

6 tasks

mgdevstack mentioned this pull request Feb 28, 2019

Tracking Issue - Conformance Coverage for Networking #73425

Open

This was referenced Mar 8, 2019

Promote DNS e2e verifying PQDN resolution for cluster #74977

Merged

Promote DNS e2e verifying PQDN resolution for services #74980

Closed

Conversation

nagiesek commented Oct 24, 2018

Uh oh!

k8s-ci-robot commented Oct 24, 2018

Uh oh!

k8s-ci-robot commented Oct 24, 2018

Uh oh!

nagiesek commented Oct 24, 2018

Uh oh!

dineshgovindasamy commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dineshgovindasamy commented Oct 24, 2018

Uh oh!

dineshgovindasamy commented Oct 24, 2018

Uh oh!

dineshgovindasamy commented Oct 24, 2018

Uh oh!

nagiesek commented Oct 24, 2018

Uh oh!

dineshgovindasamy commented Oct 25, 2018

Uh oh!

spiffxp commented Oct 30, 2018

Uh oh!

daschott commented Nov 12, 2018

Uh oh!

thockin commented Nov 14, 2018

Uh oh!

daschott commented Nov 14, 2018

Uh oh!

thockin commented Jan 4, 2019

Uh oh!

thockin commented Jan 4, 2019

Uh oh!

PatrickLang commented Jan 23, 2019

Uh oh!

claudiubelu commented Jan 29, 2019

Uh oh!

claudiubelu commented Jan 29, 2019

Uh oh!

claudiubelu commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claudiubelu commented Jan 29, 2019

Uh oh!

claudiubelu commented Jan 30, 2019

Uh oh!

k8s-ci-robot commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claudiubelu Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 1, 2019

Uh oh!

dims commented Feb 5, 2019

Uh oh!

dims commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

dineshgovindasamy commented Oct 24, 2018 •

edited

Loading

claudiubelu commented Jan 29, 2019 •

edited

Loading

k8s-ci-robot commented Jan 30, 2019 •

edited

Loading