-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 routing capabilities to tcp_proxy #377
Conversation
This patch allows listeners to: - be instantiated without binding to the specified port (config option bind_to_port) - have connections that have been redirected via iptables be handled by the listener associated with the original port rather than the listener that received the connection request. Use cases for these changes include: - Funnelling through the proxy connections for an entire range of destination port. If the proxy has a listener for the original destination port (before the redirect) it is used, otherwise the listener that received the connection handles it - Transparently proxy connections for an app listening on port X. The proxy can have an unbound listener for port X and a bound on for port Y. Connections originally directed to the app on port X are redirected with iptables to the proxy on port Y. The listener for port Y picks up the connection and hands it off to the listener for port X. Neither the app, nor the client, have to be changed or become aware of the presence of the proxy (except for the different src IP seen by the app). See https://docs.google.com/document/d/1v870Igrj5QS52G9O43fhxbV_S3mpvf_H6Hb8r85KZLY/ for the entire story
Regresion from envoyproxy#334. I'm in the process of rewriting all of the scope stuff currently so I'm not going to bother with tests. I will add tests in the real change.
… other review comments
"0.0.0.0/0" is a well-known subnet that contains all possible IPv4 addresses. Envoy whitelist code currently does not handle the "/0" case correctly. The code uses this formula to compute the subnet mask: white_list_entry.ipv4_mask_ = ~((1 << (32 - mask)) - 1); The LHS is an int32_t and the RHS is also computed as a 32-bit value. However, the (<< 32) operation is invalid for a 32-bit value (undefined behavior). The fix makes the RHS first compute as a 64-bits value that later gets truncated to 32-bits on assignment.
Allows the tcp_proxy filter to pick the destination cluster based on a combination of L4 connection parameters (source/destination IP address/port) See envoyproxy#345
@RomanDzhabarov can you take a first pass over this today. We would like to try to get this merged this week. |
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'll check tests in a bit, LGTM overall
Route | ||
===== | ||
|
||
A TCP proxy route consists of a set of optional L4 criteria and the name of a :ref:`cluster manager <arch_overview_cluster_manager>` to use for a connection if it mateches all the specified criteria. Routes are tried in the order specified until a match is found. If no match is found, the connection is closed. A route with no criteria is valid and always produces a match. |
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.
typos
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.
fixed
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 is another typo. "mateches" -> matches
source/common/filter/tcp_proxy.cc
Outdated
if (config.hasObject("cluster")) { | ||
cluster_name_ = config.getString("cluster"); | ||
} else { | ||
throw EnvoyException(fmt::format("tcp proxy: route without cluster")); |
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.
remove fmt::format
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.
done
source/common/filter/tcp_proxy.cc
Outdated
throw EnvoyException(fmt::format("tcp proxy: unknown cluster '{}'", cluster_name_)); | ||
: stats_(generateStats(config.getString("stat_prefix"), stats_store)) { | ||
if (!config.hasObject("route_config")) { | ||
throw EnvoyException(fmt::format("tcp proxy: missing route config")); |
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.
ditto
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.
done
@@ -130,6 +157,22 @@ class Utility { | |||
* @return true if the operation succeeded, false otherwise | |||
*/ | |||
static bool getOriginalDst(int fd, sockaddr_storage* orig_addr); | |||
|
|||
/** | |||
* Parses a string containing a comma-separated list of port numbers and/or |
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 you also add some example here in the comment
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.
done
"stat_prefix": "mongo_{{ key }}", | ||
"route_config": { | ||
"routes": [ | ||
{ |
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'd probably put here those optional config settings here as well, so one checking this template file can quickly get a sense of all options.
will leave up to you.
These configs are loaded as part of Envoy tests as well.
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.
ok, I had the impression that these templates were used for generating actual configs. If they are just loaded during Envoy tests I will add them so that the parsing logic gets exercised.
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 didn't find a good real-world example for this because in configgen.py the tcp_proxy instances and the clusters are coupled (since there used to be a 1:1 mapping). Let me think a bit more about this while you guys review the new version.
source/common/filter/tcp_proxy.cc
Outdated
conn_log_debug("Creating connection to cluster {}", read_callbacks_->connection(), | ||
cluster_name); | ||
} else { | ||
return Network::FilterStatus::StopIteration; |
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.
would be good to have a stat on this scenario
should we explicitly close connection here as well?
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.
Currently this can't happen, because you verify that all the clusters exist. If we eventually want to support CDS/RDS like constructs for tcp_proxy, we will need to deal with this, but for now I would not have this logic at all and just crash if cluster is nullptr.
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.
The clusters in the config always exist, but the downstream connection parameters may not match any of the programmed routes (if there's no default), in which case getClusterForConnection() will return "". I think Roman's concern is valid and we should close the connection in that case.
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 am also adding a counter for total number of connections handled by the filter and number of connections closed due to no route match
source/common/filter/tcp_proxy.h
Outdated
const std::string& clusterName() { return cluster_name_; } | ||
/** | ||
* Find out which cluster an upstream connection should be opened to. | ||
* @param connection supplies the parameters of the downstream connection for |
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.
upstream connection may be, can you fix the comment
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.
The comment is actually correct, but maybe not clear. I have clarified.
source/common/network/utility.cc
Outdated
for (const std::string& s : ranges) { | ||
uint32_t min = 0; | ||
uint32_t max = 0; | ||
char dash = 0; |
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.
super nit: i'd move it closer to usage scope
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.
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.
Some high level comments from a quick scan to get started on. Will look more tomorrow when I am fresh.
Route | ||
===== | ||
|
||
A TCP proxy route consists of a set of optional L4 criteria and the name of a :ref:`cluster manager <arch_overview_cluster_manager>` to use for a connection if it mateches all the specified criteria. Routes are tried in the order specified until a match is found. If no match is found, the connection is closed. A route with no criteria is valid and always produces a match. |
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.
"cluster", not "cluster manager" (and please link to cluster docs)
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.
also, nit, please try to wrap docs to approximately 100 cols, same in other file.
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.
done
include/envoy/network/connection.h
Outdated
@@ -116,6 +116,25 @@ class Connection : public Event::DeferredDeletable, public FilterManager { | |||
virtual const std::string& remoteAddress() PURE; | |||
|
|||
/** | |||
* @return The port number used by the remote client. | |||
*/ | |||
virtual uint32_t remotePort() PURE; |
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'm not thrilled with how remoteAddress() is currently implemented, and will clean that up at some point, but currently remoteAddress already contains the port if it is a TCP connection. E.g., tcp://1.2.3.4:8080. There may be no remote port if this is a UDS connection also. I would just delete this new function, and get the port out of remoteAddress if it is a TCP connection via the existing utility functions. Similarly, I would just add destinationAddress(), which should look like tcp://1.2.3.4:8080, and remove destinationPort(), so that we can also handle UDS, UDP, etc. connections in the future. I understand the strings are hack, but we will fix that later.
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.
done
source/common/filter/tcp_proxy.cc
Outdated
conn_log_debug("Creating connection to cluster {}", read_callbacks_->connection(), | ||
cluster_name); | ||
} else { | ||
return Network::FilterStatus::StopIteration; |
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.
Currently this can't happen, because you verify that all the clusters exist. If we eventually want to support CDS/RDS like constructs for tcp_proxy, we will need to deal with this, but for now I would not have this logic at all and just crash if cluster is nullptr.
source/common/filter/tcp_proxy.h
Outdated
static TcpProxyStats generateStats(const std::string& name, Stats::Store& store); | ||
|
||
std::string cluster_name_; | ||
std::list<Route> routes_; |
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 make this std::vector, a bit faster.
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.
done
@@ -395,9 +396,35 @@ void ConnectionImpl::updateWriteBufferStats(uint64_t num_written, uint64_t new_s | |||
buffer_stats_->write_current_); | |||
} | |||
|
|||
const std::string Network::ConnectionImpl::destinationAddress() { |
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.
per the previous comment on just having destinationAddress(), can we form the address in the constructor, and then just store it as a string like tcp://1.2.3.4:80 blah. Basically, this code can't assume that a connection is a TCP connection. It might not be.
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.
done
Route | ||
===== | ||
|
||
A TCP proxy route consists of a set of optional L4 criteria and the name of a :ref:`cluster manager <arch_overview_cluster_manager>` to use for a connection if it mateches all the specified criteria. Routes are tried in the order specified until a match is found. If no match is found, the connection is closed. A route with no criteria is valid and always produces a match. |
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 is another typo. "mateches" -> matches
*(required, string)* The :ref:`cluster manager <arch_overview_cluster_manager>` cluster to connect | ||
to when a new downstream network connection is received. | ||
:ref:`route_config <config_network_filters_tcp_proxy_route_config>` | ||
*(required, object)* The route table for the filter. All filter instances must have a route table, even if it is empty. |
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.
In the interest of brevity, I suggest merging the route_config with this document and linking to the _route doc from here as well.
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.
done
source/common/filter/tcp_proxy.h
Outdated
* @return the cluster name to be used for the upstream connection. | ||
If no route applies, returns the empty string. | ||
*/ | ||
const std::string& getClusterForConnection(Network::Connection& connection); |
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.
In an attempt to keep this consistent with HTTP conn manager, is it possible to atleast rename the function to getRouteFromEntries() ?
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.
ok
const TcpProxyStats& stats() { return stats_; } | ||
|
||
private: | ||
struct Route { |
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.
Move out of class, to make it extensible? similar to weighted clusters use case? [it can be done later as well. But a TODO might help keep track of this]. @mattklein123 WDYT? Weighted clusters for tcp could follow a similar implementation like HTTP.
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.
@rshriram I agree that ultimately we will want to make this more complicated, and probably use RDS, etc., but for now in the interest of time I think it's OK to keep this here since it's so simple. We can refactor it out in a future change.
@RomanDzhabarov @mattklein123 @rshriram Thanks for the review. I will incorporate the comments and re-push tomorrow morning. If something else comes to mind in the meanwhile feel free to add. |
@RomanDzhabarov @mattklein123 @rshriram uploaded new version. Thanks! |
Thanks i'll get another pass today. |
source/common/json/config_schemas.cc
Outdated
"type" : "string" | ||
} | ||
}, | ||
"source__ports": { |
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.
source_ports
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.
thanks. Besides the typo there was an error in the schema. I fixed it and verified that without the fix the typo would have been caught by a test.
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 took another quick look through and I only have one medium comment after thinking about it a bit more. Other than I will go through the rest tomorrow morning carefully but it looks good at a high level.
include/envoy/network/connection.h
Outdated
* It can be different from the proxy address if the downstream connection | ||
* has been redirected or the proxy is operating in transparent mode. | ||
*/ | ||
virtual const std::string destinationAddress() PURE; |
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.
In thinking about this more, this is still confusing as this is a base class for both client and server connections. Here is what I would do which shouldn't be too hard: rename this function to localAddress(). For server connections, local address would be the incoming address, and for client connections local address would be the origin address. I think this makes sense.
I will comment on some implementation stuff below.
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.
yes, that makes sense. The only issue is that the name becomes inaccurate when the connection is intercepted. It's probably ok since the comment above mentions that explicitly.
Anyway, I will also try to think of a better name.
} | ||
} | ||
|
||
return EMPTY_STRING; |
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.
Per my previous interface comment, can we actually add a member variable called local_address_, and populate in the constructor correctly. The way to do this would be to have ConnectionImpl() constructor take a local_address also as a param. This will plumb through quite nicely, since now you can populate the address directly in the listener.
NOTE: In the interest of time, I'm OK with it if it's currently the empty string for client connections, and even server connection that don't come in via getOrginalDst. We can add better support later for these things.
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.
The reason why I didn't do that was to avoid paying the cost of the getsockopt() system call if it is not needed. This would be the majority of the cases, since we only need that if we use L4 routing based on destination IP/port. If you think it might be useful for other purposes, we can do it in the constructor like you said and also have localAddress() return a const &.
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 see your point on the performance issue. To me this seems strange though since you could still call this on a client connection, a UDS connection, etc. I realize it would return empty string, but it still seems off. I think I would go with adding the member and returning const&. Even though we will pay for the extra getsockopt(), I don't really see CPS as a major performance benchmark currently. If it really becomes an issue, I think we could plumb it through such that you use the getsockopt before the listener redirect to populate it. But for now my vote would be to not worry about it and get the interface right.
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.
@mattklein123 I have incorporated the changes, PTAL. Thanks!
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.
@enricoschiattarella this is getting closer but realistically I'm not sure this is going to get merged today. Do you know who will be picking up this change if it doesn't get merged today? Can they take a look at it today just to have context?
include/envoy/network/connection.h
Outdated
@@ -116,6 +116,13 @@ class Connection : public Event::DeferredDeletable, public FilterManager { | |||
virtual const std::string& remoteAddress() PURE; | |||
|
|||
/** | |||
* @return The address the remote client is trying to connect to. |
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.
This comment is still confusing from an interface perspective and I think should read (this would also eventually allow us to use this for logging client connection outbound addresses for example):
"@return the local address of the connection. For client connections, this is the origin address. For server connections, this is the local destination address. For server connections it can be different from the proxy address if the downstream connection has been redirected or the proxy is operating in transparent mode.
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.
ok
source/common/filter/tcp_proxy.cc
Outdated
if (config.hasObject("cluster")) { | ||
cluster_name_ = config.getString("cluster"); | ||
} else { | ||
throw EnvoyException("tcp proxy: route without cluster"); |
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.
With schema support now, can this actually happen? I don't think it can, so should probably delete.
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.
right, will remove
source/common/filter/tcp_proxy.cc
Outdated
} | ||
|
||
if (!route.destination_port_ranges_.empty() && | ||
!Network::Utility::portInRangeList(Network::Utility::portFromUrl(connection.localAddress()), |
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.
if localAddress() is empty, this will throw an exception, so I think for now need to check if it's not empty.
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.
ok
|
||
if (!route.destination_ips_.empty() && | ||
!route.destination_ips_.contains( | ||
Network::Utility::hostFromUrl(connection.localAddress()))) { |
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.
if localAddress() is empty, this will throw an exception, so I think for now need to check if it's not empty.
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.
ok
source/common/filter/tcp_proxy.h
Outdated
* @param connection supplies the parameters of the downstream connection for | ||
* which the proxy needs to open the corresponding upstream. | ||
* @return the cluster name to be used for the upstream connection. | ||
If no route applies, returns the empty string. |
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.
nit: missing *
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.
ok
@@ -34,8 +35,9 @@ std::atomic<uint64_t> ConnectionImpl::next_global_id_; | |||
|
|||
ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, |
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.
this is still not quite what I had in mind. Per the interface comment, here is what I would do:
Make the ConnectionImpl constructor look like this:
ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address, const std::string& local_address);
This will force you to pre-populate the local address when creating the connection. For client connections, and redirect connections, just pass empty string for now, but in the future we can easily update to support all of those scenarios and also support UDS connections, etc.
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.
ok
*/ | ||
virtual void newConnection(int fd, const std::string& remote_address); | ||
virtual void newConnection(int fd, const std::string& remote_address, uint32_t remote_port); |
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 would not change the signature of this function. I would pass a fully resolved tcp://
:port string as remote_address, per other comments you might also pass local_address depending on how you implement.Also, important note, this PR is subtly changing remoteAddress() to return a tcp://<>:<> style string instead of just a raw string. I think this is correct, but that behavior is going to break the client SSL filter here so this code needs to be fixed:
https://github.com/lyft/envoy/blob/master/source/common/filter/auth/client_ssl.cc#L100
(Perhaps the IP list code should just be fixed to expect a tcp:// style string)
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 have fixed this and searched for all other calls to remoteAddress().
@mattklein123 New version pushed. I think this addresses all your comments. If it needs more iterations, I will get back to it on Monday. Thanks. |
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.
Enrico, this is great work, thanks. A few small things and then I think we are good to go.
include/envoy/network/connection.h
Outdated
@@ -111,11 +111,20 @@ class Connection : public Event::DeferredDeletable, public FilterManager { | |||
virtual bool readEnabled() PURE; | |||
|
|||
/** | |||
* @return The address of the remote client | |||
* @return The address of the remote client. | |||
* For TCP connections, it is in the form tcp://a.b.c.d:port |
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.
it might not be of the of the form tcp:// if it's a UDS connection. I would just delete that part of the comment for now. I opened #390 to clean this up and will get to this soon.
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.
done
@@ -38,10 +39,11 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea | |||
// peer. Cases where we don't "use remote address" include trusted double proxy where we expect | |||
// our peer to have already properly set XFF, etc. | |||
if (config.useRemoteAddress()) { | |||
if (Network::Utility::isLoopbackAddress(connection.remoteAddress().c_str())) { | |||
const std::string& remote_address = Network::Utility::hostFromUrl(connection.remoteAddress()); |
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.
hostFromUrl returns a temporary, so this should be std::string (not reference). Per above I realize this is terrible. I will fix this soon but for now let's just fix it up.
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.
done
|
||
bool success = Utility::getOriginalDst(fd, &orig_dst_addr); | ||
bool success = Utility::getOriginalDst(fd, &local_addr); |
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.
What does SO_ORIGINAL_DST return if it wasn't redirected and the success if false? Is is the true address? Or will it contain just an empty address?
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.
If the connection was not redirected, getOriginalDst will return the destination address that the client had specified.
The call will return false only when the getsockopt call itself fails.
That will only happen if the socket is not valid, not accepted or in some other error condition.
|
||
/** | ||
* Accept/process a new connection with the given remote address. | ||
* @param fd supplies the new connection's fd. | ||
* @param remote_address supplies the remote address for the new connection. | ||
* @param remote_address supplies the local address for the new connection. |
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.
param name wrong
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.
fixed
|
||
if (s.find('-') != std::string::npos) { | ||
char dash = 0; | ||
ss >> min; |
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.
Don't we need some error checking in here somewhere in case what is passed isn't all numbers, commas, and dashes in correct sequence?
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.
added
source/common/network/utility.h
Outdated
std::vector<Ipv4Entry> ipv4_list_; | ||
}; | ||
|
||
class IpWhiteList { |
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 just get rid of this class and just have IpList have a constructor that accepts JSON?
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.
done
source/common/network/utility.h
Outdated
bool contains(uint32_t port) const { return (port >= min_ && port <= max_); } | ||
|
||
private: | ||
uint32_t min_; |
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.
const here and next line
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.
done
test/common/filter/tcp_proxy_test.cc
Outdated
// The TcpProxyConfig constructor checks if the clusters mentioned in the route_config are valid. | ||
// We need to make sure to return a non-null pointer for each, otherwise the constructor will | ||
// throw an exception and fail. | ||
EXPECT_CALL(cm_, get("with_destination_ip_list")).WillRepeatedly(Return(cm_.cluster_.info_)); |
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.
CM mock already has:
ON_CALL(*this, get()).WillByDefault(Return(cluster.info_));
So I don't think any of these lines are necessary?
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.
removed
@@ -47,13 +74,235 @@ TEST(TcpProxyConfigTest, BadTcpProxyConfig) { | |||
Json::Exception); | |||
} | |||
|
|||
TEST(TcpProxyConfigTest, Routes) { |
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.
Do we have a test for the no route case? We should verify that connection gets closed, and stat gets incremented.
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.
added
Hi @mattklein123 PTAL. Thanks! |
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.
tiny nit, please fix then will merge.
source/common/filter/tcp_proxy.cc
Outdated
@@ -9,22 +9,96 @@ | |||
|
|||
#include "common/common/assert.h" | |||
#include "common/json/config_schemas.h" | |||
#include "common/common/empty_string.h" |
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.
nit: alpha order
* Added ConfigLoadingStatus to ApiManager interface, Simplified config loading error logs * Created LoadServiceRollouts * Removed unnecessary class variable config_loading_status_
…#377) Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
modify term file
Allows the tcp_proxy filter to pick the destination cluster based on a combination of L4 connection parameters (source/destination IP address/port)
See #345 for related discussion.
WARNING: This is a breaking change! Old tcp_proxy configs will be rejected.