fix: update XAccessPolicy status when rules are skipped during translation#236
Conversation
✅ Deploy Preview for kube-agentic-networking ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @HarshithaMS005. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
haiyanmeng
left a comment
There was a problem hiding this comment.
@HarshithaMS005 , thanks for the PR!
Can you add unit test for this change?
|
/cc @guicassolato /hold @guicassolato please take a look at this change. |
05ae15c to
4945ab5
Compare
ce1e662 to
46a849f
Compare
|
/hold |
46a849f to
e74d678
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: HarshithaMS005 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
e74d678 to
f3a9ee3
Compare
a0817bb to
7a2b27d
Compare
7a2b27d to
5791bcd
Compare
Collect semantic translation failures during Gateway xDS translation (ext_authz build + externalAuth fingerprint) and set XAccessPolicy Accepted=False with Gateway API PolicyReasonInvalid. Re-evaluate per-target acceptance when translation recovers. Also added unittests for the same. Signed-off-by: Harshitha MS <harshitha.ms@ibm.com>
5791bcd to
40d64e5
Compare
|
/test pull-kube-agentic-networking-e2e |
|
/lgtm /hold for @LiorLieberman to approve |
|
Two questions as I read through this:
Neither of these block the current PR, just want to make sure we've thought through the design before it sets a pattern. |
|
/hold |
| return | ||
| } | ||
| for _, ap := range attached { | ||
| policy, err := c.agentic.accessPolicyLister.XAccessPolicies(ap.Namespace).Get(ap.Name) |
There was a problem hiding this comment.
What's the goal of getting the policy object again? Is it to ensure the latest spec.targetRefs after getting state of the object from listing at line 347.
This seems almost the same as #236 (comment). I wonder if we this step is really needed.
If we can confirm this builds on the informer's cache and won't issue a request to the API server for each policy object unless the generation of the resource has changed, then perhaps there's no harm in keeping it. Otherwise, I'd probably advocate for avoiding the overhead on the API server, given another reconciliation event may have been enqueued already by the time this happens.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR implements [translation feedback] so the controller no longer leaves XAccessPolicy in a misleading Accepted: True state when the translator skips or only partially applies rules that still pass API validation.
Today, failures such as HTTP ExternalAuth with a non-Service backend or externalAuthUniqueID errors are only logged; Envoy may omit behaviour while status still looks healthy. Issue #233 asks for Gateway API–style policy conditions: Accepted: False with reason Invalid (PolicyReasonInvalid).
Which issue(s) this PR fixes:
Fixes #233
Does this PR introduce a user-facing change?: