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

Validate queue and consumer arguments #4572

Merged
merged 8 commits into from Apr 18, 2022
Merged

Validate queue and consumer arguments #4572

merged 8 commits into from Apr 18, 2022

Conversation

ansd
Copy link
Member

@ansd ansd commented Apr 14, 2022

Follow up of #4404.

Throw a PRECONDITION_FAILED error when a queue is created with an
invalid argument or when a consumer subscribes with an invalid argument.

There can be RabbitMQ plugins and extensions out there defining arbitrary
queue arguments. Therefore we only check that a queue is not declared
with arguments that we know are supported only by other queue types.

The benefit of this change is that queues are not mistakenly declared with the wrong arguments.

This change will reduce a lot of confusion when it comes to what arguments are supported by what queue type.

Example 1:
Before this PR, a stream could be created with x-dead-letter-exchange queue argument set. Users wonder why the stream does not dead letter any messages. After this PR, stream creation fails early with a meaningful error message because that argument is only supported for classic and quorum queues.

Example 2:
Before this PR, a stream could be created with x-max-length queue argument set. Users wonder why the stream contains more messages than configured via x-max-length. After this PR, stream creation fails early with a meaningful error message because that argument is only supported for classic and quorum queues. Streams support x-max-length-bytes and x-max-age.

Example 3:
Before this PR, a quorum queue could be created with x-queue-master-locator queue argument set. Users wonder why it doesn't have any effect. After this PR, queue creation will fail with a meaningful error message.

All changes are backwards compatible, except invalid consumer arguments which I expect to be rare.
For example, when a consumer had consumer argument x-priority set to connect to a stream via AMQP, this will start failing after this PR.

Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

My first reading of this is that such a validation would make it impossible for plugins to
rely on custom x-args, or provide custom queue types.

I doubt those are common but we can't verify that.

@ansd
Copy link
Member Author

ansd commented Apr 15, 2022

Plugins using custom x-args will continue to work after this PR because custom x-args are never validated or invalidated (assuming the custom x-arg is not part of the list of supported x-args of any existing queue type).

This is because this PR uses some form of "weak" validation:
Let's say we have queue type A with supported x-arg 1 and queue type B with supported x-arg 2. Whenever a user includes x-arg 1 during declaration of queue type B, an error will be thrown. However, when a user includes any other x-args (e.g. x-arg 2 and a custom x-arg 3) when declaring queue type B, validation will succeed.

I'm not sure whether it's possible to extend RabbitMQ with custom queue types at the moment. I think there are some hard-coded assumptions that only classic, quorum and stream queues exist, but maybe once we move these types to the registry (as indicated in this TODO), this will become possible, yes. In this case, the implementation of this PR will continue to work, and do the right thing.

We could also enable the new validation of this PR by default, but could add a config option allowing to disable this new validation (just in case I overlooked a breaking change scenario).

Alternatively, if we think this validation will cause more trouble than people find it helpful (my opinion is it will overall cause less confusion) we can also close this PR and just delete the list of supported x-args per queue type as originally done in #4404 since they are currently not used anywhere.

@michaelklishin
Copy link
Member

Thanks for the explanation, @ansd. "Validate known and accept the rest" sounds like a good balance. No objections from me.

@michaelklishin
Copy link
Member

This needs conflicts to be resolved, otherwise LGTM.

Throw a PRECONDITION_FAILED error when a queue is created with an
invalid argument.
There can be RabbitMQ plugins and extensions out there defining arbitrary
queue arguments. Therefore we only check that a queue is not declared
with arguments that we know are supported only by OTHER queue types.

The benefit of this change is that queues are not mistakenly declared
with the wrong arguments. For example, a stream should not be
declared with `x-dead-letter-exchange` because this argument is not
supported in streams. It is only supported by classic and quorum queues.
Instead of silently allowing this argument and users wondering why the stream
does not dead letter any messages, it's better to fail early with an
meaningful error message.

We still allow any other arbitrary queue arguments.
(Therefore and unfortunately, when `x-dead-letter-exchange` is misspelled,
it will still be accepted as an argument).

For argument equivalence, we only validate the arguments that are
supported for a queue type. There is no benefit in validating any other
arguments that are not supported anyways.
Throw a PRECONDITION_FAILED error when subscribing with an invalid
consumer argument.
Delete arguments:

x-dead-letter-exchange
x-dead-letter-routing-key
x-max-length
x-single-active-consumer

None of these arguments are supported in streams.

Dead lettering does not apply to streams.
x-max-length does not apply, only x-max-length-bytes and x-max-age apply.

Single active consumer does not work on the queue level, it will only
work on the consumer level. This is different to single active consumer
for classic and quorum queues.
Delete x-max-in-memory-length and x-max-in-memory-bytes from
classic queues since they were only supported by quorum queues.

Add delivery-limit to unsupported policies of classic queues, they apply
only to quorum queues.
@michaelklishin michaelklishin marked this pull request as ready for review April 16, 2022 08:50
@michaelklishin michaelklishin added this to the 3.10.0 milestone Apr 16, 2022
This reverts commit 457f0fb.

rabbit_misc:rs/2 includes object type (e.g. queue) already
@michaelklishin michaelklishin merged commit e24c447 into master Apr 18, 2022
@michaelklishin michaelklishin deleted the validate-args branch April 18, 2022 08:52
michaelklishin added a commit that referenced this pull request Apr 18, 2022
mergify bot pushed a commit that referenced this pull request Apr 18, 2022
(cherry picked from commit 8ff6ed7)
@michaelklishin
Copy link
Member

@Mergifyio backport v3.10.x

@mergify
Copy link

mergify bot commented Apr 18, 2022

backport v3.10.x

✅ Backports have been created

michaelklishin added a commit that referenced this pull request Apr 18, 2022
Validate queue and consumer arguments (backport #4572)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants