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

Avoid variable name conflicts in complex routing rules implementation #1789

Open
pleshakov opened this issue Mar 29, 2024 · 1 comment
Open
Labels
backlog Currently unprioritized work. May change with user feedback or as the product progresses. tech-debt Short-term pain, long-term benefit

Comments

@pleshakov
Copy link
Contributor

To implement complex routing rules like

- matches:
- path:
type: PathPrefix
value: /coffee
headers:
- name: version
value: v2
- path:
type: PathPrefix
value: /coffee
queryParams:
- name: TEST
value: v2
, we use njs, where we encode the routing rule into a variable $http_matches to pass it to njs code for further processing.

    location @rule0-route0 {
        proxy_set_header Host "$gw_api_compliant_host";
        proxy_set_header X-Forwarded-For "$proxy_add_x_forwarded_for";
        proxy_set_header Upgrade "$http_upgrade";
        proxy_set_header Connection "$connection_upgrade";
        proxy_http_version 1.1;
        proxy_pass http://default_coffee-v2-svc_80$request_uri;
    }

    location @rule0-route1 {
        proxy_set_header Host "$gw_api_compliant_host";
        proxy_set_header X-Forwarded-For "$proxy_add_x_forwarded_for";
        proxy_set_header Upgrade "$http_upgrade";
        proxy_set_header Connection "$connection_upgrade";
        proxy_http_version 1.1;
        proxy_pass http://default_coffee-v1-svc_80$request_uri;
    }

    location /coffee/ {
        set $http_matches "[{\"redirectPath\":\"@rule0-route0\",\"params\":[\"matches=v2\"]},{\"redirectPath\":\"@rule0-route1\",\"any\":true}]";
        js_content httpmatches.redirect;
    }

    location = /coffee {
        set $http_matches "[{\"redirectPath\":\"@rule0-route0\",\"params\":[\"matches=v2\"]},{\"redirectPath\":\"@rule0-route1\",\"any\":true}]";
        js_content httpmatches.redirect;
    }

Unfortunately, $http_ variable is a built-in variable used to look up headers - https://nginx.org/en/docs/http/ngx_http_core_module.html#var_http_` , which can override the original header value and create problems.

Note: right now, it doesn't cause any problems - because njs code gets access to headers separately through r.headersIn

let found = headersMatch(r.headersIn, match.headers);

However, when we implement configurable access logging or start using NGINX variables in other places, it can create problems, if users uses the request header with the name matches.

For example, consider the following NGINX configurations and curl requests:

server {
    listen 80;
    set $http_matches "my-value";
    return 200 "$http_matches\n";
}
curl localhost -H "matches: 123"
my-value
server {
    listen 80;
    #set $http_matches "my-value";
    return 200 "$http_matches\n";
}
curl localhost -H "matches: 123"
123
Copy link
Contributor

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Apr 13, 2024
@sjberman sjberman removed the stale Pull requests/issues with no activity label Apr 15, 2024
@mpstefan mpstefan added tech-debt Short-term pain, long-term benefit backlog Currently unprioritized work. May change with user feedback or as the product progresses. labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Currently unprioritized work. May change with user feedback or as the product progresses. tech-debt Short-term pain, long-term benefit
Projects
Status: 🆕 New
Development

No branches or pull requests

3 participants