Skip to content

Commit

Permalink
Create KEP for built-in Defaulting
Browse files Browse the repository at this point in the history
  • Loading branch information
apelisse committed Aug 6, 2020
1 parent 3ac6645 commit c85ce91
Show file tree
Hide file tree
Showing 2 changed files with 321 additions and 0 deletions.
302 changes: 302 additions & 0 deletions keps/sig-api-machinery/NNNN-built-in-default/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,302 @@
<!--
**Note:** When your KEP is complete, all of these comment blocks should be removed.
To get started with this template:
- [x] **Pick a hosting SIG.**
Make sure that the problem space is something the SIG is interested in taking
up. KEPs should not be checked in without a sponsoring SIG.
- [ ] **Create an issue in kubernetes/enhancements**
When filing an enhancement tracking issue, please make sure to complete all
fields in that template. One of the fields asks for a link to the KEP. You
can leave that blank until this KEP is filed, and then go back to the
enhancement and add the link.
- [x] **Make a copy of this template directory.**
Copy this template into the owning SIG's directory and name it
`NNNN-short-descriptive-title`, where `NNNN` is the issue number (with no
leading-zero padding) assigned to your enhancement above.
- [x] **Fill out as much of the kep.yaml file as you can.**
At minimum, you should fill in the "Title", "Authors", "Owning-sig",
"Status", and date-related fields.
- [ ] **Fill out this file as best you can.**
At minimum, you should fill in the "Summary" and "Motivation" sections.
These should be easy if you've preflighted the idea of the KEP with the
appropriate SIG(s).
- [x] **Create a PR for this KEP.**
Assign it to people in the SIG who are sponsoring this process.
- [ ] **Merge early and iterate.**
Avoid getting hung up on specific details and instead aim to get the goals of
the KEP clarified and merged quickly. The best way to do this is to just
start with the high-level sections and fill out details incrementally in
subsequent PRs.
Just because a KEP is merged does not mean it is complete or approved. Any KEP
marked as `provisional` is a working document and subject to change. You can
denote sections that are under active debate as follows:
```
<<[UNRESOLVED optional short context or usernames ]>>
Stuff that is being argued.
<<[/UNRESOLVED]>>
```
When editing KEPS, aim for tightly-scoped, single-topic PRs to keep discussions
focused. If you disagree with what is already in a document, open a new PR
with suggested changes.
One KEP corresponds to one "feature" or "enhancement" for its whole lifecycle.
You do not need a new KEP to move from beta to GA, for example. If
new details emerge that belong in the KEP, edit the KEP. Once a feature has become
"implemented", major changes should get new KEPs.
The canonical place for the latest set of instructions (and the likely source
of this file) is [here](/keps/NNNN-kep-template/README.md).
**Note:** Any PRs to move a KEP to `implementable`, or significant changes once
it is marked `implementable`, must be approved by each of the KEP approvers.
If none of those approvers are still appropriate, then changes to that list
should be approved by the remaining approvers and/or the owning SIG (or
SIG Architecture for cross-cutting KEPs).
-->
# KEP-NNNN: Built-in declarative defaults

