feat(crdvalidator): adding webhook to ensure safety of crd create/upgrades #213

tylerslaton · 2022-04-04T15:00:01Z

NOTES

This is an alternative approach to [WIP] feat(bundleinstance): add call to crd validator inside of controller #199
Adding a few E2E test cases for this logic
This webhook is intended to eventually live in its own repository separate from RukPak components. The implementation has been done with that in mind.

Summary

Doing a few things here that we should talk through:

Creating a new webhook (crdvalidator) under the cmd directory. There has been talk of moving this webhook into its own repo in the future and this should make that very easy.
Adding Makefile targets to deploy the webhook
Moving the crd util package along side the webhook code.

Open Questions

Should this have an option to force crd applies through or would that option just be removing this webhook?

exdx

Very nice work! This looks really good.

For an example idea of how an e2e could look like, check out https://github.com/operator-framework/operator-lifecycle-manager/blob/ac3aa278ddc8d59add546bf9e33e583f17d62bb0/test/e2e/crd_e2e_test.go#L115

cmd/crdvalidator/manifests/00_service_account.yaml

cmd/crdvalidator/internal/handlers/crd.go

cmd/crdvalidator/manifests/04_webhook.yaml

cmd/crdvalidator/manifests/00_namespace.yaml

cmd/crdvalidator/internal/crd/crd.go

Makefile

exdx · 2022-04-08T20:59:27Z

Dockerfile.crdvalidator

+
+RUN make bin/crdvalidator
+
+FROM gcr.io/distroless/static:debug


Does the webhook need a shell, is there a reason to use the :debug tag? Best practices for building container images are to not include a shell unless it's necessary for the application somehow.

🤔 It doesn't, you're right. I was mainly copying this over from the root Dockerfile. Do you think that means we should be moving that container away from having a shell as well?

I think so, yes. I would be for it personally.

I think @exdx brings up a good point but I wonder whether it's good to use a debug tag in the short term while we continue to iterate, and then later on work towards removing it.

Makefile

cmd/crdvalidator/internal/crd/crd.go

cmd/crdvalidator/manifests/01_cluster_role.yaml

cmd/crdvalidator/test/e2e/crdvalidator_test.go

exdx · 2022-04-08T21:14:36Z

cmd/crdvalidator/internal/crd/crd.go

+//   3. New CRD adds a version that Old CRD does not have =>
+//      - If conversion strategy is None, ensure existing CRs validate with new schema.
+//      - If conversion strategy is Webhook, allow update (assume webhook handles conversion correctly)
+func validateCRDCompatibility(ctx context.Context, cl client.Client, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {


nit: all the code in this function and following it is fairly in-depth, and could use some unit tests as a follow-up. Since it came from elsewhere, largely from OLM, it's fairly safe to rely on, but we could test it more on the rukpak side as a follow-up.

I agree, I'll make a follow-up item for this PR that adds unit testing for this particular package.

exdx

Looks great, thanks for all the work on this. Let's get one more approval before merging.

Makefile

timflannagan · 2022-04-12T19:18:13Z

cmd/crdvalidator/test/e2e/crdvalidator_test.go

+			It("should deny admission", func() {
+				Eventually(func() bool {
+					if err := c.Get(ctx, client.ObjectKeyFromObject(crd), crd); err != nil {
+						return false
+					}
+
+					crd.Spec.Versions = []apiextensionsv1.CustomResourceDefinitionVersion{
+						{
+							Name:    "v1alpha2",
+							Served:  true,
+							Storage: true,
+							Schema: &apiextensionsv1.CustomResourceValidation{
+								OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+									Type:        "object",
+									Description: "my crd schema",
+								},
+							},
+						},
+					}
+
+					err := c.Create(ctx, crd)
+					if err == nil {
+						return false
+					}
+
+					return errorsLooselyEqual(err, errors.New("cannot remove stored versions"))
+				}).Should(BeTrue())
+			})


A couple of things:

I'm slightly confused whether we should be firing off an update request vs. a create one

Can we return an error here and use gomega matchers to enforce the expected error message, e.g.

rukpak/test/e2e/plain_provisioner_test.go

Line 838 in bf174ef

ContainSubstring(`no matches for kind "CatalogSource" in version "operators.coreos.com/v1alpha1"`),

Yeah so the Update here was attempted and continuously failed. If you have the Create it succeeds - however, this is not what we should really be expecting and just obfuscated the issue. Something under-the-hood in Kube is now blocking unsafe storage removals and thus the error that we are expecting is not coming through. This is because the Update request does not seem to even hit the webhook and Kube returns its own error. I'm making some changes in an upcoming commit along with a todo comment to address this in the future by reevaluating if we need to ensure safe storage removals or if Kube does it by default now.

cmd/crdvalidator/test/e2e/crdvalidator_test.go

go.mod

cmd/crdvalidator/test/e2e/crdvalidator_test.go

timflannagan

Okay I just did a quick passthrough of these changes. I think my main takeaways were the following:

I'm not a huge fan of the current directory structure as it's difficult to discover where packages live in what directories and we don't have a concrete want/need to split this kind of component into it's own repository, or attempt to introduce it to upstream k/k.
The make run command doesn't install this component like I would've expected
It's not entirely evident how we could disable this behavior dynamically (or whether that's something we'd need to support)
We can punt on the debug/static distroless tag discussion
I think we can drop that k8s.io/apiserver dependency that was introduced with these changes
We're potentially introducing a confusing installation UX by having a deploy and install Makefile targets
It doesn't look like the CRD validator e2e test Makefile target loads the kind-load-crdvalidator target unless I'm reading these changes wrong
We'll need to update goreleaser to build this new executable package

I also ran these changes locally and ran into e2e failures. I haven't dived too deep into those yet though.

timflannagan · 2022-04-12T19:43:26Z

FYI - It looks like I ran into a container image build warning locally too:

Step 3/14 : COPY go.mod go.mod
 ---> 73952c5724e3
Step 4/14 : COPY go.sum go.sum
 ---> 358d31c7b255
Step 5/14 : RUN go mod download
 ---> Running in eeb714ec03fd
Removing intermediate container eeb714ec03fd
 ---> d370149889ea
Step 6/14 : COPY Makefile Makefile
 ---> ddfb93c559ff
Step 7/14 : COPY cmd/crdvalidator cmd/crdvalidator
 ---> 1ba657d6e072
Step 8/14 : RUN make bin/crdvalidator
 ---> Running in 49439f4699bf
fatal: not a git repository (or any of the parent directories): .git
CGO_ENABLED=0 go build -ldflags "-X github.com/operator-framework/rukpak/internal/version.GitCommit=" -o bin/crdvalidator ./cmd/crdvalidator

tylerslaton · 2022-04-12T19:44:56Z

Okay I just did a quick passthrough of these changes. I think my main takeaways were the following:

The make run command doesn't install this component like I would've expected

It doesn't look like the CRD validator e2e test Makefile target loads the kind-load-crdvalidator target unless I'm reading these changes wrong

I also ran these changes locally and ran into e2e failures. I haven't dived too deep into those yet though.

Yep, so I recently rebased this PR and it removed my addition to make run that had the crdvalidator being deployed. This resulted in the crdvalidators e2e suite not passing as a side effect. I'll work to address the other issues you listed.

Makefile

cmd/crdvalidator/test/e2e/crdvalidator_test.go

Makefile

timflannagan

I think we're super close and then I'm ready to approve:

Can we remove that test-crdvalidator-e2e target which appears to be unused?
Can we remove that unused Dockerfile.crdvalidator now that we've consolidated this component in the current repository structure?

timflannagan · 2022-04-13T22:29:20Z

Note: it looks like this branch references out-of-date files compared to the main branch. It might be worth rebasing against the main branch, and respinning e2e once more to make sense we're not potentially silently regressing, but I'll leave that up to you as I just approved this.

timflannagan · 2022-04-13T22:29:45Z

Really nice work btw @tylerslaton

tylerslaton · 2022-04-14T00:10:09Z

Note: it looks like this branch references out-of-date files compared to the main branch. It might be worth rebasing against the main branch, and respinning e2e once more to make sense we're not potentially silently regressing, but I'll leave that up to you as I just approved this.

Rebased and pushed. Looks like everything is good to go. Going to squash all the commits together. @exdx anything you want to touch on before we merge this?

Add a new webhook that integrates the CRD validation logic into a webhook. With this enabled on cluster, bundle installs and pivots can be guranteed as safe by the webhook. It works by intercepting all CRD create/update requests that come through and declining or admitting them based on the internal/crd package. Signed-off-by: Tyler Slaton <tyslaton@redhat.com>

exdx

Looks great, just some small comments.

exdx · 2022-04-14T14:37:07Z

Makefile

 install: install-plain ## Install all rukpak core CRDs and provisioners

-deploy: install-apis ## Deploy the operator to the current cluster
+deploy: install-apis install-crdvalidator ## Deploy the operator to the current cluster


nit: install-apis already calls cert-mgr so we end up installing cert-manager twice on deploy. I'm not sure a good way around this -- is there a way to only run a target if it hasn't already been run?

I was also thinking on this. I haven't come up with a great solution other than having all the cert-mgr dependent installs in a single target.

That sounds reasonable, we can have something like install-webhooks which calls cert-mgr under the hood. I suppose we can address this is in a follow-up

Totally, but I agree with you. Its an easy addition that I can just tack onto the follow-ups.

exdx · 2022-04-14T14:39:27Z

test/e2e/crdvalidator_test.go

+		// it through. This is fine and ultimately what the safe storage logic of the
+		// CRDValidator was designed to prevent but is unknown why it is occurring. We
+		// Should come back to this test case, figure out what is preventing it from
+		// hitting the webhook and decide if we want to keep that logic or not.


can we open an issue to track this and put the link to it here in the test comment?

I believe that this action will create a issue for it:

https://github.com/operator-framework/rukpak/blob/main/.github/workflows/todo.yaml

exdx · 2022-04-14T14:43:42Z

test/e2e/crdvalidator_test.go

+		// CRDValidator was designed to prevent but is unknown why it is occurring. We
+		// Should come back to this test case, figure out what is preventing it from
+		// hitting the webhook and decide if we want to keep that logic or not.
+		PWhen("an incoming crd event removes a stored version", func() {


I know we had some issues getting this test to work -- but we did confirm the test case where the schema changed in an incompatible way between upgrades. It's worth adding that test so we have at least one scenario where the admission was denied due to the webhook in our tests. This can be added in a follow-up though.

Yep - good call out. I'm also cool with a follow-up here.

Yep I'm planning on doing a follow-up PR like the recent git work after this lands.

timflannagan · 2022-04-14T15:37:45Z

LGTM again - do you want to push this through @tylerslaton.

tylerslaton · 2022-04-14T15:38:24Z

@timflannagan Yep, I'm good with this going through and addressing the remaining comments in a follow up. Merging now.

tylerslaton requested a review from a team as a code owner April 4, 2022 15:00

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2022

tylerslaton force-pushed the crd-admission-webhook branch 2 times, most recently from 711a12c to 834a63c Compare April 4, 2022 15:10

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2022

tylerslaton force-pushed the crd-admission-webhook branch 2 times, most recently from f4c2747 to 4a84301 Compare April 4, 2022 15:12

exdx reviewed Apr 4, 2022

View reviewed changes

tylerslaton mentioned this pull request Apr 4, 2022

[WIP] feat(bundleinstance): add call to crd validator inside of controller #199

Closed

tylerslaton force-pushed the crd-admission-webhook branch 2 times, most recently from 440bf8f to fd9c2f7 Compare April 4, 2022 22:00

akihikokuroda reviewed Apr 5, 2022

View reviewed changes

cmd/crdvalidator/manifests/00_namespace.yaml Outdated Show resolved Hide resolved

perdasilva reviewed Apr 6, 2022

View reviewed changes

cmd/crdvalidator/internal/crd/crd.go Outdated Show resolved Hide resolved

perdasilva reviewed Apr 6, 2022

View reviewed changes

cmd/crdvalidator/internal/crd/crd.go Outdated Show resolved Hide resolved

perdasilva reviewed Apr 6, 2022

View reviewed changes

cmd/crdvalidator/internal/crd/crd.go Outdated Show resolved Hide resolved

perdasilva reviewed Apr 6, 2022

View reviewed changes

cmd/crdvalidator/internal/crd/crd.go Outdated Show resolved Hide resolved

timflannagan reviewed Apr 6, 2022

View reviewed changes

cmd/crdvalidator/internal/crd/crd.go Outdated Show resolved Hide resolved

timflannagan reviewed Apr 6, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

tylerslaton force-pushed the crd-admission-webhook branch 7 times, most recently from 7a4760f to 97000b1 Compare April 8, 2022 20:53

exdx reviewed Apr 8, 2022

View reviewed changes

tylerslaton force-pushed the crd-admission-webhook branch from 97000b1 to f0fb36f Compare April 11, 2022 20:19

exdx approved these changes Apr 12, 2022

View reviewed changes

tylerslaton force-pushed the crd-admission-webhook branch from f0fb36f to bf174ef Compare April 12, 2022 14:38