Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Validation support for strings, arrays, numbers #329

Merged
merged 37 commits into from
Dec 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
48c2b2c
Handle validations!
Porges Nov 18, 2020
0a53c6c
Change how properties have 'required' validation
Porges Nov 19, 2020
ee499dc
Add SetAzureName and webhook for AzureName defaulting
Porges Nov 20, 2020
52e6214
Remove thing about "must be default" because it's fixed!
Porges Nov 20, 2020
8cb396e
Add required sideEffects setting
Porges Nov 20, 2020
465433a
Tweaks to field rendering
Porges Nov 22, 2020
801d0f4
Configuration for webhooks
Porges Nov 23, 2020
0fb70e6
Add webhook folder to .gitignore
Porges Nov 23, 2020
a7aa1c9
Add setup for k8s webhook certificates
Porges Nov 23, 2020
9212e47
Make webhooks work!
Porges Nov 23, 2020
00f69ba
Fix typo
Porges Nov 23, 2020
89822ab
Extract generateDefaulter function
Porges Nov 23, 2020
fd15cf7
Fix golden test comment
Porges Nov 23, 2020
2065102
Update golden tests
Porges Nov 23, 2020
aca4e1d
Remove an outstanding TODO
Porges Nov 24, 2020
f526a6c
Fix other unit tests
Porges Nov 24, 2020
ce697e2
Update new golden files after rebase
Porges Nov 24, 2020
3eaf167
Add link to issue
Porges Dec 1, 2020
69702ec
Expand comment
Porges Dec 1, 2020
cd48289
Fix up const name
Porges Dec 1, 2020
73c7dbe
Fix bad error 😊
Porges Dec 1, 2020
438ef9c
Strip validations from ARM types
Porges Dec 2, 2020
d121c76
Fix names of property tests
Porges Dec 2, 2020
421ddaa
Lint fix
Porges Dec 2, 2020
4f6fd72
Merge branch 'master' into validation-support
Porges Dec 2, 2020
3d67b9c
Implement test case generation for validated types
Porges Dec 2, 2020
2191426
Support for validated types in testcase generation
Porges Dec 2, 2020
a5d568f
Fixup validation conversion
Porges Dec 2, 2020
67b67d1
Reformat very long line
Porges Dec 6, 2020
478a9af
Use astbuilder helper
Porges Dec 6, 2020
2abe786
Introduce DeclarationContext
Porges Dec 6, 2020
fddd910
Fix go.mod for generated tests
Porges Dec 6, 2020
bb0550b
Fix gopter codegen
Porges Dec 6, 2020
30d3bfd
Merge branch 'master' into validation-support
Porges Dec 7, 2020
7a3d305
Document resource interface functions
Porges Dec 7, 2020
fc441d0
Merge branch 'master' into validation-support
Porges Dec 17, 2020
34dbb99
Post-merge fixup
Porges Dec 17, 2020
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
16 changes: 11 additions & 5 deletions .devcontainer/install-dependencies.sh
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ echo "Installing Go tools…"
# go tools for vscode are preinstalled by base image (see first comment in Dockerfile)
go get \
github.com/jandelgado/[email protected] \
github.com/mikefarah/yq/[email protected] \
github.com/mitchellh/[email protected] \
k8s.io/code-generator/cmd/[email protected] \
sigs.k8s.io/controller-tools/cmd/[email protected] \
Expand All @@ -67,11 +68,6 @@ if [ "$1" != "devcontainer" ]; then
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b "$TOOL_DEST" 2>&1
fi

echo "Installing kubectl…"
Porges marked this conversation as resolved.
Show resolved Hide resolved
curl -LO https://storage.googleapis.com/kubernetes-release/release/v1.19.0/bin/linux/amd64/kubectl \
&& chmod +x ./kubectl \
&& mv ./kubectl "$TOOL_DEST/kubectl"

# Install go-task (task runner)
echo "Installing go-task…"
curl -sL "https://github.com/go-task/task/releases/download/v3.0.0/task_linux_amd64.tar.gz" | tar xz -C "$TOOL_DEST" task
Expand All @@ -84,3 +80,13 @@ curl -L "https://go.kubebuilder.io/dl/2.3.1/${os}/${arch}" | tar -xz -C /tmp/
mv "/tmp/kubebuilder_2.3.1_${os}_${arch}" "$KUBEBUILDER_DEST"

