KEP-4427: Relaxed DNS search validation: Initial draft

[sethev]

• One-line PR description: Initial draft of KEP for relaxed DNS search string validation

• Issue link: Relaxed DNS search string validation #4427

• Other comments:

[k8s-ci-robot]

Welcome @sethev!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @sethev. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[thockin]

KEP freeze is looming - ping me here when this is ready for re-review.

[sethev]

@thockin ready for re-review

[danwinship]

Based on the updated text, the reason we want this specific change is not about "searching" vs "defining", it's about "hostnames in DNS" vs "non-hostnames in DNS". Theoretically, some future Kubernetes feature might result in CoreDNS creating SRV records based on Kubernetes API fields, and in that case, we might want to allow using underscores in those fields, right?

So the reason we want to change searches and we don't want to change anything else is because searches is (currently) the only API field whose value gets used for things related to non-hostname DNS data.

[sethev]

Yes, i agree with that reasoning. If a future feature introduces that case, I could see having a discussion about whether allowing underscores there encourages good practices, since then the defining vs using distinction would come in (the RFC does point out that sticking to the hostname rules leads to fewer problems, which matches my experience as well).

[thockin]

I'm LGTM on the feature, but the PRR and metadata sections need to be finished.

[sethev]

/retest

[sethev]

@thockin added file to prod-readiness folder (https://github.com/kubernetes/enhancements/pull/4428/files#diff-c3468f7eae017dc2068f76e5a37fa99d19a59f686ecad81e51936210387ab61b)

[thockin]

sorry, I am not PRR approver - @jpbetz @johnbelamaric @wojtek-t or @deads2k - can one of you serve? This one should be easy.

[jpbetz]

Yes, go ahead and put my GitHub handle here and I'll review.

[thockin]

/lgtm

except PRR reviewer

[jpbetz]

Once integration test list is updated and my other minor comments are reviewed I'm happy to approve for PRR

[jpbetz]

nit: validation is behavior, so this is a yes, but we're ratcheting in a widening of validation, which is OK

[jpbetz]

This is where the most important tests should be written. The apiserver can be started in integration with feature flags set and we have confidence that those tests mirror what will happen in production. Let's list out the test cases we need to validate the ratcheting here:

• feature gate on
• feature gate off
• feature gate on, name with_ written, feature gate off, updates still work that change other fields or that change name to something without _, (what if the name is changed to a new value with _?)
• ...

[jpbetz]

Very minor point here, but they could write a resource with _ and see that the write succeeds.

[danwinship]

/hold cancel
lgtm too, just needs PRR updates

[sethev]

@jpbetz @danwinship @thockin updated integration test scenarios and corrected the PRR reviewer.

[danwinship]

/lgtm

[aojea]

any libc or muslc possible problems?

[sethev]

Not beyond what already exists (which is that different implementations of libc may have different length restrictions for the total - all search string lengths added together). I checked glibc and muslc and they don't validate anything besides the length when parsing resolv.conf (same is true of golang's resolver).

[jpbetz]

/approve
For PRR for alpha (I see some beta PRR questions are also answered, we're review those when we promote to beta)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, sethev, thockin

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:

• keps/prod-readiness/OWNERS [jpbetz]
• keps/sig-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