Add Functional tests for ProxySettingsPolicy #4482

tataruty · 2025-12-17T19:23:56Z

Problem: As an NGF developer
I want functional tests for the ProxySettingsPolicy
So that I can ensure that nginx behaves properly when defining proxy settings.

Acceptance:
Functional tests are added for ProxySettingsPolicy
https://github.com/nginx/nginx-gateway-fabric/blob/main/docs/proposals/proxy-settings.md#testing
verify nginx config looks correct

Solution: Added next Functional tests that test the attachment and inheritance behavior, including:

Policy attached to Gateway only

policy accepted
checked nginx.conf
checked request to coffee(failed) and tea)

Policy attached to Route only

checked policy accepted,
checked nginx.conf
checked request to coffee and tea)

Policy attached to both Gateway and Route

valid ProxySettingsPolicies are created for both: Gateway and HTTPRoute
valid ProxySettingsPolicies are created for both: Gateway and GRPCRoute
valid ProxySettingsPolicies are disabled for both: Gateway and HTTPRoute
valid big buffer Gateway ProxySettingsPolicies are inherited by HTTPRoute
valid enable for HTTPRoute ProxySettingsPolicies overrides disabled Gateway settings
valid disable for Gateway ProxySettingsPolicies is ovewritten by HTTPRoute default disable=false value
valid disable for HTTPRoute ProxySettingsPolicies with unsufficient buffer size
valid disable for HTTPRoute ProxySettingsPolicies does not override enabled Gateway setting
valid HTTPRoute ProxySettingsPolicies with more than one TargetRef

Validation tests for invalid configurations

invalid conflicting HTTPRoute ProxySettingsPolicies
invalid buffering Gateway and HTTPRoute ProxySettingsPolicies

Closes #4264

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

NONE

salonichf5 · 2025-12-18T19:17:08Z

tests/suite/proxy_settings_test.go

