Details

Reviewers

varun
bartek

Commits

rCOMMccb92e0486e1: [keyserver] Adding `tunnelbroker-client` to the keyserver napi-rs dependencies

Summary

This diff introduces the integration of the shared Tunnelbroker-client Rust library into the keyserver using the napi-rs as a dependency.
We are updating the Cargo.toml file for the rust-node-addon and updating the Dockerfile to copy the tunnelbroker-client shared library inside the container.

Linear task: ENG-2729

Test Plan

All services should be built successfully according to the CI.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Jan 17 2023, 6:40 AM

• max held this revision as a draft.

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 17 2023, 6:40 AM

Harbormaster failed remote builds in B15400: Diff 21000!Jan 17 2023, 6:43 AM

• max added a parent revision: D6281: [tunnelbroker-client] Adding Tunnelbroker tonic client initialization function.Jan 25 2023, 5:40 AM

• max added a parent revision: D5796: [keyserver] implement registerUser rpc with napi-rs/tonic/opaque-ke.Jan 25 2023, 5:42 AM

Rebasing, making changes in Dockerfile, making D5796 as a parent.

Harbormaster failed remote builds in B15633: Diff 21314!Jan 25 2023, 5:57 AM

• max edited the summary of this revision. (Show Details)Jan 25 2023, 5:59 AM

• max edited the test plan for this revision. (Show Details)

• max added reviewers: varun, bartek.

Adding copying of protobuf files into the Dockerfile.

Harbormaster failed remote builds in B15634: Diff 21315!Jan 25 2023, 6:17 AM

• max published this revision for review.Jan 25 2023, 6:27 AM

• max planned changes to this revision.

Installing protoc by the script.

Harbormaster failed remote builds in B15642: Diff 21324!Jan 25 2023, 9:28 AM

• max planned changes to this revision.Jan 25 2023, 9:28 AM

Fixing the install protoc script running.

• max planned changes to this revision.Jan 25 2023, 9:59 AM

Harbormaster failed remote builds in B15645: Diff 21327!Jan 25 2023, 10:04 AM

Forcing ot install protoc out of buildkite.

• max planned changes to this revision.Jan 25 2023, 10:14 AM

Harbormaster failed remote builds in B15646: Diff 21328!Jan 25 2023, 10:18 AM

Adding scripts to copy.

• max planned changes to this revision.Jan 25 2023, 10:27 AM

Harbormaster failed remote builds in B15647: Diff 21329!Jan 25 2023, 10:31 AM

ashoat added a subscriber: • jon.Jan 25 2023, 12:52 PM

ashoat added inline comments.

keyserver/addons/rust-node-addon/install_external_deps.sh
5–12 ↗	(On Diff #21329)	I think there's some reason for this logic, maybe @jon knows

Adding cmake to dependencies.

• max planned changes to this revision.Jan 26 2023, 4:35 AM

• max marked an inline comment as done.Jan 26 2023, 4:38 AM

• max added inline comments.

keyserver/addons/rust-node-addon/install_external_deps.sh
5–12 ↗	(On Diff #21329)	I think there's some reason for this logic, maybe @jon knows This came from D5796 where we are installing `protoc` using the `apt` when not in a CI workflow, but we must install the `protoc` using the `install_protobuf.sh` anyway because the `file contains proto3 optional fields` error (protoc version inconsistency).

Harbormaster failed remote builds in B15660: Diff 21357!Jan 26 2023, 4:39 AM

Updating ENG-2808 on the protoc build issue.

ashoat added inline comments.Jan 26 2023, 7:17 AM

keyserver/addons/rust-node-addon/install_external_deps.sh
5–12 ↗	(On Diff #21329)	You're breaking behavior though, aren't you? As mentioned above, this condition was here for a reason. It seems like you are going to cause the installation to occur in places where it should not, just so you can get your logic to work. It's important to consider "why" some code exists, instead of hammering things until they work for you

Rebasing on parent changes.

• max planned changes to this revision.Jan 26 2023, 12:07 PM

• max marked an inline comment as done.Jan 26 2023, 12:10 PM

• max added inline comments.

keyserver/addons/rust-node-addon/install_external_deps.sh
5–12 ↗	(On Diff #21329)	You're breaking behavior though, aren't you? As mentioned above, this condition was here for a reason. It seems like you are going to cause the installation to occur in places where it should not, just so you can get your logic to work. It's important to consider "why" some code exists, instead of hammering things until they work for you That was a "draft" diff which included changes that must/can be in the parent D5796. I think I should mark it as a draft in a title as well along with the "changes planned" state, until its ready.

Harbormaster completed remote builds in B15687: Diff 21390.Jan 26 2023, 12:21 PM

• max requested review of this revision.Jan 26 2023, 12:26 PM

• max marked an inline comment as done.

• max added a child revision: D6459: [keyserver] Adding Tunnelbroker-client napi-rs class constructor.Jan 30 2023, 2:30 AM

• max edited parent revisions, added: D6284: [tunnelbroker-client] Adding `publish_message` client function; removed: D6281: [tunnelbroker-client] Adding Tunnelbroker tonic client initialization function.Jan 30 2023, 2:47 AM

Accepting with one wording change inline

keyserver/Dockerfile
127 ↗	(On Diff #21390)
keyserver/addons/rust-node-addon/Cargo.toml
26 ↗	(On Diff #21390)	FWIW this kind of relative path appears to be breaking the Windows GitHub CI workflow... I paired with jon today to try to resolve it but no luck. Will try to sync with @ashoat because it might have something to do with node_modules that I don't understand

This revision is now accepted and ready to land.Jan 31 2023, 9:15 PM

Let's make sure we don't land this until we have clarity on the Windows GitHub issue, or at least we should be ready to revert it immediately if it breaks that

In D6283#194441, @ashoat wrote:

Let's make sure we don't land this until we have clarity on the Windows GitHub issue, or at least we should be ready to revert it immediately if it breaks that

I'll check if it's resolved first, or will try on the GitHub branch first.

Fixing comment.

• max marked an inline comment as done.Feb 2 2023, 5:15 AM

Harbormaster failed remote builds in B16000: Diff 21844!Feb 2 2023, 5:24 AM

Rebasing on parent changes.

Harbormaster completed remote builds in B16540: Diff 22628.Feb 15 2023, 2:06 PM

Closed by commit rCOMMccb92e0486e1: [keyserver] Adding `tunnelbroker-client` to the keyserver napi-rs dependencies (authored by • max). · Explain WhyFeb 15 2023, 2:08 PM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMMccb92e0486e1: [keyserver] Adding `tunnelbroker-client` to the keyserver napi-rs dependencies.

• max removed a child revision: D6459: [keyserver] Adding Tunnelbroker-client napi-rs class constructor.Feb 21 2023, 10:53 AM