Skip to content

Commit

Permalink
Merge pull request #830 from rabbitmq/fix/inmutable-queue-args
Browse files Browse the repository at this point in the history 
Disallow updates of Queue arguments
  • Loading branch information
@Zerpet
Zerpet authored May 28, 2024
2 parents 1bf4ff7 + f13f359 commit 1a93d84
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 25 deletions.
37 changes: 18 additions & 19 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand All @@ -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))
Expand Down
54 changes: 48 additions & 6 deletions api/v1beta1/queue_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package v1beta1

import (
"context"
"encoding/json"
"fmt"
"maps"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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()
}

Expand Down
24 changes: 24 additions & 0 deletions api/v1beta1/queue_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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",
},
Expand Down Expand Up @@ -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("not 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")))
})
})
})

0 comments on commit 1a93d84

Please sign in to comment.