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

Handle URI Query Parameters #534

Merged
merged 3 commits into from Dec 20, 2017

Conversation

Projects
None yet
3 participants

Tensho added some commits Dec 20, 2017

require "amq/protocol/version"
puts "Using Ruby #{RUBY_VERSION}, amq-protocol #{AMQ::Protocol::VERSION}"

This comment has been minimized.

@Tensho

Tensho Dec 20, 2017

Contributor

Maybe it's more relevant to show SHA of the amq-protocol master branch here instead of version.

@Tensho

Tensho Dec 20, 2017

Contributor

Maybe it's more relevant to show SHA of the amq-protocol master branch here instead of version.

This comment has been minimized.

@michaelklishin

michaelklishin Dec 20, 2017

Member

What is there is no "SHA" because we don't run with a local clone?

@michaelklishin

michaelklishin Dec 20, 2017

Member

What is there is no "SHA" because we don't run with a local clone?

This comment has been minimized.

@Tensho

Tensho Dec 20, 2017

Contributor

As far as CI grabs amq-protocol master branch before tests run, it'd be nice to see exact commit SHA. Probably it'd bring a little benefit locally, but I'd prefer to control deps with bundle config --local local.amq-protocol ../amq-protocol. In this case I'm free to specify any path and don't create symlinks and use #custom_gem extension to #gem.

@Tensho

Tensho Dec 20, 2017

Contributor

As far as CI grabs amq-protocol master branch before tests run, it'd be nice to see exact commit SHA. Probably it'd bring a little benefit locally, but I'd prefer to control deps with bundle config --local local.amq-protocol ../amq-protocol. In this case I'm free to specify any path and don't create symlinks and use #custom_gem extension to #gem.

This comment has been minimized.

@michaelklishin

michaelklishin Dec 20, 2017

Member

We can do that in .travis.yml.

@michaelklishin

michaelklishin Dec 20, 2017

Member

We can do that in .travis.yml.

@@ -319,9 +343,9 @@
password: "bunny_password",
vhost: "bunny_testbed",
ssl: true,
ssl_cert: "spec/tls/client_cert.pem",

This comment has been minimized.

@Tensho

Tensho Dec 20, 2017

Contributor

:ssl context test data setup is outdated, so I normalized it with :tls context.

@Tensho

Tensho Dec 20, 2017

Contributor

:ssl context test data setup is outdated, so I normalized it with :tls context.

end
it "parses extra connection parameters" do
# session.start # raises "OpenSSL::SSL::SSLError: hostname "127.0.0.1" does not match the server certificate"

This comment has been minimized.

@Tensho

Tensho Dec 20, 2017

Contributor

This comment has been minimized.

@michaelklishin

michaelklishin Dec 20, 2017

Member

I responded there.

@michaelklishin

michaelklishin Dec 20, 2017

Member

I responded there.

This comment has been minimized.

@michaelklishin

michaelklishin Dec 20, 2017

Member

Inspecting the transport suggests that it has :ssl=>0.

@michaelklishin

michaelklishin Dec 20, 2017

Member

Inspecting the transport suggests that it has :ssl=>0.

@@ -464,7 +464,7 @@ def blocked?
# @param [String] uri amqp or amqps URI to parse
# @return [Hash] Parsed URI as a hash
def self.parse_uri(uri)
AMQ::Settings.parse_amqp_url(uri)
AMQ::Settings.configure(uri)

This comment has been minimized.

@Tensho

Tensho Dec 20, 2017

Contributor

❗️ Very important change that enables defaults merge for the parsed parameters.

@Tensho

Tensho Dec 20, 2017

Contributor

❗️ Very important change that enables defaults merge for the parsed parameters.

@Tensho

This comment has been minimized.

Show comment
Hide comment
@Tensho

Tensho Dec 20, 2017

Contributor

Strange things happen at CI, I can't understand how Bunny::Session#ssl? may return 0?
https://travis-ci.org/ruby-amqp/bunny/builds/319109274

Failures:

  1) Bunny::Session initialized via connection URI when URI contains query parameters parses extra connection parameters

     Failure/Error: expect(session.ssl?).to eq(true)

       expected: true
            got: 0

       (compared using ==)
     # ./spec/higher_level_api/integration/connection_spec.rb:66:in `block (4 levels) in <top (required)>'

Finished in 3 minutes 0 seconds (files took 0.2697 seconds to load)

198 examples, 1 failure

Failed examples:

rspec ./spec/higher_level_api/integration/connection_spec.rb:55 # Bunny::Session initialized via connection URI when URI contains query parameters parses extra connection parameters
Contributor

Tensho commented Dec 20, 2017

Strange things happen at CI, I can't understand how Bunny::Session#ssl? may return 0?
https://travis-ci.org/ruby-amqp/bunny/builds/319109274

Failures:

  1) Bunny::Session initialized via connection URI when URI contains query parameters parses extra connection parameters

     Failure/Error: expect(session.ssl?).to eq(true)

       expected: true
            got: 0

       (compared using ==)
     # ./spec/higher_level_api/integration/connection_spec.rb:66:in `block (4 levels) in <top (required)>'

Finished in 3 minutes 0 seconds (files took 0.2697 seconds to load)

198 examples, 1 failure

Failed examples:

rspec ./spec/higher_level_api/integration/connection_spec.rb:55 # Bunny::Session initialized via connection URI when URI contains query parameters parses extra connection parameters

@Tensho Tensho referenced this pull request Dec 20, 2017

Merged

URI Query Parameters #46

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Dec 20, 2017

Member

I get several TLS connection test suite failures in my standard testing environment, will take a look later in the week.

Member

michaelklishin commented Dec 20, 2017

I get several TLS connection test suite failures in my standard testing environment, will take a look later in the week.

@@ -191,7 +191,7 @@ def initialize(connection_string_or_opts = ENV['RABBITMQ_URL'], optz = Hash.new)
client_props = opts[:properties] || opts[:client_properties] || {}
@client_properties = DEFAULT_CLIENT_PROPERTIES.merge(client_props)
@mechanism = normalize_auth_mechanism(opts.fetch(:auth_mechanism, "PLAIN"))
@mechanism = (opts[:auth_mechanism] || []).first || "PLAIN"

This comment has been minimized.

@michaelklishin

michaelklishin Dec 20, 2017

Member

