Skip to content
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

Clean up network address handling #390

Closed
mattklein123 opened this issue Jan 28, 2017 · 7 comments
Closed

Clean up network address handling #390

mattklein123 opened this issue Jan 28, 2017 · 7 comments
Milestone

Comments

@mattklein123
Copy link
Member

This came to a head as part of: #377

The entire string based system we are currently using for addresses, along with utility functions in Network::Utility, is a mess. We should clean this up so it properly supports TCP vs. UDP, is an interface, etc.

This will probably also help us more easily support IPv6 in the future.

@jamessynge
Copy link
Contributor

Do you have a plan/design/sketch for how to represent addresses, addresses ranges, address families and protocols?

@mattklein123
Copy link
Member Author

My current thinking here is to do something along the lines of what follows.

Basically, have a root class for an address, generic methods where possible, then a type switch to allow retrieving more detailed info when the caller needs to work with address details. For example Ip would support port/address/etc. where as unix only exposes name. I think with this design we can support IPv6 quite easily, and also actually hide some system details so that if/when we port to NT, etc. it's easier. @jamessynge lmk what you think.

enum class AddressType { Ip, Unix };

class IpAddress {
...
};

class UdsAddress {
...
};

class Address {
  virtual AddressType type() PURE;

  // returns a value IFF type() == Ip
  virtual IpAddress* ipAddress() PURE;

  // returns a value IFF type() == Unix
  virtual UdsAddress* udsAddress() PURE;

  // returns a generic friendly name for logging, etc.
  const std::string& friendlyName() PURE;
};

@jamessynge
Copy link
Contributor

Regarding AddressType, I assume the point of having Unix is to support local communications ala a named pipe (but UDS). I notice a request for Windows support in #129, and wonder if a more generic name would work.

Regarding having the Address class be virtual, would your plan be to use a shared pImpl approach, to make it easy/low-cost to copy addresses around?

@mattklein123
Copy link
Member Author

Yes we currently support UDS addresses for client connections. From an interface perspective we can call this Pipe or something similar.

Yes, the idea would be to make copying/accessing low cost, so would expose as shared_ptr where necessary.

@jamessynge
Copy link
Contributor

Regarding a shared pImpl approach (rather than shared_ptr exposure), I was thinking of something like:

class Address {
 public:
...
 private:
  std::shared_ptr<AddressImpl> pImpl_;
};

This doesn't expose the shared_ptr, though it probably requires considering the AddressImpl to be immutable, with Address itself being modifiable via assignment (i.e. "overwrite" with a pointer to a new impl).

@jamessynge
Copy link
Contributor

Note that I'm very supportive of the goal of hiding the details of a specific address (IPv4, IPv6, UDS, Pipe), and not continuing to use strings. One complication of course (the leaky abstractions problem) is that some APIs might be Internet specific, so receiving a UDS or Pipe address will necessitate arg checks and status returns.

@mattklein123
Copy link
Member Author

re: the shared impl approach. That's fine. I don't have a strong opinion. I can take a first crack at getting the interfaces plumbed through and flesh out the details. In V1 it probably won't even require an embedded impl just to get parity with existing functionality.

re: Yes, I agree that ultimately there is no way around in some cases having code that does things based on type(), though I think that it is probably possible to limit this quite a bit.

Alright it seems like we are basically in agreement on approach. I can get this going soon.

mattklein123 added a commit that referenced this issue Feb 5, 2017
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 added a commit that referenced this issue Feb 8, 2017
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
rshriram pushed a commit to rshriram/envoy that referenced this issue Oct 30, 2018
PiotrSikora pushed a commit to istio/envoy that referenced this issue Feb 24, 2020
wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this issue Jan 23, 2021
jpsim pushed a commit that referenced this issue Nov 28, 2022
Description: android: java envoy pass through implementation
Risk Level: low
Testing: local

Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this issue Nov 29, 2022
Description: android: java envoy pass through implementation
Risk Level: low
Testing: local

Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants