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

mholt · 2025-03-07T17:47:51Z

modules/caddyhttp/reverseproxy/httptransport.go

+	//
+	// If nil, defaults to reading the `HTTP_PROXY`,
+	// `HTTPS_PROXY`, and `NO_PROXY` environment variables.
+	NetworkProxyRaw json.RawMessage `json:"network_proxy,omitempty" caddy:"namespace=caddy.network_proxy.source inline_key=from"`


Are there other modules that could be made under caddy.network_proxy? (i.e. is the .source namespace useful)?

If so, let's definitely keep it -- just curious.

mholt · 2025-03-07T17:48:26Z

modules/caddytls/proxy.go

@@ -0,0 +1,3 @@
+package caddytls
+
+import _ "github.com/caddyserver/caddy/v2/modules/internal/network"


Interesting; why the indirection here?

This is embarrassing. I don't remember, and it still works without it.

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.

mholt · 2025-03-07T17:51:20Z

modules/internal/network/networkproxy.go

+var (
+	_ caddy.Module            = ProxyFromURL{}
+	_ caddy.Provisioner       = &ProxyFromURL{}


As for interface guards, caddy.Module isn't needed since you call RegisterModule().

I also prefer this pattern to avoid allocations:

var _ caddy.Provisioner = (*ProxyFromURL)(nil)

mholt · 2025-03-07T17:51:54Z

network.go

+	"net/url"
+)
+
+type ProxyFuncProducer interface {


How necessary is this for it to have its own file? Can we move this into an existing file?

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

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

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add modular `network_proxy` support #6399

core: add modular `network_proxy` support #6399

mohammed90 commented Jun 14, 2024 •

edited

Loading

ImpostorKeanu commented Jun 15, 2024

francislavoie left a comment

francislavoie Aug 26, 2024

mohammed90 Aug 27, 2024

mholt Mar 7, 2025

mohammed90 commented Dec 29, 2024

mholt commented Dec 30, 2024

mholt left a comment

mholt Mar 7, 2025

mholt Mar 7, 2025

mohammed90 Mar 7, 2025

mholt Mar 7, 2025

mholt Mar 7, 2025

mholt Mar 7, 2025

		@@ -0,0 +1,3 @@
		package caddytls

		import _ "github.com/caddyserver/caddy/v2/modules/internal/network"

core: add modular network_proxy support #6399

Are you sure you want to change the base?

core: add modular network_proxy support #6399

Conversation

mohammed90 commented Jun 14, 2024 • edited Loading

ImpostorKeanu commented Jun 15, 2024

francislavoie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammed90 commented Dec 29, 2024

mholt commented Dec 30, 2024

mholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

core: add modular `network_proxy` support #6399

core: add modular `network_proxy` support #6399

mohammed90 commented Jun 14, 2024 •

edited

Loading