limit_req_zone with 4 arguments not being analyzed

[oliveromahony]

Proposed changes

Limit request zone has an issue with not being throwing: invalid number of arguments in "limit_req_zone" directive error despite being 4 arguments.

Checklist

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

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

[ryepup]

I think this test would pass without any other changes if we use a different source

Suggested change

	DirectiveSources: []MatchFunc{NgxPlusLatestDirectivesMatchFn, AppProtectWAFv4DirectivesMatchFn},
	DirectiveSources: []MatchFunc{NgxPlusR31DirectivesMatchFn, AppProtectWAFv4DirectivesMatchFn},

NgxPlusLatestDirectivesMatchFn thinks limit_req_zone has a mask of ngxHTTPMainConf | ngxConfTake3 | ngxConfTake2,, but NgxPlusR31DirectivesMatchFn has the correct mask ngxHTTPMainConf | ngxConfTake3 | ngxConfTake4

[ornj]

Adding details from our conversation external to the PR. Take4 is only correct for NGINX+ to allow the sync flag. OSS is Take3.

[dengsh12]

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings!

For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

[oliveromahony]

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings!

For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

Happy to close this PR. Can we add tests to cover this scenario in #105 ?

[dengsh12]

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings!
For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

Happy to close this PR. Can we add tests to cover this scenario in #105 ?

Yeah! Root reason comes from a legacy version of the generator has a wrong mapping from NGX_CONF_TAKE4 to ngxConfTake2 and I added one unit test with all bitmask in nginx to the MR of generator #101

It would also be helpful to add an unit test for limit_req_zone. Your test case in this PR is a very good example, could I add it to #105 ?

[ryepup]

It would also be helpful to add an unit test for limit_req_zone. Your test case in this PR is a very good example, could I add it to #105 ?

please do

[oliveromahony]

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings!
For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

Happy to close this PR. Can we add tests to cover this scenario in #105 ?

Yeah! Root reason comes from a legacy version of the generator has a wrong mapping from NGX_CONF_TAKE4 to ngxConfTake2 and I added one unit test with all bitmask in nginx to the MR of generator #101

It would also be helpful to add an unit test for limit_req_zone. Your test case in this PR is a very good example, could I add it to #105 ?

That would be great