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

Fixes #25293 - Support Puma #774

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Oct 2, 2020

To support Puma, this takes the approach of introducing a Rack application that understands the HTTP request scheme. Depending on that, it responds either with the HTTP or HTTPS app.

This means all the responsibility of binding to HTTP and HTTPS is shifted to the application server.

To use this:

$ bundle exec puma -b tcp://127.0.0.1:8000 -b 'ssl://127.0.0.1:8443?key=config/key.pem&cert=config/cert.pem'
...
* Listening on tcp://127.0.0.1:8000
* Listening on ssl://127.0.0.1:8443?key=config/key.pem&cert=config/cert.pem
...

Note that this doesn't set the secure ciphers nor protocols. It also requires Puma to be built with OpenSSL extensions to be able to bind on HTTPS.

I verified this with by setting the BMC module to enabled only on HTTPS:

:enabled: https

Then running verification:

$ curl http://localhost:8000/bmc
Requested url was not found
$ curl -k https://localhost:8443/bmc
{"available_resources":["providers","providers/installed","host"]}

@@ -0,0 +1,29 @@
module Proxy
class App
Copy link
Member Author

Choose a reason for hiding this comment

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

For naming, perhaps this is more of a Router than an app?

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind App here. No big deal for me.

@ekohl
Copy link
Member Author

ekohl commented Oct 2, 2020

This now adds Puma support in the launcher, converting the settings to a puma config. It still misses a few things, such as the HTTPS client certificate checking. I've chosen to implement a different class for each launcher to clearly separate the two. It also doesn't install the USR1 signal handler.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

So the biggest difference is that spawning Puma is not done in an extra thread, but in the main thread so it's blocking. And also that puma can now be started using "puma" command.

Shall we simply deprecate the bin/smart-proxy binary and only keep it for optional (temporary) fallback to Webrick? I don't expect issues with Puma migration for Smart Proxy, the code has been thread safe for years.

@@ -0,0 +1,29 @@
module Proxy
class App
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind App here. No big deal for me.

end

def launch
::Puma::Launcher.new(conf).run
Copy link
Member

Choose a reason for hiding this comment

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

Offtopic: I noticed that the spawned thread in DHCP plugin does not have any shutdown implementation. It provides stop method however it's never called except tests (?), I tested this using pry (DHCP plugin must be enabled and working). This can be probably workarounded using at_exit block with calling stop and waiting until the thread joins.

Anyway, this is not relevant, I was just investigating how shutdown works in puma - it performs graceful shutdown of the Puma's resources and that's about it. Although I had impression we do some thread joining for webrick case, we only do this for server threads (not background plugin DHCP thread).

end

{
:app => app,
:server => :webrick,
:DoNotListen => true,
:Port => http_port, # only being used to correctly log http port being used
:Port => settings.http_port, # only being used to correctly log http port being used
:Logger => ::Proxy::LogBuffer::TraceDecorator.instance,
:AccessLog => [],
:ServerSoftware => "foreman-proxy/#{Proxy::VERSION}",
:daemonize => false,
}
end
Copy link
Member

Choose a reason for hiding this comment

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

Off topic: I have learned there is a MaxClients setting which limits maximum amount of threads (thus simultaneous clients). By default it's 100, I think we should make this configurable for customers struggling with choking process - this feels like too high even when we don't use database (lots of other IO however).

Interestingly, puma defaults to 5 max treads for MRI, 16 for other interpreters. From various articles this looks like the sweet-spot in regard to GIL for typical web applications (database): https://devcenter.heroku.com/articles/deploying-rails-applications-with-the-puma-web-server

def https_enabled?
settings.ssl_private_key && settings.ssl_certificate && settings.ssl_ca_file
end
Proxy::SignalHandler.install_traps
Copy link
Member

Choose a reason for hiding this comment

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

This line is missing for Puma.

Copy link
Member

Choose a reason for hiding this comment

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

My comment is actually a leftover from my shutdown investigation, it's not relevant. These traps are only for webrick, maybe we should rename this method to show these are only for Webrick to prevent confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

You found it out before I could reply. That's exactly the reasoning. Perhaps the SignalHandler should be merged in the Webrick launcher or at least moved to Proxy::Launcher::Webrick::SignalHandler.


def settings
launcher.settings
end
Copy link
Member

@lzap lzap Oct 6, 2020

Choose a reason for hiding this comment

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

Some change with settings are causing dynflow to fail:

