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

Namespace certificates API group #31887

Merged
merged 1 commit into from
Sep 2, 2016

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Sep 1, 2016

New API groups should follow best-practices for naming, including using DNS names within the k8s.io namespace

The certificates API group has been renamed to certificates.k8s.io

This change is Reviewable

@k8s-bot
Copy link

k8s-bot commented Sep 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Sep 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@k8s-github-robot k8s-github-robot added kind/new-api size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 1, 2016
@liggitt
Copy link
Member Author

liggitt commented Sep 1, 2016

@k8s-bot ok to test

@mikedanese
Copy link
Member

I'd like to get this in for v1.4

On Thu, Sep 1, 2016 at 8:49 AM, Jordan Liggitt notifications@github.com
wrote:

Assigned #31887 #31887 to
@mikedanese https://github.com/mikedanese.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#31887 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABtFIU782XkhLrEuOSgQdRWUCnftbJPZks5qlvQCgaJpZM4Jy1kC
.

@liggitt liggitt force-pushed the certificates-group branch 3 times, most recently from a89b830 to 07d4ae7 Compare September 1, 2016 17:41
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 1, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 1, 2016
@liggitt liggitt added this to the v1.4 milestone Sep 1, 2016
@mikedanese
Copy link
Member

mikedanese commented Sep 1, 2016

@pwittrock requesting that this be included in v1.4 milestone.

  1. How risky is it to merge this change?

Not risky. This is a rest path change of an API that will be introduced as alpha in v1.4.

  1. How easy is it to roll this change back?

Not hard. This is mostly a rename. Some autogenerated swagger files are touched but the change is isolated to certificates API sections of those files so there won't be merge conflicts.

  1. What's the cost if it doesn't make the 1.4 release?

Even if we wanted to make a compatible new API version of this API, we could not. Although the API is alpha and we reserve the right to break it, we should aim to avoid this if at all possible. Without this change, all API consumers will break on upgrade from v1.4 to v1.5.

cc @gtank

@k8s-bot
Copy link

k8s-bot commented Sep 1, 2016

GCE e2e build/test passed for commit a869de6.

@liggitt
Copy link
Member Author

liggitt commented Sep 1, 2016

tests are green, all the verify scripts are happy

brought up an API server and tested with:

$ kubectl create -f - <<< '{"kind":"CertificateSigningRequest","apiVersion":"certificates.k8s.io/v1alpha1","metadata":{"name":"foo"},"spec":{"request":"LS0tLS1CRUdJTiBDRVJUSUZJQ0FURSBSRVFVRVNULS0tLS0KTUlJQ3ZEQ0NBYVFDQVFBd2R6RUxNQWtHQTFVRUJoTUNWVk14RFRBTEJnTlZCQWdNQkZWMFlXZ3hEekFOQmdOVgpCQWNNQmt4cGJtUnZiakVXTUJRR0ExVUVDZ3dOUkdsbmFVTmxjblFnU1c1akxqRVJNQThHQTFVRUN3d0lSR2xuCmFVTmxjblF4SFRBYkJnTlZCQU1NRkdWNFlXMXdiR1V1WkdsbmFXTmxjblF1WTI5dE1JSUJJakFOQmdrcWhraUcKOXcwQkFRRUZBQU9DQVE4QU1JSUJDZ0tDQVFFQTgrVG83ZCsya1BXZUJ2L29yVTNMVmJKd0RyU1FiZUthbUNtbwp3cDVicUR4SXdWMjB6cVJiN0FQVU9LWW9WRUZGT0VRczZUNmdJbW5Jb2xoYmlINm00emdaL0NQdldCT2taYytjCjFQbzJFbXZCeitBRDVzQmRUNWt6R1FBNk5iV3laR2xkeFJ0aE5MT3MxZWZPaGRuV0Z1aEkxNjJxbWNmbGdwaUkKV0R1d3E0QzlmK1lrZUpoTm45ZEY1K293bThjT1FtRHJWOE5OZGlUcWluOHEzcVlBSEhKUlcyOGdsSlVDWmtUWgp3SWFTUjZjckJROFRiWU5FMGRjK0NhYTNET0lrejFFT3NIV3pUeCtuMHpLZnFjYmdYaTRESngrQzFianB0WVBSCkJQWkw4REFlV3VBOGVidWRWVDQ0eUVwODJHOTYvR2djZjdGMzN4TXhlMHljK1hhNm93SURBUUFCb0FBd0RRWUoKS29aSWh2Y05BUUVGQlFBRGdnRUJBQjBrY3JGY2NTbUZEbXhveDBOZTAxVUlxU3NEcUhnTCtYbUhUWEp3cmU2RApoSlNad2J2RXRPSzBHMytkcjRGczExV3VVTnQ1cWNMc3g1YTh1azRHNkFLSE16dWhMc0o3WFpqZ21RWEdFQ3BZClE0bUMzeVQzWm9DR3BJWGJ3K2lQM2xtRUVYZ2FRTDBUeDVMRmwvb2tLYktZd0lxTml5S1dPTWo3WlIvd3hXZy8KWkRHUnM1NXh1b2VMREovWlJGZjliSStJYUNVZDFZcmZZY0hJbDNHODdBdityNDlZVndxUkRUMFZEVjd1TGdxbgoyOVhJMVBwVlVOQ1BRR245cC9lWDZRbzd2cERhUHliUnRBMlI3WExLalFhRjlvWFdlQ1VxeTFodkphYzlRRk8yCjk3T2IxYWxwSFBvWjdtV2lFdUp3akJQaWk2YTlNOUczMG5VbzM5bEJpMXc9Ci0tLS0tRU5EIENFUlRJRklDQVRFIFJFUVVFU1QtLS0tLQo="}}'

certificatesigningrequest "foo" created

$ kubectl describe csr
Name:           foo
Labels:         <none>
Annotations:        <none>
CreationTimestamp:  Thu, 01 Sep 2016 15:30:21 -0400
Requesting User:    
Status:         Pending
Subject:
    Common Name:        example.digicert.com
    Serial Number:      
    Organization:       DigiCert Inc.
    Organizational Unit:    DigiCert
    Country:        US
    Locality:       Lindon
    Province:       Utah
No events.

verified requests are hitting the /apis/certificates.k8s.io endpoint

@mikedanese
Copy link
Member

LGTM, @caesarxuchao can you make sure the api bits look fine?

@caesarxuchao
Copy link
Member

I thought the decision in the sig meeting yesterday is using "kubernetes.io", not "k8s.io"? @lavalamp

@liggitt
Copy link
Member Author

liggitt commented Sep 1, 2016

I thought the decision in the sig meeting yesterday is using "kubernetes.io", not "k8s.io"?

I didn't think there was a strong preference, but maybe I misread. For consistency with our import package (k8s.io), existing API groups (see below), and ease of use, I went with the shorter name.

existing API groups:

$ find . -name register.go | xargs -n 1 grep 'GroupName = ' | sort -u
const GroupName = ""
const GroupName = "apps"
const GroupName = "authentication.k8s.io"
const GroupName = "authorization.k8s.io"
const GroupName = "autoscaling"
const GroupName = "batch"
const GroupName = "certificates.k8s.io"
const GroupName = "componentconfig"
const GroupName = "extensions"
const GroupName = "federation"
const GroupName = "imagepolicy.k8s.io"
const GroupName = "policy"
const GroupName = "rbac.authorization.k8s.io"

@lavalamp
Copy link
Member

lavalamp commented Sep 1, 2016

@bgrant0607 prefers it spelled out. Not sure if that list will convince him otherwise but we can ask?

@bgrant0607
Copy link
Member

Sadly, we're not consistent. It depends on who does the API review.

We have API groups qualified with domains and those without.

We use .k8s.io in the cases above and .kubernetes.io in most annotations.

My general position for the API is to not abbreviate.
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#naming-conventions

