Update AuthenticationFilter proposal #4424
Conversation
github-actions
bot
added
the
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4424 +/- ##
==========================================
- Coverage 86.24% 86.19% -0.05%
==========================================
Files 132 132
Lines 14566 14566
Branches 35 35
==========================================
- Hits 12562 12555 -7
- Misses 1791 1795 +4
- Partials 213 216 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the AuthenticationFilter proposal by refining the API design, removing the OnFailure configuration from the main spec, and enhancing documentation quality. The changes move AuthFailure customization to stretch goals while improving clarity throughout the proposal.
Key Changes:
- Removed OnFailure configuration from BasicAuth and JWTAuth specs, relocating it to stretch goals
- Enhanced functional testing section with detailed test case scenarios
- Fixed numerous spelling and grammatical errors throughout the document
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ### Functional Test Cases | ||
|
|
||
| Note: The keyword "resolved" is used to refer to a filter that the controller has found, and matches the reference of the route rule. |
There was a problem hiding this comment.
i see invalid functional tests for authentication as Authentication failed, and Authentication passed - for valid, means we get a response from the route behind auth filter.
Don't we do this full check in functional tests?
| - Expected outcome: Requests to all route rules referencing the filter successfully process authentication requests | ||
| - Resolved filter referenced by rules in multiple HTTP/GRPCRoutes | ||
| - Expected outcome: Requests to all route rules across each HTTP/GRPCRoute successfully process authentication requests | ||
|
|
There was a problem hiding this comment.
With our other functional tests, we also verify that the nginx configuration is correct.
Co-authored-by: Saloni Choudhary <146118978+salonichf5@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| This section covers deployment scenarios that are considered invalid | ||
|
|
||
| - Single route rule with a single path in an HTTPRoute/GRPCRoute referencing an invalid AuthenticationFilter |
There was a problem hiding this comment.
still cannot understand what is invalid AuthFilter. What do we expect as a result of test? Because i see it as invalid configuration, that to me should be a unit tests and not functional. And as mentioned before i expect here validation of passing/not passing auth while trying to get response
There was a problem hiding this comment.
The first line of this section says "This section covers deployment scenarios that are considered invalid"
So for the first test cases, we have a user deploying an HTTP/GRPCroute with a single route rule that is referencing an AuthenticationFilter that is invalid.
In this case, we don't care "how" the AuthenticationFilter that is invalid, we just case that it is invalid, and we are trying to use it. Does that make sense?
There was a problem hiding this comment.
The test cases should cover the expected outcomes for each one now. Let me know what you think.
Proposed changes
This change updates the AuthenticationFilter proposal.
Changes made:
Details related to AuthFailure are captured in the Stretch Goals section
Closes #4423
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.