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

Incorrect host returned for request with port #797

Closed
kate-osborn opened this issue Jun 27, 2023 · 5 comments · Fixed by #827
Closed

Incorrect host returned for request with port #797

kate-osborn opened this issue Jun 27, 2023 · 5 comments · Fixed by #827
Assignees
Labels
refined Requirements are refined and the issue is ready to be implemented.
Milestone

Comments

@kate-osborn
Copy link
Contributor

The HTTPRouteHostnameIntersection conformance test fails one test case because the host returned does not match the host sent.

The request's host is very.specific.com:1234 and nginx returns very.specific.com. Note that we are routing the request to the correct backend.

Sending Request:
< GET /s1 HTTP/1.1
< Host: very.specific.com:1234
< User-Agent: Go-http-client/1.1
< X-Echo-Set-Header:
< Accept-Encoding: gzip
Received Response:
< HTTP/1.1 200 OK
< Content-Length: 379
< Connection: keep-alive
< Content-Type: application/json
< Date: Tue, 27 Jun 2023 19:45:12 GMT
< Server: nginx/1.25.1
< X-Content-Type-Options: nosniff
<
< {
<  "path": "/s1",
<  "host": "very.specific.com",
<  "method": "GET",
<  "proto": "HTTP/1.0",
<  "headers": {
<   "Accept-Encoding": [
<    "gzip"
<   ],
<   "Connection": [
<    "close"
<   ],
<   "User-Agent": [
<    "Go-http-client/1.1"
<   ],
<   "X-Echo-Set-Header": [
<    ""
<   ]
<  },
<  "namespace": "gateway-conformance-infra",
<  "ingress": "",
<  "service": "",
<  "pod": "infra-backend-v1-6b94cf477-x5wwt"
< }

Error: expected host to be very.specific.com:1234, got very.specific.com (after 29.1768454s)

Acceptance Criteria:

  • Make sure the conformance test mentioned above passes
@mpstefan
Copy link
Collaborator

I assume the infra-backend-v1 backendRef specified in the example is pointing to an API on our control plane that is returning this info? So we would need to modify our API to include the port number?

@kate-osborn
Copy link
Contributor Author

I assume the infra-backend-v1 backendRef specified in the example is pointing to an API on our control plane that is returning this info? So we would need to modify our API to include the port number?

infra-backend-v1 is a Pod in the cluster. The example is showing debug output from an HTTP request to NKG for the host very.specific.com:1234. The test is expecting the HTTP response from NKG to have the host header very.specific.com:1234, but NKG sets the host header to very.specific.com. I'm not sure if nginx is stripping the port number from the host header value or if we need to tweak the nginx config a bit. @pleshakov may know.

@ciarams87 ciarams87 modified the milestone: v0.5.0 Jun 28, 2023
@pleshakov
Copy link
Contributor

we pass the host header to the backend using the value of $host NGINX variable. However, that value doesn't include anything after including :, like :1234 of localhost:1234. However, $http_host includes that - it gives access to the original value of the Host header.

However, note that $http_host might be empty or different from $host

Consider the following NGINX server:

    server {
        listen 80;
        server_name "hello";
        return 200 "ok,$host, $http_host\n";
    }

And requests/responses:

telnet 127.0.0.1 80
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.1 200 OK
Server: nginx/1.25.0
Date: Wed, 28 Jun 2023 14:30:52 GMT
Content-Type: application/octet-stream
Content-Length: 11
Connection: close

ok,hello
telnet 127.0.0.1 80
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
GET http://localhost HTTP/1.1
Host: hi

HTTP/1.1 200 OK
Server: nginx/1.25.0
Date: Wed, 28 Jun 2023 14:31:43 GMT
Content-Type: application/octet-stream
Content-Length: 17
Connection: keep-alive

ok,localhost, hi

see also http://nginx.org/en/docs/http/ngx_http_core_module.html#var_host and https://nginx.org/en/docs/http/ngx_http_core_module.html#var_http_

@mpstefan mpstefan added this to the v0.5.0 milestone Jun 28, 2023
@mpstefan mpstefan added the refined Requirements are refined and the issue is ready to be implemented. label Jun 28, 2023
@kate-osborn
Copy link
Contributor Author

@pleshakov thanks for the details.

So it sounds like we can't reliably use $http_host. Any ideas on how we can fix this?

@pleshakov
Copy link
Contributor

@kate-osborn
After some thinking, I think it is OK if $http_host is empty. In that case, we just pass the empty header to the backend. we will technically comply with MUST forward this header unmodified to the backend. Potential problems:
Right now we use http/1.0 protocol to talk to the backend. However, if change it to 1.1 (to enable keepalives to the backend for example), the backend will probably reject a request without a host header (NGINX rejects incoming requests without the host header for HTTP/1.1).

At the same time, we can also use the value of $host if $http_host is empty.

    map $http_host $gw_api_compliant_host {
        '' $host; # This can happen in an HTTP/1.0 request. NGINX will set $host to something, so let's use that value. See http://nginx.org/en/docs/http/ngx_http_core_module.html#var_host

            default $http_host; # use the host header unmodified as prescribed by the Gateway API.
    }

     ... 
     location / { proxy_set_header Host $gw_api_compliant_host; ... }

@kate-osborn kate-osborn self-assigned this Jul 6, 2023
@kate-osborn kate-osborn moved this from 🆕 New to 🏗 In Progress in NGINX Gateway Fabric Jul 6, 2023
@kate-osborn kate-osborn moved this from 🏗 In Progress to 👀 In Review in NGINX Gateway Fabric Jul 7, 2023
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in NGINX Gateway Fabric Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refined Requirements are refined and the issue is ready to be implemented.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants