Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct shovel properties type and expose more shovel configurations in CRD #620

Merged
merged 4 commits into from
May 24, 2023

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented May 19, 2023

This closes #614

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes

Summary Of Changes

Reasons not to exposesrc-queue-args and dest-queue-args for now <- created queues from with shovel won't be deleted when shovel is removed. I'm unsure if we want to encourage such use case. Most importantly queue creation is already possible with queues.rabbitmq.com. We could expose them if we get specific user request in the future.

Conversion webhook and why we don't need it

This PR changes Shovel CRD and it's a breaking change. However, I don't believe a conversion web hook is necessary because if you were to set any of these three properties currently, reconciliation fails.

Example shovels.rabbitmq.com manifest:

apiVersion: rabbitmq.com/v1beta1
kind: Shovel
metadata:
  name: example
spec:
  name: example
  uriSecret:
    name: shovel-secret
  srcQueue: "source-queue"
  destQueue: "destination-queue"
  destApplicationProperties: "{test: test}"
  destPublishProperties: "yo: yo"
  destProperties: "{work: work}"
  rabbitmqClusterReference:
    name: test

Reconciliation fails with rabbit-hole validation error status code 400 returned from RMQ server, e.g.

{"level":"error","ts":"2023-05-18T10:19:30Z","msg":"Reconciler error","controller":"shovel","controllerGroup":"rabbitmq.com","controllerKind":"Shovel","Shovel":{"name":"example","namespace":"rabbitmq-system"},"namespace":"rabbitmq-system","name":"example","reconcileID":"48f10d23-9c45-4dfa-aa47-f605a555e3b9","error":"Error 400 (bad_request): Validation failed\n\nUnrecognised terms [{&lt;&lt;\"dest-application-properties\"&gt;&gt;,&lt;&lt;\"{test: test}\"&gt;&gt;}] in example\n","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:274\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:235"}

{"level":"error","ts":"2023-05-18T10:23:46Z","msg":"Reconciler error","controller":"shovel","controllerGroup":"rabbitmq.com","controllerKind":"Shovel","Shovel":{"name":"example","namespace":"rabbitmq-system"},"namespace":"rabbitmq-system","name":"example","reconcileID":"a645dbb9-0fbe-4197-bc7d-6d48dd908e74","error":"Error 400 (bad_request): Validation failed\n\ndest-publish-properties not a list &lt;&lt;\"{yo: yo}\"&gt;&gt;\n","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:274\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:235"}

{"level":"error","ts":"2023-05-18T10:24:25Z","msg":"Reconciler error","controller":"shovel","controllerGroup":"rabbitmq.com","controllerKind":"Shovel","Shovel":{"name":"example","namespace":"rabbitmq-system"},"namespace":"rabbitmq-system","name":"example","reconcileID":"5145e11a-1e8f-4052-ba78-5fae5b5f1beb","error":"Error 400 (bad_request): Validation failed\n\nUnrecognised terms [{&lt;&lt;\"dest-properties\"&gt;&gt;,&lt;&lt;\"key\"&gt;&gt;}] in example\n","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:274\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:235"}

So there can't be an existng shovels.rabbitmq.com with these fields set therefore there is nothing for us to convert from.

- destApplicationProperties, destPublishProperties
and destPublishProperties should be map instead of string.
See related rabbit-hole changes:
michaelklishin/rabbit-hole#262
michaelklishin/rabbit-hole#268
@ChunyiLyu ChunyiLyu marked this pull request as draft May 19, 2023 16:40
@ChunyiLyu ChunyiLyu changed the title WIP Correct shovel properties type Correct shovel properties type and expose one more shovel configurations in CRD May 23, 2023
@ChunyiLyu ChunyiLyu changed the title Correct shovel properties type and expose one more shovel configurations in CRD Correct shovel properties type and expose more shovel configurations in CRD May 23, 2023
- expose `dest-message-annotations` and
`src-consumer-args` in Shovel as maps
- properties for different protocol amqp091 and amqp10
are different in shovel
Copy link
Member

@MirahImage MirahImage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only suggestion would be that we should consider providing an example using this new functionality.

I agree with the assessment that we don't need a conversion webhook, since the old functionality was actually broken and non-functional.

@ChunyiLyu ChunyiLyu merged commit 6488550 into main May 24, 2023
@ChunyiLyu ChunyiLyu deleted the shovel branch May 24, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect Shovel configuration types
3 participants