Page MenuHomePhabricator

[services] Tunnelbroker - Update A2 (APNs) library wrapper with return of token-related errors
ClosedPublic

Authored by max on Sep 15 2022, 7:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 6:06 AM
Unknown Object (File)
Sun, Dec 15, 10:17 AM
Unknown Object (File)
Sun, Dec 15, 10:17 AM
Unknown Object (File)
Sun, Dec 15, 10:17 AM
Unknown Object (File)
Sun, Dec 15, 10:17 AM
Unknown Object (File)
Sun, Dec 15, 10:16 AM
Unknown Object (File)
Sun, Dec 15, 10:11 AM
Unknown Object (File)
Wed, Nov 27, 12:08 PM

Details

Summary

This diff introduces changes to the A2 (APNs) library wrapper and apns_status shared enum.

To distinguish common errors and wrong/expired device token errors the function should return different results.
Following a current keyserver's approach we should check for the following errors:

To pass the different errors to the C++ level the best and most elegant way is to return enum as Ok Rust result which formats like:

enum apns_status {
    Ok,
    Unregistered,
    BadDeviceToken,
}

and use CXX shared types.

Because when can't distinguish error types from Rust on the C++ side, they are always rust::error type and have only what() method.
Also, we will catch the common errors which are not needed to be distinguished using the catch approach.

Related Linear task: ENG-1764

Test Plan

App building:

  1. Rust:

Run cargo build from services/tunnelbroker/rust-notifications directory.
Rust library will be successfully built.

  1. C++ (Docker):

Running run run-tunnelbroker-service successfully built the Rust library and link it.

  1. C++ (Nix):

Call cd services/tunnelbroker.
Running rm -dfr build && cmake -B build . && make -C build -j4 successfully built the Rust library and link it.

Notifications testings:
Library notifications push method can be called from the C++ as:

#include "rust_notifications/src/lib.rs.h"
#include "rust/cxx.h"
...
const apnsReturnStatus result = sendNotifToAPNS(
 char const * cert_path,
 char const * cert_pass,
 char const * device_token,
 char const * message_body,
 bool true);

Behavior testings:

  • Test when the error is not ResponseError type:
    • Wrong certificate path or password will throw a Rust Result exception which is caught by the catch. (Produces read error message).
  • Test when wrong device token is provided:
    • Function returns an apnsReturnStatus enum with the value of BadDeviceToken.
  • Test with good device token:
    • Function returns an apnsReturnStatus enum with the value of Ok.

Diff Detail

Repository
rCOMM Comm
Branch
add-token-errs-apns
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

max held this revision as a draft.
max published this revision for review.Sep 15 2022, 7:29 AM
max edited the summary of this revision. (Show Details)
max edited the test plan for this revision. (Show Details)
max added reviewers: varun, tomek.

for future reference the tunnelbroker unit test gate will build the rust + c++ codebase, and run the c++ unit tests https://buildkite.com/comm/tunnelbroker-unittests/builds/11#01834184-25d5-4df8-b559-b259a3eabb6b

I feel like the test plan that you have would best be served as test cases.

services/tunnelbroker/rust-notifications/src/apns.rs
35–36 ↗(On Diff #16699)

Shouldn't these be Err?

I would assume that these two cases would still cause send_by_a2_client to fail in it's intent.

Or is this to prevent cxx converting the Err into a throw? If it is, do you mind adding a comment as this is surprising behavior.

tomek requested changes to this revision.Sep 19 2022, 5:23 AM
tomek added inline comments.
services/tunnelbroker/rust-notifications/src/apns.rs
35–36 ↗(On Diff #16699)

I'm not really sure if it is a good idea to wrap with Ok - this might be really confusing. We can catch on C++ side and examine exception message - this isn't ideal, but maybe a better idea?

This revision now requires changes to proceed.Sep 19 2022, 5:23 AM
max marked 2 inline comments as done.
max added inline comments.
services/tunnelbroker/rust-notifications/src/apns.rs
35–36 ↗(On Diff #16699)

I'm not really sure if it is a good idea to wrap with Ok - this might be really confusing. We can catch on C++ side and examine exception message - this isn't ideal, but maybe a better idea?

I think examining a string of error messages is not a good idea if we can strictly distinguish errors. It would be great if we can return an ENUM in a Err but we can't. The only way we can put some information in the Error is just a text in .what(). On the other side using a shared ENUM in OK we can strictly distinguish the returns. Please take a look at D5210 on how it looks. I think it's a more elegant way.

I do not insist on it and if you guys consider this solution confusing I'll change to the .what() text.

35–36 ↗(On Diff #16699)

Shouldn't these be Err?

I would assume that these two cases would still cause send_by_a2_client to fail in it's intent.

Or is this to prevent cxx converting the Err into a throw? If it is, do you mind adding a comment as this is surprising behavior.

The reason here to use an ENUM is to distinguish different error types on C++ side. We can't send a different type of error to the C++ side it's always rust::error type. The only information we can pass is the error text message in get it by calling .what(). From my perspective, this is a more elegant way to distinguish. You can check how it looks on the C++ side in D5210.

tomek added inline comments.
services/tunnelbroker/rust-notifications/src/apns.rs
35–36 ↗(On Diff #16699)

Ok, that makes sense! Could you add a code comment explaining it?

This revision is now accepted and ready to land.Sep 22 2022, 5:50 AM
max marked an inline comment as done.

Adding a comment to explain why we are returning Ok with error types.

max added inline comments.
services/tunnelbroker/rust-notifications/src/apns.rs
35–36 ↗(On Diff #16699)

Ok, that makes sense! Could you add a code comment explaining it?

Make sense to explain it. Done! Thanks @tomek !

max marked an inline comment as done.

Merging on the latest updates.