-
Notifications
You must be signed in to change notification settings - Fork 140
feat: introduce GatewayConfig CRD for gateway-scoped configuration #1623
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
base: main
Are you sure you want to change the base?
Conversation
d26095e to
8984f5b
Compare
mathetake
left a comment
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.
Thank you for taking a shot at this!
nacx
left a comment
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.
Just reviewed the API for now and left one comment.
Apart from that, this GatewayConfig object is only relevant for Kubernets, but not in standalone modes (at least for now). Would it make sense to convey that in the API? For example, the Envoy Gateaay infrastructure resources have the "provider" concept and settings are under a kubernetes config know.
Something like:
apiVersion: aigateway.envoyproxy.io/v1alpha1
kind: GatewayConfig
metadata:
name: comprehensive-config
namespace: default
spec:
kubernetes:
extProc:
env:
(...)
or
apiVersion: aigateway.envoyproxy.io/v1alpha1
kind: GatewayConfig
metadata:
name: comprehensive-config
namespace: default
spec:
extProc:
kubernetes:
env:
(...)
WDYT?
This commit adds the GatewayConfig custom resource definition (CRD) to manage configuration for the AI Gateway external processor. It allows users to define environment variables and resource requirements at the gateway level, enhancing flexibility and reusability across multiple gateways. The AIGatewayRoute's resource configuration is now deprecated in favor of this new approach. Additionally, the GatewayConfig controller is implemented to handle the lifecycle of GatewayConfig resources, including finalizer management to prevent accidental deletions while still referenced by Gateways. Documentation updates are included to reflect the new configuration options and migration guidance from the deprecated route-level settings. Signed-off-by: Shabab Qaisar <[email protected]> Signed-off-by: Shabab Qaisar <[email protected]>
This commit refactors the GatewayConfig controller to eliminate finalizer management, simplifying the lifecycle handling of GatewayConfig resources. The controller now focuses on notifying referencing Gateways of changes without blocking deletion based on references. Additionally, the documentation has been updated to reflect these changes, including the removal of finalizer behavior descriptions from the API and capability documentation. Signed-off-by: Shabab Qaisar <[email protected]> Signed-off-by: Shabab Qaisar <[email protected]>
This commit refactors the GatewayConfig controller to eliminate finalizer management, simplifying the lifecycle handling of GatewayConfig resources. The controller now focuses on notifying referencing Gateways of changes without blocking deletion based on references. Additionally, the documentation has been updated to reflect these changes, including the removal of finalizer behavior descriptions from the API and capability documentation. Signed-off-by: Shabab Qaisar <[email protected]> Signed-off-by: Shabab Qaisar <[email protected]>
Thankyou for quickly reviewing the changes. I've resolved all of them. Please take a look. |
…pec from the Envoy Gateway API Signed-off-by: Shabab Qaisar <[email protected]>
mathetake
left a comment
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.
looking good so far!
…rSpec for defining the external processor's configuration. Signed-off-by: Shabab Qaisar <[email protected]>
Signed-off-by: Shabab Qaisar <[email protected]>
Signed-off-by: Shabab Qaisar <[email protected]>
|
@mathetake I've updated the type to |
mathetake
left a comment
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 modulo a few comments. Thank you so much for working on this! I will defer to @nacx for the rest as i will not be available to review at least for the next 48 hours
… GatewayConfig resources over route-scoped values. Signed-off-by: Shabab Qaisar <[email protected]>
…urations and list errors Signed-off-by: Shabab Qaisar <[email protected]>
|
Added more tests to make up for coverage. |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (58.36%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1623 +/- ##
==========================================
- Coverage 83.46% 82.98% -0.48%
==========================================
Files 138 143 +5
Lines 12540 12758 +218
==========================================
+ Hits 10466 10587 +121
- Misses 1440 1527 +87
- Partials 634 644 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM! I will wait for other maintainers to take a look until tomorrow otherwise good to go!
cc @envoyproxy/ai-gateway-maintainers
Co-authored-by: Takeshi Yoneda <[email protected]> Signed-off-by: Shabab Qaisar <[email protected]>
Thankyou. |
|
why not just use EnvoyProxy CR? |
Mostly because the configuration of the extproc container is generated on the fly by EAIGW, and its config can't be preconfigured in the EP resource. |
nacx
left a comment
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.
Thanks @sqaisar! This looks great.
I've left a few comments mostly related to testing and I think this will be good to go once done!
… facilitate e2e testing Signed-off-by: Shabab Qaisar <[email protected]>
I've resolved them. Looks better i think. |
Signed-off-by: Shabab Qaisar <[email protected]>
| eventCh := internaltesting.NewControllerEventChan[*gwapiv1.Gateway]() | ||
| c := NewGatewayConfigController(fakeClient, ctrl.Log, eventCh.Ch) | ||
|
|
||
| // Create a Gateway that references a GatewayConfig that doesn't exist yet. |
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.
This is not what I meant. The thing we're missing in the tests is the behaviour when the referenced config does not exist (and will never exist). For example, users make a typo in the annotation value. What is the expected behaviour then? This is what I think this test needs to validate.
| } | ||
|
|
||
| // Create Gateway and GatewayConfig for GatewayConfig test case | ||
| if tt.name == "with GatewayConfig" { |
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.
Instead of relying on the test name, which makes it harder to evolve these tests, just add a property to the test case: gatewayConfig *aigv1a1.GatewayConfig, and if it is set, then create it with the fake client. You can extract this GW config into a package variable to make it easier to reference in the test if most of its information is static.
| require.True(t, ok, "failed to get source file path") | ||
| sourceDir := filepath.Dir(filename) | ||
|
|
||
| manifest := filepath.Join(sourceDir, "testdata", "otel_tracing_gateway_config.yaml") |
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.
Instead of doing this, delete this custom file and point directly to the file in the examples directory. This is what I meant in my previous comment. You already spotted an issue where the example was missing the kubernetes keyword, and we want to keep examples always working. Let's do that by using those in the e2e tests, as we do with other examples.
Description
Introduce GatewayConfig CRD for gateway-scoped configuration of the external processor. Adds controller lifecycle handling (including finalizer management), deprecates AIGatewayRoute-level resource configuration in favor of
GatewayConfig, and updates docs with migration guidance and new options.Example
Related Issues/PRs (if applicable)
Fixes [1]
Special notes for reviewers (if applicable)
Docs, controller, and CRD changes included; route-level resources are deprecated—see migration guidance in docs.
1: #1552