Add `Finalizers`, `PolicyStatus`, `PolicyActive` condition to ClusterAdmissionPolicies #76

viccuad · 2021-09-16T15:19:23Z

In this PR:

Add defaulter, validating, mutating webhooks for ClusterAdmissionPolicies, namely for adding finalizers, and checking that the policyServer is immutable.
Given the difficulty on testing, added apis/policies/v1alpha2/webhook_suite_test.go.
Refactor ClusterAdmissionPolicyStatus.PolicyActive status into PolicyStatus: now it accepts unscheduled, unschedulable, pending, active.

Refactor PolicyServerBound Condition to ClusterAdmissionPolicyActive.
Test with:

kubectl wait --for=condition=active clusteradmissionpolicy psp-capabilities

Enable Kustomize patches for cert-manager.
Add RBAC marker for secrets and confimaps, needed to run/test in-cluster.
Drop unneeded funcs, refactor as needed to be able to test.

Closes: #69, #72.

Thanks @ereslibre & @raulcabello for the pairing!

(It may be a good idea to review per commits, instead of by changes).

To test with an out-of-cluster controller, one would need to create a proxy to the webhook server (TBD) since the webhooks are registered against the controller manager webhook server. To allow for the manager to at least start without faiiures, a new make dev-webhook-certs target has been added. It creates and sets the certs in localhost and gets called on make run.
Therefore, to test meanwhile skipping the registration of webhooks yet having a working controller:

$ kubectl create namespace kubewarden-controller-system
$ make install
$ make run ENABLE_WEBHOOKS=false NAMESPACE=kubewarden-controller-system

To test with an in-cluster controller, cert-manager has been scaffolded for creating the needed cert to register the webhooks.
Example with k3d:

$ k3d cluster create
$ kubectl apply -f https://github.com/jetstack/cert-manager/releases/download/v1.5.3/cert-manager.yaml
$ make docker-build NAMESPACE=kubewarden-controller-system
$ k3d image import controller:latest
$ kubectl create namespace kubewarden-controller-system
$ make deploy IMG=controller:latest NAMESPACE=kubewarden-controller-system
## patch deployment with ImagePullPolicy: Never

Performed with: kubebuilder create webhook --group policies --version v1alpha2 \ --kind ClusterAdmissionPolicy --defaulting --programmatic-validation

So future scaffolding of new components is not namespaced.

Add also PolicyServer webhook patches, as they were already created and added when creating the PolicyServer CR.

Drop uneeded removeKubewardenFinalizer() and its test.

…olicy Add Gingko tests to webhook_suite_test.go.

It is plain wrong. Also, it will add the finalizer again after the reconciler removes it, therefore the ClusterAdmissionPolicy will not get garbage collected. Co-Authored-By: Raul Cabello Martin <raul.cabello@suse.com>

We only use the ClusterAdmissionPolicy controller to call the PolicyServer controller. We don't need this function. Plus, we already have it in pkg/admission/reconciler.go.updateAdmissionPolicyStatus().

PolicyStatus represents whether the ClusterAdmissionPolicy is unscheduled, unschedulable, pending, or active. Run `make manifests` to update CRDs.

This has no effect for changing the default on create, as it cannot be changed there. Nor it gets changed after read later on. It's only for good UX. We keep assuming PolicyStatus can be "" and setting where corresponds. Run `make manifests`.

In the past, we set Conditions and PolicyStatus at the same time. Now, we set PolicyStatus dependending on what state we are moving onto (schedulable, pending, active..) and set the Condition only when the ClusterAdmissionPolicy is active. By removing Conditions from required, we can correctly set the default PolicyStatus to "unscheduled". Co-authored-by: Rafael Fernández López <ereslibre@ereslibre.es>

Allow manager-role to create,get,list,patch,update,watch for both - secrets: needed for webhooks - configmaps: needed for reading the configmap associated with each policy-server

Default to "unscheduled". For "unschedulable", obtain the list of policyServers that match the policy.Spec.PolicyServer. If empty, update the PolicyStatus. For that, export Reconciler.UpdateAdmissionPolicyStatus() and add a new FieldIndexer. For "pending", set it on the watch, until the PolicyServer rollout has finished and the admission webhook is registered. Co-Authored-By: Raul Cabello Martin <raul.cabello@suse.com> Co-Authored-By: Rafael Fernández López <ereslibre@ereslibre.es>

Test with: ``` kubectl wait --for=condition=active clusteradmissionpolicy psp-capabilities ```

Creates local cert and set its where the local controller manager expects it. Useful when developing with `make run WEBHOOKS_ENABLE=false`.

That way one will not get image pull errors when testing, as policy-server image is currently in v0.1.0.

apis/policies/v1alpha2/clusteradmissionpolicy_types.go

apis/policies/v1alpha2/clusteradmissionpolicy_webhook.go

controllers/policies/policyserver_controller.go

raulcabello

LGTM just some minor suggestions
Thanks @viccuad !

apis/policies/v1alpha2/clusteradmissionpolicy_types.go

Remove leftover from previous approach. Run `make manifests`.

apis/policies/v1alpha2/clusteradmissionpolicy_types.go

apis/policies/v1alpha2/clusteradmissionpolicy_webhook.go

config/crd/bases/policies.kubewarden.io_clusteradmissionpolicies.yaml

This reverts commit 8e1fdc8.

Run `make manifests`.

apis/policies/v1alpha2/clusteradmissionpolicy_types.go

config/crd/bases/policies.kubewarden.io_clusteradmissionpolicies.yaml

Run 'make manifests'.

raulcabello · 2021-09-17T09:22:28Z

@viccuad I can't create policies without settings, I get The ClusterAdmissionPolicy "privileged-pods" is invalid: spec.settings: Invalid value: "null": spec.settings in body must be of type object: "null"
But if I remove the mutating webhook then I can create them

ereslibre

Thanks @viccuad!, really great work, we learnt quite a lot of things in the process 👏

apis/policies/v1alpha2/clusteradmissionpolicy_types.go

Run `make manifests`.

This allows for policies with no settings object in the spec. (optional ) This change, in k8s >= v1.20, means that the object will not be pruned if it has no value, and it will be `settings: null` instead. This solves errors suchs as: > The ClusterAdmissionPolicy "psp-capabilities-no-settings" is invalid: > spec.settings: Invalid value: "null": spec.settings in body must be of > type object: "null" See: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#defaulting-and-nullable

viccuad and others added 18 commits September 7, 2021 10:41

Remove indirect from google/go-cmp

25f0198

Scaffold ClusterAdmissionPolicy webhooks

cae98b0

Performed with: kubebuilder create webhook --group policies --version v1alpha2 \ --kind ClusterAdmissionPolicy --defaulting --programmatic-validation

Reset namespaced: false in PROJECT

bed19c8

So future scaffolding of new components is not namespaced.

Enable Kustomize patches for webhooks

416b9bc

Add also PolicyServer webhook patches, as they were already created and added when creating the PolicyServer CR.

Add missing conversionReviewVersions to clusteradmissionpolicies

8c2b809

Refactor: Use controllerutil.RemoveFinalizer() in reconciler.go

3653bd6

Drop uneeded removeKubewardenFinalizer() and its test.

Add finalizer, make Spec.PolicyServer immutable for ClusterAdmissionP…

e024d02

…olicy Add Gingko tests to webhook_suite_test.go.

Don't run defaulter webhook on resource update

273d977

It is plain wrong. Also, it will add the finalizer again after the reconciler removes it, therefore the ClusterAdmissionPolicy will not get garbage collected. Co-Authored-By: Raul Cabello Martin <raul.cabello@suse.com>

Drop clusteradmissionpolicy_controller.go.updateAdmissionPolicyStatus()

fb3c444

We only use the ClusterAdmissionPolicy controller to call the PolicyServer controller. We don't need this function. Plus, we already have it in pkg/admission/reconciler.go.updateAdmissionPolicyStatus().

Refactor PolicyActive to PolicyStatus and set "active"

46d2b14

PolicyStatus represents whether the ClusterAdmissionPolicy is unscheduled, unschedulable, pending, or active. Run `make manifests` to update CRDs.

Enable Kustomize patches for cert-manager

5e85a08

Add RBAC marker for secrets and confimaps

8ca47a8

Allow manager-role to create,get,list,patch,update,watch for both - secrets: needed for webhooks - configmaps: needed for reading the configmap associated with each policy-server

Refactor PolicyServerBound Condition to ClusterAdmissionPolicyActive

2ea447c

Test with: ``` kubectl wait --for=condition=active clusteradmissionpolicy psp-capabilities ```

Add make dev-cert-webhooks target

5a6f40c

Creates local cert and set its where the local controller manager expects it. Useful when developing with `make run WEBHOOKS_ENABLE=false`.

