kubernetes / kubernetes

Production-Grade Container Scheduling and Management
https://kubernetes.io
Apache License 2.0
110.35k stars 39.47k forks source link

dnsConfig applies RFC-1123 too broadly to searches #92889

Open sethev opened 4 years ago

sethev commented 4 years ago

What would you like to be added:

RFC-1123 defines restrictions to host names. In Kubernetes, these restrictions are also applied to object names.

However, for search strings in dnsConfig, names that are perfectly valid according to RFC 2181 Section 11 are rejected.

An example of this is using underscores in a subdomain:

apiVersion: v1
kind: Pod
metadata:
  namespace: default
  name: dns-example
spec:
  containers:
    - name: test
      image: nginx
  dnsPolicy: "None"
  dnsConfig:
    nameservers:
      - 1.2.3.4
    searches:
      - abc_d.example.com

Since this is specifying a DNS search string specifically and not the name of an object in Kubernetes, it seems like this check could be relaxed to allow names that comply with RFC 2181.

Instead this gives the following error:

The Pod "dns-example" is invalid: spec.dnsConfig.searches[0]: Invalid value: "abc_d.example.com": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

Why is this needed:

Workloads that rely on short names can take time to change, which is why the dnsConfig is useful. However, in our case most of the names we need in the search have an underscore. Our current work-around is to re-write resolv.conf from inside the pod.

sethev commented 4 years ago

/sig network

athenabot commented 4 years ago

/triage unresolved

Comment /remove-triage unresolved when the issue is assessed and confirmed.

šŸ¤– I am a bot run by vllry. šŸ‘©ā€šŸ”¬

gaurimadhok commented 4 years ago

/assign @aojea

sethev commented 4 years ago

I can contribute a patch for this if it's something that people are open to including in k8s.

aojea commented 4 years ago

interesting, this seems that is being validated here for the RFC1123

https://github.com/kubernetes/kubernetes/blob/205d5c58296357365bfa834ecabb384c1c1042c3/pkg/apis/core/validation/validation.go#L2884-L2889

IĀ“m not a DNS expert but seems you are right, the search field contain domains names, so it should be possible to relax the validation https://linux.die.net/man/5/resolv.conf

I just was curious and tested it in my laptop, and it works

00:19:47.140623 IP (tos 0x0, ttl 64, id 54856, offset 0, flags [DF], proto UDP (17), length 60)
    172.18.0.5.54560 > 192.168.1.1.53: 58361+ A? asd.asdf_2.com. (32)

Let ask one DNS expert, @chrisohaver what do you think?

chrisohaver commented 4 years ago

Yes the "DNS1123" label regex in the k8s code base doesn't actually match RFC1123's definition (which is very permissive).

const dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
const dns1123LabelErrMsg string = "a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character"

IIUC, the set of regexes and funcs are used to validate k8s object names and namespaces, but not for validating domain names. The names on all the related functions and consts are misleading - perhaps they should be renamed - or at least documented that they don't actually follow RFC1123.

Per k8s docs, k8s objects names must be valid DNS labels. Technically this is true, since we validate a subset of valid dns labels - all k8s object names are valid DNS labels. However I don't think the intent is to allow any valid DNS label to be a k8s object name.

sethev commented 4 years ago

Thanks @chrisohaver. What i would propose on this issue is to only relax the check on the search strings in dnsConfig. AFAIK, that's the only place where a domain name is present in K8s config that's being validated using dns1123LabelFmt. Changing how object names are validated would definitely be out of scope for this.

sethev commented 4 years ago

@thockin what do you think on this issue?

athenabot commented 4 years ago

@aojea If this issue has been triaged, please comment /remove-triage unresolved.

If you aren't able to handle this issue, consider unassigning yourself and/or adding the help-wanted label.

šŸ¤– I am a bot run by vllry. šŸ‘©ā€šŸ”¬

bridgetkromhout commented 4 years ago

/assign @thockin

thockin commented 4 years ago

I am OK to loosen this for searches. We have to do it in 2 steps though. First we cut a release that allows the new extended names to persist across updates (but not net-new usage), then we cut a release that allows new usage.

This is small, but still warrants a KEP. It should be an easy-ish one to write,

Volunteers?

sethev commented 4 years ago

thanks @thockin - i can take a stab at the KEP!

thockin commented 4 years ago

Hi @sethev - I pinged the PR (#92979) also. Is this something you still want to pursue?

sethev commented 4 years ago

Yeah, it is.i just haven't found time to work on the KEP yet.

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fejta-bot commented 3 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle rotten

fejta-bot commented 3 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community. /close

k8s-ci-robot commented 3 years ago

@fejta-bot: Closing this issue.

In response to [this](https://github.com/kubernetes/kubernetes/issues/92889#issuecomment-782919842): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >Send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
sethev commented 9 months ago

@thockin I missed the last SIG Networking meeting and it looks like there isn't one for today. Are you still willing to re-open this if i draft a KEP for it?

danwinship commented 9 months ago

However, for search strings in dnsConfig, names that are perfectly valid according to RFC 2181 Section 11 are rejected.

RFC 2181 does not say "domain names don't need to obey RFC 1123", it says "DNS entries don't need to obey RFC 1123 because DNS is allowed to contain things that aren't domain names". resolv.conf(5) says that search is the "Search list for host-name lookup" so that sounds to me like it should be required to obey RFC 1123.

Are you doing lookups of non-hostname DNS records, that make use of the search list?

danwinship commented 9 months ago

If we're revisiting hostname-ish validation, note that in addition to ValidateDNS1123Subdomain we also have validation.IsFullyQualifiedDomainName and validation.IsFullyQualifiedName, and also a recurring pattern in pkg/apis/core/validation of doing IsDNS1123Subdomain && !IsValidIP because IsDNS1123Subdomain allows IPv4 addresses and there are places that want to assert "this is definitely a hostname, not an IP address".

So, maybe we could try to improve that too...

thockin commented 9 months ago

From sig-net call: @sethev to prep a small KEP to cover scopeing and safe introduction of this (via a gate).

sethev commented 8 months ago

Opened a PR with the KEP draft here: https://github.com/kubernetes/enhancements/pull/4428

sethev commented 8 months ago

KEP approved with 1.30 alpha milestone: https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/4427-relaxed-dns-search-validation

danwinship commented 8 months ago

/remove-lifecycle rotten /triage accepted