echo "Installed tools: $(ls "$TOOL_DEST")"


if [ "$1" = "devcontainer" ]; then
echo "Setting up k8s webhook certificates"

mkdir -p /tmp/k8s-webhook-server/serving-certs
openssl genrsa 2048 > tls.key
openssl req -new -x509 -nodes -sha256 -days 3650 -key tls.key -subj '/' -out tls.crt
mv tls.key tls.crt /tmp/k8s-webhook-server/serving-certs
fi
33 changes: 29 additions & 4 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -178,18 +178,43 @@ tasks:
- cp config/crd/bases/microsoft.storage.infra.azure.com_storageaccountsblobservicescontainers.yaml config/crd/bases/valid
- cp config/crd/bases/microsoft.resources.infra.azure.com_resourcegroups.yaml config/crd/bases/valid

controller:copy-valid-webhooks:
dir: "{{.CONTROLLER_ROOT}}"
deps: [generate-crds]
cmds:
# the webhook configuration file, as it stands, is too big to load into k8s 😊
Copy link
Member

Choose a reason for hiding this comment

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

Should we open an issue with kubebuilder on this topic? Presumably the issue is that the yaml file is too big but if it were split up it'd be ok?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe there already is one? Link it here if so?

Copy link
Member Author

@Porges Porges Dec 1, 2020

Choose a reason for hiding this comment

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

It might actually be small enough now; part of the earlier problem was webhook definitions were being generated for the storage types.

Copy link
Member Author

Choose a reason for hiding this comment

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

A few other thoughts:

  • Yes, I think controller-gen should generate multiple files
  • We should be able to generate fewer webhooks that apply to more types.

# instead, for now, we will pull out the bits we want
- mkdir -p config/webhook/valid
- yq delete {{.IN}} 'webhooks[*]' > {{.OUT}}
- yq read -c {{.IN}} 'webhooks.(rules[*].resources[*]. == batchaccounts)' | yq prefix - webhooks | yq merge -a append - {{.OUT}} > {{.OUT}}_2 && mv {{.OUT}}_2 {{.OUT}}
- yq read -c {{.IN}} 'webhooks.(rules[*].resources[*]. == storageaccounts)' | yq prefix - webhooks | yq merge -a append - {{.OUT}} > {{.OUT}}_2 && mv {{.OUT}}_2 {{.OUT}}
- yq read -c {{.IN}} 'webhooks.(rules[*].resources[*]. == storageaccountsblobservicescontainers)' | yq prefix - webhooks | yq merge -a append - {{.OUT}} > {{.OUT}}_2 && mv {{.OUT}}_2 {{.OUT}}
# need to remove v1 from the manifest as it doesn't work with controller-runtime currently: https://github.com/kubernetes-sigs/controller-runtime/issues/1272
- yq delete -i {{.OUT}} 'webhooks[*].admissionReviewVersions(. == v1)'
vars:
IN: config/webhook/manifests.yaml
OUT: config/webhook/valid/manifests.yaml


controller:prep-integration-tests:
# this just exists to serialize execution of these dependencies,
# since go-task won't dedupe _their_ shared dependencies
cmds:
- task controller:copy-valid-crds
- task controller:copy-valid-webhooks

controller:test-integration-envtest:
desc: Run integration tests with envtest using record/replay.
dir: "{{.CONTROLLER_ROOT}}"
deps: [controller:copy-valid-crds]
deps: [controller:prep-integration-tests]
cmds:
# -race fails at the moment in controller-runtime
- ENVTEST=1 RECORD_REPLAY=1 go test -v $({{.GENERATED_DIRS_TO_LINT_CMD}})

