1339035 - Rewrite the pingsender in Rust

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, defect, P4)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #1310703 +++

We've written the pingsender tool in (heavily platform-dependent) C++ mostly because we were in a hurry and we had no way to do HTTP operations in Rust without pulling in a lot of external code. As soon as we have a crate in m-c that can be used for HTTP operations we should rewrite the pingsender in platform-agnostic Rust.

Comment 1

[Jan-Erik Rediger [:janerik] (Away until 2024-12-23)]

Hey :gsvelto, happy to see this assigned now. In the past I made several attempts in porting pingsender to Rust, which turned out to be rather easy for the functionality itself, but I ran into some problems to take it over the finishing line:

• The resulting binary was much larger than what the current implementation achieves (mostly because of statically linking all the libraries)
• It depended on openssl due to the lack of suitable alternatives in the Rust ecosystem back then (and bringing an openssl dependency into m-c seemed ... unwanted?)
• Newer deps required nightly (which is probably not an issue anymore)

I'm curious which way you want to go here, let me know if you need any input and I'm also happy to review any finished code.

Comment 2

[Gabriele Svelto [:gsvelto]]

Hi Jan, Kris started working on this a week ago IIRC. Thanks for chiming in, I must admit I had not considered the problem of HTTPS connections until you mentioned it.

Right now pingsender is dealing with HTTPS via Windows native APIs or via curl (which will in turn depend on OpenSSL/GnuTLS/etc...). When I saw that we had vendored hyper among the other Rust crates I just assumed that it would have HTTPS support out-of-the-box... but it doesn't. Chasing crates on crates.io shows that one can add it via the hyper-tls crate which in turn uses native-tls which uses natives services on Windows and Mac, and OpenSSL on Linux.

I'm not sure if that's a viable option since it would mean vendoring even more crates.

Alternatively one could re-implement what we have now in Rust: i.e. using native calls to the WinNET API on Windows and dlopen()'ing curl on Linux and macOS. That would be a lot uglier, but still less icky than the current C++ codebase.

Kris, did you already run into this problem?

Comment 3

[Kris Wright :KrisWright]

