nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.25k stars 321 forks source link

Proxy HTTP: Added standard port for inet and inet6 #1261

Open Pavlusha311245 opened 1 month ago

Pavlusha311245 commented 1 month ago

Added a standard port for IPv4 and IPv6 addresses when using proxy and HTTP, eliminating the need to specify an additional port 80. This is defined by the RFC standards for HTTP. Currently Nginx Unit only uses HTTP connections for proxying, version 1.1.

https://datatracker.ietf.org/doc/html/rfc2616#page-12

HTTP communication usually takes place over TCP/IP connections. The default port is TCP 80

hongzhidao commented 1 month ago

Hi @Pavlusha311245, Thanks for the patches. The nxt_sockaddr.c was supposed to parse IP:PORT address. It's a generic function but not depend on http stuff. Proxy belongs to HTTP, and it makes sense to support the format you suggested as much as possible. But the way that touched nxt_sockaddr.c is incorrect. I feel the logic would be something like this:

sa = nxt_sockaddr_parse(&str);

if (sa == NULL) {
    result = getaddrinfo(...);
    /* process result */
}

https://www.man7.org/linux/man-pages/man3/getaddrinfo.3.html But it would introduce another question, it looks like it's better to support domain names as well. And it may resolve multiple addresses, this topic was discussed. https://github.com/nginx/unit/issues/779 https://github.com/nginx/unit/issues/1234

Pavlusha311245 commented 1 month ago

@hongzhidao Thanks for the review

In the current implementation, this is a quick fix that will allow you to at least allocate a default port and it is strictly aimed at only inet implementations excluding changes to unix at this stage. I think that we can start working with the proxy part of the web server :)

Regarding domains and https support, I think we should start a separate discussion and discuss further actions

hongzhidao commented 1 month ago

I guess the current implementation may introduce issues like this:

{
    "listeners": {
        "127.0.0.1": {
             "pass": "routes"
        }
    }
}

It's an invalid configuration, but it's allowed with the patch.

Pavlusha311245 commented 1 month ago

Yes it is, but it is still HTTP, which works on port 80 by default. The ability to add and use another port has not gone away. And also does not interfere with the operation of unix sockets

hongzhidao commented 1 month ago

I feel nxt_sockaddr.c has nothing to do with the scheme. The change should happen in nxt_http_proxy.c or another place that resolves the format http://IP. And these need to be based on the decision of whether or not to make the requirement. Will discuss it with the team. Personally, I feel the requirement makes sense. Just to support the format http://ip-address for the proxing.

Pavlusha311245 commented 1 month ago

Then I will wait for your decision with the team. I think I can move this part only to the proxy section

hongzhidao commented 1 month ago

I think I can move this part only to the proxy section

Indeed.

Btw, the other idea I can think of right now is to do it like this in proxy module.

addr_str = concat(host_str, ':', default_port_str);
sa = sockaddr_parse(addr_str);
ac000 commented 1 month ago

To echo @hongzhidao

I'm not against the idea, but I think the location is wrong. This should really happen in the proxy code.

hongzhidao commented 1 month ago

Hi @Pavlusha311245,

Then I will wait for your decision with the team. I think I can move this part only to the proxy section

We think it's a good idea to support the feature, welcome to continue. Let me know if there is anything I can help.