feat: switch to latest release a2 library #362

jrconlin · 2023-04-04T17:22:00Z

This switches from our semi-vendored a2, back to the official release.
This should also clear the warning about the included nom library.

Breaking Change
This removes the unused .aps field from the router_data options. This
field was only used during testing. That test was also removed.

* This switches from our semi-vendored a2, back to the official release. This should also clear the warning about the included `nom` library. *Breaking Change* This removes the unused `.aps` field from the router_data options. This field was only used during testing. That test was also removed.

* note the reason we can't update tungstenite properly

autoendpoint/src/routers/apns/router.rs

… chore/main-a2

pjenvey · 2023-04-18T22:37:08Z

We've identified some missing parts of Mark's original a2 branch that didn't get merged to a2 master likely due to merge conflicts. E.g. in this PR the Client::certificate_parts in here wasn't added. So we should double check that all of Mark's original branch made it into a2 before moving off of it.

hackebrot · 2023-05-02T07:29:49Z

Hi @jrconlin! 👋🏻

I see you've added test eng as reviewers on this pull request, but neither @Trinaa or myself have any context about this project. Did you have any specific questions that we can help with?

jrconlin · 2023-05-02T15:07:47Z

Hi @jrconlin! 👋🏻

I see you've added test eng as reviewers on this pull request, but neither @Trinaa or myself have any context about this project. Did you have any specific questions that we can help with?

Sorry, that was probably a mis-click on my part. I've removed you from review.

hackebrot · 2023-05-02T15:52:11Z

No worries, @jrconlin! Thanks. 👍🏻

… chore/main-a2

jrconlin · 2023-05-23T23:07:18Z

I compared the current version of mozilla-services/a2 against Mark's PR and looks like the only thing missing was the Client::certificate_parts method. I added it to this PR.

Once that PR lands, I'll redo this PR to include the new 0.8.0 version, and then submit a PR to wallet/a2 to include the change as well. That should let us be able to release the new server with the new library.

… chore/main-a2

jrconlin · 2023-05-25T22:12:08Z

Switched to using our mozilla-services/a2 git repo while we wait for WalletConnect/a2#72 to land.

Part of the delay was trying to debug the way that code wants to read in key/cert from the environment. I couldn't get the escape quite right. I would frequently get a Private Key error in the cert. Passing

"AUTOEND__APNS__CHANNELS": "{\"dev\":{\"cert\": \"keys/test.crt\", \"key\":\"keys/test.key\"}}",

worked, even with the same values, so guessing all the coding/recording might have changed a value.

pjenvey · 2023-05-25T23:49:22Z

Part of the delay was trying to debug the way that code wants to read in key/cert from the environment. I couldn't get the escape quite right. I would frequently get a Private Key error in the cert. Passing
"AUTOEND__APNS__CHANNELS": "{\"dev\":{\"cert\": \"keys/test.crt\", \"key\":\"keys/test.key\"}}",

Maybe that's more of a shell problem, assuming you're running with AUTOEND__APNS__CHANNELS="{\"dev\":{\"cert\": \"keys/test.crt\", \"key\":\"keys/test.key\"}}"? With the outer double quotes conflicting with the inner ones

I just tried running it surrounded in single quotes instead (preserving everything inside) and it worked as I'd expect (on master):

AUTOEND__APNS__CHANNELS='{"dev":{"cert": "keys/test.crt", "key":"keys/test.key"}}' AUTOEND__HUMAN_LOGS=true RUST_BACKTRACE=1 cargo run
...
May 25 23:48:08.863 DEBG Starting up...
settings.apns: ApnsSettings {
    channels: "{\"dev\":{\"cert\": \"keys/test.crt\", \"key\":\"keys/test.key\"}}",
    max_data: 4096,
}
May 25 23:48:08.864 DEBG initialized disabled sentry client due to disabled or invalid DSN
settings.apns.channels(): Ok(
    {
        "dev": ApnsChannel {
            cert: "keys/test.crt",
            key: "keys/test.key",
            topic: None,
            sandbox: false,
        },
    },
)

jrconlin · 2023-05-26T01:39:27Z

Oh, it's absolutely a vscode problem, and I was being stubborn.

< rant >
In VS Code, you specify your run args as a JSON structure. This includes a set of environment variables specified as key values, that you have to properly escape so that the JSON to env and then the env to JSON parser can handle, so you wind up with stuff like:

        {
            "type": "lldb",
            … ,
            "cwd": "${workspaceFolder}",
            "env": {
                "RUST_LOG": "autopush=DEBUG,autoconnect=DEBUG,autopush_common=DEBUG,autoendpoint=DEBUG,warn",
                "AUTOEND__PORT": "9160",
                //"AUTOEND__BIND_HOST": "0.0.0.0",
                "AUTOEND__HUMAN_LOGS": "1",
                "AUTOEND__CRYPTO_KEYS": "[…]",
                "AUTOEND__APNS__CHANNELS": "{\"dev\":{\"cert\":\"-----BEGIN CERTIFICATE-----\\n…=\\n-----END CERTIFICATE-----\", \"key\": \"-----BEGIN PRIVATE KEY-----\\n…----END PRIVATE KEY-----\"}}",
            }
        },

So yeah, LOTS of opportunities to screw things up, because control characters in JSON strings are big no-nos.
</ rant >

pjenvey

Couple things:

I forgot what we concluded when we last discussed this, but is there a way to add a test that ensures we fixed the panic? Can we just shoot a message to Endpoint::Sandbox?
probably want to rename the branch to feat/ at this point

pjenvey · 2023-05-26T19:13:27Z

autoendpoint/Cargo.toml

-# The version of a2 at the time of the fork is v0.5.3.
-a2 = { git = "https://github.com/mozilla-services/a2.git", branch = "autoendpoint" }
-# several of these libraries are pinned due to https://github.com/mozilla-services/autopush-rs/issues/249
+a2 = {version = "0.8", git = "https://github.com/mozilla-services/a2"}


We should probably pin this to a specific revision (even though we own this repo) -- actually, upstream quickly merged your pr 🎉 so let's pin to their repo instead.

?? WalletConnect/a2#72 is still pending review.

I'll pin to a specific commit, but will still keep the version, since I ought to do a release.

My fault I must have been looking at the wrong repo

pjenvey · 2023-05-26T19:14:26Z

autoendpoint/src/routers/apns/router.rs

