ORIGINAL_DST for upstream IP only, with custom port

[howardjohn]

Title: ORIGINAL_DST for upstream IP only, with custom port

Description: It would be great to allow an ORIGINAL_DST cluster that only takes the original IP address, not the port. Example use cases:

• In a mesh, I have a cluster for google.com. I want ORIGINAL_DST, not STRICT_DNS, because doing DNS lookup 2x (client and envoy) can add some overhead. However, I want the client to send requests to port 80 in plaintext, and have Envoy add TLS and direct to port 443.

Strawman API proposal: In OriginalDstLbConfig, add upstream_port_override int. Usage like

original_dst_lb_config:
  upstream_port_override: 443

I could imagine more complex use cases (ex map 80->443, but port 8080->8080), but I don't think that level of complexity is needed, as it can be solved with two clusters, unlike the above use case.

cc @stono @lambdai

[lambdai]

I can understand this is useful regarding the http port 80 and 443. But it doesn' make sense for tcp modulo http.

Ideally, the destination port is determined by the application layer, e.g. a http filter rewrite the original dest including ip and port.

I am think about a powerful http original dst to support X-forwarded-Port/X-Forwarded-Host.

What do you think? @howardjohn

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

[howardjohn]

I think this is still useful. Also I do think TCP makes sense, this is protocol agnostic. Its basically just a rewrite of port... we can do it for all cluster types except ORIGINAL_DST

[kfaseela]

@howardjohn shall we reopen the issue, to continue further discussion? @phlax would it be possible to get some help in triaging or assigning someone for the issue?

[phlax]

i can reopen and mark as help wanted

wrt assigning the issue - that would need to be someone who wants to take it on

it also has to be agreed that the change is desirable, and this is not really within my area/remit

[kfaseela]

i can reopen and mark as help wanted

wrt assigning the issue - that would need to be someone who wants to take it on

it also has to be agreed that the change is desirable, and this is not really within my area/remit

thanks @phlax

[lambdai]

Would below extended field be good? There might be security concern but it's no worse than the existing use_http_header CC @htuch

 message OriginalDstLbConfig {
    option (udpa.annotations.versioning).previous_message_type =
        "envoy.api.v2.Cluster.OriginalDstLbConfig";
    bool use_http_header = 1;

+   message ValueOverride {
+      oneof {
+         string value = 1;
+         // The value is recovered from this downstream http header name in the lbcontext.
+         string header_name = 2;
+         // potential: from filter_state path.
+       }
+   }
+
+    ValueOverride ip = 2;
+    ValueOverride port= 3;
  }

[howardjohn]

That would meet our needs for sure. We would set:

OriginalDstLbConfig {
  port: { value: 443 }
}

If it was not clear, we do not personally need header_name, although I certainly don't mind it being added

Will leave to others to answer if its good for other contexts :-)

[htuch]

I would suggest only including the support you need for your use case right now, but leave it in a oneof if you do anticipate future additions.

[lambdai]

@htuch

• fixed port value This is an optimization which saves a few clusters in istio as described as the initial post.

• ip from header and port from header This is used to terminate http CONNECT request. The current HCM route can only select cluster as the upstream. I am expecting the cluster is OriginalDst cluster.

In my proposed Config, the below CONNECT request

:method : CONNECT
preselected-ip-header-name: "10.11.12.13"
preselected-port-header-name: "80"
:authority: a-reserved-host-to-match-envoy-route

Can be routed to cluster

type: original_dst
original_dst_lb_config: { ip: "preselected-ip-header-name", port: "preselected-port-header-name"}

This actually achieves what CONNECT is invented for.

I believe there is questions

1. Why not consume from :authority in CONNECT? In our design, CONNECT can and usually wrap a HTTP request and route matching a hopreselected-ip-header-name

2. Why not relay the original address via 'x-envoy-original-dst-host"? We need to transmit the ip and port on the wire which mess up the existing settings to strip "x-envoy-" headers.

[htuch]

Shouldn't CONNECT be able to replace wholesale the original dst information? I'm not sure I follow what you have in (1). In any case, I think it would be good for Istio folks to achieve consensus on the preferred option.

[lambdai]

@htuch re: Shouldn't CONNECT be able to replace wholesale the original dst information? connect method can support host as www.example.com:443, where dns need to be involved. If we concede the original dst should cover the format of domain name, a dns resolver need to introduced in original_dst. Part of me think that's beyond what my case needs because my case guarantees ip formart. On other hands, I'd like to to see a new setting in original_dst cluster that consuming :authority.

re: (1) part of the design is using a connect request to carry a single HTTP GET request rather than a tcp stream. In the latter case, it would be great to override the "host" header of CONNECT by the "host" of GET request. That's is more friendly to route lookup. I will revisit the design to evaluate the benefits at the cost of anti general CONNECT pattern.

[kyessenov]

The extended form of CONNECT that aims to support UDP now uses URL templates in the 8th draft, e.g. https://masque.example.org/.well-known/masque/udp/{target_host}/{target_port}/.

The authority is the proxy authority, and the original destination is templatized in the path. Given that Envoy lacks an ability to parse arbitrary path templates, there is no simple way to standardize this in Envoy yet, so it should be reserved to a custom filter.

[lambdai]

Talked to @kyessenov and @howardjohn We are happy to follow the convention of :authority.

So we adding two independent configurations in original_dst, in separate PRs.

Does the blow make sense? @htuch

 message OriginalDstLbConfig {
    bool use_http_header = 1; 
// feature 1, for downstream HTTP connect request.
+  oneof { // not required. Use X-envoy-original-dst in lbcontext
+       // use ":authority" instead of X-envoy-original-dst
+       // If the authority is not a valid "ip:port", a nullptr will be returned and fail the upstream request.            
+       bool use_http_authority = 1; 
+    } 

// feature 2
+   message ValueOverride {
+      oneof {
+         int16_t i16_value = 1;
+       }
+   }
+
+    ValueOverride port = 3;  
  }

[htuch]

SGTM. One thing we should be aware of though is that at some point this degenerates into just regular dynamic forward proxy. How far are we from that?

[lambdai]

There are a few unknowns chaining with dynamic forward proxy. DFP cluster might work if we make similar changes there and unify DFP and original_dst.

Are you aware of a discussion to replace original_dst by DFP? Should I create one?

[htuch]

I'm not familiar with this, but it seems like a place there might be reasonable convergence when we're using CONNECT as the basis for overriding. Vanilla original_dst is different enough I can imagine never unifying, it really depends how far you will go with the CONNECT stuff here.

[kyessenov]

There is a variety of dynamic cases where the destination IP:PORT is derived from a request header or a filter state, and when they combine with overrides, it's hard to predict the effects. I wonder if we have enough weight to add a first class API here as opposed to a custom extension that can decide how to "resolve" a remote address based on some custom logic. Maybe the right thing to do is to define what these "resolvers" are as an extension class so that they can be shared with DFP.

[howardjohn]

Fixed by https://github.com/envoyproxy/envoy/pull/23040

[guybrushthre]

It seems that overriding the ORIGINAL_DST upstream port still creates 2 upstream connections, even if they both resolve to the same address post-override. e.g. if I have two connections, A with original_dst = 1.2.3.4:555 and B with original_dst = 1.2.3.4:999, and they both have a upstream_port_override = 111, Envoy will still create 2 separate upstream connections despite the destination resolving to the same host/port combo.

This can be observed on HTTP/2 CONNECT tunnels where multiplexing should have merged these into a single connection.