proxy: use original dst if authority doesnt look like local service #397

seanmonstar · 2018-02-20T21:12:59Z

The proxy will check that the requested authority looks like a local service, and if it doesn't, it will no longer ask the Destination service about the request, instead just using the SO_ORIGINAL_DST, enabling egress naturally.

The rules used to determine if it looks like a local service come from this comment:

If default_zone.is_none() and the name is in the form $a.$b.svc, or if !default_zone.is_none() and the name is in the form $a.$b.svc.$default_zone, for some a and some b, then use the Destination service. Otherwise, use the IP given.

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar · 2018-02-20T21:16:47Z

proxy/src/fully_qualified_authority.rs

+                &input, default_namespace, default_zone));
+        }
+
+        none("name", None, None);


The tests show the effects, but I'll group them up here. These are all considered external names:

name (no namespace, svc, or zone)

name.namespace.svc.cluster (zone doesn't match cluster.local)

name.namespace.foo (not svc)

seanmonstar · 2018-02-20T21:20:44Z

proxy/src/outbound.rs

+    fn poll(&mut self) -> Poll<Change<Self::Key, Self::Service>, Self::DiscoverError> {
+        match *self {
+            Discovery::LocalSvc(ref mut w) => w.poll(),
+            Discovery::External(ref mut opt) => {


A side effect here that an external service will get inserted into the balancer, and then probably never removed. It seems like once we have some circuit breaking, this would take care of itself when/if the service closes, and outbound never requests this address again...

Please add your GitHub comment here as a TODO comment in the source code.

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

olix0r

the approach generally looks good to me! we should get more eyes on this before merging, but nothing obvious stands out t me.

olix0r · 2018-02-20T21:31:24Z

proxy/src/fully_qualified_authority.rs

@@ -15,10 +15,10 @@ impl FullyQualifiedAuthority {
    /// This assumes the authority is syntactically valid.
    pub fn new(authority: &Authority, default_namespace: Option<&str>,


nit but maybe clearer to either rename or make it a module-fn? Seems surprising for new to return an option.

olix0r · 2018-02-20T21:33:19Z

proxy/src/outbound.rs

+                .and_then(|ctx| {
+                    ctx.orig_dst_if_not_local()
+                });
+            Destination::External(orig_dst?)


whoa! i guess this option-return-sugar must have landed.

Yep! Several months ago, even: https://blog.rust-lang.org/2017/11/22/Rust-1.22.html

briansmith · 2018-02-20T21:30:09Z

proxy/tests/support/proxy.rs

@@ -111,6 +111,10 @@ fn run(proxy: Proxy) -> Listening {
    env.put(config::ENV_PUBLIC_LISTENER, "tcp://127.0.0.1:0".to_owned());
    env.put(config::ENV_CONTROL_LISTENER, "tcp://127.0.0.1:0".to_owned());

+    env.put(config::ENV_POD_NAMESPACE, "conduit".to_owned());


In general the pod namespace won't be "conduit". I suggest we use a different one like "foo". We should only use namespace = "conduit" for the case where we're specifically testing how the proxy works when it is proxying stuff in the "conduit" namespace (e.g., how the proxy proxies the conduit control plane). I suggest using "arbitrary" or similar instead.

As mentioned in a comment further down, this was done just to support the existing tests in proxy/tests/*.rs. I'll change this test and cluster.local.

briansmith · 2018-02-20T21:31:40Z

proxy/src/fully_qualified_authority.rs

@@ -15,10 +15,10 @@ impl FullyQualifiedAuthority {
    /// This assumes the authority is syntactically valid.


Update the documentation to mention what it means for None to be returned.

briansmith · 2018-02-20T21:36:37Z

proxy/src/fully_qualified_authority.rs

+                    } else {
+                        // if "a.b.svc.foo" and zone is not "foo",
+                        // treat as external
+                        return None;


Use the early return style:

// if "a.b.svc.foo" and zone is not "foo" treat as external if !zone.eq_ignore_ascii_case(default_zone) { return None; }

briansmith · 2018-02-20T21:39:37Z

proxy/src/fully_qualified_authority.rs

        }
-        let mut parts = name.split('.');
+
+        // parts = (name, namespace, svc, zone)


Nice, I didn't know about splitn().

Let's remove this comment because it isn't necessarily the case that parts is of the form (name, namespace, svc, zone). The rest of the function spends most of its effort finding out whether or not that is true.

briansmith · 2018-02-20T21:43:58Z

proxy/src/fully_qualified_authority.rs

@@ -60,11 +62,27 @@ impl FullyQualifiedAuthority {
            (has_namespace, has_namespace)
        };

+


To simplify the control flow below, use early return style above:

// if not a "*.svc", treat as external if !part.eq_ignore_ascii_case("svc") { return None; }

In particular, we no longer need has_svc.

briansmith · 2018-02-20T22:06:55Z

proxy/src/outbound.rs

+        let dest = if let Some(local) = local {
+            Destination::LocalSvc(local)
+        } else {
+            let orig_dst = req.extensions()


Why do we look up orig_dst_if_not_local() here instead of in bind_service() below? Regardless of where we do it, let's add a comment explaining why we do it where we do it.

We already know at this moment whether the resolution can succeed. Rather, we know at this moment whether it's possible for this request to succeed at all, or if we definitely cannot recognize it. It seems like the error is a recognize error, not a bind error.

briansmith · 2018-02-20T22:08:48Z

proxy/src/outbound.rs

+                ))
+            },
+            Destination::External(addr) => {
+                Discovery::External(Some((addr, self.bind.clone().with_protocol(protocol))))


Extending my previous comment, if we use the Destination service then we resolve a name to an IP address here, but if we use don't then we did the resolution earlier. This asymmetry is surprising.

briansmith · 2018-02-20T22:10:39Z

proxy/src/outbound.rs

+                .and_then(|ctx| {
+                    ctx.orig_dst_if_not_local()
+                });
+            Destination::External(orig_dst?)


It is wrong to fail here. If we can't avoid failing here in this PR then let's add a TODO here acknowledging the problem. (Presumably this problem will be fixed when we implement DNS lookups.)

I don't see why it is wrong to fail here. If we have nothing that will allow us to route the request (recognize, as this method is called), then there's nothing to be done but to return an error.

This is also what is done in Inbound.

It's wrong to fail here because, without the proxy, the request wouldn't have failed. In practice, we expect this kind of failure to never occur because we expect the proxy to only run on Linux and we expect SO_ORIGINAL_DST to work. Also, in the future we'll be able to make it work by doing a DNS lookup to get the address.

briansmith · 2018-02-20T22:11:20Z

proxy/src/outbound.rs

+    fn poll(&mut self) -> Poll<Change<Self::Key, Self::Service>, Self::DiscoverError> {
+        match *self {
+            Discovery::LocalSvc(ref mut w) => w.poll(),
+            Discovery::External(ref mut opt) => {


Please add your GitHub comment here as a TODO comment in the source code.

briansmith · 2018-02-20T22:12:38Z

proxy/tests/support/proxy.rs

@@ -111,6 +111,10 @@ fn run(proxy: Proxy) -> Listening {
    env.put(config::ENV_PUBLIC_LISTENER, "tcp://127.0.0.1:0".to_owned());
    env.put(config::ENV_CONTROL_LISTENER, "tcp://127.0.0.1:0".to_owned());

+    env.put(config::ENV_POD_NAMESPACE, "conduit".to_owned());
+    env.put(config::ENV_POD_ZONE, "local".to_owned());


Did you mean for this to be "cluster.local", which is what we normally expect things to be? If not, then it would be useful to add a comment about why you chose "local" specifically. If you just want an arbitrary value then use "example" instead.

I actually just chose those values since all the existing tests in proxy/tests use the authority test.conduit.local.

OK, those tests are using "local" to make sure we don't match just "local" instead of "cluster.local". If they're using "conduit" then that's a mistake on my part.

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar · 2018-02-21T00:53:48Z

Review comments should be addressed. Do the exact rules of is a local vs external service seem correct?

briansmith

LGTM as long as the behavior of whether we coalesce HTTP/1 requests with different Host header field values isn't different than before. Please address the nits before merging.

briansmith · 2018-02-21T00:34:15Z

proxy/src/fully_qualified_authority.rs

+            if !part.eq_ignore_ascii_case("svc") {
+                // if not "$name.$namespace.svc", treat as external
+                return None;
+            } else {


Avoid else after return.

briansmith · 2018-02-21T00:44:02Z

proxy/src/fully_qualified_authority.rs

+        } else if has_explicit_namespace {
+            true
+        } else if namespace_to_append.is_none() {
+            // if not "*.svc" and no default namespace, treat as external


To be precise the comment would have to be If not "*.svc" and there's no namespace, treat as external. However, that's just duplicating the logic in the if {} else {}. I suggest the comment be // We can't append ".svc" without a namespace, so treat as external.

briansmith · 2018-02-21T00:50:48Z

proxy/src/outbound.rs

+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+pub enum Destination {
+    LocalSvc(FullyQualifiedAuthority),
+    External(SocketAddr),


How do we know the server is HTTP/1.1 and not HTTP/1.0? We would have had to have previously received a HTTP/1.1 response from the server to know that; we can't know it from looking at the request.

Although I mostly agree with you w.r.t. what was intended in HTTP/1.1, many HTTP/1.1 servers don't properly respect Host: beyond the first request, which is why browsers don't (IIRC) send requests for different Hosts on the same connection. This is why, in particular, browsers don't do connection coalescing for HTTP/1.1, but only for HTTP/2.

Regardless, you don't have to answer those questions here. I just want to point out that it is important to not regress in this respect. As long as you're sure that the behavior isn't changed from before, we can deal with it in another issue.

briansmith · 2018-02-21T00:56:38Z

proxy/src/outbound.rs

+                .and_then(|ctx| {
+                    ctx.orig_dst_if_not_local()
+                });
+            Destination::External(orig_dst?)


It's wrong to fail here because, without the proxy, the request wouldn't have failed. In practice, we expect this kind of failure to never occur because we expect the proxy to only run on Linux and we expect SO_ORIGINAL_DST to work. Also, in the future we'll be able to make it work by doing a DNS lookup to get the address.

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

…inkerd#397) The proxy will check that the requested authority looks like a local service, and if it doesn't, it will no longer ask the Destination service about the request, instead just using the SO_ORIGINAL_DST, enabling egress naturally. The rules used to determine if it looks like a local service come from this comment: > If default_zone.is_none() and the name is in the form $a.$b.svc, or if !default_zone.is_none() and the name is in the form $a.$b.svc.$default_zone, for some a and some b, then use the Destination service. Otherwise, use the IP given.

olix0r added the review/ready Issue has a reviewable PR label Feb 20, 2018

proxy: use original dst if authority doesnt look like local service

6b0cbac

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar force-pushed the proxy-egress branch from 3937af1 to 6b0cbac Compare February 20, 2018 21:13

seanmonstar commented Feb 20, 2018

View reviewed changes

olix0r requested review from briansmith and olix0r February 20, 2018 21:18

seanmonstar requested review from adleong and removed request for olix0r February 20, 2018 21:18

seanmonstar commented Feb 20, 2018

View reviewed changes

seanmonstar requested a review from olix0r February 20, 2018 21:21

seanmonstar mentioned this pull request Feb 20, 2018

controller: resolve external services with public DNS #360

Closed

fix telemetry test with test namespace

4bd2398

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar force-pushed the proxy-egress branch from db0caab to 4bd2398 Compare February 20, 2018 21:24

olix0r approved these changes Feb 20, 2018

View reviewed changes

olix0r added this to the Conduit 0.3 milestone Feb 20, 2018

briansmith suggested changes Feb 20, 2018

View reviewed changes

some review cleanup

25c08fb

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

briansmith approved these changes Feb 21, 2018

View reviewed changes

briansmith mentioned this pull request Feb 21, 2018

Destinations service doesn't discover services outside the local cluster #155

Closed

more nits

6370560

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar merged commit 236f71f into master Feb 21, 2018

seanmonstar removed the review/ready Issue has a reviewable PR label Feb 21, 2018

seanmonstar deleted the proxy-egress branch February 21, 2018 02:09

seanmonstar mentioned this pull request Feb 21, 2018

The proxy shouldn't ask the controller to resolve IP addresses, especially loopback addresses 127.0.0.1 and ::1 #357

Closed

This was referenced Feb 21, 2018

Proxy sets "Transfer-Encoding: chunked" when proxying GET request #402

Closed

Proxy hangs indefinitely until the Destination service returns a result #403

Closed

briansmith mentioned this pull request Feb 22, 2018

How are requests with differing Host: header field values mapped to outbound connections? #415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxy: use original dst if authority doesnt look like local service #397

proxy: use original dst if authority doesnt look like local service #397

seanmonstar commented Feb 20, 2018

seanmonstar Feb 20, 2018

seanmonstar Feb 20, 2018

briansmith Feb 20, 2018

olix0r left a comment

olix0r Feb 20, 2018

olix0r Feb 20, 2018

seanmonstar Feb 20, 2018

briansmith Feb 20, 2018

seanmonstar Feb 21, 2018

briansmith Feb 20, 2018

briansmith Feb 20, 2018

briansmith Feb 20, 2018

briansmith Feb 20, 2018

briansmith Feb 20, 2018 •

edited

Loading

seanmonstar Feb 21, 2018

briansmith Feb 20, 2018

briansmith Feb 20, 2018

seanmonstar Feb 21, 2018

briansmith Feb 21, 2018

briansmith Feb 20, 2018

briansmith Feb 20, 2018

seanmonstar Feb 20, 2018

briansmith Feb 20, 2018

seanmonstar commented Feb 21, 2018

briansmith left a comment

briansmith Feb 21, 2018

briansmith Feb 21, 2018

briansmith Feb 21, 2018

briansmith Feb 21, 2018

		@@ -15,10 +15,10 @@ impl FullyQualifiedAuthority {
		/// This assumes the authority is syntactically valid.
		pub fn new(authority: &Authority, default_namespace: Option<&str>,

		@@ -60,11 +62,27 @@ impl FullyQualifiedAuthority {
		(has_namespace, has_namespace)
		};

proxy: use original dst if authority doesnt look like local service #397

proxy: use original dst if authority doesnt look like local service #397

Conversation

seanmonstar commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r left a 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

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

Choose a reason for hiding this comment

briansmith Feb 20, 2018 • edited Loading

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

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

seanmonstar commented Feb 21, 2018

briansmith left a 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

Choose a reason for hiding this comment

briansmith Feb 20, 2018 •

edited

Loading