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

Apply configuration of SnippetsFilters to NGINX #2604

Open
wants to merge 15 commits into
base: feature/snippets-filter
Choose a base branch
from

Conversation

kate-osborn
Copy link
Contributor

@kate-osborn kate-osborn commented Sep 25, 2024

Proposed changes

Problem: As a user of NGF, I want my SnippetsFilters configuration applied to NGF's data plane, so that I can leverage NGINX features not yet available in NGF.

Solution: Apply configuration of valid SnippetsFilters referenced in HTTPRoutes and GRPCRoutes to the appropriate contexts in the NGINX config. If the SnippetsFilter referenced is invalid (wrong group or kind), the routing rule is not configured. If the SnippetsFilter cannot be resolved, the routing rule is configured, but the route will return a 500.

Documentation will be added in a separate PR and is tracked by #2377.

Testing:

Description Expectation
Basic Example. Route that references a valid SnippetsFilter. Snippets applied to appropriate contexts.
Route with multiple rules, with one that references a valid SnippetsFilter. Location snippets only applied to location corresponding to the rule with the SnippetsFilter
Route that references a SnippetsFilter with invalid group. Route not configured. Accepted:False
Route that references a SnippetsFilter with invalid kind. Route not configured. Accepted:False
Route that references a SnippetsFilter that doesn't exist. Route configured and returns 500. Accepted:True ResolvedRefs:False
Route with multiple distinct SnippetsFilters in the same rule All snippets applied to appropriate contexts.
Route with multiple distinct SnippetsFilters in different rules Location snippets applied distinctly to the right locations. Main, http, and server contexts "merged" (two included per context)
Route with multiple rules that reference same SnippetsFilter Snippets are deduplicated, only one include per context.
Route with a single rule that reference the same SnippetsFilter twice Snippets are deduplicated, only one include per context.
Multiple routes with distinct SnippetsFilters and same listener Snippets are merged at main, http, and server context, and distinct in the listener context.
Multiple routes with distinct SnippetsFilters and different listener Snippets are merged at main, and http contexts, and distinct in the http and listener contexts.
Multiple routes with the same SnippetsFilters and same listener Snippets are deduplicated, only one include per context.
Route with a valid filter and invalid ExtensionRef filter Route not configured. Accepted:False
Route with a valid filter and unresolvable ExtensionRef filter Route configured and returns a 500. Accepted:True ResolvedRefs:False
Basic Example with GRPC. Route that references a valid SnippetsFilter. Snippets applied to appropriate contexts.
Basic Example with N+. Route that references a valid SnippetsFilter. Snippets applied to appropriate contexts.

Closes #2417

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 September 25, 2024 17:33
@github-actions github-actions bot added enhancement New feature or request helm-chart Relates to helm chart labels Sep 25, 2024
@kate-osborn
Copy link
Contributor Author

I'm looking into the unit test failure. It's another data race 😭

g.Expect(servers[i].Includes).To(BeEmpty())
} else {
g.Expect(server.Includes).To(ConsistOf(servers[i].Includes))
g.Expect(server.Locations).To(HaveLen(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks the length of locations before indexing into the location in the next line. If we don't check that the length is one, and then try to access the 0th element, the test will panic.

Copy link
Contributor

@bjee19 bjee19 Sep 26, 2024

Choose a reason for hiding this comment

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

@kate-osborn Sorry maybe I should have been a little more clear, how come the check is on the expServers's server instead of the servers Location length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Nice catch. Should be servers[i]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should also switch the expected and actual in the next assertion

internal/mode/static/state/dataplane/configuration_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/configuration_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/common_filter_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
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: 🏗 In Progress
Development

Successfully merging this pull request may close these issues.

3 participants