feat: support backendtrafficpolicy for Ingress #100
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for backend traffic policy in Ingress by augmenting tests and updating the controller and translator modules to handle the new Ingress resource type alongside HTTPRoute. Key changes include:
- Extending e2e tests with new scenarios for Ingress-backed backend traffic policies.
- Adding helper functions and predicates to process backend traffic policies.
- Adjusting resource reconciliation and status update logic in controllers.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/crds/backendtrafficpolicy.go | Extended tests with new Ingress cases and updated test descriptions/log messages. |
| internal/provider/adc/translator/policies.go | Introduced a helper (convertBackendRef) and modified the backend reference matching. |
| internal/provider/adc/translator/ingress.go | Updated logic to use the new helper for backend reference conversion. |
| internal/controller/policies.go | Added a dedicated predicate function for BackendTrafficPolicy events. |
| internal/controller/ingress_controller.go | Enhanced Ingress reconciliation with event channel watches and status updates. |
| internal/controller/httproute_controller.go | Refactored the predicate usage for backend traffic policy events. |
| By("create GatewayProxy") | ||
| err := s.CreateResourceFromStringWithNamespace(gatewayProxy, "default") |
There was a problem hiding this comment.
The log message on this line does not match the resource being created (IngressClass). Please update the log message to correctly reflect that an IngressClass is being created.
| By("create GatewayProxy") | |
| err := s.CreateResourceFromStringWithNamespace(gatewayProxy, "default") | |
| By("create IngressClass with GatewayProxy reference") |
| for _, targetRef := range po.Spec.TargetRefs { | ||
| if ref.Name == targetRef.Name && | ||
| (ref.Namespace != nil && string(*ref.Namespace) == po.Namespace) { | ||
| if ref.Name == targetRef.Name { |
There was a problem hiding this comment.
The removal of the namespace check may lead to unintended associations when multiple services share the same name in different namespaces. Consider reintroducing the namespace comparison if services are expected to come from distinct namespaces.
| if ref.Name == targetRef.Name { | |
| if ref.Name == targetRef.Name && (ref.Namespace == nil || targetRef.Namespace == nil || string(*ref.Namespace) == targetRef.Namespace) { |
There was a problem hiding this comment.
reject.
targetRef does not have namespace
Co-authored-by: 悟空 <[email protected]>
Type of change:
What this PR does / why we need it:
Pre-submission checklist: