From 6d8990611ae380bed018531ddcb903d9397ee1ff Mon Sep 17 00:00:00 2001 From: Aitor Perez Cedres <1515757+Zerpet@users.noreply.github.com> Date: Mon, 27 May 2024 18:52:15 +0100 Subject: [PATCH] Disallow updates of Queue arguments Queue arguments can't be updated in RabbitMQ. The webhook now disallows updates on queue arguments, matching the behaviour of RabbitMQ. Signed-off-by: Aitor Perez Cedres <1515757+Zerpet@users.noreply.github.com> --- Makefile | 37 +++++++++++---------- api/v1beta1/queue_webhook.go | 54 +++++++++++++++++++++++++++---- api/v1beta1/queue_webhook_test.go | 24 ++++++++++++++ 3 files changed, 90 insertions(+), 25 deletions(-) diff --git a/Makefile b/Makefile index 8f8b2b9c..c0936d7c 100644 --- a/Makefile +++ b/Makefile @@ -51,23 +51,27 @@ $(KUBEBUILDER_ASSETS): .PHONY: unit-tests unit-tests::install-tools ## Run unit tests -unit-test::$(KUBEBUILDER_ASSETS) -unit-test::generate -unit-test::fmt -unit-test::vet -unit-test::vuln -unit-test::manifests -unit-test::just-unit-tests +unit-tests::$(KUBEBUILDER_ASSETS) +unit-tests::generate +unit-tests::fmt +unit-tests::vet +unit-tests::manifests +unit-tests::just-unit-tests .PHONY: just-unit-tests just-unit-tests: ginkgo -r --randomize-all api/ internal/ rabbitmqclient/ .PHONY: integration-tests -integration-tests: install-tools $(KUBEBUILDER_ASSETS) generate fmt vet manifests ## Run integration tests. Use GINKGO_EXTRA="-some-arg" to append arguments to 'ginkgo run' - ginkgo -r --randomize-all -p $(GINKGO_EXTRA) controllers/ - -just-integration-tests: $(KUBEBUILDER_ASSETS) vet +integration-tests::install-tools ## Run integration tests. Use GINKGO_EXTRA="-some-arg" to append arguments to 'ginkgo run' +integration-tests::$(KUBEBUILDER_ASSETS) +integration-tests::generate +integration-tests::fmt +integration-tests::vet +integration-tests::manifests +integration-tests::just-integration-tests + +just-integration-tests: $(KUBEBUILDER_ASSETS) ginkgo --randomize-all -r -p $(GINKGO_EXTRA) controllers/ local-tests: unit-tests integration-tests ## Run all local tests (unit & integration) @@ -169,7 +173,9 @@ ifndef DOCKER_REGISTRY_SECRET $(error DOCKER_REGISTRY_SECRET is undefined: Name of Kubernetes secret in which to store the Docker registry username and password) endif -docker-build-dev: check-env-docker-repo git-commit-sha +GIT_COMMIT=$(shell git rev-parse --short HEAD)-dev + +docker-build-dev: check-env-docker-repo $(BUILD_KIT) buildx build --build-arg=GIT_COMMIT=$(GIT_COMMIT) -t $(DOCKER_REGISTRY_SERVER)/$(OPERATOR_IMAGE):$(GIT_COMMIT) . $(BUILD_KIT) push $(DOCKER_REGISTRY_SERVER)/$(OPERATOR_IMAGE):$(GIT_COMMIT) @@ -178,13 +184,6 @@ docker-registry-secret: check-env-docker-credentials operator-namespace @kubectl -n $(K8S_OPERATOR_NAMESPACE) create secret docker-registry $(DOCKER_REGISTRY_SECRET) --docker-server='$(DOCKER_REGISTRY_SERVER)' --docker-username="$$DOCKER_REGISTRY_USERNAME" --docker-password="$$DOCKER_REGISTRY_PASSWORD" || true @kubectl -n $(K8S_OPERATOR_NAMESPACE) patch serviceaccount messaging-topology-operator -p '{"imagePullSecrets": [{"name": "$(DOCKER_REGISTRY_SECRET)"}]}' -git-commit-sha: -ifeq ("", git diff --stat) -GIT_COMMIT=$(shell git rev-parse --short HEAD) -else -GIT_COMMIT=$(shell git rev-parse --short HEAD)- -endif - check-env-registry-server: ifndef DOCKER_REGISTRY_SERVER $(error DOCKER_REGISTRY_SERVER is undefined: URL of docker registry containing the Operator image (e.g. registry.my-company.com)) diff --git a/api/v1beta1/queue_webhook.go b/api/v1beta1/queue_webhook.go index 98fe56fc..1d8d65f3 100644 --- a/api/v1beta1/queue_webhook.go +++ b/api/v1beta1/queue_webhook.go @@ -2,7 +2,9 @@ package v1beta1 import ( "context" + "encoding/json" "fmt" + "maps" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -23,8 +25,8 @@ func (q *Queue) SetupWebhookWithManager(mgr ctrl.Manager) error { var _ webhook.CustomValidator = &Queue{} -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type -// either rabbitmqClusterReference.name or rabbitmqClusterReference.connectionSecret must be provided but not both +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. +// Either rabbitmqClusterReference.name or rabbitmqClusterReference.connectionSecret must be provided but not both func (q *Queue) ValidateCreate(_ context.Context, obj runtime.Object) (warnings admission.Warnings, err error) { inQueue, ok := obj.(*Queue) if !ok { @@ -38,10 +40,11 @@ func (q *Queue) ValidateCreate(_ context.Context, obj runtime.Object) (warnings return nil, q.Spec.RabbitmqClusterReference.validate(inQueue.RabbitReference()) } -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -// returns error type 'forbidden' for updates that the controller chooses to disallow: queue name/vhost/rabbitmqClusterReference -// returns error type 'invalid' for updates that will be rejected by rabbitmq server: queue types/autoDelete/durable -// queue arguments not handled because implementation couldn't change +// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. +// +// Returns error type 'forbidden' for updates that the controller chooses to disallow: queue name/vhost/rabbitmqClusterReference +// +// Returns error type 'invalid' for updates that will be rejected by rabbitmq server: queue types/autoDelete/durable func (q *Queue) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (warnings admission.Warnings, err error) { oldQueue, ok := oldObj.(*Queue) if !ok { @@ -94,10 +97,49 @@ func (q *Queue) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) )) } + if oldQueue.Spec.Arguments != nil && newQueue.Spec.Arguments == nil { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "arguments"), + newQueue.Spec.Arguments, + "queue arguments cannot be updated", + )) + } + + if oldQueue.Spec.Arguments == nil && newQueue.Spec.Arguments != nil { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "arguments"), + newQueue.Spec.Arguments, + "queue arguments cannot be updated", + )) + } + + if oldQueue.Spec.Arguments != nil && newQueue.Spec.Arguments != nil { + previousArgs := make(map[string]any) + err := json.Unmarshal(oldQueue.Spec.Arguments.Raw, &previousArgs) + if err != nil { + return nil, apierrors.NewInternalError(fmt.Errorf("error unmarshalling previous Queue arguments: %w", err)) + } + + updatedArgs := make(map[string]any) + err = json.Unmarshal(newQueue.Spec.Arguments.Raw, &updatedArgs) + if err != nil { + return nil, apierrors.NewInternalError(fmt.Errorf("error unmarshalling current Queue arguments: %w", err)) + } + + if !maps.Equal(previousArgs, updatedArgs) { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "arguments"), + newQueue.Spec.Arguments, + "queue arguments cannot be updated", + )) + } + } + if len(allErrs) == 0 { return nil, nil } + //goland:noinspection GoDfaNilDereference return nil, allErrs.ToAggregate() } diff --git a/api/v1beta1/queue_webhook_test.go b/api/v1beta1/queue_webhook_test.go index c420ae38..4100243f 100644 --- a/api/v1beta1/queue_webhook_test.go +++ b/api/v1beta1/queue_webhook_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" ) var _ = Describe("queue webhook", func() { @@ -23,6 +24,7 @@ var _ = Describe("queue webhook", func() { AutoDelete: true, DeleteIfEmpty: true, DeleteIfUnused: false, + Arguments: &runtime.RawExtension{Raw: []byte(`{"this-argument": "cannot be updated"}`)}, RabbitmqClusterReference: RabbitmqClusterReference{ Name: "some-cluster", }, @@ -130,5 +132,27 @@ var _ = Describe("queue webhook", func() { _, err := newQueue.ValidateUpdate(rootCtx, &queue, newQueue) Expect(err).To(MatchError(ContainSubstring("autoDelete cannot be updated"))) }) + + It("does not allow updates on queue arguments", func() { + By("not allowing value changes, nor adding new values") + newQueue := queue.DeepCopy() + newQueue.Spec.Arguments = &runtime.RawExtension{ + Raw: []byte(`{"this-argument": "really cant be updated", "adding-args": "not possible"}`), + } + _, err := newQueue.ValidateUpdate(rootCtx, &queue, newQueue) + Expect(err).To(MatchError(ContainSubstring("queue arguments cannot be updated"))) + + By("now allowing removal") + newQueue.Spec.Arguments = nil + _, err = newQueue.ValidateUpdate(rootCtx, &queue, newQueue) + Expect(err).To(MatchError(ContainSubstring("queue arguments cannot be updated"))) + + By("not allowing emptying the arguments") + newQueue.Spec.Arguments = &runtime.RawExtension{ + Raw: []byte(`{}`), + } + _, err = newQueue.ValidateUpdate(rootCtx, &queue, newQueue) + Expect(err).To(MatchError(ContainSubstring("queue arguments cannot be updated"))) + }) }) })