Add a filter for applying Android socket tags #2423

RyanTheOptimist · 2022-07-14T21:28:16Z

Add a filter for applying Android socket tags

Testing: Functionality verified as part of the run of the experimentation example app. Also filed #2494 to track improvements.
Docs Changes: Updated docs/root/api/starting_envoy.rst
Release Notes: Added

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2022-07-19T18:59:45Z

@alyssawilk can you take a look? I know I need tests, but hopefully I can lean on whatever testing gets set up for the cleartext check.

alyssawilk

wohoo progress!
high level pass because I feel guilty for prioritizing my FINALLY PASSING factory PR.
let's sort out where the code should live filter-wise with Lyfties since I'm hoping we can remove some of the boilerplate

library/common/config/config.cc

library/common/extensions/filters/http/socket_tag/filter.cc

library/common/network/socket_tag_socket_option_impl.cc

test/java/integration/AndroidEnvoyExplicitFlowTest.java

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk

Sweet, very cool. Next pass comments :-)

docs/root/intro/version_history.rst

alyssawilk · 2022-07-25T18:28:29Z

library/common/extensions/filters/http/socket_tag/filter.cc

+ uid_t uid;
+ uint32_t traffic_stats_tag;
+ if (!absl::SimpleAtoi(data.first, &uid) || !absl::SimpleAtoi(data.second, &traffic_stats_tag)) {
+ return Http::FilterHeadersStatus::StopIteration;


StopIteration or sendLocalReply with error? I'd think we wanted the latter? do we have an e2e test for this?

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?

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?

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?

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.

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

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:/envoyproxy/envoy-mobile/pull/2379/files#diff-b15ec59c29644d8c6089ad0ea7370859a690563f75d02c819679270b61ab9520R60. This experimental app as runs on an Android emulator.

Thanks for the issue! I'll give the demo app a shot. Updated test/kotlin/apps/experimental/MainActivity.kt per that previous commit. Thanks!

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

library/common/extensions/filters/http/socket_tag/filter.cc

library/common/extensions/filters/http/socket_tag/filter.h

library/common/extensions/filters/http/socket_tag/filter.proto

library/common/network/socket_tag_socket_option_impl.cc

library/common/network/socket_tag_socket_option_impl.h

library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java

library/java/org/chromium/net/AndroidNetworkLibrary.java

RyanTheOptimist

Thanks for the great review! Sorry for misunderstanding the API.

RyanTheOptimist · 2022-07-25T19:15:15Z

library/common/extensions/filters/http/socket_tag/filter.cc

+ uid_t uid;
+ uint32_t traffic_stats_tag;
+ if (!absl::SimpleAtoi(data.first, &uid) || !absl::SimpleAtoi(data.second, &traffic_stats_tag)) {
+ return Http::FilterHeadersStatus::StopIteration;


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?

library/common/extensions/filters/http/socket_tag/filter.cc

library/common/extensions/filters/http/socket_tag/filter.h

library/common/extensions/filters/http/socket_tag/filter.proto

library/common/network/socket_tag_socket_option_impl.cc

library/common/network/socket_tag_socket_option_impl.h

library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java

library/java/org/chromium/net/AndroidNetworkLibrary.java

docs/root/intro/version_history.rst

alyssawilk

Looking good! FWIW I really only glanced over the Java as I'm not a useful reviewer for that. tests (when we have the infra) will verify if it's all good.

docs/root/intro/version_history.rst

alyssawilk · 2022-07-26T13:33:30Z

library/common/extensions/filters/http/socket_tag/filter.cc

+ uid_t uid;
+ uint32_t traffic_stats_tag;
+ if (!absl::SimpleAtoi(data.first, &uid) || !absl::SimpleAtoi(data.second, &traffic_stats_tag)) {
+ return Http::FilterHeadersStatus::StopIteration;


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?

library/common/network/socket_tag_socket_option_impl.cc

library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2022-07-27T21:32:50Z

/retest

alyssawilk

Ok I think I'm hitting the end of my useful review comments, so let's see if we can get a Lyft volunteer to take it from here :-)
LGTM modulo the few remaining comments

library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt

test/java/integration/AndroidEngineSocketTagTest.java

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist

Thanks for the review! Github didn't seem to properly sign-off the commits it landed so I ended up having to force merge :/

RyanTheOptimist · 2022-08-03T18:18:56Z

library/common/extensions/filters/http/socket_tag/filter.cc

+
+Http::FilterHeadersStatus SocketTagFilter::decodeHeaders(Http::RequestHeaderMap& request_headers,
+ bool) {
+ auto socket_tag_header = Http::LowerCaseString("socket-tag");


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

RyanTheOptimist · 2022-08-03T18:20:12Z

library/common/extensions/filters/http/socket_tag/filter.cc

+ uid_t uid;
+ uint32_t traffic_stats_tag;
+ if (!absl::SimpleAtoi(data.first, &uid) || !absl::SimpleAtoi(data.second, &traffic_stats_tag)) {
+ decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, "Invalid socket-tag header.",


Good idea. Done.

library/java/org/chromium/net/ThreadStatsUid.java

test/java/integration/AndroidEngineSocketTagTest.java

library/cc/engine_builder.h

RyanTheOptimist · 2022-08-03T19:20:28Z

library/common/extensions/filters/http/socket_tag/filter.cc

+ uid_t uid;
+ uint32_t traffic_stats_tag;
+ if (!absl::SimpleAtoi(data.first, &uid) || !absl::SimpleAtoi(data.second, &traffic_stats_tag)) {
+ return Http::FilterHeadersStatus::StopIteration;


Thanks for the issue! I'll give the demo app a shot. Updated test/kotlin/apps/experimental/MainActivity.kt per that previous commit. Thanks!

library/common/jni/android_jni_utility.cc

Augustyniak

Thank you for addressing/responding to comments. Tests looks so much cleaner/simpler now. 💯

Left a few more comments

Augustyniak · 2022-08-04T16:41:56Z

library/common/extensions/filters/http/socket_tag/filter.cc

+ // The socket-tag header must contain a pair of number separated by a comma, e.g.:
+ // socket-tag: 123,456
+ // The first number contains the UID and the second contains the traffic stats tag.
+ std::string tag_string(request_headers.get(socket_tag_header)[0]->value().getStringView());


can we reorganize the code to avoid having to call equest_headers.get(socket_tag_header) twice?

Good point. Done.

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2022-08-12T00:46:19Z

I'm really confused by GitHub. I see an email from 2:04 PM Pacific today (2022-08-11) with a few comments that were sent after I uploaded a few new commits. But github says the commits were yesterday, and some of my comments are missing from the conversation view, or at least hidden. And some seem completely lost. I'm not sure what happened.

In any case, @Augustyniak I tried to explain that I updated the sample app to set socket tags, but end up with a java class not found error for the class org.chromium.net.AndroidNetworkLibrary which implements the tagSocket() method. I'm not quite sure how to resolve this. Any advice?

Augustyniak · 2022-08-17T20:48:59Z

docs/root/api/starting_envoy.rst

+``enableSocketTagging``
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Specify whether to enable support for Android socket tagging. Defaults to false.


Suggested change

Specify whether to enable support for Android socket tagging. Defaults to false.

Specify whether to enable support for Android socket tagging. Unavailable on iOS. Defaults to false.

Can we add this note to make it clear that it's no intended for iOS?

Oh, whoops. I missed your suggestion earlier. Done.

I do not want to be annoying but i still do not see it being updated.

Really? Hm. If I go to https:/envoyproxy/envoy-mobile/pull/2423/files I see:

~~~~~~~~~~~~~~~~~~~~~~~ ``enableSocketTagging`` ~~~~~~~~~~~~~~~~~~~~~~~ Specify whether to enable support for Android socket tagging. Unavailable on iOS. Defaults to false. **Example**:: // Kotlin builder.enableSocketTagging(true)

I wonder if this is one of the many different ways that the github UI hides comments and changes by default :(

Augustyniak · 2022-08-17T21:05:43Z

@RyanTheOptimist I modified the example app locally to enable socket tagging and run it to see whether I can repro the crashes that you mentioned locally. This is what I got:

That's despite your BUILD file changes in here https:/envoyproxy/envoy-mobile/pull/2423/files#diff-bee4aeb6cd03b6684900f7c2d8bd51dec467011c33929eb3b34008a2a44deb23R19. This issue seems to be exactly the same issue as the one that's related is_cleartext_permitted and which was detailed in #2432.

jpsim · 2022-08-18T20:47:00Z

/retest

Signed-off-by: Ryan Hamilton <[email protected]>

Augustyniak · 2022-08-19T16:39:37Z

library/common/jni/android_jni_utility.cc

+void tag_socket(int ifd, int uid, int tag) {
+#if defined(__ANDROID_API__)
+ JNIEnv* env = get_env();
+ jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary");


The one thing that you need to do here is to start using the find_class that was introduced in #2483

Suggested change

jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary");

jclass jcls_AndroidNetworkLibrary = find_class("org.chromium.net.AndroidNetworkLibrary");

Signed-off-by: Ryan Hamilton <[email protected]>

Augustyniak

NICE! Great to see it working - this is awesome.

Left a few comments related to tests - apologies for not catching whatever I commented on earlier but I was focused on the functionality more.

General comment: Can we update the description of the PR, please? In particular, for testing can we mention that the functionality is verified as part of the run of the experimentation example app?

Augustyniak · 2022-08-19T20:11:28Z

docs/root/api/starting_envoy.rst

+``enableSocketTagging``
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Specify whether to enable support for Android socket tagging. Defaults to false.


Can we add this note to make it clear that it's no intended for iOS?

Augustyniak · 2022-08-19T20:20:36Z

test/java/integration/AndroidEngineSocketTagTest.java

+ RequestScenario requestScenario = new RequestScenario()
+ .setHttpMethod(RequestMethod.GET)
+ .setUrl(mockWebServer.url("post/flowers").toString())
+ .addHeader("x-envoy-mobile-socket-tag", "0,0");


RequestScenario becomes a little bit of a leaky abstraction as in real application we are not supposed to call addHeader("x-envoy-mobile-socket-tag", "0,0"); directly (since we added addSocketTag).

In fact, I am not sure whether this test works the way it's intended too as it tries to add x-envoy-mobile-socket-tag header in

envoy-mobile/test/java/io/envoyproxy/envoymobile/engine/testing/RequestScenario.java

Line 53 in 0d7b43e

headers.forEach(entry -> requestHeadersBuilder.add(entry.getKey(), entry.getValue()));

and add(...) call does not accept headers that start with x-envoy-mobile prefix

envoy-mobile/library/kotlin/io/envoyproxy/envoymobile/HeadersBuilder.kt

Line 80 in 0d7b43e

name.startsWith("x-envoy-mobile", ignoreCase = true) || name.equals("host", ignoreCase = true)

Maybe we could

Blergh! Yes, good point. Ok, I've added an addSocketTag() method to RequestScenario which delegates to the underlying builder. I verified (via printf) that the socket tagging code is in fact called.

great, thank you - I think that adding addSocketTag method to the test scenario is a way to go 👍 . That being said, I think that the right final solution in here is to change the underlying implementation of TestScenario so that it stores test state inside of RequestHeadersBuilder as opposed to on-the-side headers map. Only with RequestHeadersBuilder we can ensure that our tests simulate real-life condition.

I do not think that this has to be done as part of your PR so I am fine with it being merged the way it is now.

Thanks! I'm certainly not going to argue with "fine with it being merged the way it is now" :) I'm happy to do the needful in a followup.

That being said, I'm not entirely sure I understand the suggestion. The implementation in RequestScenario currently calls addSocketTag() on RequestHeadersBuilder which sounds like what you were asking for? Or is it not the socket tagging stuff that is problematic, but rather the general header stuff? Are you thinking we should get rid of the headers member in RequestScenario and use the RequestHeadersBuilder directly?

Or is it not the socket tagging stuff that is problematic, but rather the general header stuff?

Yes, that's what I meant

Are you thinking we should get rid of the headers member in RequestScenario and use the RequestHeadersBuilder directly?

That was my idea. This way our tests would be guaranteed to "get an update" every time we update RequestHeadersBuilder.

Augustyniak · 2022-08-19T20:21:27Z

test/java/integration/AndroidEngineSocketTagTest.java

+ @Override
+ public MockResponse dispatch(RecordedRequest recordedRequest) {
+ assertThat(recordedRequest.getMethod()).isEqualTo(RequestMethod.GET.name());
+ assertThat(recordedRequest.getHeader("x-envoy-mobile-socket-tag")).isEqualTo(null);


why is it expected to be equal to null if we set a valid socket tag on the request?

ah, because we remove it when we detect that it's present

Yup, exactly!

Any way we could ensure that we are really adding x-envoy-mobile-socket-tag at some point? As it is now this test is always green - even if socket tagging is not enabled / does not end up adding a tag.

Filed #2494 as a result of offline discussions. Thanks for working through this!

Signed-off-by: Ryan Hamilton <[email protected]>

Augustyniak

Left 3 more comments related to your responses/updates from the previous iterations of my review. Happy to approve once you respond to my comments.

Augustyniak

Thanks for working on this. Really excited to see this landing 🎉 Congratz - great job.

RyanTheOptimist force-pushed the SocketTag branch from ec8b3fa to 6726dea Compare July 15, 2022 15:03

RyanTheOptimist added 8 commits July 19, 2022 18:13

Implement Android Socket Tagging

31f5ed1

Signed-off-by: Ryan Hamilton <[email protected]>

Working

2fe087f

Signed-off-by: Ryan Hamilton <[email protected]>

format

9c571da

Signed-off-by: Ryan Hamilton <[email protected]>

cleanup

6c337f5

Signed-off-by: Ryan Hamilton <[email protected]>

fix

dd1990e

Signed-off-by: Ryan Hamilton <[email protected]>

Add config to botstrap and trigger in test

3fe5055

Signed-off-by: Ryan Hamilton <[email protected]>

Merge branch 'main' into SocketTag

30669c0

Add tagSocket from cronet

838b687

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist force-pushed the SocketTag branch from a379bb5 to 838b687 Compare July 19, 2022 18:13

format

3f1b8fa

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist assigned alyssawilk Jul 19, 2022

alyssawilk reviewed Jul 19, 2022

View reviewed changes

RyanTheOptimist added 4 commits July 20, 2022 16:02

address comments

3b70695

Signed-off-by: Ryan Hamilton <[email protected]>

Add config knobs

285bb29

Signed-off-by: Ryan Hamilton <[email protected]>

format

e8ce39b

Signed-off-by: Ryan Hamilton <[email protected]>

Fix cronet

702781a

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk reviewed Jul 25, 2022

View reviewed changes

RyanTheOptimist commented Jul 25, 2022

View reviewed changes

alyssawilk reviewed Jul 26, 2022

View reviewed changes

RyanTheOptimist added 5 commits July 27, 2022 18:31

Fix EnvoyConfigurationTest.kt

bf91e03

Signed-off-by: Ryan Hamilton <[email protected]>

Address comments

4f69f6f

Signed-off-by: Ryan Hamilton <[email protected]>

format

8308110

Signed-off-by: Ryan Hamilton <[email protected]>

Address comments

97302cd

Signed-off-by: Ryan Hamilton <[email protected]>

Format

6221682

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist force-pushed the SocketTag branch from 901d67d to 6221682 Compare July 27, 2022 18:38

alyssawilk reviewed Jul 28, 2022

View reviewed changes

library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt Outdated Show resolved Hide resolved

test/java/integration/AndroidEngineSocketTagTest.java Show resolved Hide resolved

test/java/integration/AndroidEngineSocketTagTest.java Show resolved Hide resolved

Comment

18030a7

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist added 5 commits August 3, 2022 20:34

Simplify test

fd83daa

Signed-off-by: Ryan Hamilton <[email protected]>

Address comments

c539773

Signed-off-by: Ryan Hamilton <[email protected]>

Rename

07af4a8

Signed-off-by: Ryan Hamilton <[email protected]>

Format

6b44c79

Signed-off-by: Ryan Hamilton <[email protected]>

Spelling

4539347

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist force-pushed the SocketTag branch from d0d1789 to 4539347 Compare August 3, 2022 20:35

RyanTheOptimist commented Aug 3, 2022

View reviewed changes

Augustyniak reviewed Aug 4, 2022

View reviewed changes

alyssawilk assigned Augustyniak Aug 8, 2022

RyanTheOptimist added 3 commits August 10, 2022 17:30

Address comments

99a1d03

Signed-off-by: Ryan Hamilton <[email protected]>

Address comments

4eebefe

Signed-off-by: Ryan Hamilton <[email protected]>

Format

ec65417

Signed-off-by: Ryan Hamilton <[email protected]>

Augustyniak reviewed Aug 17, 2022

View reviewed changes

add "org/chromium/net/*"

a1febff

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist mentioned this pull request Aug 19, 2022

android: put cronet files into final EM aar #2481

Closed

Augustyniak reviewed Aug 19, 2022

View reviewed changes

RyanTheOptimist added 2 commits August 19, 2022 17:04

Merge branch 'main' into SocketTag

dbff49a

Signed-off-by: Ryan Hamilton <[email protected]>

use find_class

e38a859

Signed-off-by: Ryan Hamilton <[email protected]>

Augustyniak reviewed Aug 19, 2022

View reviewed changes

alyssawilk removed their assignment Aug 22, 2022

RyanTheOptimist added 2 commits August 22, 2022 19:44

Address more comments

4f40e1d

Signed-off-by: Ryan Hamilton <[email protected]>

Format!

633704b

Signed-off-by: Ryan Hamilton <[email protected]>

Augustyniak reviewed Aug 23, 2022

View reviewed changes

RyanTheOptimist mentioned this pull request Aug 23, 2022

Improve testing of Socket Tagging code #2494

Open

Augustyniak approved these changes Aug 23, 2022

View reviewed changes

RyanTheOptimist merged commit 66ecf9f into envoyproxy:main Aug 23, 2022

	Specify whether to enable support for Android socket tagging. Defaults to false.
	Specify whether to enable support for Android socket tagging. Unavailable on iOS. Defaults to false.

	jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary");
	jclass jcls_AndroidNetworkLibrary = find_class("org.chromium.net.AndroidNetworkLibrary");

Add a filter for applying Android socket tags #2423

Add a filter for applying Android socket tags #2423

Conversation

RyanTheOptimist commented Jul 14, 2022 • edited Loading

RyanTheOptimist commented Jul 19, 2022

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Jul 27, 2022

alyssawilk left a comment

Choose a reason for hiding this comment

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Augustyniak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Augustyniak commented Aug 17, 2022

jpsim commented Aug 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Augustyniak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Augustyniak left a comment

Choose a reason for hiding this comment

Augustyniak left a comment

Choose a reason for hiding this comment

RyanTheOptimist commented Jul 14, 2022 •

edited

Loading