<!-- toc -->
- [Release Signoff Checklist](#release-signoff-checklist)
- [Summary](#summary)
- [Motivation](#motivation)
- [Goals](#goals)
- [Non-Goals](#non-goals)
- [Proposal](#proposal)
- [Risks and Mitigations](#risks-and-mitigations)
- [Design Details](#design-details)
- [Test Plan](#test-plan)
- [Graduation Criteria](#graduation-criteria)
- [Beta -&gt; GA Graduation](#beta---ga-graduation)
- [Production Readiness Review Questionnaire](#production-readiness-review-questionnaire)
- [Feature Enablement and Rollback](#feature-enablement-and-rollback)
- [Rollout, Upgrade and Rollback Planning](#rollout-upgrade-and-rollback-planning)
- [Monitoring Requirements](#monitoring-requirements)
- [Dependencies](#dependencies)
- [Scalability](#scalability)
- [Implementation History](#implementation-history)
<!-- /toc -->

## Release Signoff Checklist

<!--
**ACTION REQUIRED:** In order to merge code into a release, there must be an
issue in [kubernetes/enhancements] referencing this KEP and targeting a release
milestone **before the [Enhancement Freeze](https://git.k8s.io/sig-release/releases)
of the targeted release**.
For enhancements that make changes to code or processes/procedures in core
Kubernetes—i.e., [kubernetes/kubernetes], we require the following Release
Signoff checklist to be completed.
Check these off as they are completed for the Release Team to track. These
checklist items _must_ be updated for the enhancement to be released.
-->

Items marked with (R) are required *prior to targeting to a milestone / release*.

- [ ] (R) Enhancement issue in release milestone, which links to KEP dir in [kubernetes/enhancements] (not the initial KEP PR)
- [ ] (R) KEP approvers have approved the KEP status as `implementable`
- [ ] (R) Design details are appropriately documented
- [ ] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input
- [ ] (R) Graduation criteria is in place
- [ ] (R) Production readiness review completed
- [ ] Production readiness review approved
- [ ] "Implementation History" section is up-to-date for milestone
- [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io]
- [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes

[kubernetes.io]: https://kubernetes.io/
[kubernetes/enhancements]: https://git.k8s.io/enhancements
[kubernetes/kubernetes]: https://git.k8s.io/kubernetes
[kubernetes/website]: https://git.k8s.io/website

## Summary

Currently, built-in API types are written as Go structures annotated
with markers to add additional semantics. This method has inspired
controller-tools who has re-used and improved on that technique for
building CRDs. CRDs have acquired the `// +default` tag to declaratively
define defaults and that mechanisms still doesn't exist for built-in.
This proposal tries to solve that inconsistency.

## Motivation

Having a new `// +default` marker for built-in types has several
important benefits:

- It's more convenient for types authors.
- The types and their behaviors are easier to understand since they are
described together.
- They are less error-prone, and prevent people from using anti-patterns
like "cross-field dependency defaults" (default depends on another
field), non-deterministic or complicated defaults. These patterns
might be useful in limited circumstances, but it should be easier to do
the right thing in most cases.
- Removes the inconsistency with CRDs that already support this
mechanism.
- Solves bug where associative keys need to either be required or
defaulted.

### Goals

The goal is to add a new `// +default` tag to our current built-in Go
IDL. That tag will be transformed into the OpenAPI `default` tag and
then routed to defaulting functions so that defaulting can be done
declaratively.

### Non-Goals

While this proposal is probably paving the way for additional OpenAPI
based validation, validation is entirely out-of-scope for this project.

This proposal also doesn't intend to remove the existing defaulting
mechanism since some defaulting still requires complex logic that can't
be defined declaratively. We could replace some defaulting that doesn't
require to be done as code though.

## Proposal

Since the goal of this proposal is to increase consistency with the CRD
defaulting, we propose to use the same mechanism (both in OpenAPI and
similar to what kubebuilder does). This defaulting will happen BEFORE
the existing defaulting functions are called (existing functions can be
updated to panic so that we can see if defaults are properly applied).

One very important nuance with CRDs is that CRDs are deserialized in
unstructured objects that can distinguish between specified and
unspecified fields. That is not the case for built-in types that are
directly deserialized into Go-struct, where the unspecified fields are
zero-valued. It's important to understand that only zero-value
omit-empty fields will be defaulted using that mechanism.

Complex defaulting will not be supported as a declarative marker and
will continue being supported through the existing defaulting functions.

### Risks and Mitigations

The main risk is a bug that would cause a default to not be set as
expected, either because the default would be missing or not set as
expected. This would result in possible conversion failures, invalid
objects being stored in the database or being served to users.

## Design Details

There are multiple challenges to running defaults declaratively and
changing the current mechanisms:

- We don't use OpenAPI to drive validation nor defaulting
currently. There is no routing of the OpenAPI to most operations in
kubernetes (the only thing right now is server-side apply).
- OpenAPI fields don't strictly map to Go types fields
(e.g. capitalization, embedding)
- Reflection can't access comments, only structTags (which is why we have
both structTags AND comments for SMP, but that's ugly and suboptimal).

One possible way to address that second point is to serialize the object
back to json, apply the default, and re-deserialize back into a
go-struct, but that's memory intensive and Kubernetes doesn't need that.

We're proposing to use the same mechanisms that we've put in place for
server-side apply, implemented in
"sigs.k8s.io/structured-merge-diff". We currently have a way for
server-side apply to convert Go types into "json"-like values without
serializing/deserializing and allocating memory by using reflection. We
also have mechanisms in place to parse, process and use the OpenAPI.

In other words, the plan would be the following:
1. Update kube-openapi to parse the `+default` tag and include it in the
OpenAPI definition
2. Add additional "default" field to sigs.k8s.io/structured-merge-diff
internal schema, and parse that field from the OpenAPI.
3. Create a new "AddDefault" method in sigs.k8s.io/structured-merge-diff
that receives a Value (value is an interface that can be backed by a
reflection based Go-type, associated with the corresponding
OpenAPI). That method would add the defaults in-place.
4. Add and wire things so that this function can be called prior to
running the existing defaulting code.

### Test Plan

Re-use and/or adapt existing defaulting tests, make sure that the
behavior doesn't change when moving from code-based defaults to
declarative default. Also panic in specific places of the defaulters to
guarantee that the defaulting have happened before.

### Graduation Criteria

The feature will be released as Beta and enabled by default. This is to
give a chance to disable the feature if something goes wrong, but we're
expecting GA quality from the start, with no changes.

#### Beta -> GA Graduation

Feature has been stable and worked as expected for a release.

## Production Readiness Review Questionnaire

### Feature Enablement and Rollback

* **How can this feature be enabled / disabled in a live cluster?**
- [x] Feature gate (also fill in values in `kep.yaml`)
- Feature gate name: DeclarativeDefaults

* **Does enabling the feature change any default behavior?** No

* **Can the feature be disabled once it has been enabled (i.e. can we roll back
the enablement)?** Yes

* **What happens if we reenable the feature if it was previously rolled back?** Nothing

* **Are there any tests for feature enablement/disablement?** No

### Rollout, Upgrade and Rollback Planning

* **How can a rollout fail? Can it impact already running workloads?** No

* **What specific metrics should inform a rollback?** None

* **Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?** No

* **Is the rollout accompanied by any deprecations and/or removals of features, APIs,
fields of API types, flags, etc.?** No

### Monitoring Requirements

* **How can an operator determine if the feature is in use by workloads?** N/A

* **What are the SLIs (Service Level Indicators) an operator can use to determine
the health of the service?** None

* **What are the reasonable SLOs (Service Level Objectives) for the above SLIs?** N/A

* **Are there any missing metrics that would be useful to have to improve observability
of this feature?** No

### Dependencies

* **Does this feature depend on any specific services running in the cluster?** No

### Scalability

* **Will enabling / using this feature result in any new API calls?** No

* **Will enabling / using this feature result in introducing new API types?** No

* **Will enabling / using this feature result in any new calls to the cloud
provider?** No

* **Will enabling / using this feature result in increasing size or count of
the existing API objects?** No

* **Will enabling / using this feature result in increasing time taken by any
operations covered by [existing SLIs/SLOs]?** No

* **Will enabling / using this feature result in non-negligible increase of
resource usage (CPU, RAM, disk, IO, ...) in any components?** No

## Implementation History
19 changes: 19 additions & 0 deletions keps/sig-api-machinery/NNNN-built-in-default/kep.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
title: Built-in declarative defaults
kep-number: NNNN
authors:
- "@apelisse"
owning-sig: sig-api-machinery
status: provisional
creation-date: 2020-08-04
reviewers:
- TBD
approvers:
- TBD
prr-approvers:
- deads2k
see-also:
- "/keps/sig-api-machinery/20190426-crd-defaulting.md"
stage: beta
latest-milestone: "v1.20"
milestone:
stable: "v1.20"

0 comments on commit c85ce91

Please sign in to comment.