@@ -251,15 +302,13 @@ impl Router for ApnsRouter {
            .get("rel_channel")
            .and_then(Value::as_str)
            .ok_or(ApnsError::NoReleaseChannel)?;
+        // XXX: We don't really use anything that is a numeric here, aside from


Not really an XXX: (broken/fixmeish) comment IMO

Suggested change

// XXX: We don't really use anything that is a numeric here, aside from

// NOTE: We don't really use anything that is a numeric here, aside from

Actually... It is a FIXME, specifically the third line:

// Once we're off of DynamoDB, we might want to kill the map.

That's a note to future us that we probably want to recheck if we need to do this, right?

Ah, could do FIXME but XXX works

pjenvey · 2023-05-26T19:22:15Z

autoendpoint/src/routers/apns/router.rs

-                .collect(),
-            device_token: token,
-            options: NotificationOptions {
+        let mut aps = Self::default_aps();


Sorry, I probably missed some this the last time I reviewed it (is James back from PTO yet, can we just kill all this? 😄 ) -- looking at it now, I think we can at least move this large chunk into its own method, I've taken a shot at that on this branch.

I mean, sure? It's still a big ugly pile of code regardless of if we want to screw with the stack or not. Tossing it into a different function doesn't really help, and IDEs allow for block collapse if we just want to ignore it for now.

It's just too many lines for one method, this block is > 100 by itself (clippy's pedantic too_many_lines lint is that). I assumed it was in here due to lifetime trickyness, hence my branch to solve that w/ the new method. I wouldn't justify e.g. an even larger say 500 line method either with IDE collapsing help -- so I'll argue this does help readability.

Yeah, fair. I broke it out into it's own function like you suggested. (changed the name and gave it a better comment) I was debating how much of that I could reduce using macros, but I figured that was more work for what was (hopefully) a temporary thing.

pjenvey · 2023-05-26T22:20:46Z

autoendpoint/Cargo.toml

@@ -45,7 +45,7 @@ tokio.workspace = true
 url.workspace = true
 uuid.workspace = true

-a2 = {version = "0.8", git = "https://github.com/mozilla-services/a2"}
+a2 = {version = "0.8", git = "https://github.com/mozilla-services/a2", branch="master"}


I was thinking more of a specific sha (thinking of reproducable builds) but then again we weren't doing this before by pointing to the autoendpoint branch, so never mind I guess.

… chore/main-a2

jrconlin added 2 commits April 4, 2023 09:03

f cleanup comments

5c1a881

jrconlin requested review from pjenvey and a team April 4, 2023 17:22

f Cargo cleanup

fa85fa2

* note the reason we can't update tungstenite properly

pjenvey reviewed Apr 5, 2023

View reviewed changes

autoendpoint/src/routers/apns/router.rs Show resolved Hide resolved

jrconlin added 2 commits April 6, 2023 14:37

f add back the custom aps override feature.

8cba21f

Merge branch 'master' of github.com:mozilla-services/autopush-rs into…

aaabfdf

… chore/main-a2

jrconlin removed the request for review from a team May 2, 2023 15:07

Merge branch 'master' of github.com:mozilla-services/autopush-rs into…

a76685b

… chore/main-a2

Merge branch 'master' of github.com:mozilla-services/autopush-rs into…

6ed3baf

… chore/main-a2

jrconlin requested a review from pjenvey May 26, 2023 17:29

pjenvey requested changes May 26, 2023

View reviewed changes

f r's

5027227

jrconlin requested a review from pjenvey May 26, 2023 21:48

pjenvey previously approved these changes May 26, 2023

View reviewed changes

Merge branch 'master' of github.com:mozilla-services/autopush-rs into…

2d01d06

… chore/main-a2

jrconlin dismissed pjenvey’s stale review via 2d01d06 May 26, 2023 22:34

pjenvey approved these changes May 26, 2023

View reviewed changes

jrconlin merged commit 728fe16 into master May 26, 2023
1 check passed

jrconlin deleted the chore/main-a2 branch May 26, 2023 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: switch to latest release a2 library #362

feat: switch to latest release a2 library #362

jrconlin commented Apr 4, 2023

pjenvey commented Apr 18, 2023

hackebrot commented May 2, 2023

jrconlin commented May 2, 2023

hackebrot commented May 2, 2023

jrconlin commented May 23, 2023

jrconlin commented May 25, 2023

pjenvey commented May 25, 2023 •

edited

jrconlin commented May 26, 2023 •

edited

pjenvey left a comment •

edited

pjenvey May 26, 2023

jrconlin May 26, 2023

pjenvey May 26, 2023

pjenvey May 26, 2023

jrconlin May 26, 2023

pjenvey May 26, 2023

pjenvey May 26, 2023

jrconlin May 26, 2023

pjenvey May 26, 2023

jrconlin May 26, 2023

pjenvey May 26, 2023

	// XXX: We don't really use anything that is a numeric here, aside from
	// NOTE: We don't really use anything that is a numeric here, aside from

feat: switch to latest release a2 library #362

feat: switch to latest release a2 library #362

Conversation

jrconlin commented Apr 4, 2023

pjenvey commented Apr 18, 2023

hackebrot commented May 2, 2023

jrconlin commented May 2, 2023

hackebrot commented May 2, 2023

jrconlin commented May 23, 2023

jrconlin commented May 25, 2023

pjenvey commented May 25, 2023 • edited

jrconlin commented May 26, 2023 • edited

pjenvey left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey commented May 25, 2023 •

edited

jrconlin commented May 26, 2023 •

edited

pjenvey left a comment •

edited