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

Fix issue with RabbitmqClusterReference Namespace Namespace and ConnectionSecret #700

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

mindw
Copy link
Contributor

@mindw mindw commented Nov 13, 2023

This closes #501

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

Fix an issue where Namespace was ignored when using ConnectionSecret.

Additional Context

This doesn't look like a feature - it's more like a bug. Why support RabbitmqCluster via namespace but forbid secrets?

@vmwclabot
Copy link

@mindw, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@mindw, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@Zerpet Zerpet self-requested a review November 14, 2023 11:30
@Zerpet Zerpet self-assigned this Nov 14, 2023
Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

We can't accept this contribution as it stands. For historical context see #51 #147 and #201.

The main reason to reject is the security implications this change may have. The user running the Topology Operator has access to a wide set of resources, secrets among them. The Topology Operator cannot know if user Cody has access to a namespace other than the namespace a request is coming from. For example: Cody creates a Queue in namespace dev. The existence of Queue object in NS dev is sufficient to conclude that Cody has access to NS dev, at least to create a Topology object. We can't know whether Cody has access to any other NS like app-1.

As illustrated in the example above, we would be allowing Cody to bypass NS restrictions, and therefore, we can't accept this as it currenty stands.

@mindw
Copy link
Contributor Author

mindw commented Nov 14, 2023

@Zerpet , thank you for the detailed and prompt reply.

From what I gather, it would be enough to add support for the rabbitmq.com/topology-allowed-namespaces annotation in the secret, with the same semantics as it has when used on a RabbitmqClusterReference object.

I'll update the PR and tests then.

@vmwclabot
Copy link

@mindw, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

Please provide more address detail - Minimum required is Country, State, Town/City.

@Zerpet
Copy link
Contributor

Zerpet commented Nov 15, 2023

Bear in mind that when ConnectionSecret is used, there's not necessarily a RabbitmqCluster object. The ConnectionSecret was initially designed to use the Topology Operator with non-managed RabbitMQ servers [1]. Later we found some use cases where it makes sense to use ConnectionSecret in conjuction with RabbitmqCluster e.g. the creator of RabbitmqCluster overrides the default_user and default_pass.

The solution you propose will be acceptable in the specific scenario where a RabbitmqCluster object exists. Otherwise, the Secret must be in the same namespace.

[1] https://rabbitmq.com/kubernetes/operator/using-topology-operator.html#non-operator

@vmwclabot
Copy link

@mindw, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@mindw
Copy link
Contributor Author

mindw commented Nov 16, 2023

@zarpet - the annotation rabbitmq.com/topology-allowed-namespaces is now required on the referenced secret - same as it is on the RabbitmqCluster. This should satisfy the requirement.

Again, thank you the detailed explanations and your time!

@vmwclabot
Copy link

@mindw, VMware has approved your signed contributor license agreement.

Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@Zerpet
Copy link
Contributor

Zerpet commented Nov 22, 2023

I will merge after CI passes

@Zerpet Zerpet added this to the v1.13.0 milestone Nov 22, 2023
@Zerpet Zerpet merged commit 4f07a2c into rabbitmq:main Nov 23, 2023
5 checks passed
@mindw mindw deleted the apply_namespace_to_connection_secret branch November 23, 2023 19:30
Zerpet added a commit to Zerpet/rabbitmq-website that referenced this pull request Oct 14, 2024
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.

Connection Secret: Use from other namespaces
3 participants