fix: jwt provider cel and security policy validator - #9953
Conversation
Signed-off-by: kkk777-7 <kota.kimura0725@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: kkk777-7 <kota.kimura0725@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9953 +/- ##
=======================================
Coverage 81.33% 81.33%
=======================================
Files 264 264
Lines 40967 40994 +27
=======================================
+ Hits 33320 33343 +23
- Misses 7646 7650 +4
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 992efe9572
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err := validator(policy); err != nil { | ||
| status.SetConditionForPolicyAncestor(&policy.Status, | ||
| &ancestorRef, | ||
| t.GatewayControllerName, | ||
| gwapiv1.PolicyConditionAccepted, metav1.ConditionFalse, | ||
| egv1a1.PolicyReasonInvalid, | ||
| status.Error2ConditionMsg(err), | ||
| policy.Generation, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
When a merging route policy uses JWT authorization but its listener has no parent SecurityPolicy (the newly added httproute-no-parent-policy fixture exercises this), validation fails here and continue skips translation entirely. The generated route consequently has no security stanza or direct response, so traffic is unrestricted even though the policy specifies defaultAction: Deny; this can also happen transiently if the expected parent policy is removed. Preserve a fail-closed route/direct-response marker while reporting the policy as invalid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the behavior is right. with no parent policy, validation fails and translation is skipped.
that said, this is not specific to this PR. Every validateSecurityPolicy have same issue.
so I'd like to raise issue and handle follow up PRs.
1 participant
What this PR does / why we need it:
Fixed jwt provider CRD CEL. currently, reject policy that doesn't have jwt provider even when the policy has mergeType and parent policy has jwt provider.
Updated CRD CEL and added validation for jwt provider in gatewayapi translator layer.
Which issue(s) this PR fixes:
Fixes #9722
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.