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

Watch UpstreamSettingsPolicies and translate into dataplane configuration #2887

Merged

Conversation

kate-osborn
Copy link
Contributor

Proposed changes

Problem: As a user
I 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.

Testing:

  • Unit tests
  • Manually verified status is written to UpstreamSettingsPolicies
  • Manually verified that NGF watches UpstreamSettingsPolicies

Closes #2810 #2812

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

@kate-osborn kate-osborn requested a review from a team as a code owner December 11, 2024 18:21
@github-actions github-actions bot added enhancement New feature or request helm-chart Relates to helm chart labels Dec 11, 2024
internal/mode/static/state/graph/policies.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/policies.go Show resolved Hide resolved
internal/mode/static/state/graph/policies_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/policies_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/service_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/service_test.go Outdated Show resolved Hide resolved
@kate-osborn kate-osborn requested a review from sjberman December 11, 2024 21:40
Copy link
Contributor

@bjee19 bjee19 left a comment

Choose a reason for hiding this comment

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

LGTM after singular change 🚀

@kate-osborn
Copy link
Contributor Author

@bjee19 @sjberman re-requested reviews because I pushed a new commit: 3ed9e17

During manual testing yesterday, I found a bug where we were not writing status to the upstream settings policy when the Gateway was invalid. This is because we only added services to the ReferencedServices map if they were attached to at least one parent ref. This behavior also caused misleading status on routes that were attached to invalid Gateways when the Services they reference were processed after the route itself. The route would have Resolved/Refs/False condition, which would remain even after the referenced service was processed by NGF. I changed the behavior of ReferencedServices to contain all the Services referenced by routes that "belong" to the winning Gateway. I believe this is the right behavior.

@kate-osborn kate-osborn merged commit b968639 into nginx:feature/upstream-settings-policy Dec 13, 2024
44 checks passed
kate-osborn added a commit to kate-osborn/nginx-gateway-fabric that referenced this pull request Dec 20, 2024
…tion (nginx#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.
kate-osborn added a commit that referenced this pull request Dec 20, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request helm-chart Relates to helm chart
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants