Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UpstreamSetttingsPolicy #2941

Merged
merged 7 commits into from
Dec 20, 2024
Merged

Conversation

kate-osborn
Copy link
Contributor

Proposed changes

Problem: As a user, I want to be able to configure the upstream settings for a Service referenced by a HTTP or GRPCRoute.

Solution: Add UpstreamSettingsPolicy CRD. This is a direct policy that can be attached to one or more Services.
The Service must be referenced by an HTTP or GRPCRoute that is owned by the "winning" NGF Gateway.

Testing: Unit, manual, and automated functional tests.

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.

Add UpstreamSettingsPolicy to allow users to configure upstream settings for Services.

@kate-osborn kate-osborn requested review from a team as code owners December 20, 2024 20:23
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request tests Pull requests that update tests helm-chart Relates to helm chart labels Dec 20, 2024
bjee19 and others added 6 commits December 20, 2024 13:24
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.
Translate data plane intermediary UpstreamSettingsPolicy configuration into NGINX configuration.

Problem: I want the data plane configuration generated from my UpstreamSettingsPolicy to be translated
into NGINX Configuration.

Solution: Translate the data plane UpstreamSettingsPolicy configuration into NGINX configuration.

Testing: Unit tests.
…tion (#2887)

Problem: As a userI want NGF to take my configuration for an UpstreamSettingsPolicy
and transform it into data plane configuration within NGF, so that NGF can then translate
those settings into NGINX configuration, and so that NGF maintains an abstraction layer
between data plane configuration and the specific data plane NGF uses.

Solution: Add controller to watch UpstreamSettingsPolicies, and store them in the cluster state
as generic NGF Policies. Update the graph to validate and process these policies and attach
them to the relevant Services. When building the dataplane configuration, store the policies
on the relevant http upstreams.
Problem: Want to collect number of UpstreamSettingsPolicies in cluster.

Solution: Collect the count of UpstreamSettingsPolicies in cluster.
Problem: When an UpstreamSettingsPolicy targeted multiple Services, 
NGF would write duplicate ancestors to its status.

Solution: Only add unique ancestors to policy ancestors list.
Functional Tests for UpstreamSettings Policy.
@kate-osborn kate-osborn force-pushed the feature/upstream-settings-policy branch from a8120ca to b0ed7e2 Compare December 20, 2024 20:24
Copy link
Contributor

Deploy Preview will be available once build job completes!

Name Link
😎 Deploy Preview https://frontdoor-test-docs.nginx.com/previews/nginx-gateway-fabric/2941/

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 97.39884% with 9 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (938b7ff) to head (be4d349).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/mode/static/manager.go 20.00% 4 Missing ⚠️
internal/mode/static/state/graph/policies.go 94.11% 2 Missing and 1 partial ⚠️
...ternal/mode/static/nginx/config/policies/policy.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2941      +/-   ##
==========================================
+ Coverage   89.74%   89.93%   +0.18%     
==========================================
  Files         109      111       +2     
  Lines       11150    11421     +271     
  Branches       50       50              
==========================================
+ Hits        10007    10271     +264     
- Misses       1083     1089       +6     
- Partials       60       61       +1     

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

@sjberman
Copy link
Contributor

@kate-osborn When merging, can you make sure @bjee19 and @salonichf5 are also listed as contributors? Everyone deserves their credit :)

@kate-osborn
Copy link
Contributor Author

@kate-osborn When merging, can you make sure @bjee19 and @salonichf5 are also listed as contributors? Everyone deserves their credit :)

Of course! Thanks for the reminder

@kate-osborn kate-osborn requested a review from sjberman December 20, 2024 22:22
@kate-osborn kate-osborn merged commit 6fad005 into main Dec 20, 2024
50 checks passed
@kate-osborn kate-osborn deleted the feature/upstream-settings-policy branch December 20, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart tests Pull requests that update tests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants