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.

Sign up for GitHub

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

Merged
merged 36 commits into from
Aug 23, 2022
Merged

Add a filter for applying Android socket tags #2423

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
31f5ed1
Implement Android Socket Tagging
RyanTheOptimist Jul 14, 2022
2fe087f
Working
RyanTheOptimist Jul 14, 2022
9c571da
format
RyanTheOptimist Jul 14, 2022
6c337f5
cleanup
RyanTheOptimist Jul 14, 2022
dd1990e
fix
RyanTheOptimist Jul 15, 2022
3fe5055
Add config to botstrap and trigger in test
RyanTheOptimist Jul 15, 2022
30669c0
Merge branch 'main' into SocketTag
RyanTheOptimist Jul 18, 2022
838b687
Add tagSocket from cronet
RyanTheOptimist Jul 19, 2022
3f1b8fa
format
RyanTheOptimist Jul 19, 2022
3b70695
address comments
RyanTheOptimist Jul 20, 2022
285bb29
Add config knobs
RyanTheOptimist Jul 22, 2022
e8ce39b
format
RyanTheOptimist Jul 22, 2022
702781a
Fix cronet
RyanTheOptimist Jul 22, 2022
bf91e03
Fix EnvoyConfigurationTest.kt
RyanTheOptimist Jul 22, 2022
4f69f6f
Address comments
RyanTheOptimist Jul 25, 2022
8308110
format
RyanTheOptimist Jul 25, 2022
97302cd
Address comments
RyanTheOptimist Jul 27, 2022
6221682
Format
RyanTheOptimist Jul 27, 2022
18030a7
Comment
RyanTheOptimist Jul 28, 2022
f409a57
Merge branch 'main' into SocketTag
RyanTheOptimist Aug 1, 2022
ec5ae8f
Update library/common/extensions/filters/http/socket_tag/filter.proto
RyanTheOptimist Aug 3, 2022
daf58c6
Update library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt
RyanTheOptimist Aug 3, 2022
fe92d93
Merge branch 'main' into SocketTagOld
RyanTheOptimist Aug 3, 2022
fd83daa
Simplify test
RyanTheOptimist Aug 3, 2022
c539773
Address comments
RyanTheOptimist Aug 3, 2022
07af4a8
Rename
RyanTheOptimist Aug 3, 2022
6b44c79
Format
RyanTheOptimist Aug 3, 2022
4539347
Spelling
RyanTheOptimist Aug 3, 2022
99a1d03
Address comments
RyanTheOptimist Aug 10, 2022
4eebefe
Address comments
RyanTheOptimist Aug 11, 2022
ec65417
Format
RyanTheOptimist Aug 11, 2022
a1febff
add "org/chromium/net/*"
RyanTheOptimist Aug 19, 2022
dbff49a
Merge branch 'main' into SocketTag
RyanTheOptimist Aug 19, 2022
e38a859
use find_class
RyanTheOptimist Aug 19, 2022
4f40e1d
Address more comments
RyanTheOptimist Aug 22, 2022
633704b
Format!
RyanTheOptimist Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion library/common/extensions/filters/http/socket_tag/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ Http::FilterHeadersStatus SocketTagFilter::decodeHeaders(Http::RequestHeaderMap&
if (!absl::SimpleAtoi(data.first, &uid) || !absl::SimpleAtoi(data.second, &traffic_stats_tag)) {
decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, "Invalid socket-tag header.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider including the value of the socket-tag header somewhere in the error message? Could be helpful for debugging purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

nullptr, absl::nullopt, "");
// return Http::FilterDataStatus::StopIterationNoBuffer;
return Http::FilterHeadersStatus::StopIteration;
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I opened a GH issue to track the work related to adding support for running tests on Android emulator An ability to run tests on Android emulator #2450.
  2. In the meantime you can run Envoy Mobile example app to verify your changes following the setup steps from https://envoymobile.io/docs/envoy-mobile/latest/development/debugging/android_local.html
  3. It's strongly recommended that you enable your changes in Android experimental app. You can do this following this change from one of the previous PRs https://github.com/envoyproxy/envoy-mobile/pull/2379/files#diff-b15ec59c29644d8c6089ad0ea7370859a690563f75d02c819679270b61ab9520R60. This experimental app as runs on an Android emulator.

Copy link
Contributor Author

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!

Copy link
Contributor

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

}

Expand Down
2 changes: 2 additions & 0 deletions library/common/network/socket_tag_socket_option_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ bool SocketTagSocketOptionImpl::setOption(
// different socket tag socket options must end up in different socket pools.
// This happens because different socket tag socket option generate different
// hash keys.
// Futher, this only works for sockets which have a raw fd and will be a no-op
// otherwise.
int fd = socket.ioHandle().fdDoNotUse();
RyanTheOptimist marked this conversation as resolved.
Show resolved Hide resolved
tag_socket(fd, 0, 0);
tag_socket(fd, uid_, traffic_stats_tag_);
Expand Down
12 changes: 12 additions & 0 deletions library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,18 @@ open class EngineBuilder(
return this
}

/**
* Specify whether to do brotli response decompression or not. Defaults to false.
RyanTheOptimist marked this conversation as resolved.
Show resolved Hide resolved
*
* @param enableBrotli whether or not to brotli decompress responses.
*
* @return This builder.
*/
fun enableSocketTag(enableSocketTag: Boolean): EngineBuilder {
this.enableSocketTag = enableSocketTag
return this
}

/**
* Specify whether sockets may attempt to bind to a specific interface, based on network
* conditions.
Expand Down
Loading