nginxinc / nginx-go-crossplane

A library for working with NGINX configs in Go
Apache License 2.0
46 stars 12 forks source link

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

Closed dareste closed 10 months ago

dareste commented 10 months ago

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

Expected behavior

The mentioned config shouldn't be throwing an error.

Additional context

This is likely caused by the directives map config here: https://github.com/nginxinc/nginx-go-crossplane/blob/73c76f92a13211c7b1488dfa746d9f3353b82ff8/analyze.go#L791

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

    "include": {
        ngxAnyConf | ngxConfTake1 | ngxHTTPLifConf,
    },
ornj commented 10 months ago

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 commented 10 months ago

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":

https://github.com/nginxinc/nginx-go-crossplane/blob/73c76f92a13211c7b1488dfa746d9f3353b82ff8/analyze.go#L57-L61

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.