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

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

Closed
SamuelMarks opened this issue May 5, 2020 · 4 comments

Comments

@SamuelMarks
Copy link

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

  • Operating System macOS 10.15.4
  • Version of crossplane 0.5.3

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

@aluttik
Copy link
Contributor

aluttik commented May 5, 2020

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
Copy link
Author

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
Copy link
Contributor

aluttik commented May 6, 2020

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
Copy link
Contributor

aluttik commented May 6, 2020

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

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

No branches or pull requests

3 participants