nginxinc / crossplane

Quick and reliable way to convert NGINX configurations into JSON and back.
Apache License 2.0
717 stars 86 forks source link

Quoting variables leaves two kinds of quotes, e.g., Connection '"upgrade"'; #82

Closed SamuelMarks closed 4 years ago

SamuelMarks commented 4 years ago

Describe the bug A clear and concise description of what the bug is.

To Reproduce

$ cat foo.json
{"status":"ok","errors":[],"config":[{"file":"/tmp/nginx.conf","status":"ok","errors":[],"parsed":[{"directive":"include","line":13,"args":["/tmp/sites-available/*.conf"],"includes":[2]}]},{"file":"/tmp/sites-available/server.conf","status":"ok","errors":[],"parsed":[{"directive":"server","line":1,"args":[],"block":[{"directive":"server_name","line":2,"args":["localhost"]},{"directive":"listen","line":3,"args":["8080"]},{"directive":"location","line":8,"args":["/"],"block":[
                {
                  "directive": "proxy_set_header",
                  "line": 11,
                  "args": [
                    "Connection",
                    "\"upgrade\""
                  ]
                }
]}]}]}]}
$ crossplane build foo.json
$ cat /tmp/sites-available/server.conf
# This config was built from JSON using NGINX crossplane.
# If you encounter any bugs please report them here:
# https://github.com/nginxinc/crossplane/issues

server {
    server_name localhost;
    listen 8080;
    location / {
        proxy_set_header Connection '"upgrade"';
    }
}

PS: I have also tried manually changing that line to:

"args": [
    "Connection",
    "'upgrade'"
]

Expected behavior

proxy_set_header Connection 'upgrade';

Your environment

Additional context Keep in mind that this also breaks Windows support, as paths cannot be quoted properly.

aluttik commented 4 years ago

That's intended behavior. There is no way to produce the expected behavior that you posted there because nginx reads upgrade and 'upgrade' and "upgrade" the exact same way. Quotes in nginx syntax are only for escaping whitespace, and there's no difference between the two types of quotes.

Can you provide an example of how this breaks windows support?

SamuelMarks commented 4 years ago

Windows paths with spaces wrapped in ".

@aluttik So are you saying that these are equivalent:

A

proxy_set_header Connection 'upgrade';

B

proxy_set_header Connection "upgrade";

C

proxy_set_header Connection '"upgrade"';

D

proxy_set_header Connection '"upgrade"';
aluttik commented 4 years ago

No, A and B are and C and D are, but A and C are not the same.

I'm saying that, to nginx, A and B and this example are all the same:

proxy_set_header Connection upgrade;

Crossplane decides if an argument needs quotes when written to the nginx config and adds them automatically for you.

For example, it is not possible to make crossplane write 'update' to the config because it will always decide to write the unquoted update instead of 'update' (which is equivalent so far as nginx is concerned). However, it will write the quoted 'foo bar' to the config if you pass in "args":["foo bar"] because it knows that nginx would read "foo" and "bar" as two separate args if there were no quotes.

If you use escaped quotes like "args":["\"foo bar\""], you're telling crossplane "I want nginx to see these quotes as part of the argument" so it will wrap the argument (that contains quotes) with another set of quotes like '"foo bar"' instead of what you probably wanted: 'foo bar'.

If crossplane is making the wrong choice and not putting quotes around something that it should, causing nginx to behave in unexpected ways, then that's a bug.

aluttik commented 4 years ago

I said earlier that "quotes in nginx syntax are only for escaping whitespace", but that's not exactly true. I should have said that they are used only for escaping purposes, which includes escaping whitespace as well as special characters like ' and ".