2020-10-06T10:47:24  [W] Error details for Error during startup, terminating: <NoMethodError>: undefined method `settings' for #<Hash:0x0000000002748d38>
/home/lzap/work/smart_proxy_dynflow/lib/smart_proxy_dynflow_core/settings.rb:82:in `load_from_proxy'
(eval):8:in `block (2 levels) in build_http_app'
/home/lzap/work/smart-proxy/lib/proxy/launcher/webrick.rb:46:in `instance_eval'
/home/lzap/work/smart-proxy/lib/proxy/launcher/webrick.rb:46:in `block (2 levels) in build_http_app'
/home/lzap/work/smart-proxy/lib/proxy/launcher/webrick.rb:46:in `each'
/home/lzap/work/smart-proxy/lib/proxy/launcher/webrick.rb:46:in `block in build_http_app'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/builder.rb:125:in `instance_eval'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/builder.rb:125:in `initialize'
/home/lzap/work/smart-proxy/lib/proxy/launcher/webrick.rb:45:in `new'
/home/lzap/work/smart-proxy/lib/proxy/launcher/webrick.rb:45:in `build_http_app'
/home/lzap/work/smart-proxy/lib/proxy/launcher/webrick.rb:14:in `launch'
/home/lzap/work/smart-proxy/lib/proxy/launcher.rb:93:in `launch'
bin/smart-proxy:7:in `<top (required)>'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/cli/exec.rb:74:in `load'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/cli/exec.rb:74:in `kernel_load'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/cli/exec.rb:28:in `run'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/cli.rb:463:in `exec'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/cli.rb:27:in `dispatch'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/cli.rb:18:in `start'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/exe/bundle:30:in `block in <top (required)>'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/lib/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/home/lzap/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/bundler-1.17.2/exe/bundle:22:in `<top (required)>'
/home/lzap/.rbenv/versions/2.7.1/bin/bundle:23:in `load'
/home/lzap/.rbenv/versions/2.7.1/bin/bundle:23:in `<main>'

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know the smart proxy used these internals. I'll take a look.

launcher = ::Launcher::Webrick.new(self)
when 'puma'
require 'proxy/launcher/puma'
launcher = ::Launcher::Puma.new(self)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Aren't we actually switching from using our own startup binary to calling "puma" directly?

Copy link
Member

Choose a reason for hiding this comment

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

This was a bit of my confusion as well trying to understand the layers and I think @lzap worded the question better than I was going to. I attempted to read up on the Rack pattern and it's uses in places and apply it to here. I'm going to attempt to write out what I think I understand (and am probably wrong about in some way):

Puma  -> Rack Middleware -> Smart Proxy Plugins

Where Puma serves as the application server handling spawning processes and threads to handle incoming requests.
Rack middleware (Proxy::App) handles mounting each Smart Proxy plugin as a Rack app as an http, https or both endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this needed? Aren't we actually switching from using our own startup binary to calling "puma" directly?

Both are valid. The Puma launcher keeps everything compatible from a packaging perspective: no need to change all config files and service files.

Puma -> Rack Middleware -> Smart Proxy Plugins

This is slightly incorrect. More correct would be:

Application server -> Rack application

Passenger would also be a valid application server since there is a config.ru file. Any Rack application can also apply middleware (we don't do this today).

If you note, previously it also built a Rack application here:

app = Rack::Builder.new do
plugins.each { |p| instance_eval(p.http_rackup) }
end

And the HTTPS one here:

app = Rack::Builder.new do
plugins.each { |p| instance_eval(p.https_rackup) }
end

So the solution I chose was to write a third Rack application that embeds boths those and switches it based on the received protocol.

Note that a Rack application is defined as a class that has a call(env) method. Env is a simple Ruby hash. To get a request, you can call Rack::Request.new(env). It's surprisingly simple.

https://github.com/rack/rack/blob/master/lib/rack/lobster.rb is the example they ship.

This matches the module layout
To support Puma, this takes the approach of introducing a Rack
application that understands the HTTP request scheme. Depending on that,
it responds either with the HTTP or HTTPS app.

This means all the responsibility of binding to HTTP and HTTPS is
shifted to the application server.

To use this:

  bundle exec puma -b tcp://127.0.0.1:8000 -b 'ssl://127.0.0.1:8443?key=config/key.pem&cert=config/cert.pem'

Note that this doesn't set the secure ciphers nor protocols. It also
requires Puma to be built with OpenSSL extensions to be able to bind on
HTTPS.
@ekohl
Copy link
Member Author

ekohl commented Oct 19, 2020

Rebased just so the big __dir__ commit is out of it so it's easier to read. No other changes.

@ekohl
Copy link
Member Author

ekohl commented Oct 19, 2020

Another question I had: do we need to keep the daemonize code in? We don't use it with systemd and not for Windows (AFAIK). It may only be useful on BSD but there are other solutions to that.

@lzap
Copy link
Member

lzap commented Oct 19, 2020

It may only be useful on BSD but there are other solutions to that.

I vote for removing this, it's okay to run software non-daemonized or even in tmux/screen.

What solutions are on your mind? Technically speaking, a daemon is a process that has some unique attributes and no external tool can daemonize a process. There are tools however which can help supervising such processes like DJ's daemontools or even systemd these days.

I am totally fine that smart-proxy would not support daemonization in true UNIX sense, supervisor should be enough.

@ekohl
Copy link
Member Author

ekohl commented Oct 19, 2020

What solutions are on your mind? Technically speaking, a daemon is a process that has some unique attributes and no external tool can daemonize a process. There are tools however which can help supervising such processes like DJ's daemontools or even systemd these days.

https://software.clapper.org/daemonize/ is one of the first things to show up when I look for bsd daemonize. So roughly the ones you mentioned.

I am totally fine that smart-proxy would not support daemonization in true UNIX sense, supervisor should be enough.

Let's start with that then. It'll make this PR smaller.

@ekohl
Copy link
Member Author

ekohl commented Oct 21, 2020

I opened #777 to remove the daemonize code.

@adamruzicka
Copy link
Contributor

adamruzicka commented Dec 7, 2020

This seems to completely break smart_proxy_dynflow{,_core}. When running the two as separate processes, smart_proxy_dynflow fails to "proxy" the request to smart_proxy_dynflow_core and fails with rather strange error

2020-12-07T12:41:49 b697edb7 [I] Started GET /dynflow/tasks/count state=running
2020-12-07T12:41:49 b697edb7 [E] unable to resolve hostname for ip address ::ffff:192.168.122.85

no name for ::ffff:192.168.122.85
2020-12-07T12:41:49 b697edb7 [W] Error details for unable to resolve hostname for ip address ::ffff:192.168.122.85

no name for ::ffff:192.168.122.85: <Exception>: unable to resolve hostname for ip address ::ffff:192.168.122.85

no name for ::ffff:192.168.122.85
2020-12-07T12:41:49 b697edb7 [W] unable to resolve hostname for ip address ::ffff:192.168.122.85

no name for ::ffff:192.168.122.85: <Exception>: unable to resolve hostname for ip address ::ffff:192.168.122.85

no name for ::ffff:192.168.122.85
2020-12-07T12:41:49 b697edb7 [I] Finished GET /dynflow/tasks/count with 403 (197.35 ms)

When running smart_proxy_dynflow_core inside smart proxy, I'm getting this on startup

2020-12-07T12:44:22  [E] Error during startup, terminating
2020-12-07T12:44:22  [W] Error details for Error during startup, terminating: <NoMethodError>: undefined method `settings' for #<Hash:0x0000000002bf3270>
/usr/share/foreman-proxy/lib/proxy/app.rb:16:in `instance_eval'
(eval):8:in `block (2 levels) in initialize'
/usr/share/foreman-proxy/lib/proxy/app.rb:16:in `instance_eval'
/usr/share/foreman-proxy/lib/proxy/app.rb:16:in `block (2 levels) in initialize'
/usr/share/foreman-proxy/lib/proxy/app.rb:16:in `each'
/usr/share/foreman-proxy/lib/proxy/app.rb:16:in `block in initialize'
/opt/theforeman/tfm/root/usr/share/gems/gems/rack-2.2.3/lib/rack/builder.rb:125:in `instance_eval'
/opt/theforeman/tfm/root/usr/share/gems/gems/rack-2.2.3/lib/rack/builder.rb:125:in `initialize'
/usr/share/foreman-proxy/lib/proxy/app.rb:15:in `new'
/usr/share/foreman-proxy/lib/proxy/app.rb:15:in `initialize'
/usr/share/foreman-proxy/lib/proxy/launcher/puma.rb:58:in `new'
/usr/share/foreman-proxy/lib/proxy/launcher/puma.rb:58:in `app'
/usr/share/foreman-proxy/lib/proxy/launcher/puma.rb:22:in `block in conf'
/opt/theforeman/tfm/root/usr/share/gems/gems/puma-4.3.6/lib/puma/configuration.rb:148:in `configure'
/opt/theforeman/tfm/root/usr/share/gems/gems/puma-4.3.6/lib/puma/configuration.rb:141:in `initialize'
/usr/share/foreman-proxy/lib/proxy/launcher/puma.rb:20:in `new'
/usr/share/foreman-proxy/lib/proxy/launcher/puma.rb:20:in `conf'
/usr/share/foreman-proxy/lib/proxy/launcher/puma.rb:14:in `launch'
/usr/share/foreman-proxy/lib/proxy/launcher.rb:93:in `launch'
/usr/share/foreman-proxy/bin/smart-proxy:7:in `<main>'

@adamruzicka
Copy link
Contributor

@ekohl What remains to be done here? The needed rebase is fairly trivial, smart_proxy_dynflow_core does not exist anymore and so it doesn't cause any issues.

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.

5 participants