Change sample to use policy-server:latest image

31583a9

That way one will not get image pull errors when testing, as policy-server image is currently in v0.1.0.

viccuad requested review from ereslibre, flavio and raulcabello September 16, 2021 15:19

viccuad added this to Pending review in Development Sep 16, 2021

viccuad added the new-architecture label Sep 16, 2021

viccuad self-assigned this Sep 16, 2021

viccuad requested a review from jvanz September 16, 2021 18:00

raulcabello reviewed Sep 17, 2021

View reviewed changes

apis/policies/v1alpha2/clusteradmissionpolicy_types.go Outdated Show resolved Hide resolved

raulcabello reviewed Sep 17, 2021

View reviewed changes

apis/policies/v1alpha2/clusteradmissionpolicy_webhook.go Outdated Show resolved Hide resolved

raulcabello reviewed Sep 17, 2021

View reviewed changes

controllers/policies/policyserver_controller.go Show resolved Hide resolved

raulcabello requested changes Sep 17, 2021

View reviewed changes

viccuad commented Sep 17, 2021

View reviewed changes

apis/policies/v1alpha2/clusteradmissionpolicy_types.go Outdated Show resolved Hide resolved

Generate ClusterAdmissionPolicyActive status correctly

eb9fe88

Remove leftover from previous approach. Run `make manifests`.

ereslibre reviewed Sep 17, 2021

View reviewed changes

viccuad added 2 commits September 17, 2021 10:05

Revert "Set PolicyStatus "unscheduled" as default in CRD"

84a55a2

This reverts commit 8e1fdc8.

Add markers for admissionregistration.k8s.io webhooks

ce1df17

Run `make manifests`.

ereslibre reviewed Sep 17, 2021

View reviewed changes

apis/policies/v1alpha2/clusteradmissionpolicy_types.go Outdated Show resolved Hide resolved

config/crd/bases/policies.kubewarden.io_clusteradmissionpolicies.yaml Outdated Show resolved Hide resolved

viccuad added 2 commits September 17, 2021 10:25

Drop PolicyServerWebhookRegistered condition docs

bded155

Run 'make manifests'.

Check type assertion on webhook & controller watches

b975030

ereslibre approved these changes Sep 17, 2021

View reviewed changes

viccuad commented Sep 17, 2021

View reviewed changes

apis/policies/v1alpha2/clusteradmissionpolicy_types.go Show resolved Hide resolved

viccuad added 3 commits September 17, 2021 17:40

Add RBAC for deployments,services,configmaps,secrets

e7285c2

Run `make manifests`.

Add Log.Info when policy becomes active

808ff6a

viccuad requested a review from raulcabello September 20, 2021 08:54

raulcabello approved these changes Sep 20, 2021

View reviewed changes

viccuad merged commit 954a59b into kubewarden:new-architecture Sep 20, 2021

ereslibre moved this from Pending review to Done in Development Sep 20, 2021

This was referenced Sep 20, 2021

ClusterAdmissionPolicy reconciliation loop: handle policies that refence non-existing PolicyServer #72

Closed

Implement validation webhooks for ClusterAdmissionPolicy #69

Closed

viccuad deleted the webhooks branch October 6, 2021 09:42

viccuad mentioned this pull request Oct 15, 2021

Add ServerStatus and Conditions to PolicyServer CRD #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Finalizers`, `PolicyStatus`, `PolicyActive` condition to ClusterAdmissionPolicies #76

Add `Finalizers`, `PolicyStatus`, `PolicyActive` condition to ClusterAdmissionPolicies #76

viccuad commented Sep 16, 2021 •

edited

raulcabello left a comment

raulcabello commented Sep 17, 2021

ereslibre left a comment

Add Finalizers, PolicyStatus, PolicyActive condition to ClusterAdmissionPolicies #76

Add Finalizers, PolicyStatus, PolicyActive condition to ClusterAdmissionPolicies #76

Conversation

viccuad commented Sep 16, 2021 • edited

raulcabello left a comment

Choose a reason for hiding this comment

raulcabello commented Sep 17, 2021

ereslibre left a comment

Choose a reason for hiding this comment

Add `Finalizers`, `PolicyStatus`, `PolicyActive` condition to ClusterAdmissionPolicies #76

Add `Finalizers`, `PolicyStatus`, `PolicyActive` condition to ClusterAdmissionPolicies #76

viccuad commented Sep 16, 2021 •

edited