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.22%. Comparing base (89aee48) to head (bb91ce1).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4424      +/-   ##
==========================================
- Coverage   86.24%   86.22%   -0.02%     
==========================================
  Files         132      132              
  Lines       14566    14566              
  Branches       35       35              
==========================================
- Hits        12562    12560       -2     
- Misses       1791     1793       +2     
  Partials      213      213

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

docs/proposals/authentication-filter.md

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

docs/proposals/authentication-filter.md

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.

docs/proposals/authentication-filter.md

…al-tests

Co-authored-by: Saloni Choudhary <[email protected]>

Copilot

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.

tataruty · 2025-12-15T09:28:20Z

docs/proposals/authentication-filter.md

+
+This section covers deployment scenarios that are considered invalid
+
+- Single route rule with a single path in an HTTPRoute/GRPCRoute referencing an invalid AuthenticationFilter


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

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?

The test cases should cover the expected outcomes for each one now. Let me know what you think.

salonichf5

There is a couple of places with typos for marked but looks good to me, approving it

sjberman · 2025-12-15T19:22:20Z

docs/proposals/authentication-filter.md

-The full path will use the `name` and `key` of the secret referenced by `AuthenticationFilter`
-In this case, the full path will be `/etc/nginx/secrets/basic-auth-users/htpasswd`
+The full path will use the `name`, `namespace`, and `key` of the secret referenced by `AuthenticationFilter`
+In this case, the full path will be `/etc/nginx/secrets/default/basic-auth-users/auth`


We use a different pattern right now for storing user secrets. For example, /etc/nginx/secrets/cert_bundle_<namespace>_<name>. I think we do something here to avoid the unnecessary nested directories. It doesn't need to be named auth, because we aren't mounting the file, we're sending it and it can be named whatever we want.

Maybe something like /etc/nginx/secrets/basic_auth_<namespace>_<name>

Ah, good to know thanks!

Now that you mention it, we don't actually need to include the key in the file name either since it will be unique enough with the namespace and name.

So we can use /etc/nginx/secrets/<secret-namespace>_<secret-name>

Actually, lets include basic_auth at the start. It'll make debugging easier.

sjberman · 2025-12-15T19:22:45Z

docs/proposals/authentication-filter.md

-The full path will use the `name` and `key` of the secret referenced by `AuthenticationFilter`
-In this case, the full path will be `/etc/nginx/keys/jwt-keys-secure/jwks.json`
+The full path will use the `name`, `namespace`, and `key` of the secret referenced by `AuthenticationFilter`
+In this case, the full path will be `/etc/nginx/keys/default/jwt-keys-secure/jwks.json`


Same comment as above regarding naming.

sjberman · 2025-12-15T19:24:10Z

docs/proposals/authentication-filter.md

+- An AuthenticationFilter deployed with an empty `Realm` value
+- An AuthenticationFilter deployed with an empty `secretRef.Name` value


I don't think we verify OpenAPI schema on our other policies, so these can be skipped.

sjberman · 2025-12-15T19:25:01Z

docs/proposals/authentication-filter.md

+Two or more route rules each with two or more paths in an HTTPRoute/GRPCRoute referencing an invalid AuthenticationFilter
+- Expected outcomes:
+  Both route rules are marked as invalid.
+  Requets to each paths in each route rule will return a 500 error.


I'd do a typo pass on this doc again, there are quite a few misspellings.

sjberman · 2025-12-15T19:26:49Z

internal/controller/nginx/agent/grpc/context/doc.go

 Package context contains the functions for storing extra information in the gRPC context.
 */
-package context //nolint:revive // ignoring conflicting package name
+package context


This has been an inconsistent lint failure and I'm not sure why. We probably want a separate PR to handle this if needed, not tagged onto a doc update.

Interesting, I didn't notice this file was here in the files changed.
Did the linter remove the //nolint comment? I don't remember doing this myself.

Hm, it must have. That's odd.

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

docs/proposals/authentication-filter.md Show resolved Hide resolved

tataruty reviewed Dec 9, 2025

View reviewed changes

docs/proposals/authentication-filter.md Outdated Show resolved Hide resolved

tataruty reviewed Dec 9, 2025

View reviewed changes

docs/proposals/authentication-filter.md Outdated Show resolved Hide resolved

tataruty reviewed Dec 9, 2025

View reviewed changes

docs/proposals/authentication-filter.md Outdated Show resolved Hide resolved

tataruty reviewed Dec 9, 2025

View reviewed changes

docs/proposals/authentication-filter.md Outdated Show resolved Hide resolved

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

docs/proposals/authentication-filter.md Show resolved Hide resolved

tataruty reviewed Dec 10, 2025

View reviewed changes

sjberman reviewed Dec 10, 2025

View reviewed changes

Mention testing of correct NGINX configuration

3bb040e

salonichf5 reviewed Dec 10, 2025

View reviewed changes

docs/proposals/authentication-filter.md Outdated Show resolved Hide resolved

docs/proposals/authentication-filter.md Show resolved Hide resolved

docs/proposals/authentication-filter.md Show resolved Hide resolved

docs/proposals/authentication-filter.md Show resolved Hide resolved

shaun-nx and others added 7 commits December 12, 2025 14:50

Update functional test cases

137b22d

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

67292f2

…al-tests

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

8671366

…al-tests

Add details on custom secret types for Basic and JWT auth

5f8baf5

Change stretch goals to future updates

364b294

Fix punctuation

f9a31cc

Update docs/proposals/authentication-filter.md

72765f3

Co-authored-by: Saloni Choudhary <[email protected]>

shaun-nx requested review from Copilot, salonichf5, sjberman and tataruty December 15, 2025 08:17

Copilot AI reviewed Dec 15, 2025

View reviewed changes

tataruty reviewed Dec 15, 2025

View reviewed changes

Update tests to cover expected outcomes for each scenario

0ca1e03

salonichf5 approved these changes Dec 15, 2025

View reviewed changes

sjberman reviewed Dec 15, 2025

View reviewed changes

shaun-nx added 4 commits December 16, 2025 08:22

Correct typos and grammer

d69ee1a

Remove scenarios related to invalid AuthenticationFilter deployments

d8b66b0

Update details on secret path and filename creation

98ceae3

Update heading casing

bb91ce1


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


		This section covers deployment scenarios that are considered invalid

		- Single route rule with a single path in an HTTPRoute/GRPCRoute referencing an invalid AuthenticationFilter

		- An AuthenticationFilter deployed with an empty `Realm` value
		- An AuthenticationFilter deployed with an empty `secretRef.Name` value

Update AuthenticationFilter proposal #4424

Are you sure you want to change the base?

Update AuthenticationFilter proposal #4424

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

salonichf5 left a comment

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!

shaun-nx Dec 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov bot commented Dec 9, 2025 •

edited

Loading

shaun-nx Dec 16, 2025 •

edited

Loading