If we modify this at all (not sure why it's a part of this PR but OK), normalize_auth_mechanism should then be deleted.

@michaelklishin

michaelklishin Dec 20, 2017

Member

If we modify this at all (not sure why it's a part of this PR but OK), normalize_auth_mechanism should then be deleted.

This comment has been minimized.

@Tensho

Tensho Dec 20, 2017

Contributor

Yeah, nice spot, I forgot to remove the method. Basically auth_mechanism confused me too. According to specification client may specify several auth mechanism, so I decided to adheer in amq-protocol. But when I proceed with bunny, I realised that session initialization expects a single string value, not array. TBH, I didn't have time to analyze, how several auth mechanisms should be handled properly, so i decided to adheer bunny fallback to "PLAIN" for array case.

Let me recheck, maybe normalize_auth_mechanism is OK for array default value, I don't remember exact reason, why I replaced it...

@Tensho

Tensho Dec 20, 2017

Contributor

Yeah, nice spot, I forgot to remove the method. Basically auth_mechanism confused me too. According to specification client may specify several auth mechanism, so I decided to adheer in amq-protocol. But when I proceed with bunny, I realised that session initialization expects a single string value, not array. TBH, I didn't have time to analyze, how several auth mechanisms should be handled properly, so i decided to adheer bunny fallback to "PLAIN" for array case.

Let me recheck, maybe normalize_auth_mechanism is OK for array default value, I don't remember exact reason, why I replaced it...

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Dec 20, 2017

Member

So far everything points at the fact that @opts in Bunny::Session in your test have :ssl => 0. I pushed Tensho-init-params with my changes that were necessary to make the tests except for one (the same as yours) pass for me.

Please open a new PR based on that branch once you figure out what's going on in the newly added test example. Thanks.

Member

michaelklishin commented Dec 20, 2017

So far everything points at the fact that @opts in Bunny::Session in your test have :ssl => 0. I pushed Tensho-init-params with my changes that were necessary to make the tests except for one (the same as yours) pass for me.

Please open a new PR based on that branch once you figure out what's going on in the newly added test example. Thanks.

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Dec 20, 2017

Member

So the issue comes from AMQ::Settings.configure:

require "pp"

pp AMQ::Settings.configure("amqps://bunny_gem:bunny_password@/bunny_testbed?heartbeat=10&connection_timeout=100&channel_max=1000&verify=false&cacertfile=spec/tls/ca_certificate.pem&certfile=spec/tls/client_certificate.pem&keyfile=spec/tls/client_key.pem")

produces

{:host=>"127.0.0.1",
 :port=>5671,
 :user=>"bunny_gem",
 :pass=>"bunny_password",
 :vhost=>"bunny_testbed",
 :ssl=>0,
 :frame_max=>131072,
 :heartbeat=>10,
 :connection_timeout=>100,
 :channel_max=>1000,
 :auth_mechanism=>nil,
 :verify=>"false",
 :fail_if_no_peer_cert=>nil,
 :cacertfile=>"spec/tls/ca_certificate.pem",
 :certfile=>"spec/tls/client_certificate.pem",
 :keyfile=>"spec/tls/client_key.pem",
 :scheme=>"amqps"}
Member

michaelklishin commented Dec 20, 2017

So the issue comes from AMQ::Settings.configure:

require "pp"

pp AMQ::Settings.configure("amqps://bunny_gem:bunny_password@/bunny_testbed?heartbeat=10&connection_timeout=100&channel_max=1000&verify=false&cacertfile=spec/tls/ca_certificate.pem&certfile=spec/tls/client_certificate.pem&keyfile=spec/tls/client_key.pem")

produces

{:host=>"127.0.0.1",
 :port=>5671,
 :user=>"bunny_gem",
 :pass=>"bunny_password",
 :vhost=>"bunny_testbed",
 :ssl=>0,
 :frame_max=>131072,
 :heartbeat=>10,
 :connection_timeout=>100,
 :channel_max=>1000,
 :auth_mechanism=>nil,
 :verify=>"false",
 :fail_if_no_peer_cert=>nil,
 :cacertfile=>"spec/tls/ca_certificate.pem",
 :certfile=>"spec/tls/client_certificate.pem",
 :keyfile=>"spec/tls/client_key.pem",
 :scheme=>"amqps"}

michaelklishin added a commit that referenced this pull request Dec 20, 2017

@michaelklishin michaelklishin merged commit 61ffc6f into ruby-amqp:master Dec 20, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Tensho Tensho deleted the Tensho:init-params branch Dec 20, 2017

@@ -349,8 +353,8 @@ def prepare_tls_context(opts)
@tls_key = tls_key_from(opts)
@tls_certificate_store = opts[:tls_certificate_store]
@tls_ca_certificates = opts.fetch(:tls_ca_certificates, default_tls_certificates)
@verify_peer = (opts[:verify_ssl] || opts[:verify_peer])
@tls_ca_certificates = tls_ca_certificates_paths_from(opts) || default_tls_certificates

This comment has been minimized.

@carlhoerberg

carlhoerberg Jan 10, 2018

Contributor

I susptect that tls_ca_certificates_paths_from will never evaluate to nil, so the default_tls_certificates will never be used

@carlhoerberg

carlhoerberg Jan 10, 2018

Contributor

I susptect that tls_ca_certificates_paths_from will never evaluate to nil, so the default_tls_certificates will never be used

This comment has been minimized.

@Tensho

Tensho Jan 10, 2018

Contributor

You are totally right, what do you think if we call Array() out of #tls_ca_certificates_paths_from method?

@tls_ca_certificates  = Array(tls_ca_certificates_paths_from(opts) || default_tls_certificates)

Should I create a PR with fix?

@Tensho

Tensho Jan 10, 2018

Contributor

You are totally right, what do you think if we call Array() out of #tls_ca_certificates_paths_from method?

@tls_ca_certificates  = Array(tls_ca_certificates_paths_from(opts) || default_tls_certificates)

Should I create a PR with fix?

This comment has been minimized.

@carlhoerberg

carlhoerberg Jan 10, 2018

Contributor

yes, great

@carlhoerberg

carlhoerberg Jan 10, 2018

Contributor

yes, great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment