-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Mark Chandler <[email protected]>
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
methods::Open openCopy = d_open; | ||
openCopy.setVirtualHost(d_sessionState_p->getBackendVirtualHost()); | ||
sendResponse(openCopy, false); |
There was a problem hiding this comment.
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
methods::Open openCopy = d_open; | |
openCopy.setVirtualHost(d_sessionState_p->getBackendVirtualHost()); | |
sendResponse(openCopy, false); | |
methods::Open openMethod{d_sessionState_p->getBackendVirtualHost()}; | |
sendResponse(openMethod, false); |
There was a problem hiding this comment.
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
@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 |
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