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

upgrade puma gem to 3.7 to work with openssl 1.1 and Fedora 26 #15583

Merged
merged 2 commits into from
Jul 24, 2017

Conversation

ailisp
Copy link
Member

@ailisp ailisp commented Jul 17, 2017

Fedora 26 use openssl 1.1 and in our Gemfile requires puma 3.3.x which doesn't support openssl 1.1. Update to 3.9, it should work with both openssl 1.1 and older version of openssl 1.0. Mangeiq web application looks fine after this modification. Also, after this upgrade, when browse 127.0.0.1:3000 firefox doesn't complain it's insecure now (before it did).

@Fryguy
Copy link
Member

Fryguy commented Jul 17, 2017

@jrafanie Please review. I think we just need to be sure this builds on older Fedoras as well. Not sure who in the community is using what version of Fedora.

cc @bdunne @carbonin What version are you all using?

@bdunne
Copy link
Member

bdunne commented Jul 17, 2017

I'm on Fedora 24, everything installed and starts Fine 😉

But.....

=> Booting Puma
=> Rails 5.0.4 application starting in development on http://localhost:3000
=> Run `rails server -h` for more startup options
** ManageIQ master; Database: adapter=postgresql, name=vmdb_development, host=
** Using session_store: ActionDispatch::Session::MemCacheStore
I, [2017-07-17T15:23:27.467374 #2682]  INFO -- : Initializing websocket worker!
Puma starting in single mode...
* Version 3.9.1 (ruby 2.3.3-p222), codename: Private Caller
* Min threads: 5, max threads: 5
* Environment: development
* Listening on tcp://0.0.0.0:9292
* Listening on tcp://localhost:3000
Use Ctrl-C to stop

What's that 0.0.0.0:9292?

@NickLaMuro
Copy link
Member

What's that 0.0.0.0:9292?

The websocket worker? Not actually sure, just a guess.

For what it is worth, works for me when updating with no problems.

@bdunne
Copy link
Member

bdunne commented Jul 17, 2017

More specifically...

  • shouldn't it also be localhost?
  • what even is it? (assuming websockety things)
  • will it cause problems when traffic doesn't get there on appliances? (since nothing is forwarding traffic to it)

@NickLaMuro
Copy link
Member

@bdunne well, this looks like it is the default rack config...

https://github.com/puma/puma/blob/4af4a006d1f1a214ee0b2c97c5ee401f76064911/lib/puma/configuration.rb#L10-L11

Not sure how that is being activated though. Got the same thing as you did.

@NickLaMuro
Copy link
Member

Doing some spelunking into @bdunne's concern to see where that comes from, and why the upgrade changed things.

@jrafanie
Copy link
Member

I haven't had a chance to look into puma. I don't think it should be a problem but I won't be able to take a look. @skateman can you review the websocket side? As long as the existing config/puma.rb is honored, it should be fine.

@NickLaMuro
Copy link
Member

So, as far as I have been able to tell so far, the change that Brandon and myself are seeing only seems to affect the bin/rails s path for starting the server, and when running a worker through the appliance, there is no affect. Seems to be a change with the puma configuration in some way, as there have been a lot, but haven't been able to track down the specifics as to why just yet.

@NickLaMuro
Copy link
Member

Okay, I think I located the issue. Basically it is combining the DefaultTCPHost and DefaultTCPPort with what is defined by rails in the bin/rails s command. More verbose explanation below.

Confident it is related to puma/puma#1234 where the user_supplied_options gets defaulted by bin/rails s to [], but then the block that combines these options basically takes what is a bunch of "higher level configs", and puts them into "defaults". This then gets merged here:

https://github.com/schneems/puma/blob/852f52f/lib/puma/configuration.rb#L130

Adding both of the binds together when a port option is not defined in the config/puma.rb (which is now now generated with rails new.

I would argue that PR puma/puma#1234 is a "Rails induced puma feature", and thinking the bug is really there versus anything else. Will open up a PR/issue regarding it.

@NickLaMuro
Copy link
Member

NickLaMuro commented Jul 18, 2017

Regarding the puma fix, brought it up in puma/puma#1371, and am attempting to address it in puma/puma#1372.

A workaround to solve the fedora issues might be to use 3.7.x, since I think that doesn't have these config changes applied.

@skateman
Copy link
Member

Well, I don't see the tcp://localhost:9292 in the output and the websockets work both with 3.7 and 3.9 so it's good to go from my side. 👍

@NickLaMuro
Copy link
Member

@skateman the duplicate address binding only shows up if you are using bin/rails s (I mentioned that here, but didn't emphasize well I guess...), which I expect at least some developers will use.

Booting the UI worker or the websocket worker will properly set the puma config because they aren't using the Rails defaults directly from the rails s command, which passes them to the puma rack handler directly.

@ailisp
Copy link
Member Author

ailisp commented Jul 18, 2017

Sorry for a late response. @bdunne How does 3.7 work on fedora 24? @NickLaMuro Good job. you decide to make a PR in puma and in the future we can use the next version of puma?

@NickLaMuro
Copy link
Member

Sorry for a late response.

Uh... I was working late, no need to apologize 😄

you decide to make a PR in puma and in the future we can use the next version of puma?

Yup, filed and issue in puma/puma#1371 and am working on a fix in puma/puma#1372 . Whether it gets accepted and released in the next version of puma is up in the air though.

@skateman
Copy link
Member

skateman commented Jul 18, 2017

@NickLaMuro gotcha alias server="bundle exec rails s -b 0.0.0.0", I tried 3.7 on F26 only 😞

@NickLaMuro
Copy link
Member

@skateman ah, yeah, that -b 0.0.0.0 will actually prevent the problem, because that gets counted as a :user_supplied_option. Makes sense now.

@bdunne
Copy link
Member

bdunne commented Jul 18, 2017

@ailisp I just modified my Gemfile and ran bundle update, everything installed fine.

@ailisp
Copy link
Member Author

ailisp commented Jul 18, 2017

@bdunne So we can safely upgrade to puma 3.7 to fit both F26 and F24? @skateman use F26. He is also fine with 3.7

@ailisp ailisp changed the title upgrade puma gem to 3.9 to work with openssl 1.1 and Fedora 26 upgrade puma gem to 3.7 to work with openssl 1.1 and Fedora 26 Jul 19, 2017
@ailisp
Copy link
Member Author

ailisp commented Jul 19, 2017

Now change version to 3.7 to work in both Fedora 24 and 26.

@miq-bot
Copy link
Member

miq-bot commented Jul 19, 2017

Checked commits ailisp/manageiq@e2a32c0~...deacfb8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Member

@Yadnyawalkya Yadnyawalkya left a comment

Choose a reason for hiding this comment

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

This works for me on Fedora26, need to merge this as early as possible !

@jrafanie
Copy link
Member

Looks like we have some different versions of fedora and osx having great success with this version. Merging.

@jrafanie jrafanie merged commit 1c47aa9 into ManageIQ:master Jul 24, 2017
@jrafanie jrafanie added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 24, 2017
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.

None yet

8 participants