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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pavlusha311245
Copy link

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

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.
@hongzhidao
Copy link
Contributor

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.
#779
#1234

@Pavlusha311245
Copy link
Author

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

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

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

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

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

@hongzhidao
Copy link
Contributor

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

ac000 commented May 14, 2024

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

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.

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

Successfully merging this pull request may close these issues.

3 participants