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

Added option for backend to have a vhost override #103

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mchandler-plato
Copy link

Describe your changes
Allows a backend to specify a different vhost to connect to than the one provided by the client. This allows easy migration of clients to new rabbitmq cluster with a different vhost.

Testing performed
Run setup in production on live rabbitmq cluster with clients using default vhost '/' and redirecting them to other vhosts

Additional context
Prob missing a few things, please let me know what improvements i can make

Signed-off-by: Mark Chandler <mark@platoteam.com>
Copy link
Contributor

@adamncasey adamncasey left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

The workflow makes sense to me. In the past we had discussed holding a vhostname map, e.g.

amqpprox_ctl /tmp/amqpprox BACKEND ADD backend-1 dc hostname 5672
amqpprox_ctl /tmp/amqpprox BACKEND ADD backend-2 dc hostname-2 5672
amqpprox_ctl /tmp/amqpprox MAP BACKEND old-vhost backend-1
amqpprox_ctl /tmp/amqpprox MAP BACKEND new-vhost backend-2

amqpprox_ctl /tmp/amqpprox VHOST REMAP old-vhost new-vhost

This would remove the need to configure backends twice (e.g. if backend-2 also already mapped a few unrelated vhosts).

What do you think?

@@ -30,6 +30,7 @@ class Backend {
std::string d_datacenterTag;
std::string d_host;
std::string d_ip;
std::string d_virtualHost;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we use std::optional<std::string> here - mainly for the benefit of indicating that it may not be set.

Comment on lines +224 to +226
methods::Open openCopy = d_open;
openCopy.setVirtualHost(d_sessionState_p->getBackendVirtualHost());
sendResponse(openCopy, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

The only property which is encoded/decoded in amqpprox on Open is the vhost so copying the inbound open is a little pointless now, I feel it's neater to keep the Open object immutable and add a constructor which takes a vhost string. I think we could go with something like this, and ditch the d_open property on the Connector

Suggested change
methods::Open openCopy = d_open;
openCopy.setVirtualHost(d_sessionState_p->getBackendVirtualHost());
sendResponse(openCopy, false);
methods::Open openMethod{d_sessionState_p->getBackendVirtualHost()};
sendResponse(openMethod, false);

Copy link
Author

Choose a reason for hiding this comment

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

I wasnt sure what else used d_open so making a copy was 'safer'. Open to doing what ever way makes more sense

@mchandler-plato
Copy link
Author

@adamncasey thanks for the review, VHOST remap does sound like a better solution, i take it when we get the open package from the client, we would then look up the mapping and update it?

Will have to wait a few weeks till i get some time to try that change

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