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

protect kubernetes community owned API groups in CRDs #1111

Merged
merged 5 commits into from
Jul 23, 2019
+138 −0
Merged

protect kubernetes community owned API groups in CRDs #1111

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ddc786
protect kubernetes community owned API groups in CRDs
deads2k Jun 12, 2019
c80a1d1
clarify the URL for CRD
deads2k Jun 26, 2019
4dfe009
CRD group protection: add details about how to handle a conflict
deads2k Jul 2, 2019
2d59129
allow indicating the CRD is unapproved, but remain createable
deads2k Jul 3, 2019
7d8375f
fix editor tag
deads2k Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 116 additions & 0 deletions keps/sig-api-machinery/20190612-crd-group-protection.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
---
title: k8s.io Group Protection
authors:
- "@deads2k"
owning-sig: sig-api-machinery
participating-sigs:
- sig-api-machinery
liggitt marked this conversation as resolved.
Show resolved Hide resolved
reviewers:
- "@sttts"
- "@jpbetz"
- "@liggitt"
approvers:
- "@liggitt"
- "@sttts"
editor:
name: "@deads2k"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

correction: it should be editor: "@deads2k"

Copy link
Member

Choose a reason for hiding this comment

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

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.

creation-date: 2019-06-12
last-updated: 2019-06-12
status: implementable
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

---

# k8s.io Group Protection

## Table of Contents

## Summary

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](https://github.com/kubernetes/community/blob/master/sig-architecture/api-review-process.md#what-apis-need-to-be-reviewed),
deads2k marked this conversation as resolved.
Show resolved Hide resolved
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.
liggitt marked this conversation as resolved.
Show resolved Hide resolved

```yaml
metadata:
liggitt marked this conversation as resolved.
Show resolved Hide resolved
annotations:
"api-approved.kubernetes.io": "https://github.com/kubernetes/kubernetes/pull/78458"
liggitt marked this conversation as resolved.
Show resolved Hide resolved
```

## Motivation

* Prevent accidentally including an unapproved API in community owned API groups

### Goals

* Ensure API consistency.
* Prevent accidentally claiming reserved named.

### Non-Goals

* Prevent malicious users from claiming reserved names.

## Proposal

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.

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

This field is used by new kube-apiservers to set the `KubeAPIApproved` condition.
1. If a new server sees a CRD for a resource in a kube group and sees the annotation, it will set the `KubeAPIApproved` condition to true.
2. If a new server sees a CRD for a resource in a kube group and does not see the annotation, it will set the `KubeAPIApproved` condition to false.
Copy link
Member

Choose a reason for hiding this comment

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

When this condition is false, I think it needs to have a human-readable reason which e.g. contains a link to this KEP or some other way of making it easy for users to understand what they need to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this condition is false, I think it needs to have a human-readable reason which e.g. contains a link to this KEP or some other way of making it easy for users to understand what they need to do.

That's a good idea. It would make this very discoverable.

3. If a new server sees a CRD for a resource outside a kube group, it does not set the `KubeAPIApproved` condition at all.

In v1, this annotation will be required in order to create a CRD for a resource in one of the kube API groups.

### Behavior of new clusters
1. Current CRD for a resource in the kube group already in API is missing valid `api-approved.kubernetes.io` - `KubeAPIApproved` condition will be false.
2. CRD for a resource in the kube group creating via CRD.v1beta1 is missing valid `api-approved.kubernetes.io` - create as normal. This ensures compatibility. `KubeAPIApproved` condition will be false.
3. CRD for a resource in the kube group creating via CRD.v1 is missing valid `api-approved.kubernetes.io` - fail validation and do not store in etcd.
4. CRD for a resource outside the kube group creating via CRD.v1 is contains the `api-approved.kubernetes.io` - fail validation and do not store in etcd.
5. In CRD.v1, remove a required `api-approved.kubernetes.io` - fail validation.
5. In all versions, any update that does not change the `api-approved.kubernetes.io` will go through our current validation rules.


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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


This doesn't actively prevent bad actors from simply setting the annotation, but it does prevent accidentally claiming
an inappropriate name.

## References

* [Accidental name in Kanary](https://libraries.io/github/AmadeusITGroup/kanary))
liggitt marked this conversation as resolved.
Show resolved Hide resolved

### Test Plan

**blockers for GA:**

* Document in the release notes. The impact is very low

### Graduation Criteria

* the test plan is fully implemented for the respective quality level

### Upgrade / Downgrade Strategy

* annotations and conditions are always persisted. If set, they remain consistent. If unset, they also remain consistent.

### Version Skew Strategy

* annotations and conditions are always persisted. If set, they remain consistent. If unset, they also remain consistent.

## Alternatives considered

## Implementation History