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

caddyhttp: add http.request.local{,.host,.port} placeholder #6182

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

emilylange
Copy link
Member

This is the counterpart of http.request.remote{,.host,.port}.

http.request.remote operates on the remote client's address, while
http.request.local operates on the address the connection arrived on.

Take the following example:

  • Caddy serving on 203.0.113.1:80
  • Client on 203.0.113.2

http.request.remote.host would return 203.0.113.2 (client IP)

http.request.local.host would return 203.0.113.1 (server IP)
http.request.local.port would return 80 (server port)

I find this helpful for debugging setups with multiple servers and/or multiple network paths (multiple IPs, AnyIP, Anycast).

A version of this using respond http.request.local is currently deployed at https://localip.geklaute.cloud/ (but might not be there forever).
I successfully tested http versions 1.1, 2 and 3 with curl on that url.

cc @networkException


The implementation is heavily based on http.request.remote.

But I would like to comment on two code paths that I find a bit confusing:

case "http.request.remote.port":
_, port, _ := net.SplitHostPort(req.RemoteAddr)
if portNum, err := strconv.Atoi(port); err == nil {
return portNum, true
}
return port, true

For http.request.remote.port we try to cast the port from net.SplitHostPort as integer.
But if the string -> int conversion fails, we return it as string.
I cannot think of a case where strconv.Atoi(port) would ever fail.
And if it actually does, I don't think returning it as string it a good fallback.
I would argue if the conversion fails, something went very wrong.

Similarly, for http.request.remote.host:

case "http.request.remote.host":
host, _, err := net.SplitHostPort(req.RemoteAddr)
if err != nil {
return req.RemoteAddr, true
}
return host, true

If net.SplitHostPort(req.RemoteAddr) errors, we return req.RemoteAddr.

Would be cool if someone could comment on those two things :)
Because I feel like those are just dead code paths.

emilylange and others added 3 commits March 20, 2024 22:20
This is the counterpart of `http.request.remote{,.host,.port}`.

`http.request.remote` operates on the remote client's address, while
`http.request.local` operates on the address the connection arrived on.

Take the following example:

- Caddy serving on `203.0.113.1:80`
- Client on `203.0.113.2`

`http.request.remote.host` would return `203.0.113.2` (client IP)

`http.request.local.host` would return `203.0.113.1` (server IP)
`http.request.local.port` would return `80` (server port)

I find this helpful for debugging setups with multiple servers and/or
multiple network paths (multiple IPs, AnyIP, Anycast).

Co-authored-by: networkException <git@nwex.de>
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you! This will have my approval as soon as the tests finish running (assuming they pass)

I think the way you're handling the int conversion is fine.

…ix sockets

The implementation matches the one of `http.request.remote.host` now and
returns the unix socket path (just like `http.request.local` already did)
instead of an empty string.
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, LGTM @emilylange ! Thank you for the contribution 😃

@mholt mholt enabled auto-merge (squash) March 27, 2024 21:31
@mholt mholt added this to the v2.8.0 milestone Mar 27, 2024
@mholt mholt merged commit ddb1d2c into caddyserver:master Mar 27, 2024
25 checks passed
@emilylange emilylange deleted the localaddr-placeholder branch April 9, 2024 00:04
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.

None yet

3 participants