Skip to content

Conversation

shaun-nx
Copy link
Contributor

@shaun-nx shaun-nx commented Aug 27, 2025

Proposed changes

This change ensures that, when deploying a HTTPRoute with an advanced routing configuration, and a ClientSettingsPolicy is targeting this kind of route, we do not duplicate client settings in NGINX

Fixes (#3763)

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.

Prevent policy `includes` duplication in advanced routing configuration

@shaun-nx shaun-nx requested a review from a team as a code owner August 27, 2025 16:37
@github-actions github-actions bot added the bug Something isn't working label Aug 27, 2025
Copy link

codecov bot commented Aug 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.00%. Comparing base (c3d544b) to head (9839fd4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3799   +/-   ##
=======================================
  Coverage   87.00%   87.00%           
=======================================
  Files         128      128           
  Lines       16404    16404           
  Branches       62       62           
=======================================
  Hits        14272    14272           
  Misses       1957     1957           
  Partials      175      175           

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

@salonichf5
Copy link
Contributor

salonichf5 commented Aug 28, 2025

does the bug only occur when we attach the CSP to HTTPRoute? have we verified this for the gateway?

@shaun-nx
Copy link
Contributor Author

does the bug only occur when we attach the CSP to HTTPRoute? have we verified this for the gateway?

That's a great question. I'll check if that's the case.

@shaun-nx
Copy link
Contributor Author

@salonichf5 @sjberman the test is working now.
I ran the test against the version of the code that contains the bug and the test case fails

This here is an example of the Actual and Expected config results when running the test against the code with the bug

Actual:

[{SSL:<nil> Hostname: PathRules:[] Policies:[0x140005d8a88 0x140005d9508] Port:80 IsDefault:true} {SSL:<nil> Hostname:policy.com PathRules:[{Path:/rest PathType:prefix MatchRules:[{Filters:{InvalidFilter:<nil> RequestRedirect:<nil> RequestURLRewrite:<nil> RequestMirrors:[] RequestHeaderModifiers:<nil> ResponseHeaderModifiers:<nil> SnippetsFilters:[]} Source:&ObjectMeta{Name:hr-advanced-route-with-policy-header-match,GenerateName:,Namespace:test,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{},OwnerReferences:[]OwnerReference{},Finalizers:[],ManagedFields:[]ManagedFieldsEntry{},} Match:{Method:<nil> Headers:[{Name:Referrer Value:(?i)(mydomain|myotherdomain).+\.example\.(cloud|com) Type:RegularExpression}] QueryParams:[]} BackendGroup:{Source:test/hr-advanced-route-with-policy-
header-match Backends:[{VerifyTLS:<nil> UpstreamName:test_foo_80 Weight:1 Valid:true}] RuleIdx:0}} {Filters:{InvalidFilter:<nil> RequestRedirect:<nil> RequestURLRewrite:<nil> RequestMirrors:[] RequestHeaderModifiers:<nil> ResponseHeaderModifiers:<nil> SnippetsFilters:[]} Source:&ObjectMeta{Name:hr-advanced-route-with-policy-header-match,GenerateName:,Namespace:test,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{},OwnerReferences:[]OwnerReference{},Finalizers:[],ManagedFields:[]ManagedFieldsEntry{},} Match:{Method:<nil> Headers:[] QueryParams:[]} BackendGroup:{Source:test/hr-advanced-route-with-policy-header-match Backends:[{VerifyTLS:<nil> UpstreamName:test_foo_80 Weight:1 Valid:true}] RuleIdx:0}}] Policies:[0x140005f0008 0x140005f0008] GRPC:false}] Policies:[0x140005d8a88 0x140005d9508] Port:80 IsDefault:false}]

Expected:

[{SSL:<nil> Hostname: PathRules:[] Policies:[0x140005d8a88 0x140005d9508] Port:80 IsDefault:true} {SSL:<nil> Hostname:policy.com PathRules:[{Path:/rest PathType:prefix MatchRules:[{Filters:{InvalidFilter:<nil> RequestRedirect:<nil> RequestURLRewrite:<nil> RequestMirrors:[] RequestHeaderModifiers:<nil> ResponseHeaderModifiers:<nil> SnippetsFilters:[]} Source:&ObjectMeta{Name:hr-advanced-route-with-policy-header-match,GenerateName:,Namespace:test,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{},OwnerReferences:[]OwnerReference{},Finalizers:[],ManagedFields:[]ManagedFieldsEntry{},} Match:{Method:<nil> Headers:[{Name:Referrer Value:(?i)(mydomain|myotherdomain).+\.example\.(cloud|com) Type:RegularExpression}] QueryParams:[]} BackendGroup:{Source:test/hr-advanced-route-with-policy
-header-match Backends:[{VerifyTLS:<nil> UpstreamName:test_foo_80 Weight:1 Valid:true}] RuleIdx:0}} {Filters:{InvalidFilter:<nil> RequestRedirect:<nil> RequestURLRewrite:<nil> RequestMirrors:[] RequestHeaderModifiers:<nil> ResponseHeaderModifiers:<nil> SnippetsFilters:[]} Source:&ObjectMeta{Name:hr-advanced-route-with-policy-header-match,GenerateName:,Namespace:test,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{},OwnerReferences:[]OwnerReference{},Finalizers:[],ManagedFields:[]ManagedFieldsEntry{},} Match:{Method:<nil> Headers:[] QueryParams:[]} BackendGroup:{Source:test/hr-advanced-route-with-policy-header-match Backends:[{VerifyTLS:<nil> UpstreamName:test_foo_80 Weight:1 Valid:true}] RuleIdx:0}}] Policies:[0x140005f0008] GRPC:false}] Policies:[0x140005d8a88 0x140005d9508] Port:80 IsDefault:false}]

Difference

  • Actual:
    The "policy.com" VirtualServer has Policies: [0x140005f0008 0x140005f0008] (i.e., two policies, but both are the same pointer, so it's a duplicate).
  • Expected:
    The "policy.com" VirtualServer has Policies: [0x140005f0008] (i.e., only one policy).

What this means

  • The actual config is attaching the same policy object twice to the Policies field of the "policy.com" VirtualServer.
  • The expected config is attaching it only once.

Copy link
Collaborator

@sjberman sjberman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@shaun-nx
Copy link
Contributor Author

does the bug only occur when we attach the CSP to HTTPRoute? have we verified this for the gateway?

That's a great question. I'll check if that's the case.

@salonichf5 just on this. This bug doesn't affect the Gateway resource as the bug is related to how Policies are added to hostPathRules specifically.

Copy link
Contributor

@salonichf5 salonichf5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the clarification

@shaun-nx shaun-nx merged commit 7e3e6fc into main Aug 28, 2025
44 checks passed
@shaun-nx shaun-nx deleted the bug/clientsettingspolicy-multiple-route-rule branch August 28, 2025 16:23
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in NGINX Gateway Fabric Aug 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release-notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

"client_max_body_size" directive is duplicate for a ClientSettingsPolicy attached to an HTTPRoute with multiple rules
3 participants