+	// ????????????????????????????????????????????????// ????????????????????????????????
+	// Route policy specifies buffer sizes without setting disable: false (should set PartiallyInvalid condition)
+	When("valid disable for Gateway ProxySettingsPolicies is inherited by HTTPRoute", func() {
+		var (
+			policies = []string{
+				"proxy-settings-policy/gateway-disabled-coffee-not-enabled-proxy-settings.yaml",
+			}
+
+			baseURL string
+		)
+
+		BeforeAll(func() {
+			Expect(resourceManager.ApplyFromFiles(policies, namespace)).To(Succeed())
+
+			port := 80
+			if portFwdPort != 0 {
+				port = portFwdPort
+			}
+
+			baseURL = fmt.Sprintf("http://cafe.example.com:%d", port)
+		})
+
+		AfterAll(func() {
+			Expect(resourceManager.DeleteFromFiles(policies, namespace)).To(Succeed())
+		})
+
+		Specify("they are accepted by the target resource", func() {
+			policyNames := []string{
+				"gateway-proxy-settings",
+				"coffee-proxy-settings",
+			}
+
+			for _, name := range policyNames {
+				nsname := types.NamespacedName{Name: name, Namespace: namespace}
+
+				err := waitForPSPolicyStatus(nsname)
+				Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name))
+			}
+		})
+
+		Context("verify working traffic", func() {
+			// ????????????????????????????????????????????????
+			It("should return a 200 response only for HTTPRoute tea, and fail for coffee", func() {
+				baseCoffeeURL := baseURL + "/coffee"
+				baseTeaURL := baseURL + "/tea"
+
+				Eventually(


In this case reading the design proposal

Important: If a Gateway policy disables buffering (disable: true), and a Route policy specifies buffer size fields (bufferSize, buffers, or busyBuffersSize) without explicitly setting disable: false, the buffer size fields will be ignored since buffering is disabled. A Condition will be set on the Route policy to indicate that these fields are being ignored. To use buffer size settings when the Gateway has disabled buffering, the Route policy must explicitly set disable: false.

If gateway disables proxy buffering and disable: false is not explicitly set which is in the example you are using, the proxy buffer fields are ignored and a proper error message should be communicated on the route saying these fields are ignored. Are you able to see the error message on coffee route about these fields being ignored? That's what needs to be checked here.

But if disable: false was explicitly set on the route proxy settings policy the request would succeed (not your case)

nothing like this

salonichf5 · 2025-12-18T19:23:43Z

tests/suite/proxy_settings_test.go

+	When("valid disable for HTTPRoute ProxySettingsPolicies overrides enabled Gateway setting", func() {
+		var (
+			policies = []string{
+				"proxy-settings-policy/gateway-enabled-coffee-disabled-proxy-settings.yaml",
+			}
+
+			baseURL string
+		)
+
+		BeforeAll(func() {
+			Expect(resourceManager.ApplyFromFiles(policies, namespace)).To(Succeed())
+
+			port := 80
+			if portFwdPort != 0 {
+				port = portFwdPort
+			}
+
+			baseURL = fmt.Sprintf("http://cafe.example.com:%d", port)
+		})
+
+		AfterAll(func() {
+			Expect(resourceManager.DeleteFromFiles(policies, namespace)).To(Succeed())
+		})
+
+		Specify("they are accepted by the target resource", func() {
+			policyNames := []string{
+				"gateway-proxy-settings",
+				"coffee-proxy-settings",
+			}
+
+			for _, name := range policyNames {
+				nsname := types.NamespacedName{Name: name, Namespace: namespace}
+
+				err := waitForPSPolicyStatus(nsname)
+				Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name))
+			}
+		})
+


Looking at the implementation, we set a proxy_buffering off directive on the location block when buffering is disabled at Route level, are you seeing that? We should verify that directive exists and request for coffee should fail,

Whereas tea should pass because it will inherit the settings from the Gateway which has proxy buffering enabled.

I guess if we didn't explicitly set it off then coffee location would still inherit Gateway settings.

Let me know if you are tests results look off from the desired and I will look deeper.

i've added more tests and still a lot of unclear inheritance behaviour. Asked in slack channel if someone can help

tataruty · 2025-12-23T15:52:12Z

tests/framework/request.go

 	return resp.StatusCode, body.String(), nil
 }

+func printResponseBody(body *bytes.Buffer) {


to cut body in logs:

sjberman · 2025-12-23T19:12:54Z

tests/framework/request.go

+		// Create request without a short-lived context; rely on http.Client.Timeout
+		// so that the response body can be fully read by the caller without
+		// being canceled when this function returns.
+		req, err = http.NewRequest(method, url, body) //nolint:noctx


Why not just provide a context with a smaller timeout? That way we don't need this conditional.

that is what i did at the beginning: i added parameter and passed timeout from test, but the thing is that it had no sense somehow: once request was sent - context was cancelled regardless size of timeout.
If i get bored next week i can try again :D

why smaller timeout?

Oh I misread. Could just do a larger timeout.

yes, i tried larger :D so, smaller made me think i did it wrongly

Oh, context is cancelled because of the defer cancel() when the function exits.

exactly. But i still want to try to use timeout, but in this case i guess i'll need to cancel it in the parent func.

sjberman · 2025-12-23T19:16:33Z

tests/suite/proxy_settings_test.go

+					Should(Succeed())
+			})
+		})
+	})


There's a lot of duplication in these tests. Every case, we verify that the policies are accepted and that traffic can flow. Let's de-duplicate all of this.

i'm not sure what can be de-duplicated, since the polilicy acceptance doesn't mean there is expected behaviour on requests.
In case if request behaviour is not what we can impact, then we can remove tests with requests.
But i'd like to keep them till we understand the behaviour and maybe we will need to document it for such configurations. I realy would like @ciarams87 to be available for this discussion

I just mean define a common function so we don't have duplicated code.

ah, ok.
Because i thought you mean that there is too many tests and it can slow down CI

Add Functional tests for ProxySettingsPolicy

28df2f8

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

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

github-actions bot added the tests Pull requests that update tests label Dec 17, 2025

salonichf5 reviewed Dec 18, 2025

View reviewed changes

Add more conflicts

8785389

tataruty marked this pull request as ready for review December 23, 2025 15:48

tataruty requested a review from a team as a code owner December 23, 2025 15:48

tataruty commented Dec 23, 2025

View reviewed changes

tataruty requested a review from ciarams87 December 23, 2025 18:28

sjberman reviewed Dec 23, 2025

View reviewed changes

Add Functional tests for ProxySettingsPolicy #4482

Are you sure you want to change the base?

Add Functional tests for ProxySettingsPolicy #4482

Uh oh!

Conversation

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

Uh oh!

Checklist

Release notes

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tataruty commented Dec 17, 2025 •

edited

Loading