Update AuthenticationFilter proposal #4424

shaun-nx · 2025-12-09T10:00:39Z

Proposed changes

This change updates the AuthenticationFilter proposal.

Changes made:

Update API validation
Removal of OnFailure configuration
Add additional details on functional testing

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

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.

…al-tests

codecov · 2025-12-09T10:05:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.09%. Comparing base (7c76b0f) to head (1bb11c0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4424   +/-   ##
=======================================
  Coverage   86.09%   86.09%           
=======================================
  Files         132      132           
  Lines       14381    14381           
  Branches       35       35           
=======================================
  Hits        12382    12382           
+ Misses       1788     1787    -1     
- Partials      211      212    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tataruty · 2025-12-09T10:15:05Z

docs/proposals/authentication-filter.md

+// LocalObjectReference specifies a local Kubernetes object.
+type LocalObjectReference struct {
+  // Name is the referenced object.
+  Name string `json:"name"`


what is this for?

This object is for the SecretRef for storing the name of the secret.
It used to be here in the API. It just got moved up.

tataruty · 2025-12-09T10:17:57Z

docs/proposals/authentication-filter.md

+
+- Resolved filter referenced by a single route rule within a single HTTP/GRPCRoute
+- Resolved filter referenced by multiple route rules within a single HTTP/GRPCRoute
+- Resolved filter referenced by multiple HTTP/GRPCRoutes


each root has own auth

The 2nd test in this list is intended to capture that test case.

maybe we have different dictionary. for me second i see like: one route with two rules, that is not valid

Ah right, I see what you mean. My scenario is multiple separate route rules. This is the same as what you mean.
But, we don't have a test case for multiple routes in one rule. I'll add a case for that as well 😄

tataruty · 2025-12-09T10:19:28Z

docs/proposals/authentication-filter.md

+Invalid reference scenarios:
+
+- Resolved filter referenced multiple times in a single route rule within a single HTTP/GRPCRoute
+- Resolved filter referenced multiple times in a single route rule within a single HTTP/GRPCRoute


twice the same>

tataruty · 2025-12-09T10:21:03Z

docs/proposals/authentication-filter.md

+
+- Resolved filter referenced multiple times in a single route rule within a single HTTP/GRPCRoute
+- Resolved filter referenced multiple times in a single route rule within a single HTTP/GRPCRoute
+- Resolved filter referenced multiple times by multiple route rules within a single HTTP/GRPCRoute


actually. What is expected behaviour here and above? i cannot understand the scenario as a functional test and not a unit

These are intended to be invalid testing cases.
But you make a good point, I don't list the expected outcome. I'll add those here as well

i see invalid functional tests for authentication as: Authentication failed or Authentication passed - for valid, means we get a response from the route behind auth filter
Resolved/Unresolved more sound like a unit test

tataruty · 2025-12-09T10:21:52Z

docs/proposals/authentication-filter.md

+- Resolved filter referenced multiple times in a single route rule within a single HTTP/GRPCRoute
+- Resolved filter referenced multiple times by multiple route rules within a single HTTP/GRPCRoute
+- Unresolved filter referenced by a single route rule within a single HTTP/GRPCRoute
+- Unresolved filter referenced by multiple route rules within a single HTTP/GRPCRoute


same question

…al-tests

Copilot

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.

docs/proposals/authentication-filter.md

Co-authored-by: Copilot <[email protected]>

…al-tests

Copilot

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.

Copilot · 2025-12-09T18:23:58Z

docs/proposals/authentication-filter.md

+type LocalObjectReference struct {
+  // Name is the referenced object.
+  Name string `json:"name"`
 }


The LocalObjectReference struct definition has been moved to after BasicAuth, but the comment on line 154 still refers to it. Consider adding a comment explaining that this is a simplified version compared to the Kubernetes core LocalObjectReference, which includes a key field.

tataruty · 2025-12-10T15:47:57Z

docs/proposals/authentication-filter.md

+
+### 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.


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?

sjberman · 2025-12-10T16:03:33Z

docs/proposals/authentication-filter.md

+  - 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
+


With our other functional tests, we also verify that the nginx configuration is correct.

salonichf5 · 2025-12-10T16:48:47Z

docs/proposals/authentication-filter.md

 ```text
 The realm value is a free-form string
 that can only be compared for equality with other realms on that
 server.  The server will service the request only if it can validate


Suggested change

server. The server will service the request only if it can validate

salonichf5 · 2025-12-10T16:50:34Z

docs/proposals/authentication-filter.md

 // +kubebuilder:object:root=true
 // +kubebuilder:storageversion
 // +kubebuilder:subresource:status
 // +kubebuilder:resource:categories=nginx-gateway-fabric,shortName=authfilter;authenticationfilter


is the short name only descriptive? or it can be used in the spec to specify fields?

salonichf5 · 2025-12-10T16:57:50Z

docs/proposals/authentication-filter.md

+      statusCode: 401
+      scheme: Basic


nit: use any other code to show case custom option, 401 is the default.

salonichf5 · 2025-12-10T18:59:49Z

docs/proposals/authentication-filter.md

 ### Additional Fields for JWT

-`require`, `tokenSource` and `propagation` are some additional fields that may be incldued in future updates to the API.
+`require`, `tokenSource` and `propagation` are some additional fields that may be included in future updates to the API.


it would be nice to just put the future updates in another section, i saw some spread out and got a little confused on what's part of this feature work and what could get added in the future.

shaun-nx added 4 commits December 9, 2025 08:51

Add initial valid and invalid filter reference scenarios

16338ce

Add details around resolved references

fdcc504

Update API spec for Basic Auth only

c82527a

Remove AuthFailure from API. Move to stretch goals

4163a49

shaun-nx requested review from a team as code owners December 9, 2025 10:00

github-project-automation bot added this to NGINX Gateway Fabric Dec 9, 2025

github-project-automation bot moved this to 🆕 New in NGINX Gateway Fabric Dec 9, 2025

Merge branch 'main' into docs/authentication-filter-proposal-function…

f1b1df6

…al-tests

github-actions bot added the documentation Improvements or additions to documentation label Dec 9, 2025

shaun-nx added 2 commits December 9, 2025 10:06

Fix typos and indentation

eb0b8ee

Fix reference to KeyCloak

bc31f0d

tataruty reviewed Dec 9, 2025

View reviewed changes

Add expected outcomes for each tests case

32c609b

shaun-nx requested a review from tataruty December 9, 2025 15:29

shaun-nx added 2 commits December 9, 2025 17:26

Merge branch 'main' into docs/authentication-filter-proposal-function…

2f5e400

…al-tests

Merge branch 'main' into docs/authentication-filter-proposal-function…

e202905

…al-tests

shaun-nx requested a review from Copilot December 9, 2025 17:35

Copilot AI reviewed Dec 9, 2025

View reviewed changes

Apply suggestions from code review

c90f2ac

Co-authored-by: Copilot <[email protected]>

shaun-nx requested a review from Copilot December 9, 2025 18:23

Merge branch 'main' into docs/authentication-filter-proposal-function…

1bb11c0

…al-tests

Copilot AI reviewed Dec 9, 2025

View reviewed changes

tataruty reviewed Dec 10, 2025

View reviewed changes

sjberman reviewed Dec 10, 2025

View reviewed changes

salonichf5 reviewed Dec 10, 2025

View reviewed changes


		### 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.


	server. The server will service the request only if it can validate

Update AuthenticationFilter proposal #4424

Are you sure you want to change the base?

Update AuthenticationFilter proposal #4424

Uh oh!

Conversation

shaun-nx commented Dec 9, 2025

Proposed changes

Checklist

Release notes

Uh oh!

codecov bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaun-nx Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tataruty Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tataruty Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Dec 9, 2025 •

edited

Loading

shaun-nx Dec 9, 2025 •

edited

Loading

tataruty Dec 9, 2025 •

edited

Loading

tataruty Dec 9, 2025 •

edited

Loading