Add agentgateway provider support to the chart#272
Conversation
fcb2743 to
3d0c923
Compare
|
xref llm-d/llm-d#421 for the associated llm-d/llm-d changes |
74a1262 to
83a7cb1
Compare
|
I reviewed this setting across both |
Gregory-Pereira
left a comment
There was a problem hiding this comment.
This is really awesome - regretting it took me so long to review. I have a few suggested refactors here.
Refactor in this PR that we should go with:
- decoupling the gateway provider and the class name
Specific changes requested:
- move
data-science-gateway-classto a classname of istio because OSSM uses this class name but its built on top of Istio
Refactor suggested:
- Refactor the way that we configure and pass the
parameterRefs. We want to open the door to supporting everything that GIE supports. I want a common way to configure all those. I have proposed a solution above, take a look and see if you like it. - Gateway Exposure type. I suggest we compute this taking 3 factors into account:
- Gateway provider's default exposure type - This is the chart’s opinionated default based on the gateway implementation.
- Users intended exposure behaviour - This is the user saying what kind of reachability they want, regardless of the provider default.
- Hard override for service type - here be dragons but let people do what they want
| {{- define "gateway.serviceName" -}} | ||
| {{- if eq .Values.gateway.gatewayClassName "istio" -}} | ||
| {{ include "gateway.fullname" . }}-istio | ||
| {{- else }} | ||
| {{ include "gateway.fullname" . }} | ||
| {{- if eq (include "gateway.provider" .) "istio" -}} | ||
| {{- printf "%s-istio" (include "gateway.fullname" .) -}} | ||
| {{- else -}} | ||
| {{- include "gateway.fullname" . -}} | ||
| {{- end -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
IIRC this was a stylistic thing to help distinguish what provider we have by the name easily. Should we do this for each provider? Or is that a bad practice?
There was a problem hiding this comment.
Commit 3259ffb keeps the -istio suffix only for the gatewayClassName: istio path. The data-science-gateway-class stays unsuffixed while still using the Istio-backed ConfigMap path.
There was a problem hiding this comment.
As discussed, I want to have a consistent place to configure the gateway. Every implementation may call this something unique but they all controll configuration of the gateway (ex: gatewayConfig for envoy AI gateway, gatewayParameters for istio and kgateway, AgentgatewayParameters for agentgateway).
My proposal is to have common configurations at the top level, ex:
gateway:
provider: istio | kgateway ...
gatewayParameters:
enabled: true
resources: {}
logLevel: warn
kgateway:
floatingUserId: true
...
There was a problem hiding this comment.
+1. I'll keep gateway.gatewayParameters as the user interface for common gateway configuration, with:
- Common knobs at the top level of
gateway.gatewayParameters. - Provider-specific knobs under nested provider blocks.
That should give us a scalable shape for additional GIE-supported gateways without making every implementation invent a different top-level values block.
Split chart provider selection from GatewayClass naming, route kgateway compatibility through the agentgateway class, and add AgentgatewayParameters rendering. Refresh generated chart docs and schema, deprecate kgateway in favor of standalone agentgateway, and update CI workflow pins and provider handling. Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
83a7cb1 to
3259ffb
Compare
Changes
Testing
Partially Fixes: #269