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
Merged
Changes from 4 commits
Commits
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
139 changes: 139 additions & 0 deletions keps/sig-api-machinery/20190612-crd-group-protection.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
---
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
- sig-architecture
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-07-03
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),
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 a
to a URL where the current spec was approved, so updates to the spec should also update the URL.

```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. If the API is unapproved, you may set the annotation to a string starting
with `"unapproved"`. For instance, `"unapproved, temporarily squatting` or `"unapproved, experimental-only"`. This
is discouraged.

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

```yaml
metadata:
annotations:
"api-approved.kubernetes.io": "unapproved, experimental-only"
```

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 set to a URL, it will set the `KubeAPIApproved` condition to true.
2. If a new server sees a CRD for a resource in a kube group and sees the annotation set to `"unapproved.*"`, it will set the `KubeAPIApproved` condition to false.
3. 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.
4. 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. If the `KubeAPIApproved` condition is false,
the condition message will include a link to https://github.com/kubernetes/enhancements/pull/1111 for reference.

### Behavior of new clusters
1. Current CRD for a resource in the kube group already in API is missing valid `api-approved.kubernetes.io` or has set the value to `"unapproved.*"` - `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.
6. 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.

### What to do if you accidentally put an unapproved API in a protected group
1. Get the current state and future changes approved. For community projects, this is the best choice if the current state
is approvable.
2. If there are structural problems with the API's current state that prevent approval, you have two choices.
1. restructure in a new version, maintaining a conversion webhook, and plan to stop serving the old version. There are
some cases where this may not work if the changes are not roundtrippable, but they should be rare.
2. restructure in a new API group. There will be no connection to existing data. This may be disruptive for non-alpha APIs, but these
names are reserved and the process of API review has been in place for some time. The expectation is that this is
the exceptional case of an exceptional case.
3. Indicate that your API is unapproved by setting the `"api-approved.kubernetes.io"` annotation to something starting with
unapproved. For instance, `"unapproved, temporarily squatting` or `"unapproved, experimental-only"`.

## 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