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 28 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.
maybe let's prefix the header name with
x-
so that it's clearer that it's for Envoy's internal use only?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.
not sure about this anymore as the header is set by the user of the library so maybe not having a prefix here is just OK.
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.
I'd be inclined to avoid the "x-" prefix because of RFC 6648, "Deprecating the "X-" Prefix and Similar Constructs in Application Protocols". But happy to use it if that's the convention.
https://datatracker.ietf.org/doc/html/rfc6648
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 for a detailed response - the link to RFC is helpful. Looking at our codebase I can see that we use
x-envoy-mobile
prefix specifically and headers that follow this convention have special guards in place - i.e., the application cannot just set them using normal "addHeader" method as they are considered to be "restricted":envoy-mobile/library/kotlin/io/envoyproxy/envoymobile/HeadersBuilder.kt
Lines 79 to 81 in 308f15c
Instead, the app needs to use strongly typed methods on
RequestHeaderBuilder
to add such headers:envoy-mobile/library/kotlin/io/envoyproxy/envoymobile/RequestHeadersBuilder.kt
Lines 88 to 95 in 308f15c
I think that I should've commented with this earlier but if we want to follow an established pattern here we should add
addSocketTag
method toRequestHeadersBuilder
and make this method addx-envoy-mobile-socket-tag
header to the outgoing request (following the logic from theaddUpstreamHttpProtocol
method referenced above.I do not have a strong opinion about the existing pattern related to
x-envoy-mobile
prefix in headers but I think that there is some value in following it. On top of this, the existence ofaddSocketTag
method inRequestHeadersBuilder
class would help with the discoverability of the feature and act as a natural place for its documentation. I think that some concerns related to discoverability and lack of a place to document the feature were raised in #2423 (comment) and the proposed change could address both of them (the addition ofaddSocketTag
method specifically).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.
also - can we avoid re-creating this header instance (
Http::LowerCaseString("socket-tag")
) on each request?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 great explanation of the
x-envoy-mobile
prefix. This seems very compelling. I've switched over to it, and added anaddSocketTag()
method toRequestHeadersBuilder
.Also have made the header instance static.
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.
can we reorganize the code to avoid having to call
equest_headers.get(socket_tag_header)
twice?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 point. Done.
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 hereThere 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.
The one thing that you need to do here is to start using the
find_class
that was introduced in #2483There 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.
Done!