Add UpstreamSettingsPolicy CRD

[bjee19]

Proposed changes

Problem: Users want to configure the behavior of the connection between NGINX and their upstream applications.

Solution: Add the UpstreamSettingsPolicy CRD, which is a direct policy that will attach to a Service that is referenced in an HTTPRoute or GRPCRoute.

Testing: Tested that validation works.

Closes #2502

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.

NONE

[bjee19]

Would like a review on the validation of fields, then I will test that the CRD can be installed and validation works. Then I will promote the PR to be ready for review.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.34%. Comparing base (bf17bd5) to head (186f2bf).

Additional details and impacted files

@@                        Coverage Diff                        @@
##           feature/upstream-settings-policy    #2515   +/-   ##
=================================================================
  Coverage                             89.34%   89.34%           
=================================================================
  Files                                   100      100           
  Lines                                  7628     7628           
  Branches                                 50       50           
=================================================================
  Hits                                   6815     6815           
  Misses                                  756      756           
  Partials                                 57       57           

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

[kate-osborn]

Just one small change to validation

[sjberman]

The design doc does mention adding the zoneSize field to the NginxProxy CRD as well. @kate-osborn Think it's worth adding that as part of this PR?

[kate-osborn]

The design doc does mention adding the zoneSize field to the NginxProxy CRD as well. @kate-osborn Think it's worth adding that as part of this PR?

Let's handle that separately. I can create an issue for it.

[bjee19]

Manually tested that current validation works.

[kate-osborn]

LGTM 🚀

[sindhushiv]

@bjee19 Shouldn't we include an example of how the CRD looks like?

[kate-osborn]

@bjee19 Shouldn't we include an example of how the CRD looks like?

I don't think we need to, but if you have an example @bjee19 it wouldn't hurt to add it. I recommend doing something like this: https://github.com/nginxinc/nginx-gateway-fabric/tree/feature/snippets-filter/examples/snippets-filter

[bjee19]

@kate-osborn

Looking at the default for keepalive time being 1 hour here https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_time

our current Duration definition we use here:

nginx-gateway-fabric/apis/v1alpha1/shared_types.go

Line 8 in bf17bd5

type Duration string

seems to only be in seconds to milliseconds.

This is an issue right? Does this affect other policies whose duration are longer and yet are being limited to be seconds or ms such as ClientSettingsPolicy's keepalive time here: https://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_time

[kate-osborn]

@bjee19 good find and question.

This came up on the PR for adding the ClientSettingsPolicy CRD, and we decided to support only ms and s: #1793 (comment). Looking back at all the fields that use this type, ms and s work make sense for everything except keepalive_timeout. I think this was just an oversight.

It's not a problem, per se, since you can express hours in seconds, but it's inconvenient. Let's change the validation on Duration to ^[0-9]{1,4}(ms|s|m|h)?$. Like Michael says in the linked thread, relaxing validation isn't a breaking change, so we can make this change without bumping the version of the CRDs that depend on it.

[bjee19]

@kate-osborn

I see, thanks for explaining and giving me more background.

Since this PR is going into a feature branch, should I make the change to the Duration validation in a separate PR targeting main? Also, would it be beneficial to cherry-pick that PR onto the recent release?

[github-actions]

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
_{Posted by the CLA Assistant Lite bot.}

[bjee19]

I have hereby read the F5 CLA and agree to its terms

[bjee19]

I messed some stuff up with the feature branch, i created it a while ago so it was quite behind main. When I rebased my local branch with main and didn't update the feature branch stuff got messed up. Things should be fixed.

[kate-osborn]

@kate-osborn

I see, thanks for explaining and giving me more background.

Since this PR is going into a feature branch, should I make the change to the Duration validation in a separate PR targeting main? Also, would it be beneficial to cherry-pick that PR onto the recent release?

Yes, let's fix in a separate PR that targets main. No, we don't need to cherry-pick it to the release branch. That would require a patch release, which this issue doesn't warrant.