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

Add TLSAcceptor and Builder #186

Merged
merged 9 commits into from
May 26, 2023
Merged

Add TLSAcceptor and Builder #186

merged 9 commits into from
May 26, 2023

Conversation

Licenser
Copy link
Contributor

TLSAcceptor, including a builder, also adds a builder() method to the Connector for convenience.

@Licenser
Copy link
Contributor Author

Licenser commented Feb 3, 2023

Sorry I didn't saw that the tests failed :) fixed it

@Licenser
Copy link
Contributor Author

Licenser commented Feb 3, 2023

idk why but I can't reproduce the failure locally :( but I think I found the reason

@Licenser
Copy link
Contributor Author

Licenser commented Feb 3, 2023

I'm stumped, the CI errors with:

error[E0432]: unresolved imports `hyper::server::conn::AddrIncoming`, `hyper::server::conn::AddrStream`
 --> src/acceptor.rs:5:12
  |
5 |     conn::{AddrIncoming, AddrStream},
  |            ^^^^^^^^^^^^  ^^^^^^^^^^ no `AddrStream` in `server::conn`
  |            |
  |            no `AddrIncoming` in `server::conn`

the server feature is part of the Cargo.toml and locally it works.

AddrIncoming requires just the server feature regarding to the docs too so I honestly don't know why the CI hates me :(

@Licenser
Copy link
Contributor Author

Licenser commented Feb 3, 2023

This is what happens locally for me and I can't find the difference between my box and the CI

❯ cargo test --no-default-features
   Compiling rustls v0.20.8
   Compiling tokio-rustls v0.23.4
   Compiling hyper-rustls v0.23.2 (/home/heinz/hyper-rustls)
warning: unused import: `rustls::client::WantsTransparencyPolicyOrClientCert`
 --> src/config.rs:1:5
  |
1 | use rustls::client::WantsTransparencyPolicyOrClientCert;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: unused macro definition: `trace`
  --> src/lib.rs:90:18
   |
90 |     macro_rules! trace    ( ($($tt:tt)*) => {{}} );
   |                  ^^^^^
   |
   = note: `#[warn(unused_macros)]` on by default

warning: unused macro definition: `debug`
  --> src/lib.rs:91:18
   |
91 |     macro_rules! debug    ( ($($tt:tt)*) => {{}} );
   |                  ^^^^^

warning: unused imports: `debug`, `trace`
  --> src/lib.rs:92:21
   |
92 |     pub(crate) use {debug, trace};
   |                     ^^^^^  ^^^^^

warning: `hyper-rustls` (lib) generated 4 warnings
warning: unused import: `super::ConnectorBuilder as HttpsConnectorBuilder`
   --> src/connector/builder.rs:264:9
    |
264 |     use super::ConnectorBuilder as HttpsConnectorBuilder;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: `hyper-rustls` (lib test) generated 5 warnings (4 duplicates)
    Finished test [unoptimized + debuginfo] target(s) in 4.03s
     Running unittests src/lib.rs (target/debug/deps/hyper_rustls-5f605abbeb6420e6)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/tests.rs (target/debug/deps/tests-649482a5c3748cb9)

running 3 tests
Starting to serve on https://127.0.0.1:1338.
Starting to serve on https://127.0.0.1:1337.
test client ... ok
test server ... ok
test custom_ca_store ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.02s

   Doc-tests hyper-rustls

running 3 tests
test src/lib.rs - (line 8) - compile ... ok
test src/lib.rs - (line 32) - compile ... ok
test src/connector/builder.rs - connector::builder::ConnectorBuilder (line 17) ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.33s

@Licenser
Copy link
Contributor Author

Licenser commented Feb 4, 2023

@djc when you have some time I could use a extra pairs of eyes since the errors in CI make no sense to me :(

@Licenser
Copy link
Contributor Author

I may or may not have solved the CI issue, I made a feature flag for the server as it pulls in additional (sub) dependencies otherwise and I wanted to avoid that as default

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, and sorry for the slow follow-up!

Would be great to get this over the finish line.

(Note, this also needs a rebase.)

examples/server.rs Outdated Show resolved Hide resolved
src/acceptor.rs Show resolved Hide resolved
src/acceptor.rs Outdated Show resolved Hide resolved
src/acceptor/builder.rs Show resolved Hide resolved
src/acceptor/builder.rs Show resolved Hide resolved
src/acceptor/builder.rs Outdated Show resolved Hide resolved
src/connector.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Licenser and others added 3 commits May 23, 2023 11:46
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
@Licenser
Copy link
Contributor Author

Should be all done :)

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Sorry, a few more thoughts, shaping up nicely though!

examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
src/acceptor.rs Show resolved Hide resolved
src/acceptor/builder.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/acceptor.rs Show resolved Hide resolved
Licenser and others added 2 commits May 23, 2023 16:39
Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
@Licenser
Copy link
Contributor Author

Okay second try, should be all done now :D

@Licenser
Copy link
Contributor Author

The failing tests is why this existed. Are you OK with putting that code back?

@djc
Copy link
Member

djc commented May 24, 2023

Yup -- sorry, forgot about that gotcha.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

I'm not sure I'm familiar enough with the async ecosystem and Hyper in particular to give very meaningful feedback but I can surface a couple small nits :-)

src/acceptor.rs Outdated Show resolved Hide resolved
src/acceptor.rs Outdated Show resolved Hide resolved
src/acceptor/builder.rs Outdated Show resolved Hide resolved
src/acceptor/builder.rs Outdated Show resolved Hide resolved
Licenser and others added 2 commits May 24, 2023 21:33
Co-authored-by: Daniel McCarney <daniel@binaryparadox.net>
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
@Licenser
Copy link
Contributor Author

I may have found an alternative by updating the required features for the example, in theory ™️ (aka on my local machine) it should now not build during unless the acceptor flag is present

@djc
Copy link
Member

djc commented May 24, 2023

Nice, I was just looking for that this afternoon but couldn't quite find the documentation for it.

@Licenser
Copy link
Contributor Author

I know you didn't like the Default implementation for the Builder, but clippy wants it and the CI is not passing. Do you prefer Default to return or would you prefer a #[allow(...)] for the function?

grafik

@djc
Copy link
Member

djc commented May 26, 2023

I'm okay with adding it back, I guess.

Signed-off-by: Heinz N. Gies <heinz@licenser.net>
@Licenser
Copy link
Contributor Author

Done, also added HTTP/1.0 support I think that's why windows / OS X were failing.

@djc djc merged commit 286e1fa into rustls:main May 26, 2023
@djc
Copy link
Member

djc commented May 26, 2023

Thanks for working through all that!

@Licenser
Copy link
Contributor Author

Woooh :D thanks!

@robinfriedli robinfriedli mentioned this pull request Jun 30, 2023
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