From 6deb19d75fc34bc652795a123ce0eb9ef7d2361c Mon Sep 17 00:00:00 2001 From: Tyler Slaton Date: Wed, 13 Apr 2022 18:06:12 -0400 Subject: [PATCH] feat(crdvalidator): nesting crdvalidator components to better integrate with core Rukpak Signed-off-by: Tyler Slaton --- Dockerfile | 1 + Makefile | 37 +--- .../{internal => }/handlers/crd.go | 2 +- cmd/crdvalidator/main.go | 2 +- .../test/e2e/crdvalidator_suite_test.go | 39 ---- .../internal => internal}/crd/crd.go | 0 internal/util/crd.go | 192 ------------------ .../crdvalidator}/00_namespace.yaml | 0 .../crdvalidator}/01_cluster_role.yaml | 0 .../crdvalidator}/01_service_account.yaml | 0 .../02_cluster_role_binding.yaml | 0 .../crdvalidator}/03_service.yaml | 0 .../crdvalidator}/04_webhook.yaml | 0 .../crdvalidator}/05_deployment.yaml | 4 +- manifests/crdvalidator/kustomization.yml | 8 + .../test => test}/e2e/crdvalidator_test.go | 10 +- 16 files changed, 23 insertions(+), 272 deletions(-) rename cmd/crdvalidator/{internal => }/handlers/crd.go (97%) delete mode 100644 cmd/crdvalidator/test/e2e/crdvalidator_suite_test.go rename {cmd/crdvalidator/internal => internal}/crd/crd.go (100%) delete mode 100644 internal/util/crd.go rename {cmd/crdvalidator/manifests => manifests/crdvalidator}/00_namespace.yaml (100%) rename {cmd/crdvalidator/manifests => manifests/crdvalidator}/01_cluster_role.yaml (100%) rename {cmd/crdvalidator/manifests => manifests/crdvalidator}/01_service_account.yaml (100%) rename {cmd/crdvalidator/manifests => manifests/crdvalidator}/02_cluster_role_binding.yaml (100%) rename {cmd/crdvalidator/manifests => manifests/crdvalidator}/03_service.yaml (100%) rename {cmd/crdvalidator/manifests => manifests/crdvalidator}/04_webhook.yaml (100%) rename {cmd/crdvalidator/manifests => manifests/crdvalidator}/05_deployment.yaml (80%) create mode 100644 manifests/crdvalidator/kustomization.yml rename {cmd/crdvalidator/test => test}/e2e/crdvalidator_test.go (95%) diff --git a/Dockerfile b/Dockerfile index 74190813..07fd1cf9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -4,6 +4,7 @@ WORKDIR / COPY plain plain COPY unpack unpack COPY core core +COPY crdvalidator crdvalidator EXPOSE 8080 ENTRYPOINT ["/plain"] diff --git a/Makefile b/Makefile index 3493b243..20c2eb52 100644 --- a/Makefile +++ b/Makefile @@ -102,6 +102,9 @@ install-apis: cert-mgr generate kustomize ## Install the core rukpak CRDs install-plain: install-apis ## Install the rukpak CRDs and the plain provisioner $(KUSTOMIZE) build manifests/provisioners/plain | kubectl apply -f - +install-crdvalidator: cert-mgr kustomize ## Install the crdvalidator webhook manifests to the current cluster. + $(KUSTOMIZE) build manifests/crdvalidator | kubectl apply -f - + install: install-plain ## Install all rukpak core CRDs and provisioners deploy: install-apis install-crdvalidator ## Deploy the operator to the current cluster @@ -122,7 +125,7 @@ cert-mgr: ## Install the certification manager # Binary builds VERSION_FLAGS=-ldflags "-X $(VERSION_PATH).GitCommit=$(GIT_COMMIT)" -build: plain unpack core +build: plain unpack core crdvalidator plain: CGO_ENABLED=0 go build $(VERSION_FLAGS) -o $(BIN_DIR)/$@ ./internal/provisioner/plain @@ -133,6 +136,9 @@ unpack: core: CGO_ENABLED=0 go build $(VERSION_FLAGS) -o $(BIN_DIR)/$@ ./cmd/core/... +crdvalidator: + CGO_ENABLED=0 go build $(VERSION_FLAGS) -o $(BIN_DIR)/$@ ./cmd/crdvalidator + build-container: export GOOS=linux build-container: BIN_DIR:=$(BIN_DIR)/$(GOOS) build-container: build ## Builds provisioner container image locally @@ -159,35 +165,6 @@ kind-load-bundles: ## Load the e2e testdata container images into a kind cluster kind-load: ## Loads the currently constructed image onto the cluster ${KIND} load docker-image $(IMAGE) --name $(KIND_CLUSTER_NAME) -################################## -# CRDValidator Webhook Targets # -################################## -.PHONY: crdvalidator build-crdvalidator-image kind-load-crdvalidator install-crdvalidator deploy-crdvalidator - -##@ crdvalidator: - -crdvalidator: - CGO_ENABLED=0 go build $(VERSION_FLAGS) -o $(BIN_DIR)/$@ ./cmd/crdvalidator - -CRD_WEBHOOK_IMAGE_REPO=quay.io/operator-framework/crd-validation-webhook -CRD_WEBHOOK_IMAGE=$(CRD_WEBHOOK_IMAGE_REPO):main -build-crdvalidator-image: ## Builds crd-validation-webhook container image locally - docker build -f Dockerfile.crdvalidator -t $(CRD_WEBHOOK_IMAGE) . - -kind-load-crdvalidator: ## Loads the currently constructed crd-validation-webhook onto the cluster - ${KIND} load docker-image $(CRD_WEBHOOK_IMAGE) --name $(KIND_CLUSTER_NAME) - -install-crdvalidator: build-crdvalidator-image kind-load-crdvalidator ## Install the crdvalidator manifests to the current cluster. Requires that cert-manager is installed prior. - kubectl apply -f cmd/crdvalidator/manifests - -deploy-crdvalidator: cert-mgr install-crdvalidator ## Install the crdvalidator manifests to the current cluster and ensures that cert-manager is installed prior. - -test-crdvalidator-e2e: ginkgo ## Run the crdvalidator e2e tests. Assumes you have already installed the crdvalidator webhook. - $(GINKGO) -trace -progress $(FOCUS) cmd/crdvalidator/test/e2e - -crdvalidator-e2e: KIND_CLUSTER_NAME=crdvalidator-e2e -crdvalidator-e2e: kind-cluster deploy-crdvalidator test-crdvalidator-e2e ## Run the crdvalidator e2e tests. Assumes you have already installed the crdvalidator webhook. - ################ # Hack / Tools # ################ diff --git a/cmd/crdvalidator/internal/handlers/crd.go b/cmd/crdvalidator/handlers/crd.go similarity index 97% rename from cmd/crdvalidator/internal/handlers/crd.go rename to cmd/crdvalidator/handlers/crd.go index 60226ac1..304a539c 100644 --- a/cmd/crdvalidator/internal/handlers/crd.go +++ b/cmd/crdvalidator/handlers/crd.go @@ -22,7 +22,7 @@ import ( "net/http" "github.com/go-logr/logr" - "github.com/operator-framework/rukpak/cmd/crdvalidator/internal/crd" + "github.com/operator-framework/rukpak/internal/crd" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" diff --git a/cmd/crdvalidator/main.go b/cmd/crdvalidator/main.go index 3346fc96..b31c816a 100644 --- a/cmd/crdvalidator/main.go +++ b/cmd/crdvalidator/main.go @@ -19,7 +19,7 @@ package main import ( "os" - "github.com/operator-framework/rukpak/cmd/crdvalidator/internal/handlers" + "github.com/operator-framework/rukpak/cmd/crdvalidator/handlers" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/config" diff --git a/cmd/crdvalidator/test/e2e/crdvalidator_suite_test.go b/cmd/crdvalidator/test/e2e/crdvalidator_suite_test.go deleted file mode 100644 index 9664e578..00000000 --- a/cmd/crdvalidator/test/e2e/crdvalidator_suite_test.go +++ /dev/null @@ -1,39 +0,0 @@ -package e2e - -import ( - "testing" - "time" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/runtime" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -var ( - c client.Client -) - -func TestE2E(t *testing.T) { - RegisterFailHandler(Fail) - SetDefaultEventuallyTimeout(60 * time.Second) - SetDefaultEventuallyPollingInterval(1 * time.Second) - RunSpecs(t, "crdvalidator E2E Suite") -} - -var _ = BeforeSuite(func() { - config := ctrl.GetConfigOrDie() - - scheme := runtime.NewScheme() - err := apiextensionsv1.AddToScheme(scheme) - Expect(err).To(BeNil()) - - err = corev1.AddToScheme(scheme) - Expect(err).To(BeNil()) - - c, err = client.New(config, client.Options{Scheme: scheme}) - Expect(err).To(BeNil()) -}) diff --git a/cmd/crdvalidator/internal/crd/crd.go b/internal/crd/crd.go similarity index 100% rename from cmd/crdvalidator/internal/crd/crd.go rename to internal/crd/crd.go diff --git a/internal/util/crd.go b/internal/util/crd.go deleted file mode 100644 index a27216f1..00000000 --- a/internal/util/crd.go +++ /dev/null @@ -1,192 +0,0 @@ -package util - -import ( - "context" - "fmt" - "reflect" - - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/client-go/util/retry" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" -) - -func CreateOrUpdateCRD(ctx context.Context, cl client.Client, crd *apiextensionsv1.CustomResourceDefinition) (controllerutil.OperationResult, error) { - createCRD := crd.DeepCopy() - createErr := cl.Create(ctx, createCRD) - if createErr == nil { - return controllerutil.OperationResultCreated, nil - } - if !apierrors.IsAlreadyExists(createErr) { - return controllerutil.OperationResultNone, createErr - } - if updateErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - currentCRD := &apiextensionsv1.CustomResourceDefinition{} - if err := cl.Get(ctx, client.ObjectKeyFromObject(crd), currentCRD); err != nil { - return err - } - crd.SetResourceVersion(currentCRD.GetResourceVersion()) - - if err := validateCRDCompatibility(ctx, cl, currentCRD, crd); err != nil { - return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", crd.Name, err) - } - - // check to see if stored versions changed and whether the upgrade could cause potential data loss - safe, err := safeStorageVersionUpgrade(currentCRD, crd) - if !safe { - return fmt.Errorf("risk of data loss updating %q: %w", crd.Name, err) - } - if err != nil { - return fmt.Errorf("checking CRD for potential data loss updating %q: %w", crd.Name, err) - } - - // Update CRD to new version - if err := cl.Update(ctx, crd); err != nil { - return fmt.Errorf("error updating CRD %q: %w", crd.Name, err) - } - return nil - }); updateErr != nil { - return controllerutil.OperationResultNone, updateErr - } - return controllerutil.OperationResultUpdated, nil -} - -func keys(m map[string]apiextensionsv1.CustomResourceDefinitionVersion) sets.String { - return sets.StringKeySet(m) -} - -func validateCRDCompatibility(ctx context.Context, cl client.Client, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error { - // Cases to test: - // New CRD removes version that Old CRD had => Must ensure nothing is stored at removed version - // New CRD changes a version that Old CRD has => Must validate existing CRs with new schema - // 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) - - oldVersions := map[string]apiextensionsv1.CustomResourceDefinitionVersion{} - newVersions := map[string]apiextensionsv1.CustomResourceDefinitionVersion{} - - for _, v := range oldCRD.Spec.Versions { - oldVersions[v.Name] = v - } - for _, v := range newCRD.Spec.Versions { - newVersions[v.Name] = v - } - - existingStoredVersions := sets.NewString(oldCRD.Status.StoredVersions...) - removedVersions := keys(oldVersions).Difference(keys(newVersions)) - invalidRemovedVersions := existingStoredVersions.Intersection(removedVersions) - if invalidRemovedVersions.Len() > 0 { - return fmt.Errorf("cannot remove stored versions %v", invalidRemovedVersions.List()) - } - - similarVersions := keys(oldVersions).Intersection(keys(newVersions)) - diffVersions := sets.NewString() - for _, v := range similarVersions.List() { - if !reflect.DeepEqual(oldVersions[v].Schema, newVersions[v].Schema) { - diffVersions.Insert(v) - } - } - convertedCRD := &apiextensions.CustomResourceDefinition{} - if err := apiextensionsv1.Convert_v1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil { - return err - } - for _, v := range diffVersions.List() { - oldV := oldVersions[v] - if oldV.Served { - listGVK := schema.GroupVersionKind{Group: oldCRD.Spec.Group, Version: v, Kind: oldCRD.Spec.Names.ListKind} - err := validateExistingCRs(ctx, cl, listGVK, newVersions[v]) - if err != nil { - return err - } - } - } - - // If the new CRD has no conversion configured or a "None" conversion strategy, we need to check to be sure that the - // new schema validates all of the existing CRs of the existing versions. - addedVersions := keys(newVersions).Difference(keys(oldVersions)) - if addedVersions.Len() > 0 && (newCRD.Spec.Conversion == nil || newCRD.Spec.Conversion.Strategy == apiextensionsv1.NoneConverter) { - for _, va := range addedVersions.List() { - newV := newVersions[va] - for _, vs := range similarVersions.List() { - oldV := oldVersions[vs] - if oldV.Served { - listGVK := schema.GroupVersionKind{Group: oldCRD.Spec.Group, Version: oldV.Name, Kind: oldCRD.Spec.Names.ListKind} - err := validateExistingCRs(ctx, cl, listGVK, newV) - if err != nil { - return err - } - } - } - } - } - - return nil -} - -func validateExistingCRs(ctx context.Context, dynamicClient client.Client, listGVK schema.GroupVersionKind, newVersion apiextensionsv1.CustomResourceDefinitionVersion) error { - convertedVersion := &apiextensions.CustomResourceDefinitionVersion{} - if err := apiextensionsv1.Convert_v1_CustomResourceDefinitionVersion_To_apiextensions_CustomResourceDefinitionVersion(&newVersion, convertedVersion, nil); err != nil { - return err - } - - crList := &unstructured.UnstructuredList{} - crList.SetGroupVersionKind(listGVK) - if err := dynamicClient.List(ctx, crList); err != nil { - return fmt.Errorf("error listing objects for %s: %w", listGVK, err) - } - for _, cr := range crList.Items { - validator, _, err := validation.NewSchemaValidator(convertedVersion.Schema) - if err != nil { - return fmt.Errorf("error creating validator for the schema of version %q: %w", newVersion.Name, err) - } - err = validation.ValidateCustomResource(field.NewPath(""), cr.UnstructuredContent(), validator).ToAggregate() - if err != nil { - return fmt.Errorf("existing custom object %s/%s failed validation for new schema version %s: %w", cr.GetNamespace(), cr.GetName(), newVersion.Name, err) - } - } - return nil -} - -// safeStorageVersionUpgrade determines whether the new CRD spec includes all the storage versions of the existing on-cluster CRD. -// For each stored version in the status of the CRD on the cluster (there will be at least one) - each version must exist in the spec of the new CRD that is being installed. -// See https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#upgrade-existing-objects-to-a-new-stored-version. -func safeStorageVersionUpgrade(existingCRD, newCRD *apiextensionsv1.CustomResourceDefinition) (bool, error) { - existingStatusVersions, newSpecVersions := getStoredVersions(existingCRD, newCRD) - if newSpecVersions == nil { - return false, fmt.Errorf("could not find any versions in the new CRD") - } - if existingStatusVersions == nil { - // every on-cluster CRD should have at least one stored version in its status - // in the case where there are no existing stored versions, checking against new versions is not relevant - return true, nil - } - - for name := range existingStatusVersions { - if _, ok := newSpecVersions[name]; !ok { - // a storage version in the status of the old CRD is not present in the spec of the new CRD - // potential data loss of CRs without a storage migration - throw error and block the CRD upgrade - return false, fmt.Errorf("new CRD removes version %s that is listed as a stored version on the existing CRD", name) - } - } - - return true, nil -} - -// getStoredVersions returns the storage versions listed in the status of the old on-cluster CRD -// and all the versions listed in the spec of the new CRD. -func getStoredVersions(oldCRD, newCRD *apiextensionsv1.CustomResourceDefinition) (sets.String, sets.String) { - newSpecVersions := sets.NewString() - for _, version := range newCRD.Spec.Versions { - newSpecVersions.Insert(version.Name) - } - - return sets.NewString(oldCRD.Status.StoredVersions...), newSpecVersions -} diff --git a/cmd/crdvalidator/manifests/00_namespace.yaml b/manifests/crdvalidator/00_namespace.yaml similarity index 100% rename from cmd/crdvalidator/manifests/00_namespace.yaml rename to manifests/crdvalidator/00_namespace.yaml diff --git a/cmd/crdvalidator/manifests/01_cluster_role.yaml b/manifests/crdvalidator/01_cluster_role.yaml similarity index 100% rename from cmd/crdvalidator/manifests/01_cluster_role.yaml rename to manifests/crdvalidator/01_cluster_role.yaml diff --git a/cmd/crdvalidator/manifests/01_service_account.yaml b/manifests/crdvalidator/01_service_account.yaml similarity index 100% rename from cmd/crdvalidator/manifests/01_service_account.yaml rename to manifests/crdvalidator/01_service_account.yaml diff --git a/cmd/crdvalidator/manifests/02_cluster_role_binding.yaml b/manifests/crdvalidator/02_cluster_role_binding.yaml similarity index 100% rename from cmd/crdvalidator/manifests/02_cluster_role_binding.yaml rename to manifests/crdvalidator/02_cluster_role_binding.yaml diff --git a/cmd/crdvalidator/manifests/03_service.yaml b/manifests/crdvalidator/03_service.yaml similarity index 100% rename from cmd/crdvalidator/manifests/03_service.yaml rename to manifests/crdvalidator/03_service.yaml diff --git a/cmd/crdvalidator/manifests/04_webhook.yaml b/manifests/crdvalidator/04_webhook.yaml similarity index 100% rename from cmd/crdvalidator/manifests/04_webhook.yaml rename to manifests/crdvalidator/04_webhook.yaml diff --git a/cmd/crdvalidator/manifests/05_deployment.yaml b/manifests/crdvalidator/05_deployment.yaml similarity index 80% rename from cmd/crdvalidator/manifests/05_deployment.yaml rename to manifests/crdvalidator/05_deployment.yaml index 28101fed..079f9c45 100644 --- a/cmd/crdvalidator/manifests/05_deployment.yaml +++ b/manifests/crdvalidator/05_deployment.yaml @@ -14,7 +14,9 @@ spec: spec: serviceAccountName: crd-validation-webhook containers: - - image: quay.io/operator-framework/crd-validation-webhook:main + - image: quay.io/operator-framework/plain-provisioner:latest + imagePullPolicy: IfNotPresent + command: ["/crdvalidator"] name: crd-validation-webhook volumeMounts: - name: tls diff --git a/manifests/crdvalidator/kustomization.yml b/manifests/crdvalidator/kustomization.yml new file mode 100644 index 00000000..eb8798fb --- /dev/null +++ b/manifests/crdvalidator/kustomization.yml @@ -0,0 +1,8 @@ +resources: + - 00_namespace.yaml + - 01_cluster_role.yaml + - 01_service_account.yaml + - 02_cluster_role_binding.yaml + - 03_service.yaml + - 04_webhook.yaml + - 05_deployment.yaml diff --git a/cmd/crdvalidator/test/e2e/crdvalidator_test.go b/test/e2e/crdvalidator_test.go similarity index 95% rename from cmd/crdvalidator/test/e2e/crdvalidator_test.go rename to test/e2e/crdvalidator_test.go index 647f8c02..512c3e56 100644 --- a/cmd/crdvalidator/test/e2e/crdvalidator_test.go +++ b/test/e2e/crdvalidator_test.go @@ -46,11 +46,8 @@ var _ = Describe("crd validation webhook", func() { }, ) - err := c.Create(ctx, crd) - Expect(err).To(BeNil()) - Eventually(func() error { - return c.Get(ctx, client.ObjectKeyFromObject(crd), crd) + return c.Create(ctx, crd) }).Should(Succeed(), "should be able to create a safe crd but was not") }) @@ -126,11 +123,8 @@ var _ = Describe("crd validation webhook", func() { }, ) - err := c.Create(ctx, crd) - Expect(err).To(BeNil()) - Eventually(func() error { - return c.Get(ctx, client.ObjectKeyFromObject(crd), crd) + return c.Create(ctx, crd) }).Should(Succeed(), "should be able to create a safe crd but was not") })