feat(extensionManager): add support for multiple ExtensionManagers with sequential chaining#8458
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
1a2f133 to
724d5fe
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8458 +/- ##
==========================================
+ Coverage 74.36% 74.55% +0.19%
==========================================
Files 246 249 +3
Lines 39292 39575 +283
==========================================
+ Hits 29221 29507 +286
+ Misses 8041 8034 -7
- Partials 2030 2034 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b68d616 to
b69ed21
Compare
eb460a9 to
0fe85de
Compare
| for _, gvk := range em.Resources { | ||
| extGKs = append(extGKs, schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind}) | ||
| } | ||
| // Include backend resources in extension group kinds for custom backend support | ||
| for _, gvk := range em.BackendResources { | ||
| extGKs = append(extGKs, schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind}) | ||
| } |
There was a problem hiding this comment.
Could it be that it was forgotten to also handle em.PolicyResources.
If so, should they be assigned to extGKs, which are assigned to Translator.ExtensionGroupKinds. Or do we need to create a different list and a new receiving property in Translator ?
There was a problem hiding this comment.
I think em.PolicyResources is not needed here because the Translator.ExtensionGroupKinds is only used for route custom backends and httpFilters in the code.
9c930e2 to
b2469b9
Compare
|
/retest |
|
can we add some e2e tests for multiple ext-managers demonstrating chaining behavior and resource isolation? |
1acd61e to
3437151
Compare
@guydc Would you mind taking a look and let me know if the e2e test that I've added are enough? If not not let me know we other cases we should cover. Thanks in advance. |
3437151 to
54d56d5
Compare
|
Overall looks LGTM. Two asks:
|
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
6e77db9 to
559ede4
Compare
Thanks for the feedback @guydc. I have added your suggestions. Hope that I understood them correctly, and the approach I took is correct. Looking forward for more feedback, and many thanks in advance |
|
/retest |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca1a4defe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if r == nil { | ||
| continue | ||
| } | ||
| if gvkSet.Has(r.GroupVersionKind()) { |
There was a problem hiding this comment.
Match extension resources by group/kind, not full GVK
This filter uses exact GroupVersionKind matching, but the rest of extension discovery still treats resources as GroupKind (for example extension-kind registration and HasExtension checks). In multi-manager mode, a resource/policy that matches group+kind but comes from a different served version is accepted upstream, then silently dropped here, so the extension hook receives an empty context and behavior diverges from single-manager mode. This can break existing configs when CRD versions roll or multiple versions are served.
Useful? React with 👍 / 👎.
|
Hi @toffentoffen Could you please fix the |
68f98c4 to
28516d7
Compare
| lastDomain := len(req.VirtualHost.Domains) - 1 | ||
| newDomain := fmt.Sprintf("%s.%s", req.VirtualHost.Domains[lastDomain], s.suffix) | ||
| s.log.Info("PostVirtualHostModify appending suffix to last domain", | ||
| slog.String("originalDomain", req.VirtualHost.Domains[lastDomain]), | ||
| slog.String("newDomain", newDomain)) | ||
|
|
||
| req.VirtualHost.Domains = append(req.VirtualHost.Domains, newDomain) | ||
| } |
There was a problem hiding this comment.
This fixed the failing e2e test.
Previously it was always appending to the first domain generating domain.ext-a and domain.ext-b.
But what the multiple extensionManagers test were expecting is to chain the modifications from ext-a to ext-b, generating a domain called domain.ext-a.ext-b that the test will call.
|
/retest |
…ning Add a new `extensionManagers` plural field to `EnvoyGatewaySpec` that allows registering multiple extension managers with sequential chaining semantics. Each extension's output becomes the next extension's input. Key changes: - Add `Name` field to `ExtensionManager` and `ExtensionManagers` list to `EnvoyGatewaySpec` - Add `GetExtensionManagers()` helper to normalize singular/plural fields - Add mutual exclusivity validation between singular and plural fields - Implement `CompositeManager` wrapping multiple managers behind the `Manager` interface - Implement `compositeXDSHookClient` with per-extension policy filtering and per-extension resource-type gating in `PostTranslateModifyHook` - Merge `TranslationConfig` using OR semantics across all managers - Add `CleanupHookConns()` to the `Manager` interface - Unify `NewManager` factory to handle 0, 1, and N extensions Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml
Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
…esources accordingly Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # internal/extension/registry/extension_manager_test.go
Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
…ager Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
…urce isolation Enhance the simple-extension-server with a configurable --suffix flag and resource-aware PostRouteModify to support testing multiple extension managers. Add resilience tests that verify sequential chaining of VirtualHost mutations and per-extension resource isolation via extensionRef filters. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # examples/simple-extension-server/go.mod # examples/simple-extension-server/go.sum
…ural ExtensionManagers Mirror the existing extension-manager translator error-handling tests through CompositeManager via a new NewInMemoryCompositeManager helper: a 1-entry composite verifies child errors are swallowed when failOpen is true, and a 2-entry composite verifies errors are propagated with the 'extension "<name>":' prefix when failOpen is false. Extract a shared buildManagerGVKSets helper to avoid duplication between NewManager and the new in-memory constructor. Also extend the extension-server docs with a "Multiple extension servers" subsection covering sequential chaining, per-server resource isolation, per-server failOpen, the unique name requirement, and mutual exclusivity with the singular extensionManager field. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
…ry composite Two fixes in the composite-extension path: 1. CompositeManager filter now matches by group/kind only, aligning with runner.ExtensionGroupKinds, Manager.HasExtension, and Gateway API extensionRef. Exact-GVK matching could silently drop resources whose served version differed from the one declared in ExtensionManager.Resources, diverging from single-manager behavior when CRDs serve multiple versions. 2. NewInMemoryCompositeManager wires a sync.Once-guarded cleanup into every entry's cleanupHookConn, so CompositeManager.CleanupHookConns() (from the Manager interface) tears down the shared bufconn/server. The separate cleanup func return is removed; callers use CleanupHookConns() on the returned Manager. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
* +kubebuilder:validation:MinItems=1 on EnvoyGatewaySpec.ExtensionManagers, mirrored by a runtime check that rejects an explicitly-set-but-empty list (a nil slice still means "omitted"). * +kubebuilder:validation:XValidation on EnvoyGatewaySpec declaring that extensionManager and extensionManagers are mutually exclusive. The same constraint is already enforced at runtime in validateEnvoyGatewayExtensionManagers; the marker documents the schema and is wired up for consumers that validate against the generated OpenAPI. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
28516d7 to
f61a4b5
Compare
|
/retest |
…th sequential chaining (envoyproxy#8458) * feat: add support for multiple ExtensionManagers with sequential chaining Add a new `extensionManagers` plural field to `EnvoyGatewaySpec` that allows registering multiple extension managers with sequential chaining semantics. Each extension's output becomes the next extension's input. Key changes: - Add `Name` field to `ExtensionManager` and `ExtensionManagers` list to `EnvoyGatewaySpec` - Add `GetExtensionManagers()` helper to normalize singular/plural fields - Add mutual exclusivity validation between singular and plural fields - Implement `CompositeManager` wrapping multiple managers behind the `Manager` interface - Implement `compositeXDSHookClient` with per-extension policy filtering and per-extension resource-type gating in `PostTranslateModifyHook` - Merge `TranslationConfig` using OR semantics across all managers - Add `CleanupHookConns()` to the `Manager` interface - Unify `NewManager` factory to handle 0, 1, and N extensions Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml * Fix lint issues Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Add more test cases Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Per-extension manager filter Resources, BackendResources, and PolicyResources accordingly Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # internal/extension/registry/extension_manager_test.go * Implement new hook method PostEndpointsModifyHook Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Respect failOpen when GetPre/PostXDSHookClient errors in CompositeManager Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Simplified and unified client getter and tests Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Add resilience tests for multiple ExtensionManagers chaining and resource isolation Enhance the simple-extension-server with a configurable --suffix flag and resource-aware PostRouteModify to support testing multiple extension managers. Add resilience tests that verify sequential chaining of VirtualHost mutations and per-extension resource isolation via extensionRef filters. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # examples/simple-extension-server/go.mod # examples/simple-extension-server/go.sum * Add CompositeManager coverage to xDS translator tests and document plural ExtensionManagers Mirror the existing extension-manager translator error-handling tests through CompositeManager via a new NewInMemoryCompositeManager helper: a 1-entry composite verifies child errors are swallowed when failOpen is true, and a 2-entry composite verifies errors are propagated with the 'extension "<name>":' prefix when failOpen is false. Extract a shared buildManagerGVKSets helper to avoid duplication between NewManager and the new in-memory constructor. Also extend the extension-server docs with a "Multiple extension servers" subsection covering sequential chaining, per-server resource isolation, per-server failOpen, the unique name requirement, and mutual exclusivity with the singular extensionManager field. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Fix lint errors Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Match extension resources by group/kind, and wire cleanup for in-memory composite Two fixes in the composite-extension path: 1. CompositeManager filter now matches by group/kind only, aligning with runner.ExtensionGroupKinds, Manager.HasExtension, and Gateway API extensionRef. Exact-GVK matching could silently drop resources whose served version differed from the one declared in ExtensionManager.Resources, diverging from single-manager behavior when CRDs serve multiple versions. 2. NewInMemoryCompositeManager wires a sync.Once-guarded cleanup into every entry's cleanupHookConn, so CompositeManager.CleanupHookConns() (from the Manager interface) tears down the shared bufconn/server. The separate cleanup func return is removed; callers use CleanupHookConns() on the returned Manager. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Add MinItems=1 and mutual-exclusion validation on extensionManagers * +kubebuilder:validation:MinItems=1 on EnvoyGatewaySpec.ExtensionManagers, mirrored by a runtime check that rejects an explicitly-set-but-empty list (a nil slice still means "omitted"). * +kubebuilder:validation:XValidation on EnvoyGatewaySpec declaring that extensionManager and extensionManagers are mutually exclusive. The same constraint is already enforced at runtime in validateEnvoyGatewayExtensionManagers; the marker documents the schema and is wired up for consumers that validate against the generated OpenAPI. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Fix MultiextensionManagers tests Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> --------- Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io>
…th sequential chaining (envoyproxy#8458) * feat: add support for multiple ExtensionManagers with sequential chaining Add a new `extensionManagers` plural field to `EnvoyGatewaySpec` that allows registering multiple extension managers with sequential chaining semantics. Each extension's output becomes the next extension's input. Key changes: - Add `Name` field to `ExtensionManager` and `ExtensionManagers` list to `EnvoyGatewaySpec` - Add `GetExtensionManagers()` helper to normalize singular/plural fields - Add mutual exclusivity validation between singular and plural fields - Implement `CompositeManager` wrapping multiple managers behind the `Manager` interface - Implement `compositeXDSHookClient` with per-extension policy filtering and per-extension resource-type gating in `PostTranslateModifyHook` - Merge `TranslationConfig` using OR semantics across all managers - Add `CleanupHookConns()` to the `Manager` interface - Unify `NewManager` factory to handle 0, 1, and N extensions Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml # Conflicts: # release-notes/current.yaml * Fix lint issues Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Add more test cases Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Per-extension manager filter Resources, BackendResources, and PolicyResources accordingly Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # internal/extension/registry/extension_manager_test.go * Implement new hook method PostEndpointsModifyHook Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Respect failOpen when GetPre/PostXDSHookClient errors in CompositeManager Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Simplified and unified client getter and tests Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Add resilience tests for multiple ExtensionManagers chaining and resource isolation Enhance the simple-extension-server with a configurable --suffix flag and resource-aware PostRouteModify to support testing multiple extension managers. Add resilience tests that verify sequential chaining of VirtualHost mutations and per-extension resource isolation via extensionRef filters. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> # Conflicts: # examples/simple-extension-server/go.mod # examples/simple-extension-server/go.sum * Add CompositeManager coverage to xDS translator tests and document plural ExtensionManagers Mirror the existing extension-manager translator error-handling tests through CompositeManager via a new NewInMemoryCompositeManager helper: a 1-entry composite verifies child errors are swallowed when failOpen is true, and a 2-entry composite verifies errors are propagated with the 'extension "<name>":' prefix when failOpen is false. Extract a shared buildManagerGVKSets helper to avoid duplication between NewManager and the new in-memory constructor. Also extend the extension-server docs with a "Multiple extension servers" subsection covering sequential chaining, per-server resource isolation, per-server failOpen, the unique name requirement, and mutual exclusivity with the singular extensionManager field. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Fix lint errors Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Match extension resources by group/kind, and wire cleanup for in-memory composite Two fixes in the composite-extension path: 1. CompositeManager filter now matches by group/kind only, aligning with runner.ExtensionGroupKinds, Manager.HasExtension, and Gateway API extensionRef. Exact-GVK matching could silently drop resources whose served version differed from the one declared in ExtensionManager.Resources, diverging from single-manager behavior when CRDs serve multiple versions. 2. NewInMemoryCompositeManager wires a sync.Once-guarded cleanup into every entry's cleanupHookConn, so CompositeManager.CleanupHookConns() (from the Manager interface) tears down the shared bufconn/server. The separate cleanup func return is removed; callers use CleanupHookConns() on the returned Manager. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Add MinItems=1 and mutual-exclusion validation on extensionManagers * +kubebuilder:validation:MinItems=1 on EnvoyGatewaySpec.ExtensionManagers, mirrored by a runtime check that rejects an explicitly-set-but-empty list (a nil slice still means "omitted"). * +kubebuilder:validation:XValidation on EnvoyGatewaySpec declaring that extensionManager and extensionManagers are mutually exclusive. The same constraint is already enforced at runtime in validateEnvoyGatewayExtensionManagers; the marker documents the schema and is wired up for consumers that validate against the generated OpenAPI. Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> * Fix MultiextensionManagers tests Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> --------- Signed-off-by: Marc Navarro Sonnenfeld <marcnavarro@tetrate.io> Signed-off-by: Jake Oliver <jake@truelayer.com>
Add a new
extensionManagersplural field toEnvoyGatewaySpecthat allows registering multiple extension managers with sequential chaining semantics. Each extension's output becomes the next extension's input.Key changes:
Namefield toExtensionManagerandExtensionManagerslist toEnvoyGatewaySpecGetExtensionManagers()helper to normalize singular/plural fieldsCompositeManagerwrapping multiple managers behind theManagerinterfacecompositeXDSHookClientwith per-extension policy filtering and per-extension resource-type gating inPostTranslateModifyHookTranslationConfigusing OR semantics across all managersCleanupHookConns()to theManagerinterfaceNewManagerfactory to handle 0, 1, and N extensionsFixes: #8264
Release Notes: Yes
Hints for reviewers:
PostTranslatedModifyHookand provide a slices of valuesextensionPolicies []ir.UnstructuredRefinstead of pointersextensionPolicies []*ir.UnstructuredRefto avoid mutations, by cloning the content. But this would have been to much overhead I think. Instead we improved the interface.extensionManagerhas not been added to this PR as I am not familiar with the deprecation policies. Advice welcome.