-
Notifications
You must be signed in to change notification settings - Fork 16
Add max connections configuration to GatewayClassConfig CRD #405
Conversation
Notably, K8sGateway.Config was not previously exported and so wasn't included by the marshaler when writing Gateways into the store backend. This meant that the GatewayClassConfig was missing when re-syncing the Gateways from the store into Consul in the background, and so the max_connections value would be lost
c504ba0
to
f78f03b
Compare
f78f03b
to
ec283d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal review
description: Configuration information for managing connections in | ||
Envoy | ||
properties: | ||
maxConnections: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connectionManagement.maxConnections
is somewhat long; however, I opted not to abbreviate to something like connMgmt.maxConns
since we haven't abbreviated any other parts of the spec.
ID GatewayID | ||
Meta map[string]string | ||
Listeners []ResolvedListener | ||
MaxConnections *uint32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ResolvedGateway
is what we have access to when deriving the ingress-gateway
config entry that we're sending to Consul, so it needs to be aware of any config that we depend on there. This is internal, so it can evolve in the future if we wind up having other connection management config values to pipe through.
@@ -20,15 +26,15 @@ type K8sGateway struct { | |||
*gwv1beta1.Gateway | |||
GatewayState *state.GatewayState | |||
|
|||
config apigwv1alpha1.GatewayClassConfig | |||
Config apigwv1alpha1.GatewayClassConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported this so that the JSON marshaler includes it when we serialize for writing to the store backend.
If not exported, we don't have any idea what the max connections value is when rehydrating from the store and syncing into Consul, which we do at an interval in the background for resiliency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, appreciate your clarifying comments as always
Changes proposed in this PR:
Add a new field to our
GatewayClassConfig
CRD that is piped down to theingress-gateway
config entry for overriding the default connection maximum in Envoy (1,024)How I've tested this PR:
Create new
Gateways
with and without this setting in theGatewayClassConfig
. For eachGateway
, check the correspondingingress-gateway
config entry in Consul for the proper override.Example
How I expect reviewers to test this PR:
See above
Checklist: