Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FCM v1: use async version of google-auth and add HTTP proxy support #372

Merged
merged 9 commits into from
May 18, 2024

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented May 15, 2024

Fix #371.

@MatMaul MatMaul requested a review from a team as a code owner May 15, 2024 09:06
@MatMaul
Copy link
Contributor Author

MatMaul commented May 15, 2024

Another kick is required to trigger the workflows, this is annoying. @devonh if you can trigger them again please 🙏 .

Copy link
Contributor

@devonh devonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this out locally and it does appear to fix the proxy issue.
I'm just double checking with the team whether we are okay with using the experimental aiohttp support from the google-auth library.

Otherwise there are just a few small changes requested.

sygnal/gcmpushkin.py Show resolved Hide resolved
sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/gcmpushkin.py Show resolved Hide resolved
@MatMaul
Copy link
Contributor Author

MatMaul commented May 16, 2024

Thanks for the fast review.

I've address the comments and also differentiate the error cases for service_account_file between not specified and invalid file, so we can surface the underlying load error.

@MatMaul MatMaul requested a review from devonh May 16, 2024 07:43
@MatMaul
Copy link
Contributor Author

MatMaul commented May 16, 2024

Also perhaps an useful info, this is deployed in our preprod (with an HTTP proxy) with no trouble.

@devonh
Copy link
Contributor

devonh commented May 17, 2024

This is looking good.

Would you be able to add a couple of unit tests?
Particularly around loading the service account file and _get_auth_header(). (patching out the call to credentials.refresh)

Once those are in place I am comfortable with merging this.

@MatMaul
Copy link
Contributor Author

MatMaul commented May 17, 2024

I've added a fake service account file to test loading, and create a FakeCredentials to exercise _get_auth_header.

Unfortunately refresh is never called and is blocking, for some async magic I don't really understand between Twisted and asyncio I believe.
I am a bit stuck so I pushed the changes in case someone has an idea.

@devonh
Copy link
Contributor

devonh commented May 17, 2024

I've added a fake service account file to test loading, and create a FakeCredentials to exercise _get_auth_header.

Unfortunately refresh is never called and is blocking, for some async magic I don't really understand between Twisted and asyncio I believe. I am a bit stuck so I pushed the changes in case someone has an idea.

Thank you for adding the tests.
I'll take a look at the blocking issue today to see if I can figure anything out with that.

sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
tests/test_gcm.py Outdated Show resolved Hide resolved
tests/test_gcm.py Show resolved Hide resolved
@MatMaul MatMaul requested a review from devonh May 18, 2024 10:00
@devonh devonh merged commit bb62b17 into matrix-org:main May 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FCM API Credential refresh mechanism doesn't support HTTP proxies and blocks the event loop
2 participants