DNM: WIP: minimal example of DV for create - #981
Open
Tim Hockin (thockin) wants to merge 27 commits into
Open
Conversation
Run:
go -C hack/tools/code-generator \
get -u \
k8s.io/code-generator@v0.37.0-rc.0 \
k8s.io/apimachinery@v0.37.0-rc.0 \
k8s.io/streaming@v0.37.0-rc.0
We need to carry at least 1 patch.
Run:
pushd hack/tools/code-generator
mkdir -p third_party/k8s.io
git clone \
http://github.com/kubernetes/code-generator \
-b v0.37.0-rc.0 \
third_party/k8s.io/code-generator
rm -rf third_party/k8s.io/code-generator/.git
git add third_party/k8s.io/code-generator
go mod edit -replace \
k8s.io/code-generator=./third_party/k8s.io/code-generator
git clone \
http://github.com/kubernetes/apimachinery \
-b v0.37.0-rc.0 \
third_party/k8s.io/apimachinery
rm -rf third_party/k8s.io/apimachinery/.git
git add third_party/k8s.io/apimachinery
go mod edit -replace \
k8s.io/apimachinery=./third_party/k8s.io/apimachinery
This PR is actually 141383 and 141395 combined. It implements some fixes to validation-gen that we need and applies cleanly (modulo files that are not needed here) over v0.37.0-rc.0 Fix 1: maps of pointers Fix 2: custom deep-equal Run: pushd hack/tools/code-generator/third_party wget https://github.com/kubernetes/kubernetes/pull/141395.diff -O k_k_141395.diff git add k_k_141395.diff patch -f -p3 < ../141395.diff git add third_party/k8s.io/code-generator
Run:
go get -u \
k8s.io/apimachinery@v0.37.0-rc.0 \
k8s.io/api@v0.37.0-rc.0 \
k8s.io/client-go@v0.37.0-rc.0
go get \
github.com/go-openapi/swag/jsonutils/adapters/stdlib/json@v0.27.1
go mod vendor
go mod tidy
We need to carry at least 1 patch
Run:
mkdir -p third_party/k8s.io
git clone \
http://github.com/kubernetes/apimachinery \
-b v0.37.0-rc.0 \
third_party/k8s.io/apimachinery
rm -rf third_party/k8s.io/apimachinery/.git
git add third_party/k8s.io/apimachinery
go mod edit -replace \
k8s.io/apimachinery=./third_party/k8s.io/apimachinery
go mod vendor
go mod tidy
This PR is actually 141383 and 141395 combined. It implements some fixes to validation-gen that we need and applies cleanly (modulo files that are not needed here) over v0.37.0-rc.0 Fix 1: maps of pointers Fix 2: custom deep-equal Run: pushd third_party wget https://github.com/kubernetes/kubernetes/pull/141395.diff -O k_k_141395.diff git add k_k_141395.diff patch -f -p3 < k_k_141395.diff) rm -rf k8s.io/code-generator/ popd go mod vendor go mod tidy
This produces no-op output for easier reviewing.
This requires marking it as "opaque" in those places we do not want to emit code for yet. This also demonstrates how the presence/absence of metadata.atespace can be driven by each resource definition. More will follow.
This also makes the test compare by origin and not by value, which allows us to be confident we matched the right error without embedding the error string (brittle).
This is fairly exhaustive, so the tests for individual resources do not have to be.
This required marking other uses of Actor as opaque for now. We don't want to do everything all at once.
Earlier commits put validation code alongside the schema (pkg/proto/ateapipb). The problem is that there are things about validation which are known to the API server (cmd/ateapi/internal/controlapi) but are not exactly part of the schema. Example: Any type which supports updates has a FieldMask in the request. Most of those types do not support updating arbitrary fields, so they have manual validation to ensure only "safe" fields are in the field mask. That looks like a map of string to "apply" function, which is called during update to extract values from the "new" object and apply them to the "old". ateapipb cannot call controlapi (circular). Our options: 1) Put the list of valid fields in ateapipb and reference it from controlapi. That feels weird to me - the protobuf pkg having handwritten go code. 2) Generate validation code into controlapi, so it is all self-contained. This is a little bit uglier because of cross-package references, but keeps the details of what is updatable alongside the main API serving code. 3) Add a way to DV to validate allowed values. This would be nice but does not exist. 4) Duplicate the list of allowed update fields in ateapipb and controlapi, and keep them in sync. ateapipb just needs a `[]string`, while controlapi needs a `map[string]func(...)` I opted for model 2 in this commit
Some of the tools depend on each other now (validation has to run after protobuf) but `go generate` doesn't guarantee ordering across packages.
This breaks a lot of tests and doesn't handle workflows or anything that doesn't come thru the RPC layer first. We need a middle layer for that still.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This does not handle all fields - just enough to make an example.
This does not add the "middle" layer to make workflows safe.
This does not handle updates.
What it does do it show how we might wield DV to effect solid tests and invariants.
It requires patches on top of the 1.37 k8s code, which is ugly but (IMO) manageable.
Most of the first commits are just setting up the code. Everything from "Main commits" on should be reviewable
Julian Gutierrez Oschmann (@juli4n) if this aligns with how you were thinking, I can proceed a bit more.
Luiz Oliveira (@laoj2) for your input
Joe Betz (@jpbetz) Lalit Chauhan (@lalitc375) Yongrui Lin (@yongruilin) to see how badly I abused DV