Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Refactoring of https connector to make root certificates optional #47

Merged
merged 14 commits into from
Apr 13, 2022

Conversation

pawelchcki
Copy link
Contributor

Root certificates can be missing on the machine/container this library runs in. Currently the existing hyper_rusttls implementation with panic, and exit.

One way to avoid this is to rewrite part of the client initialization code to make it fail gracefully. Started this PR to see how that could look, but don't have time this week to bring this past the finish line

@pawelchcki pawelchcki changed the title Proposal for refactoring of https connector to make root certificates optional Refactoring of https connector to make root certificates optional Apr 12, 2022
@pawelchcki pawelchcki marked this pull request as ready for review April 12, 2022 08:26
Tcp{ #[pin] transport: tokio::net::TcpStream },
Tls{ #[pin] transport: tokio_rustls::client::TlsStream<tokio::net::TcpStream>},
// Tokio doesn't handle unix sockets on windows
#[cfg(unix)]
Copy link
Contributor

Choose a reason for hiding this comment

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

very cool having this cfg right inside the enum

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice, I thought pin_project_lite didn't understand cfg macros

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for confusion, The cfg macro got silently ignored by the pin_project_lite.

I've added windows builds to catch these in the future

Copy link
Contributor

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

Thanks !
LGTM. Another set of eyes would be nice to check on lifetimes and ownership (topic on which I'm not relevant enough).

Tcp{ #[pin] transport: tokio::net::TcpStream },
Tls{ #[pin] transport: tokio_rustls::client::TlsStream<tokio::net::TcpStream>},
// Tokio doesn't handle unix sockets on windows
#[cfg(unix)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice, I thought pin_project_lite didn't understand cfg macros

ddprof-exporter/src/connector/mod.rs Outdated Show resolved Hide resolved
@pawelchcki
Copy link
Contributor Author

#47 (comment)

TBF I'm not certain it works as expected, I'll try quickly adding a Windows build 🤞

@r1viollet
Copy link
Contributor

#47 (comment)

TBF I'm not certain it works as expected, I'll try quickly adding a Windows build 🤞

@gleocadie fyi, as you have interests in building on windows

@paullegranddc
Copy link
Contributor

Ok, I've tried compiling locally for windows using cross , and it actually doesn't work.

I get this error

% cross check --target x86_64-pc-windows-gnu

error[E0412]: cannot find type `UnixStream` in module `tokio::net`
  --> ddprof-exporter/src/connector/conn_stream.rs:21:44
   |
21 |         Udp{ #[pin] transport: tokio::net::UnixStream },
   |                                            ^^^^^^^^^^ not found in `tokio::net`

error[E0599]: no variant named `Udp` found for enum `ConnStream`
  --> ddprof-exporter/src/connector/conn_stream.rs:21:9
   |
13 | / pin_project! {
14 | |     #[derive(Debug)]
15 | |     #[project = ConnStreamProj]
16 | |     pub enum ConnStream {
...  |
21 | |         Udp{ #[pin] transport: tokio::net::UnixStream },
   | |         ^^^ variant not found in `ConnStream`
22 | |     }
23 | | }
   | |_- variant `Udp` not found here

@paullegranddc
Copy link
Contributor

Maybe if the cleaner code is worth it, we could try using https://github.com/taiki-e/pin-project instead of pin_project_lite as it supports more features.

@paullegranddc
Copy link
Contributor

Or we just write the projection code ourselves. Looking at the kind of code generated by pin-project https://github.com/taiki-e/pin-project/blob/HEAD/examples/struct-default-expanded.rs , anf removing the cruft it doesn't look to bad.

@gleocadie
Copy link

#47 (comment)
TBF I'm not certain it works as expected, I'll try quickly adding a Windows build 🤞

@gleocadie fyi, as you have interests in building on windows

thanks for the heads up

@pawelchcki
Copy link
Contributor Author

pawelchcki commented Apr 13, 2022

Thanks @paullegranddc - with your suggestion I reimplemented the project() method.
Its a bit verbose but not by much - explicit and visible use of Unsafe is a bit of an eyesore.

https://github.com/DataDog/libddprof/pull/47/files#diff-1d9699d6f15d9c4af24ade58efdaadaa21f28f7150cdbb6a95f65c20e8eca6f9R42-R60

Edit: after sleeing on this I decided to switch back to pin_project (non-lite) just to reduce the number of visible code lines and the unsafe eyesore (out of sight out of mind 😂 ).

@pawelchcki
Copy link
Contributor Author

Thanks All for the review!

@pawelchcki pawelchcki merged commit 4019c38 into main Apr 13, 2022
@pawelchcki pawelchcki deleted the pawel/fix_hyper_certs branch April 13, 2022 12:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants