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

chore: update tokio in tls #3188

Merged
merged 25 commits into from
Aug 3, 2020
Merged

chore: update tokio in tls #3188

merged 25 commits into from
Aug 3, 2020

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Jul 24, 2020

Ref. #2945

Tokio update in src/tls. Still need figure out lifetime problem for warp, see seanmonstar/warp#675 (currently works with patched warp: https://github.com/timberio/warp/tree/serve-incoming-lifetime)

@fanatid fanatid added type: tech debt A code change that does not add user value. sink: vector Anything `vector` sink related source: splunk_hec Anything `splunk_hec` source related source: logplex source: http_server Anything `http_server` source related sink: papertrail Anything `papertrail` sink related sink: datadog_logs Anything `datadog_logs` sink related labels Jul 24, 2020
@fanatid fanatid self-assigned this Jul 24, 2020
@lukesteensen lukesteensen mentioned this pull request Jul 24, 2020
47 tasks
@fanatid
Copy link
Contributor Author

fanatid commented Jul 24, 2020

I'm not sure that we need these cyclical dependencies in features:

sources-http = ["warp", "sources-tls"]
sources-logplex = ["warp", "sources-tls"]
sources-socket = ["bytesize", "listenfd", "tokio/uds", "tokio-util/udp", "sources-tls"]
sources-splunk_hec = ["bytesize", "warp", "sources-tls"]
sources-tls = ["sources-http", "sources-logplex", "sources-socket", "sources-splunk_hec"]

I think that sources-tls should be equal [], tls do not depend on listed features.

Addressed in #3210

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

I love how much this simplifies the TLS wrappers. Some questions about some of the patches though.

src/topology/config/mod.rs Outdated Show resolved Hide resolved
src/sinks/util/tcp.rs Outdated Show resolved Hide resolved
src/sources/util/tcp.rs Outdated Show resolved Hide resolved
src/tls/mod.rs Outdated Show resolved Hide resolved
src/tls/maybe_tls.rs Show resolved Hide resolved
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
src/tls/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@fanatid
Copy link
Contributor Author

fanatid commented Jul 27, 2020

TcpSink do not work properly now on shutdown :( need learn how compat works for fixing code =\

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@fanatid fanatid marked this pull request as ready for review July 28, 2020 11:19
@fanatid fanatid requested a review from ktff July 28, 2020 11:20
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

Almost there! I'll try to be faster on the review next time, sorry.

.incoming()
.take(2)
.for_each(|connection| {
let mut close_rx = close_rx.take();
Copy link
Member

Choose a reason for hiding this comment

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

Why did this need to become an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tokio::sync::oneshot::Receiver not Copy and not Clone and can be used only once, but we have two connections, so I need say for a second that I do not have Receiver to it.

Comment on lines 96 to 102
// enum TcpSinkState2 {
// Disconnected,
// ResolvingDns(BoxFuture<'static, Result<crate::dns::LookupIp, crate::dns::DnsError>>),
// Connecting(BoxFuture<'static, Result<MaybeTlsStream<TcpStream>, TlsError>>),
// Connected(FramedWrite<MaybeTlsStream<TcpStream>, BytesCodec>),
// Backoff(BoxFuture<'static, ()>),
// }
Copy link
Member

Choose a reason for hiding this comment

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

We should clean up all the commented out code before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I understand that this code not related anyhow with current TcpSink, but can be very useful in future for saving time of anybody who will work on migrating to new Sink trait. If you insist to delete this, can I at least add a link to relevant code in this PR with a comment about new Sink trait impl?

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@fanatid fanatid merged commit 9b05bdc into master Aug 3, 2020
@fanatid fanatid deleted the maybetls-update branch August 3, 2020 20:05
fanatid added a commit to fanatid/vector that referenced this pull request Aug 4, 2020
fanatid added a commit that referenced this pull request Aug 4, 2020
This reverts commit 9b05bdc.

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
fanatid added a commit that referenced this pull request Aug 4, 2020
This reverts commit 9b05bdc.

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
fanatid added a commit that referenced this pull request Aug 13, 2020
* chore: update tokio in tls

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
* chore: update tokio in tls

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
)

This reverts commit 9b05bdc.

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sink: datadog_logs Anything `datadog_logs` sink related sink: papertrail Anything `papertrail` sink related sink: vector Anything `vector` sink related source: http_server Anything `http_server` source related source: splunk_hec Anything `splunk_hec` source related type: tech debt A code change that does not add user value.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants