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

Allow to set connection options on a per server basis #107

Merged
merged 25 commits into from
Aug 15, 2024

Conversation

david-krentzlin
Copy link
Contributor

@david-krentzlin david-krentzlin commented Aug 7, 2024

This adds the possibility to configure connection options on a per server basis.

It introduces a new configuration setting server_connection_options which is a Hash, that maps a servername to a hash of options with keys host, port, user, pass, vhost, ssl. Those options default to the corresponding settings in the configuration.

In essence this allows us to configure specific options for individual servers and leave the rest as is.
This is required to add an AmazonMQ broker, which requires different credentials and TLS.

Breaking changes

This change removes the Beetle::Configuration.api_port attribute accessor without a replacement.
Instead the API port is always derived from the server's amqp port by prepending a 1.

Implementation note

This change is not the best in terms of software design, but the overall objective for me was to keep the change minimal, and minimize risks of unintended breakage. I don't know the codebase very well, and we intend to update many clients with this change, so we need to play safe.

Additionally, we plan to deprecate and remove the library soon, so it really doesn't make sense to make it extra pretty and ready for evolution.

Release plan

We intent to integrate this change into a new release branch v4.x in order to keep upstream relatively undisturbed.
This PR targets that branch already.

Testing

We will test this change end-to-end in multiple steps.

  • Integrate in one of our applications in an isolated environment with a default (non-AWS) config
  • Integrate in one of our applications in an isolated environment which configures both AWS and on-prem brokers
  • Test end-to-end that it also works for additional subscription servers
  • Integrate on our preview environment in one of our applications

Todo

  • Decide on strartegy to roll-out (integration branch or version bump directly?)
    • we create a new major version and corresponding branch (if that's not enough we can even release under a new name)
    • release to 1, few, all (start with our app, find a small cohort of other apps, release to everyone in the organisation)
  • Test change with xing-amqp

Refs

  • APII-1589

@david-krentzlin david-krentzlin changed the title Allow to set connection options on a per server basis APII-1589 Allow to set connection options on a per server basis Aug 7, 2024
@david-krentzlin
Copy link
Contributor Author

@0robustus1 and @JHK I added you as reviewers. Please do on a best effort basis, as I'm aware you're not officially maintainers, but I'd like to get at least also some sanity check from your side.

Copy link
Member

@JHK JHK left a comment

Choose a reason for hiding this comment

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

Honestly I don't really know that code. However, the mixture of using @client and options for different kinds of configuration for a new Bunny instance feels a bit odd. @client is used to store the per server config already. Also this PR removes the usage of current_host without removing the method.

Copy link
Contributor

@0robustus1 0robustus1 left a comment

Choose a reason for hiding this comment

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

It's been years since i last read beetle code. So I'm also not particularly familiar with it.
I think before proceeding we should answer the question why Publisher and Subscriber have a @server concept when that is to be controlled by the client. (Never mind misread the code)

lib/beetle/publisher.rb Show resolved Hide resolved
@david-krentzlin
Copy link
Contributor Author

david-krentzlin commented Aug 9, 2024

@JHK

@client is used to store the per server config already

That's what I would've thought but as far as I understand it, that's not actually true, because there is this code in the publisher and subscriber that does the real thing.

In any case I will double check we don't overlook an opportunity to take a more principled route.
It doesn't help that many attributes are exposed via accessors, which means they can be altered anytime and are part of the public interface.

And as far as I can see, the client is meant to be used for connections to different servers, judging from how the library is used. Only one client is created in processors.

@david-krentzlin
Copy link
Contributor Author

We've orchestrated the changes with xing-amqp and have setup an end-to-end tests with an isolated environment.
We also had overlooked the code that speaks via the admin api with the rabbitmq broker to manage queues and queue properties.

We'll now feed back what we've learned and bring this PR to a state where it can be reviewed again.

danielgoncharov and others added 4 commits August 13, 2024 10:20
Adds a set of tests that verifies that per server
connection options are used also for http api requests.
This introduced bigger changes in the queue_properties implementation.
We change the way the requests are build in order to be able to use per server
settings reliably.

Most notably we always derive the api port from the server's port, by prepending
a 1.

Breaking Change:
================

Configuration.api_port has been removed without replacement.
Version has been bumped accordingly.
@david-krentzlin
Copy link
Contributor Author

david-krentzlin commented Aug 13, 2024

We will likely integrate this into a new 4.x branch and corresponding tags to be able to move, without breaking potential users outside our organization.

@david-krentzlin david-krentzlin changed the base branch from master to v4.x August 13, 2024 14:20
@david-krentzlin david-krentzlin changed the title APII-1589 Allow to set connection options on a per server basis Allow to set connection options on a per server basis Aug 13, 2024
@david-krentzlin david-krentzlin marked this pull request as ready for review August 15, 2024 11:02
@david-krentzlin david-krentzlin merged commit 613108f into v4.x Aug 15, 2024
8 checks passed
@david-krentzlin david-krentzlin deleted the per-server-config branch August 15, 2024 14:25
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.

5 participants