Kubernetes contributors know what K8s means, but I wouldn't expect users to. However, I don't hear people being confused by K8s as often as I used to.

In any case, my other rule is that consistency trumps correctness, so if we've used .k8s.io in other API group names but not .kubernetes.io, then stick with .k8s.io.

cc @thockin @erictune

@thockin
Copy link
Member

thockin commented Sep 2, 2016

k8s.io is supposed to be shorthand. I've intentionally used kubernetes.io for all public domains. The one exception is the go-get path, and perhaps that was a mistake.

That said, it IS shorter and I am lazy. We should use the short form in places that humans type a lot. I don't think this is one of those cases. Inconsistency annoys me, so if the ship has sailed, then consistency wins. If it's not too late, I'd still argue for "kubernetes.io".

I suppose aliasing it with "secondary" names is right out? Accept either but always emit kubernetes.io .. ?

@gtank
Copy link

gtank commented Sep 2, 2016

Good change! I would also like to see this in 1.4. Consistency with the other API groups (and future norms) should be done before anyone depends on this naming scheme.

It may be a developer-centric opinion but, as someone who has typed these names frequently, I side with using .k8s.io.

@mikedanese
Copy link
Member

We need to make a decision on this. Going with kubernetes.io will require migrating 4 other api groups and we should do that before v1.4. I am not opinionated.

@@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// +groupName=certificates.k8s.io
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you change the staging area? It looks like you manually changed it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, the changes look fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, was manual, found it in a search for the old name

@liggitt
Copy link
Member Author

liggitt commented Sep 2, 2016

@bgrant0607
In any case, my other rule is that consistency trumps correctness, so if we've used .k8s.io in other API group names but not .kubernetes.io, then stick with .k8s.io.

@thockin
Inconsistency annoys me, so if the ship has sailed, then consistency wins. If it's not too late, I'd still argue for "kubernetes.io".

@mikedanese
Going with kubernetes.io will require migrating 4 other api groups and we should do that before v1.4.

Agree on being consistent. Since some of the suffixed groups existed in 1.3, switching to kubernetes.io would be pretty disruptive, not just for the 1.4 release. Without objection, I'll leave the PR as-is (and open a pick to the release-1.4 branch)

I suppose aliasing it with "secondary" names is right out? Accept either but always emit kubernetes.io .. ?

we could look into aliasing groups in future releases, but in the meantime, keeping things consistent seems ideal

@bgrant0607
Copy link
Member

I agree with .k8s.io.

Please no aliasing. It's confusing for users to see the multiple different forms in use and returning a different form than what the client expected won't work -- imagine accepting multiple apiversions but returning only 1.

@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Sep 2, 2016

GCE e2e build/test passed for commit a869de6.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@thockin
Copy link
Member

thockin commented Sep 5, 2016

we really need to get in from of the k8s.io vs kubernetes.io mess and lay
some rules, even if they are arbitrary.

On Fri, Sep 2, 2016 at 3:09 PM, Kubernetes Submit Queue <
notifications@github.com> wrote:

Merged #31887 #31887.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#31887 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFVgVAR56NKAyOzzNuuZjg13pg-givDUks5qmJ6ugaJpZM4Jy1kC
.

@liggitt liggitt deleted the certificates-group branch September 8, 2016 14:58
k8s-github-robot pushed a commit that referenced this pull request Sep 13, 2016
Automatic merge from submit-queue

Doc API group suffix, add test to catch new groups

Spawned from discussion in #31887

Doc and add tests to ensure new API groups are suffixed.

Also changed the doc to reference an API group containing the suffix as a starting point for new API groups.
michelleN pushed a commit to michelleN/community that referenced this pull request Nov 19, 2016
Automatic merge from submit-queue

Doc API group suffix, add test to catch new groups

Spawned from discussion in kubernetes/kubernetes#31887

Doc and add tests to ensure new API groups are suffixed.

Also changed the doc to reference an API group containing the suffix as a starting point for new API groups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants