Feat/proxypass #98
Open
Feat/proxypass #98
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduces a new proxypass reverse proxy subsystem with Cloudflare-based visitor resolution and SSE-driven lifecycle management, adjusts ACL loading and IP extraction timing, allows custom ACME directory URLs, and improves HTTP error handling semantics for missing views. Sequence diagram for proxypass SSE connect and reverse proxy lifecyclesequenceDiagram
actor Operator
participant Client
participant ProxypassMiddleware as Proxypass_Middleware
participant HandleConnect as HandleConnect_Handler
participant SSEServer as SSE_Server
participant Proxy as ReverseProxy
participant Backend as Backend_Service
Operator->>Client: Invoke Connect(domain, targetURL)
Client->>HandleConnect: HTTP GET connectURL
activate HandleConnect
HandleConnect->>HandleConnect: Read headers X_Code, X_Domain, X_Target
HandleConnect->>HandleConnect: Parse targetURL
HandleConnect->>HandleConnect: Adjust target host when hostname is 0_0_0_0
HandleConnect->>Proxy: create(target)
HandleConnect->>SSEServer: Join(code, responseWriter)
SSEServer-->>HandleConnect: client, cid
HandleConnect->>ProxypassMiddleware: up(domain, proxy)
Note over ProxypassMiddleware,Proxy: Domains are mapped to Proxy instance in servers map
HandleConnect->>SSEServer: client.Wait(requestContext)
deactivate HandleConnect
loop For each incoming HTTP request
participant Visitor
Visitor->>ProxypassMiddleware: HTTP request Host domain
activate ProxypassMiddleware
ProxypassMiddleware->>ProxypassMiddleware: Extract host
ProxypassMiddleware->>ProxypassMiddleware: get(host, fallback)
alt Proxy found
ProxypassMiddleware->>Proxy: ServeHTTP(response, request)
Proxy->>Backend: Reverse proxy to target
Backend-->>Proxy: Response
Proxy-->>Visitor: Proxied response
else No proxy
ProxypassMiddleware->>Backend: Pass to next middleware
end
deactivate ProxypassMiddleware
end
Client-->>SSEServer: SSE connection closes or error
SSEServer-->>HandleConnect: Wait returns error
activate HandleConnect
HandleConnect->>ProxypassMiddleware: down(domain)
HandleConnect->>SSEServer: Leave(code, cid)
HandleConnect->>SSEServer: client.Close()
HandleConnect-->>Client: HTTP stream ends
deactivate HandleConnect
Class diagram for new proxypass and Cloudflare reverse proxy subsystemclassDiagram
class Proxy {
+Target url_URL
-proxy httputil_ReverseProxy
+ServeHTTP(rw http_ResponseWriter, req *http_Request) void
+create(target url_URL) Proxy
}
class Client {
-code string
-connectURL string
-apiUser string
-apiPasswd string
-client http_Client
+NewClient(code string, connectURL string, apiUser string, apiPasswd string) Client
+Connect(ctx context_Context, domain string, targetURL string) error
}
class Options {
<<struct>>
}
class Forwarder {
<<interface>>
+Name() string
+GetVisitor(r *http_Request) (string, string)
}
class NoForwarder {
+Name() string
+GetVisitor(r *http_Request) (string, string)
}
class proxypass_package {
<<package>>
-connections sse_Server
-servers atomic_Value
-mu sync_Mutex
+New(opts Option) xun_Middleware
+RegisterForwarder(name string, f Forwarder) void
+WithForwarder(c Forwarder) Option
}
class Option {
<<function type>>
+invoke(o *Options)
}
class sse_Server {
<<external>>
+Join(code string, w http_ResponseWriter) (client sse_Client, cid string, retry int, err error)
+Leave(code string, cid string) void
}
class xun_Middleware {
<<function type>>
}
class Cloudflare {
-latestNets atomic_Value
-defaultNets IPNets
+New(ctx context_Context) Cloudflare
+Contains(s string) bool
+GetVisitor(r *http_Request) (string, string)
-reload(ctx context_Context) void
-fetch(url string) ([]string, error)
}
class IPNets {
<<slice of net_IPNet>>
+Contains(s string) bool
+toIPNets(l []string) IPNets
}
class http_Request {
<<external>>
}
class http_ResponseWriter {
<<external>>
}
class context_Context {
<<external>>
}
class url_URL {
<<external>>
}
class httputil_ReverseProxy {
<<external>>
}
class atomic_Value {
<<external>>
}
class sync_Mutex {
<<external>>
}
proxypass_package ..> Proxy : uses
proxypass_package ..> Client : uses
proxypass_package ..> Options : configures
proxypass_package ..> Forwarder : registers
proxypass_package ..> sse_Server : manages connections
proxypass_package ..> xun_Middleware : returns
Forwarder <|.. NoForwarder : implements
Cloudflare ..|> Forwarder : implements
Cloudflare o-- IPNets : manages
Proxy o-- httputil_ReverseProxy : wraps
Proxy o-- url_URL : Target
Client o-- http_Client : client
Cloudflare o-- atomic_Value : latestNets
proxypass_package o-- atomic_Value : servers
proxypass_package o-- sync_Mutex : mu
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| client: &http.Client{ | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, |
Check failure
Code scanning / CodeQL
Disabled TLS certificate check High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
In general, the fix is to stop disabling TLS certificate verification and instead rely on the default TLS behavior, which validates the server’s certificate chain and hostname against the system or configured root CAs. That means InsecureSkipVerify should not be set to true; either omit TLSClientConfig entirely to use the defaults, or provide a tls.Config without changing InsecureSkipVerify from its default false value.
The single best fix here, without changing existing functionality beyond re-enabling proper security, is to remove the explicit setting of InsecureSkipVerify: true and, since there is no other customization in tls.Config, avoid creating a custom tls.Config at all. We can simply omit the TLSClientConfig field from the http.Transport so that Go uses its standard verified TLS behavior while preserving the existing proxy configuration (Proxy: http.ProxyFromEnvironment). This only requires editing the NewClient function in ext/proxypass/client.go to remove the crypto/tls-based configuration and, since tls is then unused, removing its import.
Concretely:
- In
ext/proxypass/client.go, delete theTLSClientConfig: &tls.Config{InsecureSkipVerify: true},block inside thehttp.Transportliteral inNewClient. - Remove the unused
crypto/tlsimport from the import block.
No new methods or external libraries are required.
Suggested changeset
1
ext/proxypass/client.go
| @@ -2,7 +2,6 @@ | ||
|
|
||
| import ( | ||
| "context" | ||
| "crypto/tls" | ||
| "net/http" | ||
|
|
||
| "github.com/yaitoo/xun/ext/sse" | ||
| @@ -24,9 +23,6 @@ | ||
| apiPasswd: apiPasswd, | ||
| client: &http.Client{ | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| }, | ||
| Proxy: http.ProxyFromEnvironment, | ||
| }, | ||
| }, |
Copilot is powered by AI and may make mistakes. Always verify output.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
- Coverage 92.10% 84.32% -7.79%
==========================================
Files 66 74 +8
Lines 2102 2296 +194
==========================================
Hits 1936 1936
- Misses 136 329 +193
- Partials 30 31 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
ext/proxypass/cloudflare.Cloudflare.reload, when both fetches fail youreturnfrom the goroutine and never retry; consider continuing the loop (while keeping the in-memory default ranges) so the IP ranges can eventually self-heal when Cloudflare becomes reachable again. - In
HandleConnect, theurl.Parseerror is ignored and headers likeX-Target/X-Domainare assumed valid; it would be safer to validate these inputs and return a 4xx on parse errors or missing values rather than proceeding with a possibly nil or malformed target. - The proxypass
ClientusesInsecureSkipVerify: trueon its TLS config, which disables certificate verification; if this is not strictly required, consider removing it or making it configurable to avoid weakening TLS security by default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ext/proxypass/cloudflare.Cloudflare.reload`, when both fetches fail you `return` from the goroutine and never retry; consider continuing the loop (while keeping the in-memory default ranges) so the IP ranges can eventually self-heal when Cloudflare becomes reachable again.
- In `HandleConnect`, the `url.Parse` error is ignored and headers like `X-Target`/`X-Domain` are assumed valid; it would be safer to validate these inputs and return a 4xx on parse errors or missing values rather than proceeding with a possibly nil or malformed target.
- The proxypass `Client` uses `InsecureSkipVerify: true` on its TLS config, which disables certificate verification; if this is not strictly required, consider removing it or making it configurable to avoid weakening TLS security by default.
## Individual Comments
### Comment 1
<location> `ext/proxypass/proxy.go:25-34` </location>
<code_context>
+ }
+}
+
+func up(domain string, s *Proxy) {
+ mu.Lock()
+ defer mu.Unlock()
+ items := servers.Load().(map[string]*Proxy)
+ items[domain] = s
+ servers.Store(items)
+}
+
+func down(domain string) {
+ mu.Lock()
+ defer mu.Unlock()
+ items := servers.Load().(map[string]*Proxy)
+ delete(items, domain)
+ servers.Store(items)
+}
+
+func get(domain, fallback string) *Proxy {
+ items := servers.Load().(map[string]*Proxy)
+ s, ok := items[domain]
</code_context>
<issue_to_address>
**issue (bug_risk):** Concurrent access to the `servers` map is racy: writers hold a mutex but readers don't, and the same map instance is mutated after being stored in `atomic.Value`.
In `up`/`down` you load the map from `servers`, mutate it in place under `mu`, and store it back, while `get` reads the same map via `servers.Load()` without taking `mu`.
This leads to:
1. A race between `get` and `up`/`down` on the same map instance, which can cause a `concurrent map read and map write` panic.
2. Incorrect use of `atomic.Value`: atomicity only covers replacing the map reference, not concurrent mutations of the map it points to.
Either guard both reads and writes with the same mutex, or switch to a copy-on-write approach where `up`/`down` build a new map instance, modify it, and `Store` that, while `get` only reads the loaded map without mutating it.
</issue_to_address>
### Comment 2
<location> `ext/proxypass/proxypass.go:19-28` </location>
<code_context>
+ mu sync.Mutex
+)
+
+func New(opts ...Option) xun.Middleware { // skipcq: GO-R1005
+ options := &Options{}
+
+ for _, opt := range opts {
+ opt(options)
+ }
+
+ connections = sse.New()
+
+ servers = &atomic.Value{}
+ servers.Store(make(map[string]*Proxy))
+
+ return func(next xun.HandleFunc) xun.HandleFunc {
+ return func(c *xun.Context) error {
+ host, _, err := net.SplitHostPort(c.Request.Host)
</code_context>
<issue_to_address>
**issue (bug_risk):** The `Options` constructed in `New` are never used, so `WithForwarder` and other options have no effect.
`options` is constructed and modified by the `opt` functions but never used afterward, so configuration like `WithForwarder` has no effect. Please either integrate `options` into the middleware logic (e.g., to choose forwarders/behavior) or remove the options mechanism to avoid a misleading, non-functional API surface.
</issue_to_address>
### Comment 3
<location> `ext/proxypass/cloudflare/cloudflare.go:75-84` </location>
<code_context>
+ select {
+ case <-ctx.Done():
+ return
+ case <-time.After(IPNetsReloadInterval):
+ ranges := []string{}
+
+ v4, err := c.fetch(IPv4URL)
+ if err == nil {
+ ranges = append(ranges, v4...)
+ }
+
+ v6, err := c.fetch(IPv6URL)
+ if err == nil {
+ ranges = append(ranges, v6...)
+ }
+
+ // If we couldn't fetch from API, use fallback ranges
+ if len(ranges) == 0 {
+ return
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The reload loop stops permanently if both IPv4 and IPv6 fetches fail once, instead of retrying on the next interval.
In `reload`, when both fetches fail and `len(ranges) == 0`, the goroutine `return`s and stops all future refreshes. A temporary network/Cloudflare issue would then permanently disable reloading.
If you want periodic retries, use `continue` instead of `return` so the loop waits for the next `IPNetsReloadInterval` and tries again, while still relying on `defaultNets` via `Contains` as the fallback.
</issue_to_address>
### Comment 4
<location> `ext/proxypass/client.go:48-55` </location>
<code_context>
+ req.Header.Set("X-Domain", domain)
+ req.Header.Set("X-Target", targetURL)
+
+ resp, err := c.client.Do(req)
+ if err != nil {
+ return err
+ }
+
+ defer resp.Body.Close()
+
+ reader := sse.NewReader(resp.Body)
+
+ for {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The client ignores HTTP status codes and proceeds to read SSE even on non-success responses.
After `c.client.Do(req)`, an SSE reader is always created from `resp.Body`. If the server returns an error status (e.g., 401/403/500), `Connect` will still block on `reader.Next()` until EOF instead of failing fast with a clear error.
Check `resp.StatusCode` and return an error for non-2xx responses before creating the SSE reader.
Suggested implementation:
```golang
resp, err := c.client.Do(req)
if err != nil {
return err
}
// Fail fast on non-success HTTP responses instead of attempting to read SSE.
if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
defer resp.Body.Close()
return fmt.Errorf("proxypass connect failed: unexpected HTTP status %s", resp.Status)
}
defer resp.Body.Close()
reader := sse.NewReader(resp.Body)
```
To compile successfully, ensure the following at the top of `ext/proxypass/client.go`:
1. The `net/http` package is imported (likely already present for `http.NewRequestWithContext`).
2. Add `fmt` to the import list if it's not already imported:
- `import "fmt"`
If the file currently uses a grouped import block (e.g. `import (...)`), add `fmt` within that block.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+25
to
+34
| func up(domain string, s *Proxy) { | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| items := servers.Load().(map[string]*Proxy) | ||
| items[domain] = s | ||
| servers.Store(items) | ||
| } | ||
|
|
||
| func down(domain string) { | ||
| mu.Lock() |
There was a problem hiding this comment.
issue (bug_risk): Concurrent access to the servers map is racy: writers hold a mutex but readers don't, and the same map instance is mutated after being stored in atomic.Value.
In up/down you load the map from servers, mutate it in place under mu, and store it back, while get reads the same map via servers.Load() without taking mu.
This leads to:
- A race between
getandup/downon the same map instance, which can cause aconcurrent map read and map writepanic. - Incorrect use of
atomic.Value: atomicity only covers replacing the map reference, not concurrent mutations of the map it points to.
Either guard both reads and writes with the same mutex, or switch to a copy-on-write approach where up/down build a new map instance, modify it, and Store that, while get only reads the loaded map without mutating it.
Comment on lines
+19
to
+28
| func New(opts ...Option) xun.Middleware { // skipcq: GO-R1005 | ||
| options := &Options{} | ||
|
|
||
| for _, opt := range opts { | ||
| opt(options) | ||
| } | ||
|
|
||
| connections = sse.New() | ||
|
|
||
| servers = &atomic.Value{} |
There was a problem hiding this comment.
issue (bug_risk): The Options constructed in New are never used, so WithForwarder and other options have no effect.
options is constructed and modified by the opt functions but never used afterward, so configuration like WithForwarder has no effect. Please either integrate options into the middleware logic (e.g., to choose forwarders/behavior) or remove the options mechanism to avoid a misleading, non-functional API surface.
Comment on lines
+75
to
+84
| case <-time.After(IPNetsReloadInterval): | ||
| ranges := []string{} | ||
|
|
||
| v4, err := c.fetch(IPv4URL) | ||
| if err == nil { | ||
| ranges = append(ranges, v4...) | ||
| } | ||
|
|
||
| v6, err := c.fetch(IPv6URL) | ||
| if err == nil { |
There was a problem hiding this comment.
issue (bug_risk): The reload loop stops permanently if both IPv4 and IPv6 fetches fail once, instead of retrying on the next interval.
In reload, when both fetches fail and len(ranges) == 0, the goroutine returns and stops all future refreshes. A temporary network/Cloudflare issue would then permanently disable reloading.
If you want periodic retries, use continue instead of return so the loop waits for the next IPNetsReloadInterval and tries again, while still relying on defaultNets via Contains as the fallback.
Comment on lines
+48
to
+55
| resp, err := c.client.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| defer resp.Body.Close() | ||
|
|
||
| reader := sse.NewReader(resp.Body) |
There was a problem hiding this comment.
suggestion (bug_risk): The client ignores HTTP status codes and proceeds to read SSE even on non-success responses.
After c.client.Do(req), an SSE reader is always created from resp.Body. If the server returns an error status (e.g., 401/403/500), Connect will still block on reader.Next() until EOF instead of failing fast with a clear error.
Check resp.StatusCode and return an error for non-2xx responses before creating the SSE reader.
Suggested implementation:
resp, err := c.client.Do(req)
if err != nil {
return err
}
// Fail fast on non-success HTTP responses instead of attempting to read SSE.
if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
defer resp.Body.Close()
return fmt.Errorf("proxypass connect failed: unexpected HTTP status %s", resp.Status)
}
defer resp.Body.Close()
reader := sse.NewReader(resp.Body)To compile successfully, ensure the following at the top of ext/proxypass/client.go:
- The
net/httppackage is imported (likely already present forhttp.NewRequestWithContext). - Add
fmtto the import list if it's not already imported:import "fmt"
If the file currently uses a grouped import block (e.g.import (...)), addfmtwithin that block.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed
Fixed
Added
Tests
Tasks to complete before merging PR:
make unit-teststo check for any regressions 📋make lintto check for any issuesSummary by Sourcery
Introduce a proxypass reverse proxy subsystem with dynamic backend registration, Cloudflare integration utilities, and small middleware and error-handling adjustments.
New Features:
Bug Fixes:
Enhancements: