THREESCALE-12122: Fix Que worker connection to TLS protected DB

[jlledom]

The Que worker is not able to connect to TLS protected DBs. This is because we launch the que worker via rake task, and this calls new process que.

This process is able to read Rails DB configuration properly, however, it only extracts the basic connection data from it (code). Due to this, the SSL configuration is lost and Que fails to connect. We see the error:

connection requires a valid client certificate

The only way to solve this is to pass the connection_url parameter to the que call. A you see in the code, if you send --connection_url and set all the SSL params in the url query, those parameters are actually loaded by the Que connection. Without the connection_url param, all SSL params are ignored.

The bug is important because we are in fact not supporting TLS protected DBs in Zync and we thought we did. The only supported scenario is when the connection doesn't require any of the ssl parameters, like connecting without mTLS to a server protected by a certificate already trusted in the OS certs bundle.

Jira issue

https://issues.redhat.com/browse/THREESCALE-12122

[akostadinov]

Does this make any sense and can it help?

Here's what you could do instead of the PR's approach:

Add this line to config/initializers/que.rb (after line 4):
Que.connection = ActiveRecord

This way:

• When que ./config/environment.rb runs, it loads Rails which loads the
  initializer
• The initializer sets Que to use ActiveRecord's connection
• ActiveRecord already has all the SSL settings from database.yml
• No need to manually reconstruct the connection URL

This is exactly how the test helper does it (test/test_helper.rb:45), and
it would be consistent with how the main app handles connections. The PR's
approach of manually building the connection URL is more fragile and
duplicates configuration that already exists in database.yml.

Sources:

• https://github.com/que-rb/que/blob/master/docs/README.md
• https://www.rubydoc.info/gems/que/0.14.3
• https://blog.arkency.com/how-to-connect-to-and-use-activerecord-outside-
  of-rails/

[akostadinov]

Basically, if we run from rake, then make environment a dependency of that task. In case it is not already a dependency. Seems to me more reliable than constructing the connection using different logic and the potential inconsistency between the approaches as evident in this same issue.

[jlledom]

Does this make any sense and can it help?

Here's what you could do instead of the PR's approach:
Add this line to config/initializers/que.rb (after line 4):
Que.connection = ActiveRecord

I already tried that and doesn't work. The reason is because Que in fact opens two connections, one for executing the jobs itself, and the other one for the "locker", the first one already receives all SSL parameters, but the Locker one has this bug and, by reading the code, really having a truthy connection_url seems the only way for it to read the SSL params.

https://github.com/que-rb/que/blob/17fb2c3b75b37599bf17043dbb692555f582f249/lib/que/locker.rb#L164-L174

[jlledom]

Basically, if we run from rake, then make environment a dependency of that task. In case it is not already a dependency. Seems to me more reliable than constructing the connection using different logic and the potential inconsistency between the approaches as evident in this same issue.

It's already a dependency, the que process receives the environment variables correctly. In fact, Que takes the DB object from ActiveRecord directly, already configured with SSL support. The problem is the Locker class, which takes the Rails connection and extracts its config to create another connection, and forgets the SSL params in the way. When the SSL params are given from the connection_url parameter it actually takes them. It's a bug.

[akostadinov]

Can't we monkey patch the thing to grab another connection from the ActiveRecord pool and pass it to the locker? Perhaps submit the fix upstream? This manually passing parameters feels wrong.

[jlledom]

Can't we monkey patch the thing to grab another connection from the ActiveRecord pool and pass it to the locker? Perhaps submit the fix upstream? This manually passing parameters feels wrong.

We probably can, yes. But do you really prefer monkey patching? I know the hack is ugly but I don't think it's too dangerous. It should work as long as the Rails connection parameters are correct, and if they aren't, then the que worker would fail anyway.

I don't have a strong opinion, though. @mayorova you untie here.

[akostadinov]

I looked more carefully and I don't understand, why would it checkout a connection from the pool and then create another one? Also apparently it never checks-in that connection.

The simplest way to monkey patch this would be to override Que::Connection.wrap but then I'm thinking that possibly they had a good reason to establish another connection that we don't know and it can trouble us.

So I agree that the safest option, as ugly as it is, atm is to use your update. But also propose an upstream fix, where the checked-out connection is used directly instead of just reading its parameters. And link to this task so maybe some day we remove this duplication.

[jlledom]

So I agree that the safest option, as ugly as it is, atm is to use your update. But also propose an upstream fix, where the checked-out connection is used directly instead of just reading its parameters. And link to this task so maybe some day we remove this duplication.

Nice. I didn't send a fix, but I opened a issue there: que-rb/que#442

[akostadinov]

Would be nice to add a comment line pointing at it so that later somebody may check if there was any fix. Things look ok for me.

[jlledom]

Would be nice to add a comment line pointing at it so that later somebody may check if there was any fix. Things look ok for me.

eb33751

[mayorova]

I agree with @akostadinov that it seems a bit ugly, but probably indeed it's better than monkey-patch...

What would be nice though is to put this logic into some module under lib and call it from here. This will allow us to add some unit tests.

While simple, I think this logic is error-prone, and I'd rather have some unit tests.

[jlledom]

I created this: d384f7a

And the tests: 6c03c2b

[qltysh]

❌ 2 blocking issues (3 total)

Tool	Category	Rule	Count
rubocop	Lint	Cyclomatic complexity for build\_connection\_url is too high. [19/7]	1	❌
rubocop	Lint	Class has too many lines. [263/250]	1	❌
qlty	Structure	Function with high complexity (count = 26): build_connection_url	1

[mayorova]

Looks good to me (although I haven't tested it with an actual TLS-protected DB)