fix(translator): set status for invalid EnvoyPatchPolicy target kind#8788
fix(translator): set status for invalid EnvoyPatchPolicy target kind#8788nishantbkl3345-ship-it wants to merge 4 commits intoenvoyproxy:mainfrom
Conversation
…nvoyproxy#8623) Signed-off-by: ipsitapp8 <ipsitapp8@gmail.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Nishant <nishantbkl3345@gmail.com>
There was a problem hiding this comment.
Pull request overview
Improves user-visible feedback for EnvoyPatchPolicy objects that target an unsupported targetRef.kind for the current translation mode (notably Gateway in MergeGateways mode), ensuring they are marked Accepted=False with Reason=Invalid instead of being silently skipped.
Changes:
- Move
targetRefkind/group validation earlier inProcessEnvoyPatchPoliciesso it can’t be bypassed by early exits. - Ensure invalid-target policies are still attached to the appropriate XDS IR entry so their status gets published.
- Update merge-gateways golden test output to include the expected
Accepted=Falsestatus condition.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/gatewayapi/envoypatchpolicy.go | Validates unsupported targetRef earlier and ensures invalid policies still surface status via XDS IR attachment. |
| internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind-merge-gateways.out.yaml | Updates expected output to include status for invalid target kind in merge-gateways mode. |
Comments suppressed due to low confidence (1)
internal/gatewayapi/envoypatchpolicy.go:55
- The inline comment and the user-facing status message here have awkward/incorrect grammar (e.g. "targeting to a support type" and "... and TargetRef.Kind:%s is supported"). Consider rewording to something like "targetRef must use group %q and kind %q" / "only ... are supported" to improve clarity and correctness (and optionally drop the comma after TargetRef.Kind).
"TargetRef.Group:%s TargetRef.Kind:%s TargetRef.Namespace:%s TargetRef.Name:%s not found or not accepted (MergeGateways=%t).",
refGroup, refKind, policy.Namespace, string(refName), t.MergeGateways,
)
resolveErr = &status.PolicyResolveError{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
this should be covered by #8475 |
|
Thanks — I took a closer look at #8475 and compared it with the current behavior. From what I can see, #8475 handles status in several cases, but in the invalid targetRef kind path, the validation still happens after the In this PR, I moved the validation earlier and ensured that even in those cases, the policy gets attached to an appropriate IR and It’s possible I’m misunderstanding part of #8475 though — if this case is already covered there, I’m happy to align or adjust this PR accordingly. |
Summary
This is a small follow-up for #8623 to improve how invalid
EnvoyPatchPolicytargetRef cases are handled.While testing, I noticed that when a policy targets an unsupported kind for the current mode (for example,
Gatewayin MergeGateways mode), it could be skipped without setting any status. From a user perspective, this makes it hard to understand why the policy isn’t applied.This change ensures that such cases consistently report:
Accepted=FalseReason=Invalidinstead of being silently ignored.
What changed
Validation
Ran the following:
go test ./internal/gatewayapi/...All tests passed.
Notes
This change does not introduce any new targeting behavior. It only improves visibility by making invalid configurations explicit via status conditions.