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

Update Bunny gem #6857

Merged
merged 1 commit into from
Mar 3, 2016
Merged

Update Bunny gem #6857

merged 1 commit into from
Mar 3, 2016

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Feb 22, 2016

Update Bunny gem, the old gem couldn't handle reconnect when
amqp service got restarted.

Seems like new bunny works the same, so no changes in using it
are needed.

Fixes BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1222005

@miq-bot
Copy link
Member

miq-bot commented Feb 22, 2016

Checked commit Ladas@0effc7c with ruby 2.2.3, rubocop 0.34.2, and haml-lint 0.13.0
1 file checked, 0 offenses detected
Everything looks good. 👍

@chessbyte
Copy link
Member

@blomquisg please review

@blomquisg
Copy link
Member

The only thing that jumped out at me was that after 1.6.0 Bunny now sets verify_peer = true by default, whereas before it was verify_peer = false.

This will only mess us up with situations where the user enables SSL for AMQP. Which, as I understand it, is not a standard configuration for AMQP in OSP.

Still, I think we should play it safe and do the following:

  • add a setting to control OpenStack AMQP verify_peer in vmdb.tmpl.yml
    • should we use the same CA as we use for the SSL cert verification of the OSP API?
  • default the Bunny verify_peer to false
  • add some documentation describing how to install the CA for peer verification if it's different than the OSP API CA.

@matthewd
Copy link
Contributor

default the Bunny verify_peer to false

😟

@Ladas
Copy link
Contributor Author

Ladas commented Feb 23, 2016

@blomquisg hm, but that is for followup PR, right? Because we don't support AMQP SSL at all now.

Then my guess would be that this setting will go to Endpoint data? Yeah, I would use the same CA for now.

@blomquisg
Copy link
Member

@matthewd yeah 😞

Alternatively, we could doc it away, since we don't have a better solution that helps users setup the CA cert for OpenStack API and AMQP.

I would think the right thing to do here would be to either (or both) have a page in the Appliance configuration that allows an admin to upload a CA for OpenStack connections in their organization, and/or have a mechanism in the appliance console that allows an admin to specify the same CA.

Update Bunny gem, the old gem couldn't handle reconnect when
amqp service got restarted.

Seems like new bunny works the same, so no changes in using it
are needed.

Fixes BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1222005
@Ladas Ladas force-pushed the update_bunny_gem branch from 0effc7c to 0b3328d Compare March 1, 2016 14:02
@Ladas
Copy link
Contributor Author

Ladas commented Mar 1, 2016

we need to use 2.1.0, higher versions are breaking ipv6 ruby-amqp/bunny#383

@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2016

<github_pr_commenter_batch />Some comments on commit Ladas@0b3328d

@Ladas Ladas closed this Mar 1, 2016
@Ladas Ladas reopened this Mar 1, 2016
blomquisg added a commit that referenced this pull request Mar 3, 2016
@blomquisg blomquisg merged commit bd1e600 into ManageIQ:master Mar 3, 2016
@blomquisg blomquisg added this to the Sprint 37 Ending Mar 7, 2016 milestone Mar 3, 2016
lgalis pushed a commit to lgalis/manageiq that referenced this pull request Mar 17, 2016
Update Bunny gem

Update Bunny gem, the old gem couldn't handle reconnect when
amqp service got restarted.

Seems like new bunny works the same, so no changes in using it
are needed.

Fixes BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1222005

Clean cherry-pick of:
ManageIQ#6857

Fixes 5.5.z BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1310245



See merge request !839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants