edge proxy config API#3707
Conversation
Signed-off-by: Steve Kriss <skriss@redhat.com>
Signed-off-by: Cameron Garrison <cgarriso@redhat.com>
|
😊 Welcome @cam-garrison! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/L
| } | ||
|
|
||
| // The config profile to use for this proxy. | ||
| ProxyConfigProfile profile = 43; |
There was a problem hiding this comment.
How will this and below fields work together? When I select Edge profile, do I get some default values for the below configurations? I think it would be good to tie this profile and the below fields together or explain clearly how they work together?
There was a problem hiding this comment.
Good question, thx - updated the proto comments to help clarify 6c3c4d8.
The profile sets defaults for the fields below, explicitly setting any field overrides the profile default. SIDECAR (the zero value) preserves existing behavior. EDGE applies Envoy's recommended edge defaults. The actual default values will be applied in the istio/istio implementation.
Signed-off-by: Cameron Garrison <cgarriso@redhat.com>
cam-garrison
force-pushed
the
edge-proxy-config
branch
from
1d290cb to
6c3c4d8
Compare
| // The profile determines default values for the fields below (buffer limits, | ||
| // timeouts, HTTP/2 tuning, header/path normalization, and connection limits). | ||
| // Explicitly setting any field always takes precedence over profile defaults. | ||
| enum ProxyConfigProfile { |
There was a problem hiding this comment.
it is hard to figure out which fields impacted by this profile. I would suggest adding a new Message like ProxyDefaults or some thing like that , that has all the fields that the profile controls and then select the profile ?
There was a problem hiding this comment.
That's fair, I understand. I think it is better to not have them be nested like proxyConfig.proxyDefaults.httpIdleTimeout and instead be flat on proxyConfig like proxyConfig.httpIdleTimeout. These fields are individually useful without a profile and the fields are not one conceptual unit, they cover different envoy scopes.
I agree it should be more clear which fields the profile effects. I can add a comment to each field noting that it's subject to profile defaults, would that address your concern?
There was a problem hiding this comment.
Stepping back, why do we need an Enum here? Can we just use a bool like enable_edge_defaults (default to false) and when enabled Edge defaults are enabled? If we do that, can we group all these configurations under some new message like GatewayProxySettings or EdgeProxySettings ?
There was a problem hiding this comment.
The original proposal from @skriss used a bool, and it was changed to an enum per the working group's review. "One minor change from the RFC is replacing the enableEdgeProxyConfig boolean field with a profile field, per suggestion in the WG meeting. This is more extensible and consistent with other parts of the Istio config API." from #3627.
Re: grouping, I still feel like flat fields are best. These are general-purpose proxy settings that are useful independently of the profile. A user should be able to set http_idle_timeout on a sidecar without any
edge/gateway connotation. Wrapping them in EdgeProxySettings would imply they're edge-specific when they're not, the profile just provides convenient defaults.
There was a problem hiding this comment.
http_idle_timeout on a sidecar without any edge/gateway connotation
The problem of applying this field to sidecar is - it is not clear to which side of hop/service we are setting this. It interfers with existing mechanisms like DR and ProxyConfig
There was a problem hiding this comment.
Yeah, that's fair. Tthere is overlap with DestinationRule at the cluster level. A few thoughts:
-
The EDGE profile defaults will only be applied to gateway (Router-type) proxies in the implementation. Sidecars aren't affected unless a user explicitly sets individual fields.
-
For explicit overrides, these are proxy-wide settings at the Envoy listener/HCM/cluster level, the same scope as existing ProxyConfig fields like concurrency. The sidecar inbound/outbound ambiguity exists there too.
-
The interaction with a DR is at different Envoy layers (HCM vs cluster), so they don't directly conflict, but I agree it's worth documenting.
I don't think that grouping these under a separate message wouldn't resolve the sidecar scope question the ambiguity comes from ProxyConfig being proxy-wide, not from where the fields sit in the proto. Would some more documentation on scope and the DR interaction address what you're getting at here?
There was a problem hiding this comment.
the same scope as existing ProxyConfig fields like concurrency. The sidecar inbound/outbound ambiguity exists there too.
The concurrency is not at listener/HCM/cluster level. It applies to entire proxy so it makes sense to apply at proxy level. There is no confusion there.
The interaction with a DR is at different Envoy layers (HCM vs cluster), so they don't directly conflict, but I agree it's worth documenting.
What does specifying http_idle_timeout mean on a sidecar? Does it apply to the inbound listener, outbound clusters, or both? For outbound, which upstream services are affected? This conflicts with ConnectionPoolSettings.tcp.idle_timeout in DestinationRule, which already provides per-service outbound idle timeout control. If it applies to all outbound and all inbound, we need to clearly document that and also specify what will have preference ProxyConfig or DR? Is n't that additional confusion?
I don't think that grouping these under a separate message wouldn't resolve the sidecar scope question the ambiguity comes from ProxyConfig being proxy-wide, not from where the fields sit in the proto.
What is the motivation to expose only these fields when Envoy provides 100 other knobs? I believe the reason we selected these fields is because Envoy documentation says it is best practice to set defaults to these fields. There are now 18 new fields in ProxyConfig with only a comment tying them to the profile. A future reader (or contributor adding field 61) has no structural indication of which fields the profile governs - I have to read the entire proxy config proto to understand what the profile configures.
Yes. They can be still be configured individually outside profile if we want to allow that.
I'm supportive of the feature — edge hardening is valuable. My concern is purely about API structure, clarity of scope (the issues and the PR description says "edge proxy config API") and future maintainability.
5 participants
Picks up and rebases #3627 (by @skriss) (addt'l adding release note).
Adds edge proxy configuration fields to
ProxyConfig— aprofileenum (SIDECAR/EDGE), connection buffer limits, HTTP timeouts, HTTP/2 tuning, header/path normalization, and connection limits. Enables native Istio support for Envoyedge proxy best practices.
Design per WG feedback: uses
profileenum instead of boolean. Overload manager out of scope for initial implementation.Related RFC: Edge Proxy Config
Supersedes #3627.
cc @skriss @keithmattix @dgn @frittentheke
ref. istio/istio#57973
ref. istio/istio#24715