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

"include" directive is not allowed within an "if" block #72

Closed
dareste opened this issue Oct 6, 2023 · 2 comments · Fixed by #74
Closed

"include" directive is not allowed within an "if" block #72

dareste opened this issue Oct 6, 2023 · 2 comments · Fixed by #74

Comments

@dareste
Copy link
Contributor

dareste commented Oct 6, 2023

The following block of code is a valid nginx configuration:

 19    server
 20    {
 21       listen 12345;
 22       location /
 23       {
 24          if ($request_method = 'OPTIONS')
 25          {
 26             include conf.d/cors_header;
 27
 28             return 204;
 29          }
 30
 31          root /usr/share/nginx/html/;
 32          try_files $uri $uri/ /en/index.html =404;
 33
 34       }
 35    }

Despite that, the tool detects the "include" as a non-allowed directive under the "if" context:

  "errors": [
    {
      "file": "test.conf",
      "line": 26,
      "error": "\"include\" directive is not allowed here in test.conf:26"
    }
  ],

To reproduce

  • Take the mentioned configuration, load it in an nginx environment. Verify that it succeeds and there are no deferred errors.
  • Use the same config as an input for nginx-go-crossplane latest version, and see the above error.

Expected behavior

The mentioned config shouldn't be throwing an error.

Additional context

This is likely caused by the directives map config here:

"include": {

I think we should modify the mask to include the "http > location > if"

	"include": {
		ngxAnyConf | ngxConfTake1 | ngxHTTPLifConf,
	},
@ornj
Copy link
Member

ornj commented Oct 6, 2023

Agreed that snippet should work, although I think the mask should just be nginxAnyConf | nginxConfTake1 based on the NGINX source.

Will you open a PR?

@dareste
Copy link
Contributor Author

dareste commented Oct 9, 2023

I agree that the mask should be consistent with what we see in the NGINX source definition. The problem is that here we define ngxAnyConf as "any context except server > if, location > if and location > limit_except":

// helpful directive location alias describing "any" context
// doesn't include ngxHTTPSifConf, ngxHTTPLifConf, or ngxHTTPLmtConf.
const ngxAnyConf = ngxMainConf | ngxEventConf | ngxMailMainConf | ngxMailSrvConf |
ngxStreamMainConf | ngxStreamSrvConf | ngxStreamUpsConf |
ngxHTTPMainConf | ngxHTTPSrvConf | ngxHTTPLocConf | ngxHTTPUpsConf

The only place where we use ngxAnyConf is when we define the include directive, so I think we should be ok by changing the ngxAnyConf definition to include these three exceptions, which are valid contexts for the include directive.

I'll open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants