Page MenuHomePhabricator

[shared] add new interceptor to identity gRPC clients
ClosedPublic

Authored by varun on Oct 10 2023, 9:20 PM.
Tags
None
Referenced Files
F3152559: D9448.diff
Tue, Nov 5, 4:31 AM
Unknown Object (File)
Fri, Nov 1, 11:23 AM
Unknown Object (File)
Fri, Nov 1, 11:21 AM
Unknown Object (File)
Fri, Nov 1, 11:21 AM
Unknown Object (File)
Fri, Nov 1, 11:21 AM
Unknown Object (File)
Fri, Nov 1, 11:20 AM
Unknown Object (File)
Fri, Nov 1, 11:20 AM
Unknown Object (File)
Fri, Nov 1, 11:19 AM

Details

Summary

adding a new interceptor to both gRPC clients that inserts code version and device type into the request metadata. we'll eventually need this to tell clients to upgrade to a version that supports device lists

Test Plan

inspected the request metadata on the identity service side and confirmed that code version and device type appeared

Depends on D9441

Diff Detail

Repository
rCOMM Comm
Branch
alchemy (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun requested review of this revision.Oct 10 2023, 9:37 PM
bartek added inline comments.
keyserver/addons/rust-node-addon/src/identity_client/mod.rs
27–28 ↗(On Diff #31885)

How about creating a build.rs script that reads the facrts/version.js and generates this constant? Something like this example.

shared/grpc_clients/src/identity/authenticated.rs
49–52 ↗(On Diff #31885)

Is it worth extracting this as a type alias?

shared/grpc_clients/src/identity/shared.rs
39–57 ↗(On Diff #31885)

I'm surprised that this isn't built into the tonic client library, unless it is but I couldn't find it.

This revision is now accepted and ready to land.Oct 11 2023, 2:42 AM
keyserver/addons/rust-node-addon/src/identity_client/mod.rs
27–28 ↗(On Diff #31885)

thanks for the suggestion! i've done this now

shared/grpc_clients/src/identity/authenticated.rs
49–52 ↗(On Diff #31885)

yes i think so

shared/grpc_clients/src/identity/shared.rs
39–57 ↗(On Diff #31885)

i think tower middleware is the new idiomatic way to do this stuff but i didn't want to completely change things

varun requested review of this revision.Oct 11 2023, 9:13 PM

requesting review to make sure feedback was addressed correctly

bartek requested changes to this revision.Oct 11 2023, 11:12 PM

Requesting because of that misleading comment leftover.

keyserver/addons/rust-node-addon/build.rs
9–12 ↗(On Diff #31946)

Not sure if this is still worth but some people recommend adding

const VERSIONS_JS_PATH: &str = "../../../lib/facts/version.js";
println!("cargo:rerun-if-changed={}", VERSIONS_JS_PATH);
33 ↗(On Diff #31946)

In contrast to other unwraps, this one might actually fail

keyserver/addons/rust-node-addon/src/identity_client/mod.rs
26–28 ↗(On Diff #31946)

A little more verbose and IMO cleaner way:

mod generated {
  // We get the CODE_VERSION from this generated file
  include!(concat!(env!("OUT_DIR"), "/version.rs"));
}

pub use generated::CODE_VERSION;
pub const DEVICE_TYPE: &str = "keyserver";

But not feeling very strongly.

lib/facts/version.js
5–7 ↗(On Diff #31946)

This comment is no longer relevant

This revision now requires changes to proceed.Oct 11 2023, 11:12 PM

remove misleading comment and address latest feedback

This revision is now accepted and ready to land.Oct 12 2023, 9:16 AM