Add few more utility functions for address and http proto's. by saumoh · Pull Request #2504 · envoyproxy/envoy

saumoh · 2018-02-01T05:24:42Z

Title
Add a few utility functions to the

http/protocol and http_header_map.h files
Another utility function to copy Address::Instance into it's protobuf representation

Testing
I have tested and used these new functions in the PR #2415
added a ut for the http changes.

Risk
Low: like really low.

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

mattklein123

few quick comments, thanks

mattklein123 · 2018-02-01T06:15:04Z

source/common/network/utility.cc

  return result;
 }

+void Utility::addressToProtobufAddress(const Address::Instance& address,


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.

added tests and consolidated. thanks.

mattklein123 · 2018-02-01T06:15:52Z

include/envoy/http/header_map.h

+  /**
+   * @return a std::string.
+   */
+  std::string getString() const {


Is this really necessary? Seems pretty superfluous to me, but I don't feel that strongly about it. If it is, I suspect you don't need the if statement and the constructor will do the right thing if you pass a length of 0, but I'm not sure.

the need for it came up in the feedback for pr #2415
it is kind of useful to have it here and not have the user have to convert a c_str(). thx

mattklein123 · 2018-02-01T06:17:37Z

include/envoy/http/protocol.h

 enum class Protocol { Http10, Http11, Http2 };
 const size_t NumProtocols = 3;

+inline std::string getProtocolString(const Protocol& p) {


Already implemented here: https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/access_log_formatter.cc#L33

Please consolidate (and use the string in access log formatter to be consistent).

consolidated. Thanks.

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

alyssawilk

nice change! 2 nits from my end.

alyssawilk · 2018-02-01T15:59:23Z

include/envoy/http/protocol.h

+    return Http11String;
+  case Protocol::Http2:
+    return Http2String;
+  default:


Mild preference for no default - I'd rather have a compile fail if we add a new Protocol and forget to update this function.

+1 just return directly and avoid default. Add NOT_REACHED at end of function if necessary.

alyssawilk · 2018-02-01T16:08:17Z

test/common/network/utility_test.cc

+    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());


For completeness, mind checking the value and port here, and adding a test case for pipe?

htuch · 2018-02-01T16:11:45Z

include/envoy/http/protocol.h

 const size_t NumProtocols = 3;

+static const std::string DefaultString = "";
+static const std::string Http10String = "HTTP/1.0";


Static non-POD is not allowed in the Envoy style, see https://github.com/envoyproxy/envoy/blob/master/STYLE.md. This could probably just be const char DefaultString = "" etc.

We could also put these in a struct in headers.h
I was wavering on asking that anyway - they're not so much headers as "part of the firstline" but it's where I'd look for predefined header-type strings.

IMO we should continue to return string references for perf reasons. I still think that the non-POD rule for std::string is a little silly, but, the rules are the rules. I would just use the constant class pattern we use elsewhere like in Http::Headers.

@mattklein123 i think you are referring to https://github.com/envoyproxy/envoy/blob/master/source/common/http/headers.h
if so then i can certainly adopt that patter here as well.
@alyssawilk is it ok if i keep the new constant class here?

Either way is fine with me

@mattklein123 @alyssawilk Last push addresses this. Adding NOT_REACHED in this file causes a circular dependency so i added a abort(). If that is not what we want then I can move both constants and the getProtocolString to the common/http/headers.h file.
please let me know if that would be preferable.

I would recommend moving into common and using NOT_REACHED. You can either put in headers.h or feel free to just put in Http::Utility

cool. moved. please review when u folks get a chance. thanks

mattklein123 · 2018-02-01T16:14:24Z

include/envoy/http/protocol.h

+static const std::string Http11String = "HTTP/1.1";
+static const std::string Http2String = "HTTP/2";
+
+inline const std::string& getProtocolString(const Protocol& p) {


Protocol should be passed by value: Protocol p

mattklein123 · 2018-02-01T16:16:16Z

include/envoy/http/protocol.h

 const size_t NumProtocols = 3;

+static const std::string DefaultString = "";
+static const std::string Http10String = "HTTP/1.0";


IMO we should continue to return string references for perf reasons. I still think that the non-POD rule for std::string is a little silly, but, the rules are the rules. I would just use the constant class pattern we use elsewhere like in Http::Headers.

mattklein123 · 2018-02-01T16:17:16Z

include/envoy/http/protocol.h

+    return Http11String;
+  case Protocol::Http2:
+    return Http2String;
+  default:


+1 just return directly and avoid default. Add NOT_REACHED at end of function if necessary.

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

alyssawilk

Looking good - just two nits from me otherwise LGTM

alyssawilk · 2018-02-02T02:35:02Z

include/envoy/http/protocol.h

@@ -1,4 +1,5 @@
 #pragma once
+#include <string>


Can this include be removed?

alyssawilk · 2018-02-02T02:37:00Z

source/common/http/utility.h

+   * @param protocol for which to return the string representation.
+   * @return string representation of the protocol.
+   */
+  static const std::string& getProtocolString(const Protocol p);


By docs comments I think this should be named "protocol" rather than "p"

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.

alyssawilk · 2018-02-02T02:42:59Z

Also I'm not quite sure what's with the macos build - I don't recall seeing this error mode on other PRs. Could you try merging with master and take a look if that doesn't fix it?

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

…enerics Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

mattklein123

LGTM, thanks!

) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

These changes are prerequisite for envoyproxy/envoy-mobile#1549 Signed-off-by: Chidera Olibie <colibie@google.com> Signed-off-by: JP Simard <jp@jpsim.com>

Add few more utility functions for address and http proto's.

da62afa

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

saumoh mentioned this pull request Feb 1, 2018

Support for external authorization grpc service. #2415

Merged

mattklein123 reviewed Feb 1, 2018

View reviewed changes

Saurabh Mohan added 3 commits January 31, 2018 22:28

Add more ut.

858d842

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Remove if check.

ab15cc3

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Consolidate usage.

a519fb3

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

alyssawilk reviewed Feb 1, 2018

View reviewed changes

alyssawilk self-assigned this Feb 1, 2018

htuch reviewed Feb 1, 2018

View reviewed changes

mattklein123 reviewed Feb 1, 2018

View reviewed changes

Saurabh Mohan added 2 commits February 1, 2018 15:30

Address review feedback.

57c6f26

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Move protocol string to headers and access via utility.

e4156e8

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

alyssawilk reviewed Feb 2, 2018

View reviewed changes

Saurabh Mohan added 2 commits February 1, 2018 18:47

Review comments.

49a3570

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Merge remote-tracking branch 'upstream/master' into ext-auth-review-g…

df0f0bf

…enerics Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

mattklein123 approved these changes Feb 2, 2018

View reviewed changes

htuch approved these changes Feb 2, 2018

View reviewed changes

htuch merged commit 5f8b841 into envoyproxy:master Feb 2, 2018

saumoh deleted the ext-auth-review-generics branch May 4, 2018 17:28

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Update Envoy-WASM SHA to latest based on Envoy v1.12.0. (envoyproxy#2504

97fbdb6

) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

jpsim pushed a commit that referenced this pull request Nov 28, 2022

Cronvoy: Update CronetTestRule with Cronet changes (#2504)

715fe65

These changes are prerequisite for envoyproxy/envoy-mobile#1549 Signed-off-by: Chidera Olibie <colibie@google.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

Cronvoy: Update CronetTestRule with Cronet changes (#2504)

b0460ff

These changes are prerequisite for envoyproxy/envoy-mobile#1549 Signed-off-by: Chidera Olibie <colibie@google.com> Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

saumoh commented Feb 1, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saumoh Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saumoh Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Feb 2, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

saumoh Feb 1, 2018 •

edited

Loading

saumoh Feb 1, 2018 •

edited

Loading