core: add modular `network_proxy` support #6399

mohammed90 · 2024-06-14T23:21:11Z

The PR #5369 introduced support for the standard env vars HTTP_PROXY, HTTPS_PROXY, and NO_PROXY in the reverse_proxy handler. The ACME client in Caddy has always respected the vars. The issue #6111 shows a need for configurable forward-proxy outside the env vars due to the global state of env vars.

The post (https://caddy.community/t/routing-acme-requests-via-http-proxy/24363) in the Caddy forum shows a need for forward-proxy support for ACME (external requests) but not for reverse-proxy upstreams. Again, the global state nature of the env vars impedes any effort to separate those concerns.

To brainstorm the best solution, I'm introducing modular approach, where the proxy address can be explicitly configured via module and falls back to env var. Discussion and iteration on this PR is necessary to ensure a common satisfactory solution is reached. I haven't wired up the Caddyfile parts.

CC/ @ImpostorKeanu

Note if this approach is acceptable, the ForwardProxyURL field will be deprecated in favor of the "from": "url" module.

TODO:

Caddyfile
Tests

@ImpostorKeanu

Co-authored-by: @ImpostorKeanu Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

network.go

ImpostorKeanu · 2024-06-15T09:10:48Z

Slick! I support everything about this!

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

francislavoie

Design makes sense to me 👍

modules/caddyhttp/reverseproxy/caddyfile.go

modules/caddyhttp/reverseproxy/httptransport.go

modules/caddyhttp/reverseproxy/caddyfile.go

modules/internal/network/networkproxy.go

modules/caddyhttp/reverseproxy/caddyfile.go

modules/caddyhttp/reverseproxy/httptransport.go

francislavoie · 2024-08-26T19:42:25Z

modules/internal/network/networkproxy.go

+	caddy.RegisterModule(ProxyFromNone{})
+}
+
+// The "url" proxy source uses the defined URL as the proxy


Don't we usually start godoc comments with the name of the type as the first word? I don't know why that's the convention but I think that's what we typically do?

It's a Go idiom. However, we pick up the same doc lines for our documentation, so I had to make a judgement call to either make it sensible for our documentation or meet the informal convention of Go docs. It's less confusing for our users to see the module name instead of the other way around.

Sometimes linters/vet will catch this, but if not, I guess we can keep it as-is.

I had planned at one point to tune our Caddy docs to account for the Godoc convention and help eliminate the repetition, but I haven't gotten around to it yet.

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

mohammed90 · 2024-12-29T09:36:53Z

I forgot about this PR. @mholt, @francislavoie, should we move it to 2.10? I'm fine either way.

mholt · 2024-12-30T19:12:48Z

Hm, yeah, that might be best. Thanks for updating and we'll get back to this soon!

mholt

Thanks Mohammed! Looking pretty good, just a few nits on my end.

modules/caddyhttp/reverseproxy/caddyfile.go

modules/caddyhttp/reverseproxy/httptransport.go

modules/caddytls/proxy.go

mholt · 2025-03-07T17:49:32Z

modules/internal/network/networkproxy.go

+	caddy.RegisterModule(ProxyFromNone{})
+}
+
+// The "url" proxy source uses the defined URL as the proxy


Sometimes linters/vet will catch this, but if not, I guess we can keep it as-is.

I had planned at one point to tune our Caddy docs to account for the Godoc convention and help eliminate the repetition, but I haven't gotten around to it yet.

modules/internal/network/networkproxy.go

network.go

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

mholt

LGTM, we can give it a try. Thanks @mohammed90 !

CzBiX · 2025-04-27T02:20:09Z

This chages seems to have broken the existing forward_proxy_url usage with panic:

panic: reflect: call of reflect.Value.Type on zero Value
goroutine 1 [running]:
reflect.Value.abiTypeSlow({0x0?, 0x0?, 0x0?})
	reflect/value.go:2400 +0xee
reflect.Value.typeSlow({0x0?, 0x0?, 0x8?})
	reflect/value.go:2388 +0x1d
reflect.Value.Type(...)
	reflect/value.go:2383
github.com/caddyserver/caddy/v2.Context.LoadModule({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:181 +0xee
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*HTTPTransport).NewTransport(0xc000441a40, {{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, ...}, ...})
	github.com/caddyserver/caddy/v2@v2.10.0/modules/caddyhttp/reverseproxy/httptransport.go:356 +0x80f
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*HTTPTransport).Provision(0xc000441a40, {{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, ...}, ...})
	github.com/caddyserver/caddy/v2@v2.10.0/modules/caddyhttp/reverseproxy/httptransport.go:181 +0xc5
github.com/caddyserver/caddy/v2.Context.LoadModuleByID({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:410 +0x755
github.com/caddyserver/caddy/v2.Context.loadModuleInline({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x3, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:468 +0xf0
github.com/caddyserver/caddy/v2.Context.LoadModule({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x3, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:209 +0x426
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).Provision(0xc00077e960, {{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x3, 0x4}, {0x0, 0x0, ...}, ...})
	github.com/caddyserver/caddy/v2@v2.10.0/modules/caddyhttp/reverseproxy/reverseproxy.go:241 +0x2b5

mohammed90 · 2025-04-27T06:04:55Z

This chages seems to have broken the existing forward_proxy_url usage with panic:

panic: reflect: call of reflect.Value.Type on zero Value
goroutine 1 [running]:
reflect.Value.abiTypeSlow({0x0?, 0x0?, 0x0?})
	reflect/value.go:2400 +0xee
reflect.Value.typeSlow({0x0?, 0x0?, 0x8?})
	reflect/value.go:2388 +0x1d
reflect.Value.Type(...)
	reflect/value.go:2383
github.com/caddyserver/caddy/v2.Context.LoadModule({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:181 +0xee
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*HTTPTransport).NewTransport(0xc000441a40, {{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, ...}, ...})
	github.com/caddyserver/caddy/v2@v2.10.0/modules/caddyhttp/reverseproxy/httptransport.go:356 +0x80f
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*HTTPTransport).Provision(0xc000441a40, {{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, ...}, ...})
	github.com/caddyserver/caddy/v2@v2.10.0/modules/caddyhttp/reverseproxy/httptransport.go:181 +0xc5
github.com/caddyserver/caddy/v2.Context.LoadModuleByID({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x4, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:410 +0x755
github.com/caddyserver/caddy/v2.Context.loadModuleInline({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x3, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:468 +0xf0
github.com/caddyserver/caddy/v2.Context.LoadModule({{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x3, 0x4}, {0x0, 0x0, 0x0}, ...}, ...)
	github.com/caddyserver/caddy/v2@v2.10.0/context.go:209 +0x426
github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy.(*Handler).Provision(0xc00077e960, {{0x1ed44f8, 0xc0002269c0}, 0xc0001fc690, 0xc0002073b0, {0xc000259b40, 0x3, 0x4}, {0x0, 0x0, ...}, ...})
	github.com/caddyserver/caddy/v2@v2.10.0/modules/caddyhttp/reverseproxy/reverseproxy.go:241 +0x2b5

Fixed in 737936c

mholt · 2025-04-28T15:03:07Z

Thanks @mohammed90 !

core: add modular network_proxy support

62c711c

Co-authored-by: @ImpostorKeanu Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

mohammed90 added in progress 🏃‍♂️ Being actively worked on discussion 💬 The right solution needs to be found needs docs ✍️ Requires documentation changes needs tests 💯 Requires automated tests labels Jun 14, 2024

mohammed90 requested review from mholt, francislavoie and WeidiDeng June 14, 2024 23:21

francislavoie reviewed Jun 14, 2024

View reviewed changes

network.go Outdated Show resolved Hide resolved

move modules around

5436774

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

mohammed90 force-pushed the forward-proxy branch from 91e0eb9 to 5436774 Compare June 15, 2024 11:16

Merge branch 'master' into forward-proxy

32d1c5e

mohammed90 mentioned this pull request Aug 23, 2024

[reverse_proxy] No possibility to force a NO_PROXY for reverse_proxy directive #6538

Open

mohammed90 added 2 commits August 24, 2024 14:15

Merge branch 'master' into forward-proxy

6836afb

add caddyfile implementation

18cd1c0

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

mohammed90 marked this pull request as ready for review August 24, 2024 13:11

mohammed90 requested a review from francislavoie August 24, 2024 17:48

francislavoie reviewed Aug 24, 2024

View reviewed changes

address feedbcak

38bb05c

francislavoie reviewed Aug 26, 2024

View reviewed changes

mohammed90 and others added 2 commits August 27, 2024 10:48

Apply suggestions from code review

908fa5e

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

adapt ForwardProxyURL to use the NetworkProxyRaw

b8a9870

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

mohammed90 linked an issue Aug 29, 2024 that may be closed by this pull request

Add Placeholders for forward_proxy_url #6395

Closed

mohammed90 mentioned this pull request Aug 29, 2024

Forward proxy url placeholders #6397

Closed

Merge branch 'master' into forward-proxy

e674e20

mohammed90 removed in progress 🏃‍♂️ Being actively worked on needs docs ✍️ Requires documentation changes needs tests 💯 Requires automated tests labels Dec 29, 2024

mohammed90 added the under review 🧐 Review is pending before merging label Dec 29, 2024

mholt added this to the v2.10.0-beta.1 milestone Dec 30, 2024

mohammed90 and others added 3 commits January 7, 2025 12:44

Merge branch 'master' into forward-proxy

00fc4cc

Merge branch 'master' into forward-proxy

0896d92

Merge branch 'master' into forward-proxy

2437f79

mholt modified the milestones: v2.10.0-beta.1, v2.10.0-beta.2 Mar 6, 2025

WeidiDeng approved these changes Mar 7, 2025

View reviewed changes

Merge branch 'master' into forward-proxy

f4a7cc9

mholt requested changes Mar 7, 2025

View reviewed changes

remove redundant url in log

c54f501

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

mholt modified the milestones: v2.10.0-beta.2, v2.10.0-beta.3 Mar 8, 2025

mohammed90 and others added 4 commits March 14, 2025 04:42

code review

a4f811e

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

Merge branch 'master' into forward-proxy

feebacf

Merge branch 'master' into forward-proxy

6eab324

remove .source from the module ID

64118c3

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

mohammed90 force-pushed the forward-proxy branch from 7c2426c to 64118c3 Compare March 20, 2025 23:35

mholt approved these changes Mar 21, 2025

View reviewed changes

Merge branch 'master' into forward-proxy

0f4c4a7

mohammed90 enabled auto-merge (squash) March 21, 2025 17:01

mohammed90 merged commit 1735730 into master Mar 21, 2025
20 checks passed

mohammed90 deleted the forward-proxy branch March 21, 2025 17:06

mholt removed the under review 🧐 Review is pending before merging label Mar 21, 2025

Uh oh!

core: add modular network_proxy support #6399

core: add modular network_proxy support #6399

Uh oh!

Conversation

mohammed90 commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ImpostorKeanu commented Jun 15, 2024

Uh oh!

francislavoie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

francislavoie Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

mohammed90 Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

mholt Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

mohammed90 commented Dec 29, 2024

Uh oh!

mholt commented Dec 30, 2024

Uh oh!

mholt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mholt Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mholt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CzBiX commented Apr 27, 2025

Uh oh!

mohammed90 commented Apr 27, 2025

Uh oh!

mholt commented Apr 28, 2025

Uh oh!

Uh oh!

core: add modular `network_proxy` support #6399

core: add modular `network_proxy` support #6399

mohammed90 commented Jun 14, 2024 •

edited

Loading