pigeon icon indicating copy to clipboard operation
pigeon copied to clipboard

feat: support for Goth modules for FCM

Open petermueller opened this issue 2 years ago • 14 comments

closes #232

Updates the Pigeon.FCM and Pigeon.FCM.Config implementations to delegate the token refresh logic to Goth, as Goth v1.3+ now handles that directly

petermueller avatar Feb 15 '23 16:02 petermueller

We've confirmed that this works with the metadata approach, and have no reason to believe it wouldn't work otherwise. I can test with a service account JSON using Goth if you'd like.

I can also complete any updates to this PR that you'd like. Just let me know what you'd prefer, @hpopp


Original Description:

TODOs

  • [x] module/function docs
  • [x] README updates
    • Maybe a "how to test without needing a real Goth token" section?
      • I have some useful examples but they don't work with the existing tests since they don't use the Pigeon.Sandbox adapter.

Open Questions to discuss

  • Should service_account_json still be supported?
    • I think no, but I'm fine with whatever
    • Still supporting it would require starting and supervising a Goth instance under Pigeon, making config more complicated in my opinion
    • We probably don't want to continue using the existing schedule_refresh approach as it can exhaust the number of tokens you can get per hour depending on how many Dispatcher instances of the same module you have (i.e. multiple YourApp.FCM instances, but with different names)
    • it's a "small" change for people to adopt the Goth v1.3.x approach, and better documented by Goth itself
  • Should the config key be :goth or something else?
    • maybe :token_client, that has a default value, similar to Goth?
      • token_client: YourApp.Goth -> {:goth, [YourApp.Goth]} -> Goth.fetch!(YourApp.Goth)
      • token_client: {:goth, [YourApp.Goth, 10_000] -> Goth.fetch!(YourApp.Goth, 10_000)
      • token_client: &YourApp.custom_token_fetch/0 -> apply(token_client_fun, [])
      • token_client: {&GothMock.fetch!/1, [YourApp.Goth]} -> apply(token_client_fun, [YourApp.Goth])
  • maybe a MIGRATION.md for the GCP changes?
  • Should the FCM tests be split into unit and integration tests?
    • Goth.Token.fetch documents how to pass a http_client, which makes testing this not too bad
    • testing locally is a bit onerous, and generally once it gets to the Sandbox, it's likely going to work
    • could skip the integration tests by default, and include during CI?
    • using a setup that does start_supervised! with a custom name per test instead of in test_helper? would make testing locally easier
      • This might be benefited by updating the macro for def push in the Dispatcher __using__ to use the name given to the use
      • Might also benefit to make push and child_spec defoverridable

petermueller avatar Feb 15 '23 18:02 petermueller

This is excellent work 🍻. Considering how long the release candidates have been out, it's not wise to drop :service_account_json, but that also means we can keep the :goth key as-is for custom overrides (with an additional section in the docs, not the initial getting started). One of Pigeon's main goals is having the absolute easiest Getting Started, and in the most common case, users shouldn't even need to be aware it's using Goth. This setup is still simpler than the APNS side, which supports both certs and JWT auth.

For config validation logic, if :goth is present, let that take priority and ignore :service_account_json.

Also, I agree with delegating all refreshing to Goth itself 👍

As far as testing, the integration tests have always been a pain point in PRs. Traditionally I've just pulled locally and run them for a sanity check before merge. I do finally need to split them out from unit tests, but that's a bit too much for this PR.

hpopp avatar Feb 18 '23 19:02 hpopp

Ok so I've had some time to think it over and the best I can think of is having Pigeon fall back to supervising a Goth.PigeonFallback or something like that for the service account json. Only negative is that the only logical place to do that would seem to be in the startup calls for Pigeon.Dispatcher, utilizing maybe a new children callback, or something like that. I don't believe we could do it in the FCM module's init since that's called for each Dispatch Worker. It could be done but would be really finicky and risk causing larger issues if the Worker were restarted as that would kill the linked Goth too.

What do you think of some callback or another function on Configurable or something that can affect the supervision tree in the Dispatcher module? I was thinking Configurable.children that is just an empty list for the other implementations. I can't think of another way to keep the json while getting to delegate the token refresh to Goth. Alternative would be to add a warning if FCM receives the json, and suggest a copy/paste-able chunk of code, but that's not particularly plug and play

petermueller avatar Feb 27 '23 02:02 petermueller

I'm also looking to use Pigeon with Goth metadata instead of a service account and this looks to be exactly what's needed! Is there a chance this PR will be merged anytime soon or are there more changes needed?

dylanseago avatar Feb 22 '24 00:02 dylanseago

@hpopp any updates on this? would like to use this as well

mikeover avatar Feb 22 '24 20:02 mikeover

Hey, I apologize I forgot about this PR. Let me know if you want me to take a stab at anything. I recently took on co-maintainership of waffle_gcs but I've been generally carving out more time for open source contributions on the weekends

petermueller avatar Feb 23 '24 03:02 petermueller

Finally getting a chance to catch back up to speed, I've been traveling a lot lately. I think I'm coming around on dropping the service_account_json key. Considering that legacy FCM is getting shut off in June of this year, longtime users of this package are going to have to revise their setup and upgrade anyway (Pigeon 3.0 anyone?). Let's get some setup docs behind this and consider it the last major change for the 2.0 release.

hpopp avatar Mar 16 '24 16:03 hpopp

I'll take a look over the week at writing up some docs and seeing if there's anything I missed from discussions

petermueller avatar Mar 17 '24 22:03 petermueller

@hpopp, let me know what you think. I can move stuff around, or if there's preferred word-smithing, I take no offense 😅

Final question I had though was whether we should provide a deprecation warning (and whether hard at compile, or just at runtime) if they provide :service_account_json. I left in the redact so whatever we decide I don't think it will leak their secrets to logs.

petermueller avatar Apr 01 '24 04:04 petermueller

@hpopp, is there anything else you'd like to see? :)

petermueller avatar Apr 09 '24 19:04 petermueller

I just recently started looking at Pigeon V2 in order to get FCM V1 in use. (Project owners missed the warnings about deadlines. :-( ) I have an app that accepts push notifications from two different end user apps to push to Apple and Google for two different mobile apps. Since the mobile apps are different projects I need to be able to use two different configurations. I've just started working on getting them to have two different service account files when I found docs that suggested that Goth can have multiple children, so now I'm wondering if this PR will make it possible for me to have my app, which includes Pigeon which includes Goth, provide the "source" configurations to Goth easily from within my app? The diffs look promising.

I already have my own fork of Pigeon to solve a few other problems. Is there a practical way to get my diffs merged with these ones, or do I desperately need Henry to merge this one?

(One of my problems is that I have an installed base of servers that we can't possibly get upgraded in time, so I'm trying to convert Legacy Notifications to V1 within Pigeon, which I therefore only need executing on my one central app.)

Sinc63 avatar Jul 03 '24 19:07 Sinc63

@petermueller You've been working with the Goth, so I hope you will be able to give me a tip or two. I like what you've done so far and I think it will ease my efforts. Since I have the two projects (that I noted above) that I'm sending pushes to, I need to have two different YourAPP.Goth children, and I'm expecting that I will still need them to be using service_account_json configurations.

  1. Is this the right way to get my AppA requests to use YourApp.GothA and AppB to use YourApp.GothB, all within the new Pigeon Adapter framework? It looks like it would be:
config :your_app, YourApp.FCM.Default,  
  adapter: Pigeon.FCM,  
  token_fetcher: YourApp.GothA
config :your_app, YourApp.FCM.Other,  
  adapter: Pigeon.FCM,  
  token_fetcher: YourApp.GothB
  1. Where am I going to configure those local files? Is that in my own config file, using a :pigeon config, or will it use a :goth config? Is that something like:
config :your_app, YourApp.GothA, [source: {:service_account, credentials1}]  
config :your_app, YourApp.GothB, [source: {:service_account, credentials2}]

I'm still trying to figure out how to set the credentials correctly from two service files I downloaded, but that's a separate problem. (Although tips are ALWAYS welcome!)

Sinc63 avatar Jul 03 '24 20:07 Sinc63

@Sinc63 I believe (1) should be correct. You would just have multiple Pigeon.Dispatchers in your application.ex as well.

(2) the config looks correct assuming you're doing it the way I added in the docs of this PR. Goth is agnostic about how you get your config, it just provides a child_spec. But if you follow the example from the docs I added in this PR you should be good with what you're doing in (2) to support multiple projects. Same as (1), you'd have multiple Goth children, 1 for each project.

With this PR Pigeon would not use the service account json. It's divorced from any token management.

petermueller avatar Jul 04 '24 02:07 petermueller

@Sinc63 long term, if you can get your hosts to use the Google metadata server, you can set the default service account for the VM to be a service account that has been granted the appropriate Firebase roles in both projects' IAM pages.

Then you would be able to eliminate the need for the service account json for Goth, also simplifying your build and compile stages.

I do this setup for some of our assets buckets in GCS in different ephemeral projects. It also allows us to use artifact registry cross-project for pulling images and deploying.

petermueller avatar Jul 04 '24 03:07 petermueller