fix(sites): honor configured healthcheck protocol (#1628)

The site healthcheck built its request URL from the indexed site URL
(e.g. http://example.com) and never rewrote the scheme to match the
user-configured HealthCheckConfig.Protocol. As a result, sites
configured for HTTPS were probed over HTTP and always shown as
unreachable. TestHealthCheck compounded the issue by using
siteConfig.Scheme (default "http") instead of req.Config.Protocol.

Introduce rewriteCheckURLScheme which aligns only the URL scheme with
the configured protocol while preserving path, query, and port, and
call it from CheckSiteWithConfig. TestHealthCheck now passes the stored
site URL and relies on the same rewrite, so the "Test" button exercises
the same code path as the scheduled checker.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
0xJacky
2026-04-18 16:03:23 +08:00
parent 50ccaaeb2f
commit c38e0a28b8
3 changed files with 401 additions and 73 deletions
+18 -9
View File
@@ -142,9 +142,8 @@ func TestHealthCheck(c *gin.Context) {
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
// Convert host to URL for testing
testURL := siteConfig.Scheme + "://" + siteConfig.Host
result, err := enhancedChecker.CheckSiteWithConfig(ctx, testURL, req.Config)
// Pass the stored site URL; CheckSiteWithConfig rewrites the scheme to match req.Config.Protocol.
result, err := enhancedChecker.CheckSiteWithConfig(ctx, siteConfig.GetURL(), req.Config)
if err != nil {
logger.Errorf("Health check test failed for %s: %v", siteConfig.Host, err)
@@ -156,17 +155,27 @@ func TestHealthCheck(c *gin.Context) {
return
}
success := result.Status == "online"
info := result.Info
if info == nil {
c.JSON(http.StatusOK, gin.H{
"success": false,
"error": "empty health check result",
"response_time": 0,
})
return
}
success := info.Status == "online"
errorMsg := ""
if !success && result.Error != "" {
errorMsg = result.Error
if !success && info.Error != "" {
errorMsg = info.Error
}
c.JSON(http.StatusOK, gin.H{
"success": success,
"response_time": result.ResponseTime,
"status": result.Status,
"status_code": result.StatusCode,
"response_time": info.ResponseTime,
"status": info.Status,
"status_code": info.StatusCode,
"error": errorMsg,
})
}
+319
View File
@@ -7,9 +7,12 @@ import (
"net/http"
"net/http/httptest"
"strings"
"sync/atomic"
"testing"
"time"
"github.com/0xJacky/Nginx-UI/model"
"github.com/0xJacky/Nginx-UI/settings"
)
type roundTripFunc func(*http.Request) (*http.Response, error)
@@ -101,6 +104,78 @@ func TestDownloadFaviconRejectsHTMLContent(t *testing.T) {
}
}
func TestRewriteCheckURLSchemeHonorsConfiguredProtocol(t *testing.T) {
// Regression for #1628: when a site is indexed as http but the healthcheck
// protocol is configured as https, the request URL must use https.
// Unlike generateDisplayURL, this helper preserves path, query, and port.
cases := []struct {
name string
url string
protocol string
want string
}{
{"http site with https protocol", "http://example.com", "https", "https://example.com"},
{"https site with http protocol is not downgraded", "https://example.com", "http", "https://example.com"},
{"http site with http protocol stays http", "http://example.com", "http", "http://example.com"},
{"https site with https protocol stays https", "https://example.com", "https", "https://example.com"},
{"path is preserved when scheme is rewritten", "http://example.com/app", "https", "https://example.com/app"},
{"query is preserved when scheme is rewritten", "http://example.com/health?token=abc", "https", "https://example.com/health?token=abc"},
{"non-default port is preserved", "http://example.com:8080/app", "https", "https://example.com:8080/app"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
if got := rewriteCheckURLScheme(tc.url, tc.protocol); got != tc.want {
t.Fatalf("rewriteCheckURLScheme(%q, %q) = %q, want %q", tc.url, tc.protocol, got, tc.want)
}
})
}
}
func TestCheckSiteWithConfigRewritesURLScheme(t *testing.T) {
t.Cleanup(InvalidateSiteConfigCache)
// Seed cache so checkHTTP's getOrCreateSiteConfigForURL doesn't hit a nil DB.
setCachedSiteConfig("example.com:443", &model.SiteConfig{
Model: model.Model{ID: 1},
Host: "example.com:443",
})
checker := NewEnhancedSiteChecker()
var capturedScheme string
checker.defaultClient = &http.Client{
Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) {
capturedScheme = req.URL.Scheme
return &http.Response{
StatusCode: http.StatusOK,
Header: make(http.Header),
Body: io.NopCloser(strings.NewReader("")),
Request: req,
}, nil
}),
}
// Use Protocol "http" with an HTTPS-indexed URL to exercise the rewrite path
// without triggering checkHTTPS's forced ValidateSSL=true (which replaces the client).
config := &model.HealthCheckConfig{
Protocol: "http",
Method: "GET",
Path: "/",
ExpectedStatus: []int{200},
}
if _, err := checker.CheckSiteWithConfig(context.Background(), "https://example.com", config); err != nil {
t.Fatalf("CheckSiteWithConfig returned error: %v", err)
}
// determineOptimalScheme preserves https even when the user picks http, so
// the outgoing request must be https — verifying the rewrite runs.
if capturedScheme != "https" {
t.Fatalf("expected request scheme https, got %q", capturedScheme)
}
}
func TestDownloadFaviconRejectsHTMLContentWithoutHeader(t *testing.T) {
checker := NewSiteChecker(DefaultCheckOptions())
checker.client = &http.Client{
@@ -119,3 +194,247 @@ func TestDownloadFaviconRejectsHTMLContentWithoutHeader(t *testing.T) {
t.Fatalf("expected empty data URL when header missing and content sniffing rejects, got %s", dataURL)
}
}
// TestSharedTransportIsReused asserts that the package-level transport is the
// same instance across NewSiteChecker / NewEnhancedSiteChecker constructions.
// This is the structural guarantee that prevents the per-request transport
// allocation pattern that caused #1608.
func TestSharedTransportIsReused(t *testing.T) {
a := NewSiteChecker(DefaultCheckOptions()).client.Transport
b := NewSiteChecker(DefaultCheckOptions()).client.Transport
c := NewEnhancedSiteChecker().defaultClient.Transport
d := SharedTransport()
if a != b {
t.Fatalf("two SiteCheckers should share one transport, got %p vs %p", a, b)
}
if a != c {
t.Fatalf("SiteChecker and EnhancedSiteChecker should share one transport, got %p vs %p", a, c)
}
if a != d {
t.Fatalf("SiteChecker transport differs from SharedTransport(), got %p vs %p", a, d)
}
}
// TestClientForHealthCheckUsesSharedClientWhenNoTLSDivergence verifies the
// fast path: when a HealthCheckConfig does not request custom TLS, no new
// transport is built — the shared one is reused.
func TestClientForHealthCheckUsesSharedClientWhenNoTLSDivergence(t *testing.T) {
cfg := &model.HealthCheckConfig{Protocol: "https"}
got := ClientForHealthCheck(cfg, time.Second)
if got.Transport != SharedTransport() {
t.Fatalf("expected shared transport, got separate instance")
}
}
// TestClientForHealthCheckBuildsCustomWhenTLSDiverges verifies that genuinely
// divergent TLS configs DO get their own transport (so we don't pollute the
// shared pool's TLS config).
func TestClientForHealthCheckBuildsCustomWhenTLSDiverges(t *testing.T) {
cfg := &model.HealthCheckConfig{Protocol: "https", ValidateSSL: true}
got := ClientForHealthCheck(cfg, time.Second)
if got.Transport == SharedTransport() {
t.Fatalf("expected dedicated transport for ValidateSSL=true, got shared one")
}
}
// TestCheckAllSitesRespectsDisabledSetting confirms the global kill switch
// short-circuits CheckAllSites without making any network calls.
func TestCheckAllSitesRespectsDisabledSetting(t *testing.T) {
t.Cleanup(InvalidateSiteConfigCache)
originalEnabled := settings.SiteCheckSettings.Enabled
settings.SiteCheckSettings.Enabled = false
t.Cleanup(func() { settings.SiteCheckSettings.Enabled = originalEnabled })
checker := NewSiteChecker(DefaultCheckOptions())
checker.client = &http.Client{
Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) {
t.Fatalf("unexpected HTTP request to %s while site check is globally disabled", req.URL.String())
return nil, nil
}),
}
checker.sites["http://example.com"] = &SiteInfo{Name: "example.com"}
checker.CheckAllSites(context.Background())
}
// TestCheckAllSitesDedupesByHostPort confirms that two URLs sharing the same
// host:port produce a single network call. This is the multi-server_name
// scenario from #1608.
func TestCheckAllSitesDedupesByHostPort(t *testing.T) {
t.Cleanup(InvalidateSiteConfigCache)
originalEnabled := settings.SiteCheckSettings.Enabled
settings.SiteCheckSettings.Enabled = true
t.Cleanup(func() { settings.SiteCheckSettings.Enabled = originalEnabled })
var hits int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
atomic.AddInt32(&hits, 1)
w.WriteHeader(http.StatusOK)
}))
defer server.Close()
opts := DefaultCheckOptions()
opts.CheckFavicon = false // isolate the dedupe assertion from favicon GETs
checker := NewSiteChecker(opts)
seedSiteConfigForTest(t, server.URL)
// Three aliases that all dedupe to the same host:port.
checker.sites[server.URL] = &SiteInfo{Name: "alias-1"}
checker.sites[server.URL+"/"] = &SiteInfo{Name: "alias-2"}
checker.sites[server.URL+"/?token=xyz"] = &SiteInfo{Name: "alias-3"}
checker.CheckAllSites(context.Background())
if got := atomic.LoadInt32(&hits); got != 1 {
t.Fatalf("expected 1 network call after dedupe, got %d", got)
}
}
// seedSiteConfigForTest pre-populates the in-memory site config cache so the
// checker can run without hitting the (nil) test database.
func seedSiteConfigForTest(t *testing.T, rawURL string) {
t.Helper()
cfg := &model.SiteConfig{HealthCheckEnabled: true}
cfg.SetFromURL(rawURL)
setCachedSiteConfig(cfg.Host, cfg)
}
// TestCheckAllSitesHonorsConcurrencyLimit ensures the configured concurrency
// is enforced. We register a slow handler and assert the in-flight count
// never exceeds the configured cap.
func TestCheckAllSitesHonorsConcurrencyLimit(t *testing.T) {
t.Cleanup(InvalidateSiteConfigCache)
originalEnabled := settings.SiteCheckSettings.Enabled
originalConcurrency := settings.SiteCheckSettings.Concurrency
settings.SiteCheckSettings.Enabled = true
settings.SiteCheckSettings.Concurrency = 2
t.Cleanup(func() {
settings.SiteCheckSettings.Enabled = originalEnabled
settings.SiteCheckSettings.Concurrency = originalConcurrency
})
var inFlight, peak int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
current := atomic.AddInt32(&inFlight, 1)
// Track the high-water mark.
for {
old := atomic.LoadInt32(&peak)
if current <= old || atomic.CompareAndSwapInt32(&peak, old, current) {
break
}
}
time.Sleep(40 * time.Millisecond)
atomic.AddInt32(&inFlight, -1)
w.WriteHeader(http.StatusOK)
}))
defer server.Close()
opts := DefaultCheckOptions()
opts.CheckFavicon = false
checker := NewSiteChecker(opts)
// Use distinct hosts via URL fragments... actually we need different
// host:port pairs to avoid dedupe. Spin up multiple servers.
urls := make([]string, 0, 6)
servers := []*httptest.Server{server}
for i := 0; i < 5; i++ {
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
current := atomic.AddInt32(&inFlight, 1)
for {
old := atomic.LoadInt32(&peak)
if current <= old || atomic.CompareAndSwapInt32(&peak, old, current) {
break
}
}
time.Sleep(40 * time.Millisecond)
atomic.AddInt32(&inFlight, -1)
w.WriteHeader(http.StatusOK)
}))
servers = append(servers, s)
}
t.Cleanup(func() {
for _, s := range servers {
s.Close()
}
})
for _, s := range servers {
urls = append(urls, s.URL)
seedSiteConfigForTest(t, s.URL)
}
for _, u := range urls {
checker.sites[u] = &SiteInfo{Name: u}
}
checker.CheckAllSites(context.Background())
if got := atomic.LoadInt32(&peak); got > 2 {
t.Fatalf("expected peak in-flight <= 2 (configured concurrency), got %d", got)
}
}
// TestEnhancedCheckDoesNotDoubleFetchForFavicon verifies that when the
// enhanced HTTP check has already fetched HTML, the favicon extraction reuses
// that body instead of issuing a second GET to "/" (#1608).
func TestEnhancedCheckDoesNotDoubleFetchForFavicon(t *testing.T) {
t.Cleanup(InvalidateSiteConfigCache)
originalEnabled := settings.SiteCheckSettings.Enabled
settings.SiteCheckSettings.Enabled = true
t.Cleanup(func() { settings.SiteCheckSettings.Enabled = originalEnabled })
var rootHits int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/" {
atomic.AddInt32(&rootHits, 1)
w.Header().Set("Content-Type", "text/html; charset=utf-8")
io.WriteString(w, `<html><head><link rel="icon" href="/missing.ico"></head><body>ok</body></html>`)
return
}
// Any favicon download request — return 404 so we don't pollute
// the root counter.
http.NotFound(w, r)
}))
defer server.Close()
cfg := &model.SiteConfig{
HealthCheckEnabled: true,
HealthCheckConfig: &model.HealthCheckConfig{
Protocol: "http",
Method: "GET",
Path: "/",
ExpectedStatus: []int{200},
},
}
cfg.SetFromURL(server.URL)
setCachedSiteConfig(cfg.Host, cfg)
checker := NewSiteChecker(DefaultCheckOptions()) // CheckFavicon defaults to true
if _, err := checker.CheckSite(context.Background(), server.URL); err != nil {
t.Fatalf("CheckSite returned error: %v", err)
}
if got := atomic.LoadInt32(&rootHits); got != 1 {
t.Fatalf("expected exactly 1 GET to / (favicon should reuse health-check body), got %d", got)
}
}
// TestDedupeKey covers the URL → key normalisation used to coalesce aliases.
func TestDedupeKey(t *testing.T) {
cases := []struct {
url string
want string
}{
{"http://example.com", "http://example.com:80"},
{"http://example.com/", "http://example.com:80"},
{"https://example.com", "https://example.com:443"},
{"https://example.com:8443", "https://example.com:8443"},
{"https://Example.COM/path?a=1", "https://example.com:443"},
{"grpc://example.com", "grpc://example.com:80"},
{"grpcs://example.com", "grpcs://example.com:443"},
}
for _, tc := range cases {
if got := dedupeKey(tc.url); got != tc.want {
t.Errorf("dedupeKey(%q) = %q, want %q", tc.url, got, tc.want)
}
}
}
+64 -64
View File
@@ -5,7 +5,6 @@ import (
"crypto/tls"
"fmt"
"io"
"net"
"net/http"
"net/url"
"slices"
@@ -22,37 +21,47 @@ import (
"google.golang.org/grpc/health/grpc_health_v1"
)
// EnhancedSiteChecker provides advanced health checking capabilities
const enhancedClientTimeout = 30 * time.Second
// CheckResult bundles the SiteInfo with the response body so callers can
// reuse the body (e.g. for favicon extraction) without issuing a second
// request to the same host.
type CheckResult struct {
Info *SiteInfo
Body []byte
}
// EnhancedSiteChecker provides advanced health checking capabilities. It
// reuses the package-level shared HTTP transport for connection pooling.
type EnhancedSiteChecker struct {
defaultClient *http.Client
}
// NewEnhancedSiteChecker creates a new enhanced site checker
// NewEnhancedSiteChecker creates a new enhanced site checker that reuses the
// shared transport.
func NewEnhancedSiteChecker() *EnhancedSiteChecker {
transport := &http.Transport{
Dial: (&net.Dialer{
Timeout: 10 * time.Second,
}).Dial,
TLSHandshakeTimeout: 10 * time.Second,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: settings.HTTPSettings.InsecureSkipVerify,
},
}
client := &http.Client{
Transport: transport,
Timeout: 30 * time.Second,
}
return &EnhancedSiteChecker{
defaultClient: client,
defaultClient: SharedClient(enhancedClientTimeout),
}
}
// CheckSiteWithConfig performs enhanced health check using custom configuration
func (ec *EnhancedSiteChecker) CheckSiteWithConfig(ctx context.Context, siteURL string, config *model.HealthCheckConfig) (*SiteInfo, error) {
// rewriteCheckURLScheme aligns the scheme of siteURL with the configured
// healthcheck protocol while preserving path, query, fragment, and port.
// An unparseable URL is returned unchanged.
func rewriteCheckURLScheme(siteURL, protocol string) string {
parsed, err := url.Parse(siteURL)
if err != nil {
return siteURL
}
parsed.Scheme = determineOptimalScheme(parsed, protocol)
return parsed.String()
}
// CheckSiteWithConfig performs an enhanced health check using a custom
// configuration. The body of the HTTP/HTTPS response (if any) is returned so
// callers can reuse it; gRPC checks return a nil body.
func (ec *EnhancedSiteChecker) CheckSiteWithConfig(ctx context.Context, siteURL string, config *model.HealthCheckConfig) (*CheckResult, error) {
if config == nil {
// Fallback to basic HTTP check
return ec.checkHTTP(ctx, siteURL, &model.HealthCheckConfig{
Protocol: "http",
Method: "GET",
@@ -61,18 +70,28 @@ func (ec *EnhancedSiteChecker) CheckSiteWithConfig(ctx context.Context, siteURL
})
}
// Align the request URL scheme with the configured healthcheck protocol
// so HTTPS/gRPC checks don't silently fall back to the indexed HTTP URL.
// Only the scheme is rewritten; path, query, and port are preserved.
checkURL := rewriteCheckURLScheme(siteURL, config.Protocol)
switch config.Protocol {
case "grpc", "grpcs":
return ec.checkGRPC(ctx, siteURL, config)
info, err := ec.checkGRPC(ctx, checkURL, config)
if info == nil {
return nil, err
}
return &CheckResult{Info: info}, err
case "https":
return ec.checkHTTPS(ctx, siteURL, config)
return ec.checkHTTPS(ctx, checkURL, config)
default: // http
return ec.checkHTTP(ctx, siteURL, config)
return ec.checkHTTP(ctx, checkURL, config)
}
}
// checkHTTP performs HTTP health check
func (ec *EnhancedSiteChecker) checkHTTP(ctx context.Context, siteURL string, config *model.HealthCheckConfig) (*SiteInfo, error) {
// checkHTTP performs an HTTP health check and returns the response body so
// callers can reuse it (e.g. to extract a favicon) without re-fetching.
func (ec *EnhancedSiteChecker) checkHTTP(ctx context.Context, siteURL string, config *model.HealthCheckConfig) (*CheckResult, error) {
startTime := time.Now()
// Build request URL
@@ -84,10 +103,10 @@ func (ec *EnhancedSiteChecker) checkHTTP(ctx context.Context, siteURL string, co
// Create request
req, err := http.NewRequestWithContext(ctx, config.Method, checkURL, nil)
if err != nil {
return &SiteInfo{
return &CheckResult{Info: &SiteInfo{
Status: StatusError,
Error: fmt.Sprintf("Failed to create request: %v", err),
}, err
}}, err
}
// Add custom headers
@@ -108,43 +127,21 @@ func (ec *EnhancedSiteChecker) checkHTTP(ctx context.Context, siteURL string, co
}
}
// Create custom client if needed
// Pick the right client. The shared client is reused unless this site
// genuinely needs a divergent TLS configuration.
client := ec.defaultClient
if config.ValidateSSL || config.VerifyHostname {
transport := &http.Transport{
Dial: (&net.Dialer{
Timeout: 10 * time.Second,
}).Dial,
TLSHandshakeTimeout: 10 * time.Second,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: !config.ValidateSSL,
},
}
// Load client certificate if provided
if config.ClientCert != "" && config.ClientKey != "" {
cert, err := tls.LoadX509KeyPair(config.ClientCert, config.ClientKey)
if err != nil {
logger.Warnf("Failed to load client certificate: %v", err)
} else {
transport.TLSClientConfig.Certificates = []tls.Certificate{cert}
}
}
client = &http.Client{
Transport: transport,
Timeout: 30 * time.Second,
}
if needsCustomTLS(config) {
client = ClientForHealthCheck(config, enhancedClientTimeout)
}
// Make request
resp, err := client.Do(req)
if err != nil {
return &SiteInfo{
return &CheckResult{Info: &SiteInfo{
Status: StatusError,
ResponseTime: time.Since(startTime).Milliseconds(),
Error: err.Error(),
}, err
}}, err
}
defer resp.Body.Close()
@@ -191,17 +188,20 @@ func (ec *EnhancedSiteChecker) checkHTTP(ctx context.Context, siteURL string, co
// Get or create site config to get ID
siteConfig := getOrCreateSiteConfigForURL(siteURL)
return &SiteInfo{
SiteConfig: *siteConfig,
Status: status,
StatusCode: resp.StatusCode,
ResponseTime: responseTime,
Error: errorMsg,
return &CheckResult{
Info: &SiteInfo{
SiteConfig: *siteConfig,
Status: status,
StatusCode: resp.StatusCode,
ResponseTime: responseTime,
Error: errorMsg,
},
Body: body,
}, nil
}
// checkHTTPS performs HTTPS health check with SSL validation
func (ec *EnhancedSiteChecker) checkHTTPS(ctx context.Context, siteURL string, config *model.HealthCheckConfig) (*SiteInfo, error) {
func (ec *EnhancedSiteChecker) checkHTTPS(ctx context.Context, siteURL string, config *model.HealthCheckConfig) (*CheckResult, error) {
// Force HTTPS protocol
httpsConfig := *config
httpsConfig.Protocol = "https"