controller:test-integration-envtest-cover:
desc: Run integration tests with envtest using record/replay and output coverage.
dir: "{{.CONTROLLER_ROOT}}"
deps: [controller:copy-valid-crds]
deps: [controller:prep-integration-tests]
cmds:
# -race fails at the moment in controller-runtime
- ENVTEST=1 RECORD_REPLAY=1 go test -covermode atomic -coverprofile=cover-integration-envtest.out -coverpkg="./..." -v $({{.GENERATED_DIRS_TO_LINT_CMD}})
Expand All @@ -198,7 +223,7 @@ tasks:
controller:test-integration-envtest-live:
desc: Run integration tests with envtest against live data and output coverage.
dir: "{{.CONTROLLER_ROOT}}"
deps: [controller:copy-valid-crds, cleanup-azure-resources]
deps: [controller:prep-integration-tests, cleanup-azure-resources]
cmds:
# -race fails at the moment in controller-runtime
- ENVTEST=1 go test -covermode atomic -coverprofile=cover-integration-envtest.out -coverpkg="./..." -v $({{.GENERATED_DIRS_TO_LINT_CMD}})
Expand All @@ -212,8 +237,8 @@ tasks:
- "apis/**/*_gen.go" # depends on all generated types
cmds:
- find ./apis -type f -name "zz_generated.*" -delete
- controller-gen object:headerFile=../boilerplate.go.txt paths="./..."
- if [ -d "./config/crd/bases" ]; then find "./config/crd/bases" -type f -delete; fi
- controller-gen object:headerFile=../boilerplate.go.txt paths="./..." || true
Copy link
Member Author

@Porges Porges Nov 19, 2020

Choose a reason for hiding this comment

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

TO FIX: controller-gen emitting bad code

- controller-gen {{.CRD_OPTIONS}} rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
vars:
CRD_OPTIONS: "crd:crdVersions=v1,allowDangerousTypes=true"
Expand Down
1 change: 1 addition & 0 deletions hack/generated/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
apis/
config/crd/bases/
config/webhook
6 changes: 1 addition & 5 deletions hack/generated/controllers/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
storage "github.com/Azure/k8s-infra/hack/generated/apis/microsoft.storage/v20190401"
"github.com/Azure/k8s-infra/hack/generated/pkg/armclient"
"github.com/Azure/k8s-infra/hack/generated/pkg/testcommon"
ctrl "sigs.k8s.io/controller-runtime"
)

func Test_ResourceGroup_CRUD(t *testing.T) {
Expand Down Expand Up @@ -123,10 +122,7 @@ func StorageAccount_BlobServices_CRUD(t *testing.T, testContext testcommon.KubeP
g := NewGomegaWithT(t)

blobService := &storage.StorageAccountsBlobService{
ObjectMeta: ctrl.ObjectMeta{
Name: "default", // MUST be 'default'
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the thing I’ve been working towards 😀

Copy link
Member Author

Choose a reason for hiding this comment

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

(I know @babbageclunk likes stuff like this 😉)

Copy link
Member

Choose a reason for hiding this comment

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

It's like when they say the title of the movie in the movie.

Namespace: testContext.Namespace(),
},
ObjectMeta: testContext.MakeObjectMeta("blobservice"),
Spec: storage.StorageAccountsBlobServices_Spec{
ApiVersion: "2019-04-01", // TODO [apiversion]: to be removed eventually
Owner: testcommon.AsOwner(storageAccount),
Expand Down
8 changes: 4 additions & 4 deletions hack/generated/controllers/generic_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ func (options *Options) setDefaults() {
}

func RegisterAll(mgr ctrl.Manager, applier armclient.Applier, objs []runtime.Object, log logr.Logger, options Options) []error {

options.setDefaults()

var errs []error
Expand All @@ -118,6 +117,7 @@ func RegisterAll(mgr ctrl.Manager, applier armclient.Applier, objs []runtime.Obj
errs = append(errs, err)
}
}

return errs
}

Expand Down Expand Up @@ -155,11 +155,11 @@ func register(mgr ctrl.Manager, applier armclient.Applier, obj runtime.Object, l
CreateDeploymentName: options.CreateDeploymentName,
}

ctrlBuilder := ctrl.NewControllerManagedBy(mgr).
c, err := ctrl.NewControllerManagedBy(mgr).
For(obj).
WithOptions(options.Options)
WithOptions(options.Options).
Build(reconciler)

c, err := ctrlBuilder.Build(reconciler)
if err != nil {
return errors.Wrap(err, "unable to build controllers / reconciler")
}
Expand Down
Loading