address refactor #429

mattklein123 · 2017-02-05T18:32:37Z

This is a large refactor that moves away from representing addresses
internally as strings of "URL" form into proper objects. With this
change in place, it should be very straightforward to add IPv6
support, as most places in the code no longer need to understand
anything about the underlying address. Additionally, adding NT support
in the future should also be easier.

There are still some unfortunate legacy aspects, such as the fact that
when loading from configuration we still parse tcp:// as well as unix://
style addresses. In the future this should likely become ip:// as well
as pipe:// but we will probably need to continue to support both.

Note that although this change is huge in number of lines, most of it is
very straightforward, and certain portions of the code have become
substantially simpler such as listener handling and client connection
handling.

Fixes #390

This is a large refactor that moves away from representing addresses internally as strings of "URL" form into proper objects. With this change in place, it should be very straightforward to add IPv6 support, as most places in the code no longer need to understand anything about the underlying address. Additionally, adding NT support in the future should also be easier. There are still some unfortunate legacy aspects, such as the fact that when loading from configuration we still parse tcp:// as well as unix:// style addresses. In the future this should likely become ip:// as well as pipe:// but we will probably need to continue to support both. Note that although this change is huge in number of lines, most of it is very straightforward, and certain portions of the code have become substantially simpler such as listener handling and client connection handling. Fixes #390

mattklein123 · 2017-02-05T18:33:53Z

@lyft/network-team @jamessynge I will be OOO tomorrow and am probably going to add some more tests and possibly do a little more cleanup, but this should be ready for a first pass. As I said in the title, though this change is a huge number of lines, most of it is very straightforward.

ccaraman · 2017-02-07T23:39:25Z

include/envoy/network/address.h

+  virtual const std::string& asString() const PURE;
+
+  /**
+   * Bind a socket to to this address. The socket should have been created with a call to socket()


duplicate to

ccaraman · 2017-02-08T01:08:26Z

source/common/network/utility.h

   * @param url supplies the url to resolve.
-   * @return EventAddrInfoPtr the resolved address.
+   * @return a resolved address.


nit: return type instead of a

ccaraman · 2017-02-08T01:09:29Z

source/common/network/utility.h


  /**
   * Retrieve the original destination address from an accepted fd.
   * The address (IP and port) may be not local and the port may differ from
   * the listener port if the packets were redirected using iptables
   * @param fd is the descriptor returned by accept()
-   * @param orig_addr is the data structure that contains the original address
-   * @return true if the operation succeeded, false otherwise
+   * @return the original destination or nullptr not available.


nullptr if not...

ccaraman · 2017-02-08T01:17:44Z

source/common/upstream/upstream_impl.cc

+          // TODO: Currently the DNS interface does not consider port. We need to make a new
+          //       address that has port in it. We need to both support IPv6 as well as potentially
+          //       move port handling into the DNS interface itself, which would work better for
+          //       SRV.


general knowledge question: What does SRV stand for?

https://en.wikipedia.org/wiki/SRV_record

mattklein123 · 2017-02-08T06:14:46Z

@ccaraman updated

RomanDzhabarov · 2017-02-08T19:17:35Z

include/envoy/network/address.h

+   * Bind a socket to this address. The socket should have been created with a call to socket() on
+   * this object.
+   * @param fd supplies the platform socket handle.
+   * @return the platform error code.


and zero if no errors?

it's platform specific. The idea is we can use this eventually to support NT.

RomanDzhabarov · 2017-02-08T19:17:56Z

include/envoy/network/address.h

+   * Connect a socket to this address. The socket should have been created with a call to socket()
+   * on this object.
+   * @param fd supplies the platform socket handle.
+   * @return the platform error code.


RomanDzhabarov · 2017-02-08T19:32:38Z

source/common/http/async_client_impl.cc

@@ -94,7 +95,7 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) {

 void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) {
  headers.insertEnvoyInternalRequest().value(Headers::get().EnvoyInternalRequestValues.True);
-  headers.insertForwardedFor().value(parent_.config_.local_info_.address());
+  Utility::appendXff(headers, *parent_.config_.local_info_.address());


Is that intended to add address to XFF instead of rewriting one? iirc, if we have 2 addresses in xff, isInternal and other things will not work as expected on the upstream Envoy side, but i might be missing something

appendXff now has logic to deal with not appending if not IP address type. Sharing code.

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of mixerclient This PR will be merged automatically once checks are successful. ```release-note none ```

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

… (#166) * Make callout responses accessible from stream context. (envoyproxy#429) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: Kick CI. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

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

…oot/start/sandboxes/cache.rst (envoyproxy#429)

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

mattklein123 added 3 commits February 5, 2017 12:25

fix

7518d9e

fix

2c7bf53

fix

d37ee80

mattklein123 closed this Feb 6, 2017

mattklein123 reopened this Feb 6, 2017

more tests

86aeb4b

mattklein123 force-pushed the address_refactor branch from 6a56c05 to 86aeb4b Compare February 6, 2017 18:52

mattklein123 added 3 commits February 6, 2017 11:31

more tests and refactor

9c5ee68

more tests and cleanup

d8f75f0

Merge branch 'master' into address_refactor

f2d44a0

ccaraman reviewed Feb 7, 2017

View reviewed changes

ccaraman reviewed Feb 8, 2017

View reviewed changes

mattklein123 added 2 commits February 7, 2017 22:14

comments

dd0240d

Merge branch 'master' into address_refactor

f28202e

RomanDzhabarov reviewed Feb 8, 2017

View reviewed changes

RomanDzhabarov approved these changes Feb 8, 2017

View reviewed changes

mattklein123 merged commit 86219a4 into master Feb 8, 2017

mattklein123 deleted the address_refactor branch February 12, 2017 22:52

PiotrSikora added a commit to PiotrSikora/envoy that referenced this pull request Feb 26, 2020

Make callout responses accessible from stream context. (envoyproxy#429)

8dbdb4d

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

jplevyak pushed a commit to jplevyak/envoy that referenced this pull request Apr 13, 2020

Make callout responses accessible from stream context. (envoyproxy#429)

10e108e

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

wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this pull request Jan 23, 2021

zh-translation: https://github.com/cloudnativeto/envoy/blob/zh/docs/r…

0d07b19

…oot/start/sandboxes/cache.rst (envoyproxy#429)

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

version: update versions for 'historical' v0.1.1 (#429)

87477fb

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

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

version: update versions for 'historical' v0.1.1 (#429)

7f09e29

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

address refactor #429

address refactor #429

mattklein123 commented Feb 5, 2017

mattklein123 commented Feb 5, 2017

ccaraman Feb 7, 2017

ccaraman Feb 8, 2017

ccaraman Feb 8, 2017

ccaraman Feb 8, 2017

mattklein123 Feb 8, 2017

mattklein123 commented Feb 8, 2017

RomanDzhabarov Feb 8, 2017

mattklein123 Feb 8, 2017

RomanDzhabarov Feb 8, 2017

mattklein123 Feb 8, 2017

RomanDzhabarov Feb 8, 2017 •

edited

Loading

mattklein123 Feb 8, 2017

address refactor #429

address refactor #429

Conversation

mattklein123 commented Feb 5, 2017

mattklein123 commented Feb 5, 2017

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

mattklein123 commented Feb 8, 2017

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

RomanDzhabarov Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanDzhabarov Feb 8, 2017 •

edited

Loading