-
Notifications
You must be signed in to change notification settings - Fork 84
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
Changes from 32 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
load( | ||
"@envoy//bazel:envoy_build_system.bzl", | ||
"envoy_cc_extension", | ||
"envoy_extension_package", | ||
"envoy_proto_library", | ||
) | ||
|
||
licenses(["notice"]) # Apache 2 | ||
|
||
envoy_extension_package() | ||
|
||
envoy_proto_library( | ||
name = "filter", | ||
srcs = ["filter.proto"], | ||
) | ||
|
||
envoy_cc_extension( | ||
name = "socket_tag_filter_lib", | ||
srcs = ["filter.cc"], | ||
hdrs = ["filter.h"], | ||
repository = "@envoy", | ||
deps = [ | ||
":filter_cc_proto", | ||
"//library/common/http:internal_headers_lib", | ||
"//library/common/network:socket_tag_socket_option_lib", | ||
"//library/common/types:c_types_lib", | ||
"@envoy//envoy/http:codes_interface", | ||
"@envoy//envoy/http:filter_interface", | ||
"@envoy//source/extensions/filters/http/common:pass_through_filter_lib", | ||
], | ||
) | ||
|
||
envoy_cc_extension( | ||
name = "config", | ||
srcs = ["config.cc"], | ||
hdrs = ["config.h"], | ||
repository = "@envoy", | ||
deps = [ | ||
":socket_tag_filter_lib", | ||
"@envoy//source/extensions/filters/http/common:factory_base_lib", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
#include "library/common/extensions/filters/http/socket_tag/config.h" | ||
|
||
#include <stdint.h> | ||
#include <sys/types.h> | ||
#include <unistd.h> | ||
|
||
#include "envoy/network/listen_socket.h" | ||
|
||
#include "library/common/extensions/filters/http/socket_tag/filter.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace HttpFilters { | ||
namespace SocketTag { | ||
|
||
Http::FilterFactoryCb SocketTagFilterFactory::createFilterFactoryFromProtoTyped( | ||
const envoymobile::extensions::filters::http::socket_tag::SocketTag& /*proto_config*/, | ||
const std::string&, Server::Configuration::FactoryContext& /*context*/) { | ||
|
||
return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { | ||
callbacks.addStreamFilter(std::make_shared<SocketTagFilter>()); | ||
}; | ||
} | ||
|
||
/** | ||
* Static registration for the SocketTag filter. @see NamedHttpFilterConfigFactory. | ||
*/ | ||
REGISTER_FACTORY(SocketTagFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory); | ||
|
||
} // namespace SocketTag | ||
} // namespace HttpFilters | ||
} // namespace Extensions | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
#include <string> | ||
|
||
#include "source/extensions/filters/http/common/factory_base.h" | ||
|
||
#include "library/common/extensions/filters/http/socket_tag/filter.pb.h" | ||
#include "library/common/extensions/filters/http/socket_tag/filter.pb.validate.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace HttpFilters { | ||
namespace SocketTag { | ||
|
||
/** | ||
* Config registration for the socket tag filter. @see NamedHttpFilterConfigFactory. | ||
*/ | ||
class SocketTagFilterFactory | ||
: public Common::FactoryBase<envoymobile::extensions::filters::http::socket_tag::SocketTag> { | ||
public: | ||
SocketTagFilterFactory() : FactoryBase("socket_tag") {} | ||
|
||
private: | ||
::Envoy::Http::FilterFactoryCb createFilterFactoryFromProtoTyped( | ||
const envoymobile::extensions::filters::http::socket_tag::SocketTag& config, | ||
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; | ||
}; | ||
|
||
DECLARE_FACTORY(SocketTagFilterFactory); | ||
|
||
} // namespace SocketTag | ||
} // namespace HttpFilters | ||
} // namespace Extensions | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
#include "library/common/extensions/filters/http/socket_tag/filter.h" | ||
|
||
#include "envoy/server/filter_config.h" | ||
|
||
#include "library/common/network/socket_tag_socket_option_impl.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace HttpFilters { | ||
namespace SocketTag { | ||
|
||
Http::FilterHeadersStatus SocketTagFilter::decodeHeaders(Http::RequestHeaderMap& request_headers, | ||
bool) { | ||
static auto socket_tag_header = Http::LowerCaseString("x-envoy-mobile-socket-tag"); | ||
Http::RequestHeaderMap::GetResult header = request_headers.get(socket_tag_header); | ||
if (header.empty()) { | ||
return Http::FilterHeadersStatus::Continue; | ||
} | ||
|
||
// The x-envoy-mobile-socket-tag header must contain a pair of number separated by a comma, e.g.: | ||
// x-envoy-mobile-socket-tag: 123,456 | ||
// The first number contains the UID and the second contains the traffic stats tag. | ||
std::string tag_string(header[0]->value().getStringView()); | ||
std::pair<std::string, std::string> data = absl::StrSplit(tag_string, ','); | ||
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, | ||
absl::StrCat("Invalid x-envoy-mobile-socket-tag header: ", tag_string), nullptr, | ||
absl::nullopt, ""); | ||
return Http::FilterHeadersStatus::StopIteration; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 |
||
} | ||
auto options = std::make_shared<Network::Socket::Options>(); | ||
options->push_back(std::make_shared<Network::SocketTagSocketOptionImpl>(uid, traffic_stats_tag)); | ||
decoder_callbacks_->addUpstreamSocketOptions(options); | ||
request_headers.remove(socket_tag_header); | ||
return Http::FilterHeadersStatus::Continue; | ||
} | ||
|
||
} // namespace SocketTag | ||
} // namespace HttpFilters | ||
} // namespace Extensions | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
#pragma once | ||
|
||
#include "envoy/http/filter.h" | ||
|
||
#include "source/common/common/logger.h" | ||
#include "source/extensions/filters/http/common/pass_through_filter.h" | ||
|
||
#include "library/common/extensions/filters/http/socket_tag/filter.pb.h" | ||
#include "library/common/types/c_types.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace HttpFilters { | ||
namespace SocketTag { | ||
|
||
/** | ||
* Filter to set upstream socket tags based on a request header. | ||
* See: https://source.android.com/devices/tech/datausage/tags-explained | ||
*/ | ||
class SocketTagFilter final : public Http::PassThroughFilter, | ||
public Logger::Loggable<Logger::Id::filter> { | ||
public: | ||
// Http::PassThroughDecoderFilter | ||
Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& request_headers, bool) override; | ||
}; | ||
|
||
} // namespace SocketTag | ||
} // namespace HttpFilters | ||
} // namespace Extensions | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
syntax = "proto3"; | ||
|
||
package envoymobile.extensions.filters.http.socket_tag; | ||
|
||
// Configuration for the socket tagging filer. This filter uses data from the | ||
// x-envoy-mobile-socket-tag request header to apply an Android Socket Tag to the upstream | ||
// socket. | ||
// See: https://source.android.com/devices/tech/datausage/tags-explained | ||
// See: https://developer.android.com/reference/android/net/TrafficStats#setThreadStatsTag(int) | ||
// See: https://developer.android.com/reference/android/net/TrafficStats#setThreadStatsUid(int) | ||
// See: https://developer.android.com/reference/android/net/TrafficStats#tagSocket(java.net.Socket) | ||
message SocketTag { | ||
alyssawilk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -33,3 +33,17 @@ bool is_cleartext_permitted(absl::string_view hostname) { | |||||
return true; | ||||||
#endif | ||||||
} | ||||||
|
||||||
void tag_socket(int ifd, int uid, int tag) { | ||||||
RyanTheOptimist marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
#if defined(__ANDROID_API__) | ||||||
JNIEnv* env = get_env(); | ||||||
jclass jcls_AndroidNetworkLibrary = env->FindClass("org/chromium/net/AndroidNetworkLibrary"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||||||
jmethodID jmid_tagSocket = | ||||||
env->GetStaticMethodID(jcls_AndroidNetworkLibrary, "tagSocket", "(III)V"); | ||||||
env->CallStaticVoidMethod(jcls_AndroidNetworkLibrary, jmid_tagSocket, ifd, uid, tag); | ||||||
#else | ||||||
UNREFERENCED_PARAMETER(ifd); | ||||||
UNREFERENCED_PARAMETER(uid); | ||||||
UNREFERENCED_PARAMETER(tag); | ||||||
#endif | ||||||
} |
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.
Can we add this note to make it clear that it's no intended for iOS?
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, whoops. I missed your suggestion earlier. 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.
I do not want to be annoying but i still do not see it being updated.
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.
Really? Hm. If I go to https://github.com/envoyproxy/envoy-mobile/pull/2423/files I see:
I wonder if this is one of the many different ways that the github UI hides comments and changes by default :(