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

Mirror DestinationRule connection pool configuration on Sidecar #2961

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
310 changes: 310 additions & 0 deletions kubernetes/customresourcedefinitions.gen.yaml

Large diffs are not rendered by default.

368 changes: 255 additions & 113 deletions networking/v1alpha3/sidecar.pb.go

Large diffs are not rendered by default.

121 changes: 121 additions & 0 deletions networking/v1alpha3/sidecar.pb.html

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

112 changes: 112 additions & 0 deletions networking/v1alpha3/sidecar.proto
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
syntax = "proto3";

import "google/api/field_behavior.proto";
import "networking/v1alpha3/destination_rule.proto";
import "networking/v1alpha3/gateway.proto";
import "networking/v1alpha3/virtual_service.proto";

Expand Down Expand Up @@ -479,6 +480,82 @@ import "networking/v1alpha3/virtual_service.proto";
// ```
// {{</tab>}}
// {{</tabset>}}
//
// In addition to configuring traffic capture and how traffic is forwarded to the app,
// it's possible to control inbound connection pool settings. By default, Istio pushes
// connection pool settings from `DestinationRules` to both clients (for outbound
// connections to the service) as well as servers (for inbound connections to a service
// instance). Using the `InboundConnectionPool` and per-port `ConnectionPool` settings
// in a `Sidecar` allow you to control those connection pools for the server separately
// from the settings pushed to all clients.
//
// {{<tabset category-name="example">}}
// {{<tab name="v1alpha3" category-value="v1alpha3">}}
// ```yaml
// apiVersion: networking.istio.io/v1alpha3
// kind: Sidecar
// metadata:
// name: connection-pool-settings
// namespace: prod-us1
// spec:
// workloadSelector:
// labels:
// app: productpage
// inboundConnectionPool:
// http:
// http1MaxPendingRequests: 1024
// http2MaxRequests: 1024
// maxRequestsPerConnection: 1024
// maxRetries: 100
// ingress:
// - port:
// number: 80
// protocol: HTTP
// name: somename
// connectionPool:
// http:
// http1MaxPendingRequests: 1024
// http2MaxRequests: 1024
// maxRequestsPerConnection: 1024
// maxRetries: 100
// tcp:
// maxConnections: 100
// ```
// {{</tab>}}
//
// {{<tab name="v1beta1" category-value="v1beta1">}}
// ```yaml
// apiVersion: networking.istio.io/v1beta1
// kind: Sidecar
// metadata:
// name: connection-pool-settings
// namespace: prod-us1
// spec:
// workloadSelector:
// labels:
// app: productpage
// inboundConnectionPool:
// http:
// http1MaxPendingRequests: 1024
// http2MaxRequests: 1024
// maxRequestsPerConnection: 1024
// maxRetries: 100
// ingress:
// - port:
// number: 80
// protocol: HTTP
// name: somename
// connectionPool:
// http:
// http1MaxPendingRequests: 1024
// http2MaxRequests: 1024
// maxRequestsPerConnection: 1024
// maxRetries: 100
Comment on lines +549 to +553
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// tcp:
// maxConnections: 100
// ```
// {{</tab>}}
// {{</tabset>}}
package istio.networking.v1alpha3;

option go_package = "istio.io/api/networking/v1alpha3";
Expand Down Expand Up @@ -528,6 +605,32 @@ message Sidecar {
// detected defaults from the namespace-wide or the global default Sidecar.
repeated IstioEgressListener egress = 3;

// Settings controlling the volume of connections Envoy will accept from the network.
// This default will apply for all inbound listeners and can be overridden per-port
// in the `Ingress` field. This configuration mirrors the `DestinationRule`'s
ZackButcher marked this conversation as resolved.
Show resolved Hide resolved
// [`connectionPool`](https://istio.io/latest/docs/reference/config/networking/destination-rule/#ConnectionPoolSettings) field.
//
// By default, Istio applies a service's `DestinationRule` to client sidecars
// for outbound traffic directed at the service -- the usual case folks think
// of when configuring a `DestinationRule` -- but also to the server's inbound
// sidecar. The `Sidecar`'s connection pool configures the server's inbound
// sidecar directly, so its settings can be different than clients'. This is
// valuable, for example, when you have many clients calling few servers: a
// `DestinationRule` can limit the concurrency of any single client, while
// the `Sidecar` allows you to configure much higher concurrency on the server
// side.
//
// Connection pool settings for a server's inbound sidecar are configured in the
// following precedence, highest to lowest:
// - per-port `ConnectionPool` from the `Sidecar`
// - top level `InboundConnectionPool` from the `Sidecar`
// - per-port `TrafficPolicy.ConnectionPool` from the `DestinationRule`
// - top level `TrafficPolicy.ConnectionPool` from the `DestinationRule`
// - default connection pool settings (essentially unlimited)
//
// In every case, the connection pool settings are overriden, not merged.
ConnectionPoolSettings inbound_connection_pool = 7;

// Configuration for the outbound traffic policy. If your
// application uses one or more external services that are not known
// apriori, setting the policy to `ALLOW_ANY` will cause the
Expand Down Expand Up @@ -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
Copy link
Member

@linsun linsun Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ZackButcher ZackButcher Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, reduce merging

// specific settings for this port. This configuration mirrors the `DestinationRule`'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// [`PortTrafficPolicy.connectionPool`](https://istio.io/latest/docs/reference/config/networking/destination-rule/#TrafficPolicy-PortTrafficPolicy) field.
// This port level connection pool has the highest precedence in configuration,
// overriding both the `Sidecar`'s top level `InboundConnectionPool` as well as any
// connection pooling settings from the `DestinationRule`.
ConnectionPoolSettings connection_pool = 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// `IstioEgressListener` specifies the properties of an outbound traffic
Expand Down
Loading