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.

Sign up for GitHub

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

Jump to bottom

federation: updating cluster name validation to allow DNS labels only #30956

Merged
merged 1 commit into from
Aug 19, 2016
Merged

federation: updating cluster name validation to allow DNS labels only #30956

merged 1 commit into from
Aug 19, 2016

Conversation

nikhiljindal
Copy link
Contributor

@nikhiljindal nikhiljindal commented Aug 19, 2016 •
edited
Loading

Forked from #28921

Summarizing the discussion from that PR:

  • Right now, we allow name of the Cluster resource in federation/v1beta1 group version to be a subdomain (group of DNS labels separated by dots). This prevents us from using the cluster name in our dns search paths, since there is a restriction of 6 domain labels there.
  • Restricting cluster name to DNS label will give us the flexibility to be able to do that in the future, if we want to. Though we do not have a concrete use case right now, the possibility is attractive.
  • There is not a strong argument in favor of allowing subdomains as cluster names right now. If in future, there is one then we can get more permissive but its better to start with a stricter model.

Note that we are breaking a beta API, but it should be fine since we do not expect anyone to be using subdomain as cluster name. Have added release-note-action-required label

Action required: federation-only: Please update your cluster name to be a valid DNS label.
Updating federation.v1beta1.Cluster API to disallow subdomains as valid cluster names. Only DNS labels are allowed as valid cluster names now.

This change is Reviewable

@nikhiljindal
Copy link
Contributor Author

cc @kubernetes/sig-cluster-federation @quinton-hoole @bgrant0607 @thockin @lavalamp @smarterclayton

@nikhiljindal nikhiljindal added this to the v1.4 milestone Aug 19, 2016
@nikhiljindal nikhiljindal added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Aug 19, 2016
@nikhiljindal nikhiljindal mentioned this pull request Aug 19, 2016
Merged
@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 19, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 19, 2016

GCE e2e build/test passed for commit 473c1d5.

@madhusudancs
Copy link
Contributor

I would swap the order of the two sentences in the release note to state what is important upfront.

LGTM otherwise. Feel free to add the label after fixing the release note.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@ghost ghost added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 19, 2016
@nikhiljindal
Copy link
Contributor Author

Done, updated the release notes.
Thanks @madhusudancs

@madhusudancs
Copy link
Contributor

madhusudancs commented Aug 19, 2016 •
edited
Loading

@nikhiljindal Adding LGTM label.

I should have noticed this before. Sorry that I am making you change this again. I think the release note section should include which label format. There is an RFC 952 label and an RFC 1123 label. They are slightly different. We should probably also include the regex that matches the label.

Since release note can be edited independently, please edit it even if the PR merges.

@madhusudancs madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2016
@eparis
Copy link
Contributor

eparis commented Aug 19, 2016

Why is this a P2?

@lavalamp lavalamp added retest-not-required and removed priority/backlog Higher priority than priority/awaiting-more-evidence. labels Aug 19, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5fb525a into kubernetes:master Aug 19, 2016
@analytik analytik mentioned this pull request Sep 8, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@madhusudancs madhusudancs

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
7 participants
@nikhiljindal @k8s-bot @madhusudancs @eparis @k8s-github-robot @lavalamp @googlebot