Page MenuHomePhabricator

[keyserver] opaque-ke-napi scaffolding
ClosedPublic

Authored by varun on Dec 1 2022, 2:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 6, 1:31 PM
Unknown Object (File)
Mon, May 6, 1:31 PM
Unknown Object (File)
Mon, May 6, 1:31 PM
Unknown Object (File)
Mon, May 6, 1:30 PM
Unknown Object (File)
Mon, May 6, 1:30 PM
Unknown Object (File)
Mon, May 6, 1:30 PM
Unknown Object (File)
Mon, May 6, 1:30 PM
Unknown Object (File)
Mon, May 6, 1:30 PM
Subscribers

Details

Summary

Yet another dependency... this one at least is just replacing neon, which ended up not working out very well.
napi-rs does the same thing as neon (creates an addon for Node) but with much better ergonomics. It also supports tokio
by default, which lets us just use tonic for gRPC instead of the IMO harder to use Node gRPC client.

napi-rs also handles all of the typing for us so we don't get any flow issues.

Test Plan

imported the addon in keyserver.js and successfully called the sum function

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
keyserver/addons/opaque-ke-napi/index.js
1 ↗(On Diff #19083)

Updated to ESM + Flow, delete all the parts we don't need (we don't need Windows, we don't need Node 10, etc.)

keyserver/addons/opaque-ke-napi/package.json
3 ↗(On Diff #19083)

Can we do 0.0.1 for consistency

5 ↗(On Diff #19083)

Set "type": "module",, then I don't think you'll .mjs... .js will default to .mjs instead of defaulting to .cjs

ashoat requested changes to this revision.Dec 1 2022, 3:01 PM

Back to you

This revision now requires changes to proceed.Dec 1 2022, 3:01 PM

add .ts file to gitignore

keyserver/addons/opaque-ke-napi/.gitignore
16 ↗(On Diff #19083)

done

keyserver/addons/opaque-ke-napi/.npmignore
2 ↗(On Diff #19083)

done

keyserver/addons/opaque-ke-napi/.yarnrc.yml
1 ↗(On Diff #19083)

removed

keyserver/addons/opaque-ke-napi/Cargo.toml
4 ↗(On Diff #19083)

yeah there were like 7 places where the version was set so i wanted to keep them in sync but i cleaned up most of them

keyserver/addons/opaque-ke-napi/__test__/index.spec.mjs
1 ↗(On Diff #19083)

deleted this whole directory

keyserver/addons/opaque-ke-napi/index.d.ts
6 ↗(On Diff #19083)

gitignored this

keyserver/addons/opaque-ke-napi/index.js
1 ↗(On Diff #19083)

I tried cleaning it up but it was overwritten by the yarn scripts... will have to revisit this I think.

keyserver/addons/opaque-ke-napi/npm/win32-x64-msvc/README.md
3 ↗(On Diff #19083)

deleted this whole dir

Are any of the files that are being .gitignore'd necessary for the keyserver to run? If so, we need to add the script commands to keyserver/Dockerfile

I'd rather have us avoid those scripts entirely and use them as inspiration. Is that a possibility here?

Accepting to unblock

keyserver/addons/opaque-ke-napi/index.js
1 ↗(On Diff #19083)

I guess my overall point is that we don't need to use the yarn scripts, do we?

Anyways please create a task before landing

This revision is now accepted and ready to land.Dec 1 2022, 5:59 PM

Get Flow and ESM working, move .node artifacts to napi folder

Add clean and postinstall yarn scripts

We should probably also git rm keyserver/addons/opaque-ke-node at some point

All the builds are failing because we now need cargo in order to run yarn cleaninstall. We can either make sure we have Cargo in all of these environments, or only make sure we have Cargo for the keyserver tests and make the Cargo build "optional" for the other cases. If all of our CI / builds / deployments were in Nix this probably wouldn't be an issue...

ashoat requested changes to this revision.Dec 2 2022, 9:56 AM

Need to make sure we resolve the builds before landing, otherwise we'll break the build for everyone. Happy to accept if a subsequent diff fixes those

This revision now requires changes to proceed.Dec 2 2022, 9:56 AM

Add Rust to Docker image and change supported linux arch

varun planned changes to this revision.Dec 5 2022, 7:21 PM

fix buildkite issues. shouldn't have any issues with GH Actions since the runners already have Rust

make cargo available on CI

varun planned changes to this revision.Dec 5 2022, 8:21 PM
keyserver/Dockerfile
26–36 ↗(On Diff #19162)

Please undo this

keyserver/addons/opaque-ke-napi/index.js
17–19 ↗(On Diff #19162)

Hmm, I think the production server is Linux x64, so I think we need some option for that, no?

You're probably adding Linux arm64 since perhaps Docker on macOS runs on that arch if the host has an Apple processor?

keyserver/Dockerfile
26–36 ↗(On Diff #19162)

this is how the Docker extension for vscode formats multiline instructions, but fair, I agree that it looks worse

keyserver/addons/opaque-ke-napi/index.js
17–19 ↗(On Diff #19162)

Oh I see. Yeah I needed Linux arm64 to run the Docker container on my machine. I'll add x64 back. I'm not sure how to test the Linux x64 build besides launching an ec2 instance and setting everything up there, which seems a little tedious. Is it possible for us to do a dry run on your server?

add Linux x64 back, undo some Dockerfile formatting

update buildkite yaml files

.buildkite/eslint_flow_jest.yml
5–6 ↗(On Diff #19245)

We don't actually use this file on buildkite (maybe @atul knows why?) but I wanted to keep it in sync with the changes I made on buildkite.com

.buildkite/ios.yml
5–10 ↗(On Diff #19245)

We don't actually use this file on buildkite (maybe @atul knows why?) but I wanted to keep it in sync with the changes I made on buildkite.com

keyserver/Dockerfile
116–117 ↗(On Diff #19245)

we have to copy lib.rs in or else cargo will complain

keyserver/src/opaque-ke-napi
1 ↗(On Diff #19245)

this is a symlink

ashoat requested changes to this revision.Dec 8 2022, 10:21 AM

Close!

.buildkite/eslint_flow_jest.yml
5–6 ↗(On Diff #19162)

Just curious, what was wrong with the old steps? Just source vs. .?

5–6 ↗(On Diff #19245)

Confused what you're saying, are you saying .buildkite/eslint_flow_jest.yml isn't used in Buildkite (isn't that the "ESLint & Flow & Jest" CI job?) or are you saying /root/.cargo/env isn't used? Or something else?

.buildkite/ios.yml
5–10 ↗(On Diff #19245)

Isn't this the "iOS Build" CI job?

keyserver/Dockerfile
116–117 ↗(On Diff #19245)

That makes sense – the yarn cleaninstall command below runs the Cargo build, doesn't it? Should we be copying in the entire src folder instead of just the lib.rs file?

keyserver/addons/opaque-ke-napi/index.js
17–19 ↗(On Diff #19162)

To do a full test, I think it would just require checking out a second copy of the repo, configuring .env to point directly to the AWS node and to use a second port directly without Apache proxying, and exposing that port on AWS. Probably won't be able to get to that until mid-next week unfortunately – heli flies out tomorrow afternoon

If you just want me to run the Docker build that would be easier, might be able to do that sometime tonight. Let me know

This revision now requires changes to proceed.Dec 8 2022, 10:21 AM
.buildkite/eslint_flow_jest.yml
5–6 ↗(On Diff #19162)

the source command isn't available on the node docker image.

i was initially under the impression that this file was being used by buildkite, so was making changes here to get the CI builds to pass. when i realized we weren't actually using this file, i started making changes on buildkite directly. then i updated this file to match what we have there, in case we decide to use these files some day.

5–6 ↗(On Diff #19245)

.buildkite/eslint_flow_jest.yml isn't used in Buildkite. if we were using it, the file would appear here I believe: https://buildkite.com/comm/eslint-and-flow-and-jest/steps

compare with the backup build steps, which does use our .yml: https://buildkite.com/comm/backup-build-docker/steps

.buildkite/ios.yml
5–10 ↗(On Diff #19245)

This job illustrates what I'm trying to say: https://buildkite.com/comm/backup-build-docker/steps
For the "backup build (docker)" CI job, we actually use the .buildkite/backup_build.yml file in the steps. For the "ios build" CI job, we don't actually use .buildkite/ios.yml. It sounds like the plan is to eventually use .yml files for all the jobs, so I want to keep things in sync until then

keyserver/Dockerfile
116–117 ↗(On Diff #19245)

ah i confused myself. we should create an empty rust library so that the yarn cleaninstall will pull in all the dependencies. then on line 142 we copy over all the source files including the actual lib.rs. after that, we build the actual library

keyserver/addons/opaque-ke-napi/index.js
17–19 ↗(On Diff #19162)

if you have time to do the docker build tonight that would be great

address Dockerfile build caching issue

keyserver/Dockerfile
116–117 ↗(On Diff #19245)

the purpose of this step is build caching, so I don't want to copy in any actual source files here

Before landing, can you respond to the inline comment and link a task for moving Buildkite to use the files from the GitHub repo? I think one already exists - maybe check with Atul before creating a new one

I’ll test a build from master once it’s landed and let you know how it goes, and from there we can either just let it run or wait until I can do more sophisticated testing mid next week

keyserver/Dockerfile
122 ↗(On Diff #19262)

This should trigger postinstall hook which should run build. This is probably gonna fail so I’m confused how this even proceeds

Can you copy build output into a Gist and link here?

This revision is now accepted and ready to land.Dec 8 2022, 12:14 PM

Before landing, can you respond to the inline comment and link a task for moving Buildkite to use the files from the GitHub repo? I think one already exists - maybe check with Atul before creating a new one

I’ll test a build from master once it’s landed and let you know how it goes, and from there we can either just let it run or wait until I can do more sophisticated testing mid next week

reopened and commented on the relevant linear issue https://linear.app/comm/issue/ENG-1079/check-in-buildkite-steps-to-our-github-repo

keyserver/Dockerfile
122 ↗(On Diff #19262)
This revision was automatically updated to reflect the committed changes.