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

Setup TLS adaptor when parsing an amqprs uri #121

Merged
merged 8 commits into from
Feb 28, 2024
Merged

Conversation

tyilo
Copy link
Contributor

@tyilo tyilo commented Jan 15, 2024

No description provided.

@gftea
Copy link
Owner

gftea commented Jan 23, 2024

@tyilo thanks for pr. Can you add a test for the case. And check ci error

@tyilo
Copy link
Contributor Author

tyilo commented Jan 25, 2024

@gftea Everything should be working now, but the CI still fails. I can't reproduce it locally.

@gftea
Copy link
Owner

gftea commented Jan 27, 2024

@tyilo it seems about feature flag , the test is expecting w/o tls, but still tls is used

api::connection::tests::test_amqps_scheme_without_tls stdout ----
thread 'api::connection::tests::test_amqps_scheme_without_tls' panicked at amqprs/src/api/connection.rs:1515:39:
called `Result::unwrap()` on an `Err` value: NetworkError("network io error: invalid peer certificate contents: invalid peer certificate: UnknownIssuer")
note: panic did not contain expected string
      panic message: `"called `Result::unwrap()` on an `Err` value: NetworkError(\"network io error: invalid peer certificate contents: invalid peer certificate: UnknownIssuer\")"`,
 expected substring: `"UriError"`

@tyilo
Copy link
Contributor Author

tyilo commented Jan 29, 2024

@gftea I get the same error when running the tests on the main branch using cargo test --all-features --release, so again I don't that my changes causes it.

@gftea
Copy link
Owner

gftea commented Jan 30, 2024

@tyilo I have rerun main branch, it works. see https://github.com/gftea/amqprs/actions
You can merge my latest ci update first to see if new run is ok.

Check your rustc version? Now latest stable is rustc 1.75.

Try do cargo clean and rerun on main, then if it works, try to run again on your own branch.

@tyilo
Copy link
Contributor Author

tyilo commented Feb 1, 2024

@gftea Ah I understand now, I didn't have the RabbitMQ server running.

I see two possible solutions:

  • Remove the test_amqps_scheme_without_tls test
  • Keep the test_amqps_scheme_without_tls test, but change the signature of OpenConnectionArguments::tls_adaptor to take a Option<TlsAdaptor> instead of TlsAdaptor, so that the test can explicitly disable the default TLS adaptor.

What do you prefer?

@gftea
Copy link
Owner

gftea commented Feb 10, 2024

@tyilo , the test is prevent user giving a amqps scheme (implies secure channel) but without tls adaptor enabled. it prevents the incorrect configuration from beginning

@tyilo
Copy link
Contributor Author

tyilo commented Feb 13, 2024

@tyilo , the test is prevent user giving a amqps scheme (implies secure channel) but without tls adaptor enabled. it prevents the incorrect configuration from beginning

I understand what the test does. However, it is no longer possible to create a configuration from an amqps url without a TLS adaptor.

So do we want to:

  1. remove the test, or
  2. allow setting no TLS adaptor for an amqps URL

I don't really have a strong opinion about what to do. What do you prefer?

@so-schen
Copy link
Contributor

so-schen commented Feb 13, 2024

I see, this PR implicitly setup an adaptor without client authentication if amqps scheme. HOW user can use amqps with its customized adapter?

@tyilo
Copy link
Contributor Author

tyilo commented Feb 14, 2024

I see, this PR implicitly setup an adaptor without client authentication if amqps scheme. HOW user can use amqps with its customized adapter?

You then call tls_adaptor(...) as before to override the default adaptor.

The default should be without client authentication as this is the default for other AMQP libraries (and clients using TLS in general).

@gftea
Copy link
Owner

gftea commented Feb 15, 2024

Ok, let’s remove this test case as you suggested. Please help add documentation about the default behavior and how to customize it

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (07b5924) 85.49% compared to head (36cef87) 85.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   85.49%   85.90%   +0.40%     
==========================================
  Files          40       40              
  Lines        6206     6209       +3     
==========================================
+ Hits         5306     5334      +28     
+ Misses        900      875      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gftea gftea merged commit 27af06c into gftea:main Feb 28, 2024
13 checks passed
@tyilo tyilo deleted the from-str-amqps branch May 2, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants