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

protect kubernetes community owned API groups in CRDs #1111

Merged
merged 5 commits into from Jul 23, 2019

Conversation

@deads2k
Copy link
Contributor

commented Jun 24, 2019

API groups are organized by namespace, similar to java packages. authorization.k8s.io is one example. When users create
CRDs, they get to specify an API group and their type will be injected into that group by the kube-apiserver.

The *.k8s.io or *.kubernetes.io groups are owned by the Kubernetes community and protected by API review (see What APIs need to be reviewed,
to ensure consistency and quality. To avoid confusion in our API groups and prevent accidentally claiming a
space inside of the kubernetes API groups, the kube-apiserver needs to be updated to protect these reserved API groups.

This KEP proposes adding an api-approved.kubernetes.io annotation to CustomResourceDefinition. This is only needed if
the CRD group is k8s.io, kubernetes.io, or ends with .k8s.io, .kubernetes.io. The value should be a link to the
pull request where the API has been approved.

metadata:
  annotations:
    "api-approved.kubernetes.io": "https://github.com/kubernetes/kubernetes/pull/78458"

/assign @jpbetz @liggitt @sttts

@kubernetes/sig-api-machinery-api-reviews @kubernetes/sig-architecture-api-reviews

@fejta-bot

This comment has been minimized.

Copy link

commented Jun 24, 2019

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.

@sttts

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Sgtm. Some clarification of the PR link would be good.

@deads2k

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

updated for comments.

@liggitt

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

one addition requested in #1111 (comment)

overall approach looks good to me

@liggitt liggitt referenced this pull request Jul 2, 2019
@timothysc
Copy link
Member

left a comment

Not really a fan of yet another annotation

@timothysc

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

TL;DR this should really be policy, then enforcement can be made from said policy.

@deads2k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

TL;DR this should really be policy, then enforcement can be made from said policy.

This seems unnecessary if the above policy is spelled out in full.
Right now I think we need well defined policy in the community.

I think that is exactly what this KEP is and does.

We've had a well-defined policy for 11 months here: kubernetes/community#2433 . It was written, discussed, and reviewed thoroughly. The fact that it's apparently not well known seems to be an issue of enforcement.

This KEP takes the policy and provides a simple enforcement mechanism that makes these standards unignore-able without adding significant friction along the way.

@timothysc

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

We've had a well-defined policy for 11 months here: kubernetes/community#2433 . It was written, discussed, and reviewed thoroughly. The fact that it's apparently not well known seems to be an issue of enforcement.

It is well defined for core api's but it negates the larger ecosystem, which is where I think policy should be written then made enforceable.

We currently do not have any recommended guidelines for the community of non-core CRDs that are being published around k8s core.

@liggitt

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

It is well defined for core api's but it negates the larger ecosystem, which is where I think policy should be written then made enforceable.

https://github.com/kubernetes/community/blob/master/sig-architecture/api-review-process.md#what-apis-need-to-be-reviewed is the written policy that covers *.k8s.io APIs, regardless of whether they are CRD-based or not.

This is the proposal for the mechanism to make that policy enforceable.

@timothysc

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

https://github.com/kubernetes/community/blob/master/sig-architecture/api-review-process.md#what-apis-need-to-be-reviewed is the written policy that covers *.k8s.io APIs, regardless of whether they are CRD-based or not.

Then how does this impact the rest of the community that may overlap with that namespace, but have not changed yet? What are the recommendations wrt to naming?

If the current policy does not take that question into account, which it does not, I'd assert it requires refinement.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

This is a really interesting idea... I have some questions / concerns:

What do we do about projects that unknowingly violated this?

How can we make this more discoverable? I had no idea that this review guidelines doc existed, I suspect many others in the project don't either, especially those working on SIG projects.

Can we recommend namespacing guidelines for SIG projects that are unlikely to be core APIs but might want to use CRD storage and might not be the best use of time to review?

EG if I add some CRDs to sigs.k8s.io/slack-infra for some kubernetes.slack.com configuration, or the "component config" style config for sigs.k8s.io/kind... I sort of doubt API reviewing these is the most productive route vs some alternate API group, but I don't know what the correct namespace would be would be.

@deads2k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

Took @lavalamp's suggestion for allowing the annotation to explicitly indicate that it is unapproved. This will ease transitions, but still make the violation obvious to developers and cluster-admins so it can be resolved.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Thanks @bgrant0607, +1.
@liggitt kicked off a discussion around guidance in a better venue here. I think that sums up most of it nicely and my above points are not actually relevant to this KEP in particular. Looking forward to guidance on this!

@dims

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@thockin

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

For projects like prow, perhaps the right avenue is to actually procure domain names for them. The problem with being in a kubernetes github org is that is one wants to use *.k8s.io. Prow deserves its own identity and namespace, probably.

name: "@deads2k"
creation-date: 2019-06-12
last-updated: 2019-07-03
status: implementable

This comment has been minimized.

Copy link
@jbeda

jbeda Jul 3, 2019

Contributor

KEPs should be checked in early as provisional, iterated and then be transitioned to implementable. By having the initial PR mark the KEP as implementable means that the thing is all-or-nothing and it is more difficult to converge and merge.

@timothysc

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Thanks @bgrant0607 for enumerating the areas we identified that had accidentally adopted the API namespace due to copy/paste, bad examples, etc. I agree that the project should put a safe guard in place to prevent future accidents before they become too wide-spread. I am sure there are others that we have missed, and this change will help drive that education. End users tend to look at APIs under the k8s.io namespace as standard to the project and would naturally expect common practices are followed independent of how the API is implemented (CRD or not).



### Behavior of old clusters
1. Nothing changes. The old clusters will persist and keep the annotation

This comment has been minimized.

Copy link
@bgrant0607

bgrant0607 Jul 18, 2019

Member

What does "old clusters" include, exactly? Does it mean already created CRDs in older clusters that are upgraded to newer releases?

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 19, 2019

Member

It means clusters running API servers prior to the version introducing this (either because of a downgrade or a skewed HA cluster)

@bgrant0607

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Another example: https://kudo.dev/docs/faq/

@lavalamp

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

/lgtm
/approve

This was discussed pretty thoroughly during the last SIG Arch meeting.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, lavalamp

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

- "@liggitt"
- "@sttts"
editor:
name: "@deads2k"

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 23, 2019

Member

pretty this should be:

Suggested change
name: "@deads2k"
- "@deads2k"

it's causing KEPs to fail to merge
https://prow.k8s.io/?repo=kubernetes%2Fenhancements&type=batch
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/batch/pull-enhancements-verify/1153559205718790144

keps/sig-api-machinery/20190612-crd-group-protection.md has an error: "error validating KEP metadata: "editor" must have a value"

kicking it out of the queue:
/lgtm cancel

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 23, 2019

Member

correction: it should be editor: "@deads2k"

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 23, 2019

Member

Sorry for the LGTM cancel, this did in fact let the others PRs merge, kubernetes/test-infra#13551

Happy to re-LGTM when this is fixed.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

/test pull-enhancements-verify

@deads2k deads2k added the lgtm label Jul 23, 2019

@deads2k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Sorry for the LGTM cancel, this did in fact let the others PRs merge, kubernetes/test-infra#13551

Happy to re-LGTM when this is fixed.

Fixed the format. Retagging per comment.

@k8s-ci-robot k8s-ci-robot merged commit a37f153 into kubernetes:master Jul 23, 2019

2 of 3 checks passed

tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation deads2k authorized
Details
pull-enhancements-verify Job succeeded.
Details
@jwforres jwforres referenced this pull request Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.