Mirror DestinationRule connection pool configuration on Sidecar #2961

ZackButcher · 2023-10-11T20:38:18Z

Revives #1754 to support the same ConnectionPoolSettings we use for DestinationRule on the inbound cluster via configuring the Sidecar.

Today developers can configure the inbound cluster's connection pool, via DestinationRule -- Istiod programs both the client and server to use the same connection pool settings. However, when multiple services listen on the same port it can be challenging (impossible?) for a user to ensure the correct DestinationRule is applying connection pooling settings on the server side. This API allows the user to set their inbound connection pool explicitly (and differently than the DestinationRule they configure for callers) both at the top level of the sidecar as well as per-port.

There's a paired implementation PR in istio/istio that I'll link shortly. In that PR, I've implemented it so that the connection pool is configured for the server-side sidecar with the following precedence (highest to lowest):

per-port ConnectionPool setting from Sidecar
top-level InboundConnectionPool setting from Sidecar
per-port ConnectionPool setting from DestinationRule
top-level TrafficPool.ConnectionPool setting from DestinationRule

Client-side connection pools are unchanged, where per-port then top-level configuration from the DestinationRule still apply.

…ound connections to Envoy sidecars

istio-policy-bot · 2023-10-11T20:38:21Z

😊 Welcome @ZackButcher! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

ZackButcher · 2023-10-11T20:39:46Z

I'll get release notes and all the other bits in order once we're agreed on the bigger stuff like the field names, behavior of the API, and the documentation on it.

hzxuzhonghu · 2023-10-12T07:22:56Z

@ZackButcher It can not express per port connection pool

ZackButcher · 2023-10-12T12:10:25Z

What the DestinationRule does today, which I do in the implementation PR, is configure the connection pool on the inbound cluster that's generated for the pod (e.g. the Cluster inbound|8080||). We create one cluster like this per port for the service, so we can apply the settings per port. See istio/istio#47313 and its test cases.

Again, this is exactly how the per-port connection pool settings configured by a DestinationRule work for inbound traffic, just made explicitly configurable via Sidecar in addition to inheriting from DestinationRule (which was not documented anywhere AFAIK).

howardjohn · 2023-10-12T14:13:31Z

networking/v1alpha3/sidecar.proto

+  // This setting overrides the top-level default `inboundConnectionPool` to configure
+  // specific settings for this port. This configuration mirrors the `DestinationRule`'s
+  // [`PortTrafficPolicy.connectionPool`](https://istio.io/latest/docs/reference/config/networking/destination-rule/#TrafficPolicy-PortTrafficPolicy) field.
+  ConnectionPoolSettings connection_pool = 8;


Interesting, didn't consider we would want it at 2 levels. Part of me thinks we could do

ingress: - connectionPool: ...

(no port)

for this case. But I think that is probably too complex.

An ingress without a port is not allowed by validation today, hence the top level vs the per-port level.

networking/v1alpha3/sidecar.proto

… destinationrule. Add a release note for the new field.

ZackButcher · 2023-10-12T18:41:46Z

@howardjohn PTAL at the updated comments

ZackButcher · 2023-10-12T18:58:17Z

FWIW there's an argument we could support more than just ConnectionPoolSettings at the sidecar; we could potentially support the full TrafficPolicy from DestinationRule (though not all fields there make sense).

One recurring ask of the project has been to enable the sidecar to terminate simple TLS from the app (effectively man-in-the-middle'ing the traffic) before re-encrypting with the mesh's mTLS. This would allow services that do TLS (e.g. HTTPS) internally to participate in the mesh at L7 without code changes to force the app to speak HTTP. In theory this could be achieved with TrafficPolicy.TLS settings on the egress stanza. e.g. for in-mesh traffic, app request > HTTPS > sidecar terminates with Sidecar.Egress.TLS data > sidecar does mesh mTLS to destination.
The opposite side of this is allowing apps that serves HTTPS to receive HTTP traffic from the mesh by having the sidecar initiate (and terminate) an HTTPS connection inside the pod. We could use the TrafficPolicy.TLS settings in the Ingress stanza to define this sidecar>app behavior as well. e.g. client request via mesh mTLS > sidecar > HTTPS via Sidecar.Ingress.TLS data > app.

Of course, some fields (like load balancing settings) wouldn't make sense in the sidecar inbound context. So then the whole discussion of do you use the same TrafficPolicy in DestRule and Sidecar or use a slightly different one in each, and so on.

As @keithmattix pointed out, seems to be a lot of similar DestinationRule-ish behavior to look at for sidecar inbound path.

linsun · 2023-10-12T20:55:24Z

networking/v1alpha3/sidecar.proto

+//       http:
+//         http1MaxPendingRequests: 1024
+//         http2MaxRequests: 1024
+//         maxRequestsPerConnection: 1024
+//         maxRetries: 100


Would be good to show overrides here?

Or is it http connection pool is the same but user still has to list it here due to tcp setting is different?

Correct, I repeated mainly to highlight that the user has to repeat list because the settings are not merged.

The UX is probably improved if we merge the fields, but today for DestinationRule we do not merge at that level: we either replace the entire ConnectionPoolSetting or not but do not look inside it, and I'm trying to keep the behavior as close to DestinationRule as possible (principle of least surprise).

linsun · 2023-10-12T20:57:16Z

networking/v1alpha3/sidecar.proto

@@ -576,6 +679,15 @@ message IstioIngressListener {
  // sidecar for requests originating from outside the mesh.
  // Currently supports only SIMPLE and MUTUAL TLS modes.
  ServerTLSSettings tls = 7;
+
+  // Settings controlling the volume of connections Envoy will accept from the network.
+  // This setting overrides the top-level default `inboundConnectionPool` to configure


Would be good to clarify if only overrides need to be specified (if we can merge with the top-level or not) or the entire connection pool config?

We should probably have the conversation on merging: I think it would likely improve UX, but today we do not do it for DestinationRule. Here's the code that merges per-Port and default DestinationRule config. Merging wouldn't be super hard to do, but it would be a behavioral change compared to what we do today. In any case, whatever we do w.r.t. merging here I think we need to match in DestinationRule merging its top-level and per-port settings, or the API gets harder for users to understand.

Prefer not to merge personally. Doesn't seem terribly compelling and adds a lot of complexity

+1, reduce merging

hzxuzhonghu · 2023-10-13T02:59:20Z

networking/v1alpha3/sidecar.proto

+
+  // Settings controlling the volume of connections Envoy will accept from the network.
+  // This setting overrides the top-level default `inboundConnectionPool` to configure
+  // specific settings for this port. This configuration mirrors the `DestinationRule`'s


This seems redundant comment as the ConnectionPoolSettings scheme is actually same one

Exactly -- I want to call that out explicitly for folks so they don't have to think as hard about it (rather than having them realize because the page links to the same place). Happy to cut this if we want to keep the docs more concise.

ZackButcher · 2023-10-13T15:10:47Z

So, take away is no merging inside of ConnectionPoolSetting in this PR, which I agree with. If we want to take up that issue (and more generally, merging fields inside of TrafficPolicy in DestinationRule as well, to keep behavior consistent) that can happen in another issue/PR/discussion.

With that in mind, are there updates to the examples/docs that we should do to make it more explicitly than I already have that merging is not happening?

…onfigured via the Sidecar API (see istio/api#2961). This PR implements the following precedence for picking connection pool config (highest to lowest): 1) per-port ConnectionPool from the Sidecar; 2) top-level InboundConnectionPool from the Sidecar; 3) per-port TrafficPolicy.ConnectionPool from the DestinationRule; 4) top-level TrafficPolicy.ConnectionPool from the DestinationRule. (3 and 4 are unchanged from before this PR.) This PR does not change how client-side connection pools are configured (which remains DestinationRule only).

…ters via the Sidecar API (#47313) * update API to HEAD to get new connection pool fields -- note we'll need to remove the replace directive before merge * Implement support for ConnectionPoolSettings on the inbound cluster configured via the Sidecar API (see istio/api#2961). This PR implements the following precedence for picking connection pool config (highest to lowest): 1) per-port ConnectionPool from the Sidecar; 2) top-level InboundConnectionPool from the Sidecar; 3) per-port TrafficPolicy.ConnectionPool from the DestinationRule; 4) top-level TrafficPolicy.ConnectionPool from the DestinationRule. (3 and 4 are unchanged from before this PR.) This PR does not change how client-side connection pools are configured (which remains DestinationRule only). * point to latest revision of the istio/api change * remove one useless test case, and format a few things * simplify if statement logic * remove the replace directive and update to the 1.20 release of the API repo; restore test file I messed up during rebase * Add release notes that mirror those from istio/api; fix linter error * roll back go mod changes as they weren't needed anyway -- yay caches 🙃 * include port name+number in the warning about HTTP settings on TCP ports

dasch · 2024-03-20T14:31:06Z

Has there also been discussions about doing this for egress? In particular, we have applications that want to limit their exposure to specific dependency services, and e.g. setting up concurrency circuit breakers on the source side would be beneficial. But the configuration would be specific to the source application (e.g. the level of concurrency), and so it would make sense to stick it in the Sidecar config for it.

howardjohn · 2024-03-25T17:42:16Z

Has there also been discussions about doing this for egress? In particular, we have applications that want to limit their exposure to specific dependency services, and e.g. setting up concurrency circuit breakers on the source side would be beneficial. But the configuration would be specific to the source application (e.g. the level of concurrency), and so it would make sense to stick it in the Sidecar config for it.

The workloadSelector on DR can limit the source side.

Better docs around this would help, a lot

dasch · 2024-04-03T12:37:29Z

Has there also been discussions about doing this for egress? In particular, we have applications that want to limit their exposure to specific dependency services, and e.g. setting up concurrency circuit breakers on the source side would be beneficial. But the configuration would be specific to the source application (e.g. the level of concurrency), and so it would make sense to stick it in the Sidecar config for it.

The workloadSelector on DR can limit the source side.

Better docs around this would help, a lot

But that wouldn't help source service owners if they also want to retain the destination service's DR configuration, except for the specific circuit breakers that are necessary on the source side, right? Ideally there should be something similar to this, where source owners can have specific overrides for the stuff they care about.

Add support for default and per-port connection pool settings for inb…

14b7abb

…ound connections to Envoy sidecars

ZackButcher requested a review from a team as a code owner October 11, 2023 20:38

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 11, 2023

ZackButcher requested a review from howardjohn October 11, 2023 20:38

ZackButcher mentioned this pull request Oct 11, 2023

Implement support for configuring the connection pool of inbound clusters via the Sidecar API istio/istio#47313

Merged

4 tasks

howardjohn approved these changes Oct 12, 2023

View reviewed changes

flesh out comments on connection pool and describe how they relate to…

db16adb

… destinationrule. Add a release note for the new field.

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 12, 2023

missed comments in v1beta1 that should've been in v1alpha1

9a8dc6f

keithmattix mentioned this pull request Oct 12, 2023

Settings for http2 initial stream/connection window sizes istio/istio#34169

Open

howardjohn approved these changes Oct 12, 2023

View reviewed changes

linsun reviewed Oct 12, 2023

View reviewed changes

hzxuzhonghu reviewed Oct 13, 2023

View reviewed changes

hzxuzhonghu approved these changes Oct 16, 2023

View reviewed changes

istio-testing merged commit 6d61c89 into istio:master Oct 16, 2023
5 checks passed

ramaraochavali mentioned this pull request Oct 18, 2023

Destination rules only apply to inbound istio/istio#47416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mirror DestinationRule connection pool configuration on Sidecar #2961

Mirror DestinationRule connection pool configuration on Sidecar #2961

ZackButcher commented Oct 11, 2023 •

edited

Loading

istio-policy-bot commented Oct 11, 2023

ZackButcher commented Oct 11, 2023

hzxuzhonghu commented Oct 12, 2023

ZackButcher commented Oct 12, 2023 •

edited

Loading

howardjohn Oct 12, 2023

ZackButcher Oct 12, 2023

ZackButcher commented Oct 12, 2023

ZackButcher commented Oct 12, 2023 •

edited

Loading

linsun Oct 12, 2023

ZackButcher Oct 12, 2023

linsun Oct 12, 2023 •

edited

Loading

ZackButcher Oct 12, 2023 •

edited

Loading

howardjohn Oct 12, 2023

hzxuzhonghu Oct 13, 2023

hzxuzhonghu Oct 13, 2023

ZackButcher Oct 13, 2023

ZackButcher commented Oct 13, 2023

dasch commented Mar 20, 2024

howardjohn commented Mar 25, 2024 •

edited

Loading

dasch commented Apr 3, 2024

Mirror DestinationRule connection pool configuration on Sidecar #2961

Mirror DestinationRule connection pool configuration on Sidecar #2961

Conversation

ZackButcher commented Oct 11, 2023 • edited Loading

istio-policy-bot commented Oct 11, 2023

ZackButcher commented Oct 11, 2023

hzxuzhonghu commented Oct 12, 2023

ZackButcher commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackButcher commented Oct 12, 2023

ZackButcher commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linsun Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

ZackButcher Oct 12, 2023 • 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

ZackButcher commented Oct 13, 2023

dasch commented Mar 20, 2024

howardjohn commented Mar 25, 2024 • edited Loading

dasch commented Apr 3, 2024

ZackButcher commented Oct 11, 2023 •

edited

Loading

ZackButcher commented Oct 12, 2023 •

edited

Loading

ZackButcher commented Oct 12, 2023 •

edited

Loading

linsun Oct 12, 2023 •

edited

Loading

ZackButcher Oct 12, 2023 •

edited

Loading

howardjohn commented Mar 25, 2024 •

edited

Loading