enhancement(sources): Add TLS support to socket, syslog, and vector sources #1892
Conversation
bruceg
added
source: syslog
bruceg
changed the title
Signed-off-by: Bruce Guenter <bruce@timber.io>
Signed-off-by: Bruce Guenter <bruce@timber.io>
Signed-off-by: Bruce Guenter <bruce@timber.io>
bruceg
force-pushed
the
tls-sources-acceptors
branch
from
149cd2a
to
0704fef
Compare
| @@ -15,6 +15,7 @@ use tokio::{ | |||
| reactor::Handle, | |||
| timer, | |||
| }; | |||
| use tokio_tls::TlsAcceptor; | |||
There was a problem hiding this comment.
do we want to just start using tokio_openssl instead?
There was a problem hiding this comment.
That may make sense. However, given that nothing else in Vector uses tokio_openssl yet, but other modules use tokio_tls as well (ex the parallel `src/sinks/utils/tcp.rs'), I think it would make more sense as a separate PR.
There was a problem hiding this comment.
Well that isn't true, I did add this :P https://github.com/timberio/vector/blob/ce326530da9235233355a86dcc5c27b626f40a10/src/sinks/util/rusoto.rs#L7
We really need to just pick one I think if we are already here we should just do it now instead of punting imo.
There was a problem hiding this comment.
Uh, ok, you said tokio_openssl, so I grepped the sources for tokio_openssl direct uses, of which there are still none.
I can rework the sources here, but are you also advising me to rework the sinks as well to use tokio_openssl in this PR?
There was a problem hiding this comment.
No just the stuff you touch in this PR. And sorry I totally could have been more clear. hyper-openssl and tokio-openssl are basically just wrappers around each other that implement the correct traits for the openssl crate. So I think it makes sense to write any new code using that then going back later and updating the others.
There was a problem hiding this comment.
Ok, so I looked into this, and it requires a rework of the TLS config handling that will end up being easier if I just do both acceptors and connectors at once, due to the different types involved (native_tls wrappers vs direct use of openssl).
src/sources/util/tcp.rs
Outdated
| ) | ||
| .forward(out) | ||
| .map(|_| debug!("TLS connection closed.")); | ||
| tokio::spawn(handler); |
There was a problem hiding this comment.
this has no span attached to it, we need to ensure it gets the correct span.
src/sources/util/tcp.rs
Outdated
| if let Some(tls) = tls.clone() { | ||
| match tls.acceptor() { | ||
| Err(error) => error!(message = "Failed to create a TLS connection acceptor", %error), | ||
| Ok(acceptor)=> { |
There was a problem hiding this comment.
can we break out this nested iflet matches a bit so that its a bit easier to follow?
There was a problem hiding this comment.
I pushed up a030d99, is that what you were suggesting?
src/sources/util/tcp.rs
Outdated
| match tls.acceptor() { | ||
| Err(error) => error!(message = "Failed to create a TLS connection acceptor", %error), | ||
| Ok(acceptor)=> { | ||
| let handler = TlsAcceptor::from(acceptor) |
There was a problem hiding this comment.
there is some duplicated code here, I would suggest we do the same thing we did with sinks and enum out a MaybeTlsStream and then have one FramedRead::new() setup. I think that would make this a bit more clear.
| .forward(out) | ||
| .and_then(|(_source, sink)| { | ||
| let mut stream = sink.into_inner().into_inner(); | ||
| // We should catch TLS shutdown errors here, |
There was a problem hiding this comment.
I am not directly following why we need to handle the shutdown here? the forward future should in theory flush all the way?
There was a problem hiding this comment.
The parallel send_lines function has a shutdown, so I tried to get it to work in the TLS version and was unable. I am uncomfortable removing one without the other.
There was a problem hiding this comment.
Yeah, so I think dropping the tcpstream should be fine enough since forward should ensure its all flushed. So I think its safe to ignore it.
Edit: maybe not we should explore, in theory I think just dropping should work fine for tests.
There was a problem hiding this comment.
The tests that use this new send_lines_tls have passed every time I tried, so if there is a race or other issue with this it isn't obvious. However, I left the comment there because of the discrepancy.
| } | ||
|
|
||
| pub(crate) fn acceptor(&self) -> crate::Result<TlsAcceptor> { | ||
| match self.identity() { |
There was a problem hiding this comment.
minor nit: this would be better if it were just an if let imo
There was a problem hiding this comment.
I think this is the match vs if debate again... (ref #1720)
There was a problem hiding this comment.
Though I think the reason I prefer if let here is because the option enumerates to exactly two options here therefore we don't need the extra syntax for matching None. I don't mind either way.
Signed-off-by: Bruce Guenter <bruce@timber.io>
Signed-off-by: Bruce Guenter <bruce@timber.io>
Signed-off-by: Bruce Guenter <bruce@timber.io>
Signed-off-by: Bruce Guenter <bruce@timber.io>
This is the first down payment on #1553, adding support for TLS to the three sources that accept simple TCP connections:
socket,syslog, andvector.Note that the
vectorsink does not yet support TLS connections, presumably because the source didn't support them at the time. Adding that support should be trivial, but I will put it in a follow up PR.Similarly, this does not add support for TLS to the sources that make outgoing connections. That too will be done in a separate PR.
Note also there is a hiccup in the added
send_lines_tlsfunction to do with shutdown of the socket. If somebody has an idea how to resolve that, I'd like to clean it up, but it works as it is now.