Skip to content

Commit 1a93d84

Browse files
ZerpetZerpet
authored
Merge pull request #830 from rabbitmq/fix/inmutable-queue-args
Disallow updates of Queue arguments
2 parents 1bf4ff7 + f13f359 commit 1a93d84

File tree

3 files changed

+90
-25
lines changed

3 files changed

+90
-25
lines changed

‎Makefile

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -51,23 +51,27 @@ $(KUBEBUILDER_ASSETS):
5151

5252
.PHONY: unit-tests
5353
unit-tests::install-tools ## Run unit tests
54-
unit-test::$(KUBEBUILDER_ASSETS)
55-
unit-test::generate
56-
unit-test::fmt
57-
unit-test::vet
58-
unit-test::vuln
59-
unit-test::manifests
60-
unit-test::just-unit-tests
54+
unit-tests::$(KUBEBUILDER_ASSETS)
55+
unit-tests::generate
56+
unit-tests::fmt
57+
unit-tests::vet
58+
unit-tests::manifests
59+
unit-tests::just-unit-tests
6160

6261
.PHONY: just-unit-tests
6362
just-unit-tests:
6463
ginkgo -r --randomize-all api/ internal/ rabbitmqclient/
6564

6665
.PHONY: integration-tests
67-
integration-tests: install-tools $(KUBEBUILDER_ASSETS) generate fmt vet manifests ## Run integration tests. Use GINKGO_EXTRA="-some-arg" to append arguments to 'ginkgo run'
68-
ginkgo -r --randomize-all -p $(GINKGO_EXTRA) controllers/
69-
70-
just-integration-tests: $(KUBEBUILDER_ASSETS) vet
66+
integration-tests::install-tools ## Run integration tests. Use GINKGO_EXTRA="-some-arg" to append arguments to 'ginkgo run'
67+
integration-tests::$(KUBEBUILDER_ASSETS)
68+
integration-tests::generate
69+
integration-tests::fmt
70+
integration-tests::vet
71+
integration-tests::manifests
72+
integration-tests::just-integration-tests
73+
74+
just-integration-tests: $(KUBEBUILDER_ASSETS)
7175
ginkgo --randomize-all -r -p $(GINKGO_EXTRA) controllers/
7276

7377
local-tests: unit-tests integration-tests ## Run all local tests (unit & integration)
@@ -169,7 +173,9 @@ ifndef DOCKER_REGISTRY_SECRET
169173
$(error DOCKER_REGISTRY_SECRET is undefined: Name of Kubernetes secret in which to store the Docker registry username and password)
170174
endif
171175

172-
docker-build-dev: check-env-docker-repo git-commit-sha
176+
GIT_COMMIT=$(shell git rev-parse --short HEAD)-dev
177+
178+
docker-build-dev: check-env-docker-repo
173179
$(BUILD_KIT) buildx build --build-arg=GIT_COMMIT=$(GIT_COMMIT) -t $(DOCKER_REGISTRY_SERVER)/$(OPERATOR_IMAGE):$(GIT_COMMIT) .
174180
$(BUILD_KIT) push $(DOCKER_REGISTRY_SERVER)/$(OPERATOR_IMAGE):$(GIT_COMMIT)
175181

@@ -178,13 +184,6 @@ docker-registry-secret: check-env-docker-credentials operator-namespace
178184
@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
179185
@kubectl -n $(K8S_OPERATOR_NAMESPACE) patch serviceaccount messaging-topology-operator -p '{"imagePullSecrets": [{"name": "$(DOCKER_REGISTRY_SECRET)"}]}'
180186

181-
git-commit-sha:
182-
ifeq ("", git diff --stat)
183-
GIT_COMMIT=$(shell git rev-parse --short HEAD)
184-
else
185-
GIT_COMMIT=$(shell git rev-parse --short HEAD)-
186-
endif
187-
188187
check-env-registry-server:
189188
ifndef DOCKER_REGISTRY_SERVER
190189
$(error DOCKER_REGISTRY_SERVER is undefined: URL of docker registry containing the Operator image (e.g. registry.my-company.com))

‎api/v1beta1/queue_webhook.go

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ package v1beta1
22

33
import (
44
"context"
5+
"encoding/json"
56
"fmt"
7+
"maps"
68

79
apierrors "k8s.io/apimachinery/pkg/api/errors"
810
"k8s.io/apimachinery/pkg/runtime"
@@ -23,8 +25,8 @@ func (q *Queue) SetupWebhookWithManager(mgr ctrl.Manager) error {
2325

2426
var _ webhook.CustomValidator = &Queue{}
2527

26-
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
27-
// either rabbitmqClusterReference.name or rabbitmqClusterReference.connectionSecret must be provided but not both
28+
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
29+
// Either rabbitmqClusterReference.name or rabbitmqClusterReference.connectionSecret must be provided but not both
2830
func (q *Queue) ValidateCreate(_ context.Context, obj runtime.Object) (warnings admission.Warnings, err error) {
2931
inQueue, ok := obj.(*Queue)
3032
if !ok {
@@ -38,10 +40,11 @@ func (q *Queue) ValidateCreate(_ context.Context, obj runtime.Object) (warnings
3840
return nil, q.Spec.RabbitmqClusterReference.validate(inQueue.RabbitReference())
3941
}
4042

41-
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
42-
// returns error type 'forbidden' for updates that the controller chooses to disallow: queue name/vhost/rabbitmqClusterReference
43-
// returns error type 'invalid' for updates that will be rejected by rabbitmq server: queue types/autoDelete/durable
44-
// queue arguments not handled because implementation couldn't change
43+
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
44+
//
45+
// Returns error type 'forbidden' for updates that the controller chooses to disallow: queue name/vhost/rabbitmqClusterReference
46+
//
47+
// Returns error type 'invalid' for updates that will be rejected by rabbitmq server: queue types/autoDelete/durable
4548
func (q *Queue) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (warnings admission.Warnings, err error) {
4649
oldQueue, ok := oldObj.(*Queue)
4750
if !ok {
@@ -94,10 +97,49 @@ func (q *Queue) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object)
9497
))
9598
}
9699

100+
if oldQueue.Spec.Arguments != nil && newQueue.Spec.Arguments == nil {
101+
allErrs = append(allErrs, field.Invalid(
102+
field.NewPath("spec", "arguments"),
103+
newQueue.Spec.Arguments,
104+
"queue arguments cannot be updated",
105+
))
106+
}
107+
108+
if oldQueue.Spec.Arguments == nil && newQueue.Spec.Arguments != nil {
109+
allErrs = append(allErrs, field.Invalid(
110+
field.NewPath("spec", "arguments"),
111+
newQueue.Spec.Arguments,
112+
"queue arguments cannot be updated",
113+
))
114+
}
115+
116+
if oldQueue.Spec.Arguments != nil && newQueue.Spec.Arguments != nil {
117+
previousArgs := make(map[string]any)
118+
err := json.Unmarshal(oldQueue.Spec.Arguments.Raw, &previousArgs)
119+
if err != nil {
120+
return nil, apierrors.NewInternalError(fmt.Errorf("error unmarshalling previous Queue arguments: %w", err))
121+
}
122+
123+
updatedArgs := make(map[string]any)
124+
err = json.Unmarshal(newQueue.Spec.Arguments.Raw, &updatedArgs)
125+
if err != nil {
126+
return nil, apierrors.NewInternalError(fmt.Errorf("error unmarshalling current Queue arguments: %w", err))
127+
}
128+
129+
if !maps.Equal(previousArgs, updatedArgs) {
130+
allErrs = append(allErrs, field.Invalid(
131+
field.NewPath("spec", "arguments"),
132+
newQueue.Spec.Arguments,
133+
"queue arguments cannot be updated",
134+
))
135+
}
136+
}
137+
97138
if len(allErrs) == 0 {
98139
return nil, nil
99140
}
100141

142+
//goland:noinspection GoDfaNilDereference
101143
return nil, allErrs.ToAggregate()
102144
}
103145

‎api/v1beta1/queue_webhook_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
. "github.com/onsi/gomega"
77
corev1 "k8s.io/api/core/v1"
88
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
"k8s.io/apimachinery/pkg/runtime"
910
)
1011

1112
var _ = Describe("queue webhook", func() {
@@ -23,6 +24,7 @@ var _ = Describe("queue webhook", func() {
2324
AutoDelete: true,
2425
DeleteIfEmpty: true,
2526
DeleteIfUnused: false,
27+
Arguments: &runtime.RawExtension{Raw: []byte(`{"this-argument": "cannot be updated"}`)},
2628
RabbitmqClusterReference: RabbitmqClusterReference{
2729
Name: "some-cluster",
2830
},
@@ -130,5 +132,27 @@ var _ = Describe("queue webhook", func() {
130132
_, err := newQueue.ValidateUpdate(rootCtx, &queue, newQueue)
131133
Expect(err).To(MatchError(ContainSubstring("autoDelete cannot be updated")))
132134
})
135+
136+
It("does not allow updates on queue arguments", func() {
137+
By("not allowing value changes, nor adding new values")
138+
newQueue := queue.DeepCopy()
139+
newQueue.Spec.Arguments = &runtime.RawExtension{
140+
Raw: []byte(`{"this-argument": "really cant be updated", "adding-args": "not possible"}`),
141+
}
142+
_, err := newQueue.ValidateUpdate(rootCtx, &queue, newQueue)
143+
Expect(err).To(MatchError(ContainSubstring("queue arguments cannot be updated")))
144+
145+
By("not allowing removal")
146+
newQueue.Spec.Arguments = nil
147+
_, err = newQueue.ValidateUpdate(rootCtx, &queue, newQueue)
148+
Expect(err).To(MatchError(ContainSubstring("queue arguments cannot be updated")))
149+
150+
By("not allowing emptying the arguments")
151+
newQueue.Spec.Arguments = &runtime.RawExtension{
152+
Raw: []byte(`{}`),
153+
}
154+
_, err = newQueue.ValidateUpdate(rootCtx, &queue, newQueue)
155+
Expect(err).To(MatchError(ContainSubstring("queue arguments cannot be updated")))
156+
})
133157
})
134158
})

0 commit comments

Comments
 (0)