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

Issue API warnings when workload names are not DNS labels #114412

Merged

Conversation

thockin
Copy link
Member

@thockin thockin commented Dec 11, 2022

Workload APIs like Deployment use their own name to ultimately generate pod names. Pod names are ALLOWED to be DNS Subdomains, but when we use the pod name as the pod's hostname, we are sort of in conflict. Setting the pod hostname (internally not thru API) to something like "foo.bar" is poorly defined and the hostname field doesn't allow it at all.

Fixes #104195
xref #106732

/kind feature
/kind api-change
/sig apps

Special notes for your reviewer:

Example:

$ k create -f /tmp/bad.yaml 
Warning: metadata.name: a DNS label is recommended: [a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name',  or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')]

I don't know if there is a convention for these error messages.

Added warnings about workload resources (Pods, ReplicaSets, Deployments, Jobs, CronJobs, or ReplicationControllers) whose names are not valid DNS labels.
Creating Pods, ReplicaSets, Deployments, Jobs, CronJobs, or ReplicationControllers whose `metadata.name` field is *not* a valid [DNS label](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names) will now get [API warnings](https://blog.k8s.io/2020/09/03/warnings/).   All previously valid names and API operations are still allowed - these are only warnings.

For example, naming a Deployment `foo.bar` (a DNS subdomain) now returns a  warning, whereas `foobar` or `foo-bar` (DNS labels) do not.  For backwards compatibility, the Kubernetes API for these resources allows the `metadata.name` field to be a [DNS subdomain](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names).  DNS labels are a subset of DNS subdomains.  All DNS label names are valid DNS subdomain names, but using a DNS subdomain that is not _also_ a valid DNS label (e.g. a name which includes dots) can produce unexpected behaviors when used as a pod's hostname.

See [Object Names and IDs](https://k8s.io/docs/concepts/overview/working-with-objects/names/) for more information.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 11, 2022
@thockin thockin added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Dec 11, 2022
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 11, 2022
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@aojea
Copy link
Member

aojea commented Dec 12, 2022

/assign @soltysh @liggitt

@liggitt
Copy link
Member

liggitt commented Dec 14, 2022

This PR is marked as fixing #104195, but it doesn't look like it touches statefulset... does it really fix that issue or is it just related?

Are statefulsets more impacted by this than the other types this PR is warning about (to the point where it might deserve an actual validation error on statefulset)?

@thockin
Copy link
Member Author

thockin commented Dec 14, 2022

Statefulset was fixed by #114172

@sftim
Copy link
Contributor

sftim commented Dec 14, 2022

@thockin, I can't work out what you intend for the release note text to be here. This change will need a short release note summary and would benefit from some extra notes to accompany the summary.

We could also update https://kubernetes.io/docs/concepts/workloads/controllers/deployment/ and similar pages to include advice so that an informed reader doesn't see such a warning.
If this merges, please consider filing an issue against k/website, so that we can track the need to document this for the upcoming release.

@thockin
Copy link
Member Author

thockin commented Dec 15, 2022

@sftim I thought the release-note I wrote was pretty clear - help me clarify?

@sftim
Copy link
Contributor

sftim commented Dec 15, 2022

What's the change here? I can guess from the PR title, but I'm not sure that the difference between before and after will be obvious to someone reading just the changelog.

It's OK to provide an example of the new behavior but we first need to explain the difference between before and afterwards.

@thockin
Copy link
Member Author

thockin commented Dec 15, 2022

"""
Pods, ReplicationControllers, ReplicaSets, Deployments, Jobs, and CronJobs allow their metadata.name field to be a DNS Subdomain (e.g. foo.bar), which can produce unexpected behaviors when used as a pod's hostname. Creating an instance of these types with name that is not a DNS Label (e.g. foobar, foo-bar) will get API warnings recommending the stricter format. All existing API operations are still allowed - these are only warnings.
"""

Better to lead with the behavior change?

"""
Creating Pods, ReplicationControllers, ReplicaSets, Deployments, Jobs, and CronJobs whose metadata.name field is not a DNS Label (e.g. foobar, foo-bar) will now get API warnings. These types allow their metadata.name field to be a DNS Subdomain (e.g. foo.bar), but this can produce unexpected behaviors when used as a pod's hostname. All existing API operations are still allowed - these are only warnings.
"""

@sftim
Copy link
Contributor

sftim commented Dec 16, 2022

I'd consider:

Creating Pods, ReplicaSets, Deployments, Jobs, CronJobs and ReplicationControllers whose `metadata.name`
field is *not* a valid DNS label will now get API warnings. For example, naming a CronJob `foo.bar.example`
now returns a warning. Naming a CronJob `foobar` or `foo-bar-example` does not return a warning because
those names are valid DNS labels.
The Kubernetes API for these API kinds requires that the `metadata.name` field is a DNS subdomain
name. All DNS label names are valid DNS subdomain names, but using a DNS subdomain that is not also
a valid label can produce unexpected behaviors when used as a pod's hostname.
All existing API operations are still allowed - these are only warnings.

or write a summary and use the option for further details:

Creating Pods, ReplicaSets, Deployments, Jobs, CronJobs and ReplicationControllers whose
`metadata.name` field is *not* a valid DNS label will now get API warnings.
All existing API operations are still allowed - these are only warnings.
For example, naming a CronJob `foo.bar.example` now returns a
[warning](https://blog.k8s.io/2020/09/03/warnings/).
Naming a CronJob `foobar` or `foo-bar-example` does not return a warning because
those names are valid DNS labels. The Kubernetes API for these API kinds enforces that the
`metadata.name` field is a DNS _subdomain_ name. All DNS label names are valid DNS
subdomain names, but using a DNS subdomain that is not also a valid label can produce
unexpected behaviors when used as a pod's hostname.

See [Object Names and IDs](https://k8s.io/docs/concepts/overview/working-with-objects/names/)
for more information.

I don't know the exact mechanism for changelogs but I've seen that there is space for adding additional details to a changelog entry, and this second option feels like a good way to use that.

Finally, what about StatefulSets?

@liggitt
Copy link
Member

liggitt commented Dec 16, 2022

Finally, what about StatefulSets?

statefulsets were handled differently and separately in #114172

@thockin
Copy link
Member Author

thockin commented Dec 16, 2022

PTAL at relnote

This will produce a better error message for the more common case of
using a DNS subdomain where a label is needed.
@thockin thockin force-pushed the api_warn_workloads_name_not_dnslabel branch from 5a3c94a to c722ae4 Compare December 16, 2022 21:02
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Dec 16, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@thockin thockin force-pushed the api_warn_workloads_name_not_dnslabel branch from c722ae4 to 8f62b94 Compare December 16, 2022 21:08
@liggitt
Copy link
Member

liggitt commented Dec 16, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2022
@sftim
Copy link
Contributor

sftim commented Dec 17, 2022

For these release notes, I'd expect to see two blocks. One block that starts with starting ```release-note block and another starting ```docs. That second block is there to give us room for more info.

Something like:

```release-note
Added warnings about workload names that are not valid DNS labels.
```
```docs
Creating Pods, ReplicaSets, Deployments, Jobs, CronJobs, or ReplicationControllers whose
`metadata.name` field is *not* a valid
[DNS label](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names)
will now get [API warnings](https://blog.k8s.io/2020/09/03/warnings/).
All previously valid names and API operations are still allowed - these are only warnings.

For example, naming a Deployment `foo.bar` (a DNS subdomain) now returns a  warning,
whereas `foobar` or `foo-bar` (DNS labels) do not.  For backwards compatibility, the Kubernetes
API for these resources allows the `metadata.name` field to be a
[DNS subdomain](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names).
 DNS labels are a subset of DNS subdomains.  All DNS label names are valid DNS subdomain names,
but using a DNS subdomain that is not _also_ a valid DNS label (e.g. a name which includes dots)
can produce unexpected behaviors when used as a pod's hostname.

See [Object Names and IDs](https://k8s.io/docs/concepts/overview/working-with-objects/names/)
for more information.
```

Splitting in a different place is OK. Our guidance doesn't include an example with additional docs so I'm not sure what represents good practise.
I also don't know where the information in the docs part ends up.

Also, in that example, I changed “All existing names“ to “All previously valid names”; I wanted to make it more obvious that any create that might previously have been acceptable is still acceptable after this change. I don't want readers to wrongly infer that only objects that existed before the upgrade are exempt from the validation change.

@k8s-ci-robot k8s-ci-robot merged commit 2f2021e into kubernetes:master Dec 17, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Dec 17, 2022
@thockin
Copy link
Member Author

thockin commented Dec 19, 2022

Edited as suggested, we'll see what release folks think as the release notes get assembled. :)

@sftim
Copy link
Contributor

sftim commented Dec 19, 2022

Thanks for all the edits!

@cici37
Copy link
Contributor

cici37 commented Dec 20, 2022

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 20, 2022
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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StatefulSet name accepts DNS subdomain standard, but Pods controlled by StatefulSet accepts DNS label standard
8 participants