Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a filter for applying Android socket tags #2423
Add a filter for applying Android socket tags #2423
Changes from all commits
31f5ed1
2fe087f
9c571da
6c337f5
dd1990e
3fe5055
30669c0
838b687
3f1b8fa
3b70695
285bb29
e8ce39b
702781a
bf91e03
4f69f6f
8308110
97302cd
6221682
18030a7
f409a57
ec5ae8f
daf58c6
fe92d93
fd83daa
c539773
07af4a8
6b44c79
4539347
99a1d03
4eebefe
ec65417
a1febff
dbff49a
e38a859
4f40e1d
633704b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StopIteration or sendLocalReply with error? I'd think we wanted the latter? do we have an e2e test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. sendLocalReply sounds like a good idea. I think I've done the needful.
There are no end-to-end tests because the socket tagging code, like isCleartextPermitted, needs to run in a simulator and I think we're waiting on Lyft to sort that out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh but this is a failure case based on getting bad headers from the API - you should totally be able to test sending an invalid request with the filter configured, and getting the local response you expect, so let's do that!
you could also optionally add a test for the happy path, which would create the custom transport socket and make sure hashing works, if we don't have that tested upstream. even if the tagging is a no-op it would at least cover that the memory model works.
also clean up the commented out return value above this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. Java tests added and removed the commented out code.
I looked at test/common/integration/client_integration_test.cc, which we discussed offline. I wasn't quite sure how to configure it to add the socket tagging support. Maybe we can discuss that offine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the issue! I'll give the demo app a shot. Updated test/kotlin/apps/experimental/MainActivity.kt per that previous commit. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I am happy to help with the setup of the demo app if you run into any issues with it. It's also possible that the documentation is lacking or making some assumptions - let me know and I am happy to help.
Re experimental app - I think that you can go even one step further (sorry for not pointing this out in my previous comment) and adding a
socket-tag
header to requests performed by the experimental app. This way you would get an end-to-end confirmation that requests with socket-tag go through (we verify whether our experimental example is getting 200 from the server). You could add the header somewhere in here