Fix Nplus directives definition

nginxinc / nginx-go-crossplane

A library for working with NGINX configs in Go

Apache License 2.0

46 stars 12 forks source link

Fix Nplus directives definition #105

Closed dengsh12 closed 1 month ago

dengsh12 commented 1 month ago

Proposed changes

In #100 we provided support files for ParseOptions.DirectiveSources. For some reason analyze_nplus_latest_directives.go was override to a legacy version with mismatch from NGX_CONF_TAKE4 to ngxConfTake2. This PR is to fix those directives(keyval and limit_req_zone). Thanks to @oliveromahony found this problem.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

[x] I have read the CONTRIBUTING document
[x] If applicable, I have added tests that prove my fix is effective or that my feature works
[x] If applicable, I have checked that any relevant tests pass after adding my changes
[ ] I have updated any relevant documentation (README.md)

dengsh12 commented 1 month ago

Approved, but let's think about how we might be able to test this kind of scenario to see if there are any more errors (and prevent them in the future).

Sure! I think such problems boil down to some small errors in generator leading to error in only one or two directives. Current UT unlikely to discover such problems. I'm thinking of adding a huge "standard map" which includes most of directives to test the latest OSS/Nplus matchFunc(kind of like what I was doing in POC).

ornj commented 1 month ago

@dengsh12 tests that will cover the generator tool will be useful when that lands. Maybe that's enough if there's enough coverage of the bitmasks.

ryepup commented 1 month ago

Approved, but let's think about how we might be able to test this kind of scenario to see if there are any more errors (and prevent them in the future).

Sure! I think such problems boil down to some small errors in generator leading to error in only one or two directives. Current UT unlikely to discover such problems. I'm thinking of adding a huge "standard map" which includes most of directives to test the latest OSS/Nplus matchFunc(kind of like what I was doing in POC).

yet another case where a corpus of anonymized NGINX configs would be real handy

dengsh12 commented 1 month ago

update: added ut from @oliveromahony's PR #104