(In reply to Gabriele Svelto [:gsvelto] from comment #2)

Kris, did you already run into this problem?

Hello! This is perfect timing with where I was at with getting PingSender to work - I used hyper as mentioned and aside from it not having as much support as it seemed, it also has a lot more demands than I expected to do something as simple as sending an http post. It's not really designed for this functionality. While I was able to muddle my way through the Hyper docs and work out a way to do this, it's still not ideal to me and it's hard to even interpret whether a ping was successfully sent from the client side using our Hyper version's request builder.

One suggestion I've gotten is to try using the Reqwest crate, which we don't currently vendor, as its post method is used for this exact purpose. I'm not completely sure what kind of support it has. Otherwise, at this point it would make more sense to go the route of more or less rewriting our c++ code in Rust as you mentioned.

Comment 4

[Gabriele Svelto [:gsvelto]]

Thanks Kris. Reqwest looks interesting. We have most of its dependency already vendored with the exception of native-tls which isn't huge. It also offers enough functionality for other tools to use (such as the crash reporter client which is also waiting to be rewritten in Rust). If you go down the equivalent-rewrite-of-the-C++-code route consider making a separate crate with the functionality to send HTTP POST messages because we'd reuse it elsewhere.

Also keep in mind that this isn't urgent, so if it turns out to be more work than we had anticipated and you have more urgent stuff we can let this lie.

Comment 5

[Kris Wright :KrisWright]

Looking at the options of Reqwest vs. a complete rewrite of the http post functionality, it looks like it would make more sense to use Reqwest. I had a look at both Reqwest and native-tls crates, and I'm not completely certain what to look for in the licenses but they both seem to carry the same licenses as some of the third-party code we are already vendoring. However I don't know for sure what to look for in the license. Since we want to leverage some kind of http post in more than just this one place, it makes the most sense to go with the code that already exists if possible.

:heycam, do you know the process for vendoring a new crate? The Oxidation wiki page said that there's no formal sign-off procedure, but I'm not sure if there's a general process that we like to go by or if we can use Reqwest at all.

Comment 6

[Cameron McCormack (:heycam)]

We don't really have any policies for deciding whether a crate can be vendored or not. For crates that are maintained by Mozillians, we tend to allow such crates to be vendored with only very light review. For other crates, it would be best if reviewers could review the vendored code but it's obviously a lot to ask, so in practice that doesn't really happen. Reviewers ought, though, consider whether certain crates should be used -- maybe they are large, or duplicate functionality in other crates, and so alternatives should be considered.

The ./mach vendor rust step will check the license of any newly vendored crate and complain if it's not on our list of allowed licenses. There is a process to get new licenses approved but I'm not sure what that is. The list of currently allowed licenses is in https://searchfox.org/mozilla-central/rev/5e830ac8f56fe191cb58a264e01cdbf6b6e847bd/python/mozbuild/mozbuild/vendor_rust.py#140-168 and it looks like reqwest is MIT/Apache-2.0 which is on the list.

As for using reqwest itself, when I try to add it as a dependency, it results in 5 MiB of new source files being vendored. That seems like a lot. It includes:

• base64-0.9.3
• foreign-types-shared
• foreign-types
• hyper-tls
• idna-0.1.5
• libflate
• native-tls
• openssl-probe
• openssl-sys
• openssl
• percent-encoding-1.0.1
• reqwest
• rle-decode-fast
• schannel
• security-framework-sys
• security-framework
• serde_urlencoded-0.5.5
• take_mut
• url-1.7.2

Those with a version number are crates that we already vendor but with a different (conflicting) version. We should try to avoid that where possible but that might be a bit of work.

As Gabriele mentioned, we already vendor hyper, since geckodriver uses it. Adding a dependency on hyper-tls adds 2 MiB of vendored crates:

• foreign-types-shared
• foreign-types
• hyper-tls
• native-tls
• openssl-probe
• openssl-sys
• openssl
• schannel
• security-framework-sys
• security-framework

so no duplicated crates with different versions, at least.

My feeling is that adding reqwest seems pretty heavy, and if hyper is awkward to use without reqwest, and if there's no real problem with the platform specific networking calls (and use of curl on Linux), then I would stick with that approach.

I'm not sure if Bobby has an official Rust policy role -- I recall it being mentioned at some point but I'm not sure if I can find anything written down -- but needinfo'ing him here in case he has a different view.

Comment 7

[Bobby Holley (:bholley)]

Leaving aside in-tree size for a moment: I think we really don't want to pull in openssl.

As a general rule, we want all networking activity to go through necko, especially when cryptography is involved. We have an unfortunate overlap inside the third_party/rust directory between 'things that ship in Firefox' and 'things that are used as part of automation/testing/tooling/etc'. This is why, for example, we have all the hyper and tokio stuff in there, even though we don't ship it as part of Firefox. Every once and a while a networking person looks in third_party/rust and has a heart attack, before someone explains that we're not actually shipping that code.

This third category - things we ship to users but as separate binaries - is kind of a blurry area. The most ideal case there would be to link against libXUL and delegate the networking functionality to Gecko. Is that not feasible?

Comment 8

[Kris Wright :KrisWright]

(In reply to Bobby Holley (:bholley) from comment #7)

The most ideal case there would be to link against libXUL and delegate the networking functionality to Gecko. Is that not feasible?

I first explored making a shared library that calls into Gecko but it looks like this is not ideal for PingSender. When I asked around the office about PingSender's uses, I was told that it's very important to keep a separate binary. Currently, when we send a telemetry post we run the program with runPingSender.

What it's starting to look like is that we want to rewrite the current PingSender Post code in Rust using curl for Unix and native system calls elsewhere, assuming this is possible. There is interest in reusing this code elsewhere in the crash reporter rewrite. If it can't be done, I'm curious if we can explore some other routes to pull in the native c++ 'post' code, but if at all possible I think it's best to at least directly rewrite it in Rust.

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Bobby Holley (:bholley) from comment #7)

This third category - things we ship to users but as separate binaries - is kind of a blurry area. The most ideal case there would be to link against libXUL and delegate the networking functionality to Gecko. Is that not feasible?

Unfortunately pingsender covers one of the two cases where we really can't use Necko. Specifically pingsender is used to send telemetry pings when Firefox is shutting down as well as after it has crashed. In both cases we can't rely on libxul. The other case is the crash reporter client which also need to do an HTTP POST to submit a crash report and that also has its own live-off-the-platform HTTP POST implementation.

Since this functionality (send a single HTTP POST to a well-known HTTPS url) is going to be shared by the pingsender and the crash reporter client, and since there aren't existing crates that can help us then maybe we can split this bug and roll our own crate to wrap whatever native mechanism we can use. I'd be very happy if someone from the network team would review that so that we know we're not getting something wrong. None of us is a networking expert.

The existing C++ code in pingsender relies on libcurl on Linux and macOS and on the WinINET API on Windows. I'd like if we could use the same in Rust. In both case we use the libraries also for URL manipulation but that wouldn't be needed because we already vendor the url crate.

Comment 10

[Kris Wright :KrisWright]

Attachment: Bug 1339035 - (WIP) PingSender as a standalone Rust program

This revision is just the very simple components that parse the url and compress the payload for PingSender. These are the preparatory steps the pingsender needs to take before invoking some means of HTTP POST in order to verify the destination and the payload. It does not actually post anything; see Bug 1589177.

Comment 11

[Gabriele Svelto [:gsvelto]]

Resetting this since we decided to postpone it until there's better crates available or we have the time to roll them up ourselves.

Comment 12

[Bobby Holley (:bholley)]

WONTFIXing in favor for bug 1734262.