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

core: add modular network_proxy support #6399

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

mohammed90
Copy link
Member

@mohammed90 mohammed90 commented Jun 14, 2024

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

Co-authored-by: @ImpostorKeanu
Signed-off-by: Mohammed Al Sahaf <[email protected]>
@mohammed90 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
network.go Outdated Show resolved Hide resolved
@ImpostorKeanu
Copy link
Contributor

Slick! I support everything about this!

Signed-off-by: Mohammed Al Sahaf <[email protected]>
@mohammed90 mohammed90 marked this pull request as ready for review August 24, 2024 13:11
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Design makes sense to me 👍

modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
@@ -139,6 +139,9 @@ type HTTPTransport struct {
// The pre-configured underlying HTTP transport.
Transport *http.Transport `json:"-"`

// Forward proxy module
Copy link
Member

Choose a reason for hiding this comment

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

Needs expanded godoc

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -1074,6 +1074,22 @@ func (h *HTTPTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
}
h.ForwardProxyURL = d.Val()

case "forward_proxy":
Copy link
Member

Choose a reason for hiding this comment

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

If it's "network_proxy" should that be the option name in Caddyfile?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was on the fence. I've changed them all to network_proxy.

modules/internal/network/networkproxy.go Show resolved Hide resolved
modules/internal/network/networkproxy.go Show resolved Hide resolved
modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/caddyfile.go Show resolved Hide resolved
modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/httptransport.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/httptransport.go Outdated Show resolved Hide resolved
caddy.RegisterModule(ProxyFromNone{})
}

// The "url" proxy source uses the defined URL as the proxy
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@mohammed90 mohammed90 linked an issue Aug 29, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found in progress 🏃‍♂️ Being actively worked on needs docs ✍️ Requires documentation changes needs tests 💯 Requires automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Placeholders for forward_proxy_url
3 participants