-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add few more utility functions for address and http proto's. #2504
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
Changes from all commits
da62afa
858d842
ab15cc3
a519fb3
57c6f26
e4156e8
49a3570
df0f0bf
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 |
|---|---|---|
|
|
@@ -371,5 +371,17 @@ absl::uint128 Utility::flipOrder(const absl::uint128& input) { | |
| return result; | ||
| } | ||
|
|
||
| void Utility::addressToProtobufAddress(const Address::Instance& address, | ||
|
Member
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. Already implemented here: https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/grpc_access_log_impl.cc#L67 please consolidate Also, a dedicated function like this needs dedicated tests.
Contributor
Author
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. added tests and consolidated. thanks. |
||
| envoy::api::v2::Address& proto_address) { | ||
| if (address.type() == Address::Type::Pipe) { | ||
| proto_address.mutable_pipe()->set_path(address.asString()); | ||
| } else { | ||
| ASSERT(address.type() == Address::Type::Ip); | ||
| auto* socket_address = proto_address.mutable_socket_address(); | ||
| socket_address->set_address(address.ip()->addressAsString()); | ||
| socket_address->set_port_value(address.ip()->port()); | ||
| } | ||
| } | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,6 +198,24 @@ TEST(NetworkUtility, AnyAddress) { | |
| } | ||
| } | ||
|
|
||
| TEST(NetworkUtility, AddressToProtobufAddress) { | ||
| { | ||
| envoy::api::v2::Address proto_address; | ||
| Address::Ipv4Instance address("127.0.0.1"); | ||
| Utility::addressToProtobufAddress(address, proto_address); | ||
| EXPECT_EQ(true, proto_address.has_socket_address()); | ||
|
Contributor
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. For completeness, mind checking the value and port here, and adding a test case for pipe? |
||
| EXPECT_EQ("127.0.0.1", proto_address.socket_address().address()); | ||
| EXPECT_EQ(0, proto_address.socket_address().port_value()); | ||
| } | ||
| { | ||
| envoy::api::v2::Address proto_address; | ||
| Address::PipeInstance address("/hello"); | ||
| Utility::addressToProtobufAddress(address, proto_address); | ||
| EXPECT_EQ(true, proto_address.has_pipe()); | ||
| EXPECT_EQ("/hello", proto_address.pipe().path()); | ||
| } | ||
| } | ||
|
|
||
| TEST(PortRangeListTest, Errors) { | ||
| { | ||
| std::string port_range_str = "a1"; | ||
|
|
||
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.
By docs comments I think this should be named "protocol" rather than "p"
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.
oops. fixed this and the other comment as well.
Merged master also, hopefully that will help with the macos build. fingers crossed.
Thanks for the quick review.