config: Adding support to read configuration from http endpoint - #5405
config: Adding support to read configuration from http endpoint#5405jshah-dev wants to merge 4 commits into
Conversation
Signed-off-by: jshah-dev <jigar.shah.sde@gmail.com>
📝 WalkthroughWalkthroughAlertmanager now accepts configuration from either a local file or an HTTP endpoint. CLI and option validation enforce exactly one source, loaders handle retrieval, startup and reload coordination use the selected loader, and tests and documentation cover HTTP configuration behavior. ChangesHTTP configuration source
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant App
participant ConfigLoader
participant ConfigCoordinator
CLI->>App: Provide config.file or config.http-url
App->>ConfigLoader: Load initial configuration
ConfigLoader-->>App: Return configuration bytes
App->>ConfigCoordinator: Construct with selected loader
ConfigCoordinator->>ConfigLoader: Load on configuration reload
ConfigLoader-->>ConfigCoordinator: Return updated bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
app/http_config_test.go (1)
72-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the startup assertions observe
app.Start. Both tests discard theStartresult, while thectx.Done()branch cannot run until the deferred cancellation after the assertion.
app/http_config_test.go#L72-L82: captureapp.Start()in an error channel and perform a controlled shutdown before asserting its result.app/http_config_test.go#L122-L130: apply the same assertion pattern to the file-backed startup test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/http_config_test.go` around lines 72 - 82, Update both startup tests in app/http_config_test.go at lines 72-82 and 122-130 to capture app.Start() errors through an error channel, perform a controlled shutdown, then assert the observed result; replace the ineffective ctx.Done() check while preserving each test’s existing setup and cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/options.go`:
- Around line 52-53: Document the exported Options.ConfigHTTPURL field with a
full-sentence Go comment that starts with ConfigHTTPURL and ends with a period.
In `@config/loader_test.go`:
- Around line 44-60: The “unreadable file” test should use a portable path that
guarantees os.ReadFile failure instead of relying on chmod permissions. Update
the test around NewFileLoader to pass a directory path or another
guaranteed-invalid read target, remove the Windows skip and chmod cleanup, and
retain the assertion that Load returns an error.
In `@config/loader.go`:
- Around line 35-41: Redact configuration URL secrets across all affected sites:
in config/loader.go lines 35-41, sanitize URL-bearing request and transport
errors before returning them; in config/coordinator.go lines 54-55, store a
sanitized logging source instead of the request URL; and in app/app.go lines
252-258, log only that sanitized source while retaining the original URL
exclusively for the HTTP request.
- Around line 4-48: Format all affected Go changes with gofumpt and goimports,
configuring the local import prefix as github.com/prometheus/alertmanager:
config/loader.go lines 4-48, config/coordinator.go lines 31-34,
config/coordinator_test.go lines 49-51, app/app.go lines 249-270, and
app/http_config_test.go lines 16-83.
- Around line 39-47: Update the HTTP fetch flow around http.DefaultClient.Do and
io.ReadAll to enforce both a request deadline and a documented maximum
configuration body size. Use a bounded context or client timeout so
background-context callers cannot hang indefinitely, and limit the response
reader while rejecting bodies that exceed the configured maximum before
returning data.
- Around line 24-25: Update fileLoader.Load and the related HTTP-loading path to
wrap read/request failures with operation context using fmt.Errorf and %w,
preserving errors.Is/errors.As matching. Before returning HTTP transport errors,
sanitize any URL-bearing error content so credentials cannot propagate to logs,
while retaining the underlying error chain.
In `@README.md`:
- Around line 48-53: Update the HTTP configuration example in the README to use
the repository’s indented command style instead of a fenced code block, and
remove the leading shell prompt character. Preserve the command and surrounding
explanatory text.
---
Nitpick comments:
In `@app/http_config_test.go`:
- Around line 72-82: Update both startup tests in app/http_config_test.go at
lines 72-82 and 122-130 to capture app.Start() errors through an error channel,
perform a controlled shutdown, then assert the observed result; replace the
ineffective ctx.Done() check while preserving each test’s existing setup and
cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d6cac01-6406-4152-a25f-ec5d697018fb
📒 Files selected for processing (11)
README.mdapp/app.goapp/http_config_test.goapp/lifecycle_test.goapp/options.gocmd/alertmanager/main.goconfig/coordinator.goconfig/coordinator_test.goconfig/loader.goconfig/loader_test.godocs/configuration.md
| func (f *fileLoader) Load(_ context.Context) ([]byte, error) { | ||
| return os.ReadFile(f.path) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add operation context while preserving error matching.
Wrap file and HTTP operation failures with %w; sanitize URL-bearing transport errors before returning them so a credentialed URL is not propagated to logs.
As per coding guidelines, “Wrap errors with fmt.Errorf("...: %w", err) and check with errors.Is/errors.As in Go code.”
Also applies to: 35-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/loader.go` around lines 24 - 25, Update fileLoader.Load and the
related HTTP-loading path to wrap read/request failures with operation context
using fmt.Errorf and %w, preserving errors.Is/errors.As matching. Before
returning HTTP transport errors, sanitize any URL-bearing error content so
credentials cannot propagate to logs, while retaining the underlying error
chain.
Source: Coding guidelines
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, h.url, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent configuration URL secrets from reaching logs. Raw HTTP URLs can contain basic-auth credentials, signed query tokens, or other secrets. They are currently exposed through transport errors and structured logs.
config/loader.go#L35-L41: convert URL-bearing request and transport errors into redacted errors before returning them.config/coordinator.go#L54-L55: store a sanitized logging source rather than the request URL.app/app.go#L252-L258: log the sanitized source only; retain the original URL solely for the HTTP request.
📍 Affects 3 files
config/loader.go#L35-L41(this comment)config/coordinator.go#L54-L55app/app.go#L252-L258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/loader.go` around lines 35 - 41, Redact configuration URL secrets
across all affected sites: in config/loader.go lines 35-41, sanitize URL-bearing
request and transport errors before returning them; in config/coordinator.go
lines 54-55, store a sanitized logging source instead of the request URL; and in
app/app.go lines 252-258, log only that sanitized source while retaining the
original URL exclusively for the HTTP request.
| You can also load configuration from an HTTP endpoint: | ||
| ``` | ||
| $ ./alertmanager --config.http-url=http://config-server/config.yaml | ||
| ``` | ||
|
|
||
| Note: `--config.file` and `--config.http-url` are mutually exclusive - exactly one must be specified. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the repository’s command-block style.
The added fenced block triggers MD040, MD046, and MD014. Use the existing indented-command style and omit the $ prompt.
Suggested change
-```
-$ ./alertmanager --config.http-url=http://config-server/config.yaml
-```
+ ./alertmanager --config.http-url=http://config-server/config.yaml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| You can also load configuration from an HTTP endpoint: | |
| ``` | |
| $ ./alertmanager --config.http-url=http://config-server/config.yaml | |
| ``` | |
| Note: `--config.file` and `--config.http-url` are mutually exclusive - exactly one must be specified. | |
| You can also load configuration from an HTTP endpoint: | |
| ./alertmanager --config.http-url=http://config-server/config.yaml | |
| Note: `--config.file` and `--config.http-url` are mutually exclusive - exactly one must be specified. |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 49-49: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
[warning] 50-50: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 48 - 53, Update the HTTP configuration example in the
README to use the repository’s indented command style instead of a fenced code
block, and remove the leading shell prompt character. Preserve the command and
surrounding explanatory text.
Source: Linters/SAST tools
Signed-off-by: jshah-dev <jigar.shah.sde@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/loader.go (1)
83-93: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRaw configured URL can still leak via wrapped transport errors.
Line 91's variable name
sanitizedErris misleading — no sanitization actually happens.client.Do(req)failures are typically*url.Error, whoseError()string embeds the full request URL as constructed (including any userinfo credentials or query secrets), and that string is preserved verbatim by%wwrapping. The same risk applies to thehttp.NewRequestWithContextfailure path on Line 85. This mirrors unresolved concerns raised on earlier revisions of this file about credential-bearing URLs reaching logs.Consider prefixing/annotating with the sanitized URL rather than relying on the (unsanitized) underlying error text for the URL reference:
🔒 Proposed fix
req, err := http.NewRequestWithContext(ctx, http.MethodGet, h.url, nil) if err != nil { - return nil, fmt.Errorf("failed to create HTTP request: %w", err) + return nil, fmt.Errorf("failed to create HTTP request for %s: %w", SanitizeURL(h.url), err) } resp, err := client.Do(req) if err != nil { - // Sanitize URL from error to avoid credential leakage in logs - sanitizedErr := fmt.Errorf("HTTP request failed: %w", err) - return nil, sanitizedErr + return nil, fmt.Errorf("HTTP request to %s failed: %w", SanitizeURL(h.url), err) }Note this doesn't strip the URL embedded inside
err's own message (from*url.Error); fully eliminating that requires either unwrapping to the inner reason or a custom error type that overridesError().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/loader.go` around lines 83 - 93, Update the request error handling around h.url, http.NewRequestWithContext, and client.Do so credential-bearing URLs and raw URL-containing *url.Error messages cannot reach logs. Sanitize the configured URL for error context, unwrap URL transport errors to retain only the underlying reason, and replace the misleading sanitizedErr wrapping with errors that reference the sanitized URL without embedding the original URL-bearing error text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/loader.go`:
- Around line 41-68: The SanitizeURL function returns early after redacting a
password, allowing query parameters to remain exposed. Update SanitizeURL to use
parsed.Redacted() for credential redaction, then always apply the existing
RawQuery redaction before returning, including URLs containing both credentials
and query parameters.
---
Duplicate comments:
In `@config/loader.go`:
- Around line 83-93: Update the request error handling around h.url,
http.NewRequestWithContext, and client.Do so credential-bearing URLs and raw
URL-containing *url.Error messages cannot reach logs. Sanitize the configured
URL for error context, unwrap URL transport errors to retain only the underlying
reason, and replace the misleading sanitizedErr wrapping with errors that
reference the sanitized URL without embedding the original URL-bearing error
text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6e31471-2523-4e58-826e-1c857c8eb6d3
📒 Files selected for processing (7)
app/app.goapp/http_config_test.goapp/options.goconfig/coordinator.goconfig/coordinator_test.goconfig/loader.goconfig/loader_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- config/coordinator_test.go
- config/loader_test.go
- app/options.go
- app/app.go
- app/http_config_test.go
- config/coordinator.go
| // SanitizeURL redacts any credentials from the URL for logging purposes. | ||
| func SanitizeURL(rawURL string) string { | ||
| // Try to parse the URL to extract credentials | ||
| parsed, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| // If parsing fails, just return the original URL | ||
| return rawURL | ||
| } | ||
|
|
||
| // Redact password from URL | ||
| if parsed.User != nil { | ||
| password, _ := parsed.User.Password() | ||
| if password != "" { | ||
| // Replace password with *** | ||
| userInfo := strings.Replace(rawURL, password, "***", 1) | ||
| return userInfo | ||
| } | ||
| } | ||
|
|
||
| // Redact query parameters that might contain secrets | ||
| if parsed.RawQuery != "" { | ||
| // This is a simple approach - in production you might want more sophisticated | ||
| // secret detection, but for logging purposes this provides basic protection | ||
| return strings.Replace(rawURL, parsed.RawQuery, "[redacted]", 1) | ||
| } | ||
|
|
||
| return rawURL | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)config/(loader|coordinator)\.go$|loader\.go|coordinator\.go' || true
echo "== loader.go outline/size =="
wc -l config/loader.go 2>/dev/null || true
sed -n '1,120p' config/loader.go 2>/dev/null | cat -n
echo "== coordinator relevant usages =="
rg -n "SanitizeURL|NewCoordinator|configSource|httpLoader|fl\.url" config -S || true
echo "== tests mentioning SanitizeURL =="
rg -n "SanitizeURL|sanitize" . -S || trueRepository: prometheus/alertmanager
Length of output: 8428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== coordinator.go relevant section =="
sed -n '25,70p' config/coordinator.go | cat -n
echo
sed -n '120,155p' config/coordinator.go | cat -n
echo "== loader_test.go relevant SanitizeURL tests =="
sed -n '1,240p' config/loader_test.go | cat -n | sed -n '1,240p'
echo "== Go tools availability =="
command -v go || true
go version 2>/dev/null || true
echo "== Behavioral probe for current SanitizeURL =="
python3 - <<'PY'
# Mirrors the relevant stdlib net/url URL.Redacted behavior only for the password-redacted URL case used below.
def current(raw):
from urllib.parse import urlparse
p = urlparse(raw)
if p.username is not None:
password = p.password or ""
if password:
return raw.replace(password, "***", 1)
if p.query:
return raw.replace(p.query, "[redacted]", 1)
return raw
def redacted_then_raw_query(raw):
# Mirrors proposed helper-ish behavior for "password then raw query" only.
p = urlparse(raw)
redacted = raw.replace(p.password, "xxxxx", 1)
rp = urlparse(redacted)
if p.query:
return redacted.replace(p.query, "[redacted]", 1)
return redacted
def raw_query_then_redacted(raw):
# Mirrors proposed query-then-credential helper-ish behavior for "raw query then password" only.
rp = urlparse(raw)
redacted = raw.replace(rp.query, "redacted", 1)
p = urlparse(redacted)
if p.userinfo:
pas = [rp.username or "", rp.password or ""]
secret = ":".join(pas)
# Mirrors Redacted() behavior: URL user and password, not userinfo.
return redacted.replace(p.password, "xxxxx", 1)
return redacted
cases = [
"https://user:pass@host/config?token=abc",
"https://user:p%40ss@host/config?token=bad",
]
for raw in cases:
print("raw=", raw)
print("current=", current(raw))
print("candidate=", redacted_then_raw_query(raw))
PYRepository: prometheus/alertmanager
Length of output: 9320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== precise SanitizeURL behavior probe =="
go run - <<'GO'
package main
import (
"fmt"
"net/url"
"strings"
)
func current(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
if parsed.User != nil {
password, _ := parsed.User.Password()
if password != "" {
userInfo := strings.Replace(rawURL, password, "***", 1)
return userInfo
}
}
if parsed.RawQuery != "" {
return strings.Replace(rawURL, parsed.RawQuery, "[redacted]", 1)
}
return rawURL
}
func proposed(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
sanitized := parsed.Redacted()
if parsed.RawQuery != "" {
if p, err := url.Parse(sanitized); err == nil {
p.RawQuery = "redacted"
return p.String()
}
}
return sanitized
}
cases := []string{
"https://user:pass@host/config?token=abc",
"https://user:p%40ss@host/config?token=bad",
"https://user%25colon@host:8080?secret=value",
"https://host?pass=secret",
}
for _, tc := range cases {
fmt.Printf("input: %s\n", tc)
parsed, _ := url.Parse(tc)
fmt.Printf("parsed: User=%v RawQuery=%s\n", parsed.User, parsed.RawQuery)
fmt.Printf("current: %s\n", current(tc))
fmt.Printf("proposed: %s\n", proposed(tc))
fmt.Println()
}
GORepository: prometheus/alertmanager
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cat > "$tmp/sanitize_probe.go" <<'GO'
package main
import (
"fmt"
"net/url"
"strings"
)
func current(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
if parsed.User != nil {
password, _ := parsed.User.Password()
if password != "" {
userInfo := strings.Replace(rawURL, password, "***", 1)
return userInfo
}
}
if parsed.RawQuery != "" {
return strings.Replace(rawURL, parsed.RawQuery, "[redacted]", 1)
}
return rawURL
}
func proposed(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
sanitized := parsed.Redacted()
if parsed.RawQuery != "" {
if p, err := url.Parse(sanitized); err == nil {
p.RawQuery = "redacted"
return p.String()
}
}
return sanitized
}
cases := []string{
"https://user:pass@host/config?token=abc",
"https://user:p%40ss@host/config?token=bad",
"https://user%25colon@host:8080?secret=value",
"https://host?pass=secret",
}
for _, tc := range cases {
fmt.Printf("input: %s\n", tc)
parsed, _ := url.Parse(tc)
fmt.Printf("parsed: User=%v RawQuery=%s\n", parsed.User, parsed.RawQuery)
fmt.Printf("current: %s\n", current(tc))
fmt.Printf("proposed: %s\n", proposed(tc))
fmt.Println()
}
GO
cd "$tmp"
go run sanitize_probe.goRepository: prometheus/alertmanager
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cat > "$tmp/sanitize_probe.go" <<'GO'
package main
import (
"fmt"
"net/url"
"strings"
)
func current(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
if parsed.User != nil {
password, _ := parsed.User.Password()
if password != "" {
userInfo := strings.Replace(rawURL, password,("***", 1)
return userInfo
}
}
if parsed.RawQuery != "" {
return strings.Replace(raw, parsed.Raw Query, "[redacted]", 1)
}
return rawURL
}
func proposed(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
sanitized := parsed.Redacted()
if parsed.RawQuery != "" {
if p, err := url.Parse(sanitized); err == nil {
p.RawQuery = "redacted"
return p.String()
}
}
return sanitized
}
func main() {
cases := []string{
"https://user:pass@host/config?token=abc",
"https://user:p%40ss@host/config?token=bad",
"https://user%25colon@host:8080?secret=value",
"https://host?pass=secret",
}
for _, tc := range cases {
fmt.Printf("input: %s\n", tc)
parsed, _ := url.Parse(tc)
fmt.Printf("parsed: User=%v RawQuery=%s\n", parsed.User, parsed.RawQuery)
fmt.Printf("current: %s\n", current(tc))
fmt.Printf("proposed: %s\n", proposed(tc))
fmt.Println()
}
}
GO
cd "$tmp"
go run sanitize_probe.goRepository: prometheus/alertmanager
Length of output: 365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cat > "$tmp/sanitize_probe.go" <<'ENDGO'
package main
import (
"fmt"
"net/url"
"strings"
)
func current(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
if parsed.User != nil {
password, _ := parsed.User.Password()
if password != "" {
userInfo := strings.Replace(rawURL, password, "***", 1)
return userInfo
}
}
if parsed.RawQuery != "" {
return strings.Replace(rawURL, parsed.RawQuery, "[redacted]", 1)
}
return rawURL
}
func proposed(rawURL string) string {
parsed, err := url.Parse(rawURL)
if err != nil {
return rawURL
}
sanitized := parsed.Redacted()
if parsed.RawQuery != "" {
if p, err := url.Parse(sanitized); err == nil {
p.RawQuery = "redacted"
return p.String()
}
}
return sanitized
}
func main() {
cases := []string{
"https://user:pass@host/config?token=abc",
"https://user:p%40ss@host/config?token=bad",
"https://user%25 colon@host:8080?secret=value",
"https://host?pass=secret",
}
for _, tc := range cases {
fmt.Printf("input: %s\n", tc)
parsed, _ := url.Parse(tc)
fmt.Printf("parsed: User=%v RawQuery=%s\n", parsed.User, parsed.RawQuery)
fmt.Printf("current: %s\n", current(tc))
fmt.Printf("proposed: %s\n", proposed(tc))
fmt.Println()
}
}
ENDGO
cd "$tmp"
go run sanitize_probe.goRepository: prometheus/alertmanager
Length of output: 854
Always redact query parameters in SanitizeURL.
The password case returns before checking RawQuery, so URLs with both credentials and secret-bearing query parameters leak the query string into configSource (used by NewCoordinator and config.LoadConfig). Use parsed.Redacted() for credentials and still apply query redaction regardless of whether a password was present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/loader.go` around lines 41 - 68, The SanitizeURL function returns
early after redacting a password, allowing query parameters to remain exposed.
Update SanitizeURL to use parsed.Redacted() for credential redaction, then
always apply the existing RawQuery redaction before returning, including URLs
containing both credentials and query parameters.
Signed-off-by: jshah-dev <jigar.shah.sde@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/loader.go`:
- Around line 55-63: Update the URL redaction logic around
parsed.User.Password() to redact credentials structurally using
parsed.Redacted() or by mutating the parsed URL before query redaction, ensuring
percent-encoded usernames and passwords cannot remain in rawURL. Preserve the
existing query-parameter redaction, and add a regression test covering escaped
credentials such as p%40ss.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04a4f0c9-ec96-4e46-9c63-cf635a5f1f28
📒 Files selected for processing (1)
config/loader.go
| rawURL = strings.Replace(rawURL, password, "***", 1) | ||
| } | ||
| } | ||
|
|
||
| // Redact query parameters that might contain secrets | ||
| if parsed.RawQuery != "" { | ||
| // This is a simple approach - in production you might want more sophisticated | ||
| // secret detection, but for logging purposes this provides basic protection | ||
| rawURL = strings.Replace(rawURL, parsed.RawQuery, "[redacted]", 1) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat >/tmp/url_redaction_probe.go <<'EOF'
package main
import (
"fmt"
"net/url"
"strings"
)
func main() {
raw := "https://user:p%40ss@example/config"
parsed, _ := url.Parse(raw)
password, _ := parsed.User.Password()
fmt.Println(strings.Replace(raw, password, "***", 1))
fmt.Println(parsed.Redacted())
}
EOF
go run /tmp/url_redaction_probe.goRepository: prometheus/alertmanager
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant loader.go excerpt:"
sed -n '1,90p' config/loader.go
echo
echo "URL package behavior probe with more credential forms:"
cat >/tmp/url_redaction_probe2.go <<'EOF'
package main
import (
"fmt"
"net/url"
"strings"
)
func main() {
tests := []string{
"https://user:p%40ss@example/config",
"https://user:foo%2Fbar@example/config?secret=value",
"https://user:P%40ss:foo@example/config",
}
for _, raw := range tests {
parsed, err := url.Parse(raw)
fmt.Printf("raw=%q err=%v\n", raw, err)
if parsed.User != nil {
user, pass := parsed.User.Username(), ""
if parsed.User != nil {
pass, _ = parsed.User.Password()
}
current := raw
if pass != "" {
current = strings.Replace(current, pass, "***", 1)
}
fmt.Printf(" password=%q user=%q string-replaced=%s parsed.Redacted=%s\n", pass, user, current, parsed.Redacted())
}
}
}
EOF
go run /tmp/url_redaction_probe2.goRepository: prometheus/alertmanager
Length of output: 3830
Redact URL credentials structurally before logging.
parsed.User.Password() returns the decoded password, so %-encoded credentials like p%40ss are not replaced in rawURL and remain visible in logs. Replace the password handling section with parsed.Redacted() or mutate the parsed URL before applying query-value redaction, and cover escaped credentials in the regression tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/loader.go` around lines 55 - 63, Update the URL redaction logic around
parsed.User.Password() to redact credentials structurally using
parsed.Redacted() or by mutating the parsed URL before query redaction, ensuring
percent-encoded usernames and passwords cannot remain in rawURL. Preserve the
existing query-parameter redaction, and add a regression test covering escaped
credentials such as p%40ss.
| @@ -0,0 +1,195 @@ | |||
| // Copyright 2024 Prometheus Team | |||
There was a problem hiding this comment.
make: *** [Makefile.common:146: common-check_license] Error 1
please use the license header without date. eg https://github.com/prometheus/alertmanager/blob/main/config/config.go#L1-L12
There was a problem hiding this comment.
Thank you @TheMeier for suggestions, will do the changes
Signed-off-by: jshah-dev <jigar.shah.sde@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/loader.go (1)
123-132: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllow configuration at the documented 10 MiB maximum.
LimitReadercaps reads atmaxConfigSize, so an exact 10 MiB response is still rejected bylen(data) >= maxConfigSize. The check should only reject sizes strictly above the maximum.Proposed fix
- limitedReader := io.LimitReader(resp.Body, maxConfigSize) + limitedReader := io.LimitReader(resp.Body, maxConfigSize+1) data, err := io.ReadAll(limitedReader) @@ - if len(data) >= maxConfigSize { + if len(data) > maxConfigSize {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/loader.go` around lines 123 - 132, Update the size-limit validation after io.ReadAll in the configuration loader to reject only data strictly larger than maxConfigSize, allowing responses exactly at the documented 10 MiB limit. Preserve the existing error behavior for oversized configurations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@config/loader.go`:
- Around line 123-132: Update the size-limit validation after io.ReadAll in the
configuration loader to reject only data strictly larger than maxConfigSize,
allowing responses exactly at the documented 10 MiB limit. Preserve the existing
error behavior for oversized configurations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 90dda4a6-ef57-477d-ad97-4e99c078b283
📒 Files selected for processing (3)
app/http_config_test.goconfig/loader.goconfig/loader_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- app/http_config_test.go
Spaceman1701
left a comment
Spaceman1701
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I think this is an interesting idea.
I've left some comments for a few things which I'd want to see fixed.
|
|
||
| configLogger := logger.With("component", "configuration") | ||
| if opts.ConfigHTTPURL != "" { | ||
| loader = config.NewHTTPLoader(opts.ConfigHTTPURL) |
There was a problem hiding this comment.
does this need to be reconstructed? It seems like we could use the loader that's assigned on either 253 or 258.
| @@ -0,0 +1,195 @@ | |||
| // Copyright The Prometheus Authors | |||
There was a problem hiding this comment.
small nitpick: this seems to be tests for all sorts of config loading, not just http_config - I think it would make more sense for this file to be called config_loader_test.go or something along those lines.
|
|
||
| // Validate exactly one configuration source is provided. | ||
| if *configFile == "" && *configHTTPURL == "" { | ||
| kingpin.Fatalf("Need to configure one of the following --config.file or --config.http-url") |
There was a problem hiding this comment.
I believe this is a behavior regression - there may be production users of Alertmanager who depend on the default value of the --config.file flag. We cannot change this behavior without breaking those users.
| func NewCoordinator(configFilePath string, r prometheus.Registerer, l *slog.Logger) *Coordinator { | ||
| func NewCoordinator(loader ConfigLoader, configFilePath string, r prometheus.Registerer, l *slog.Logger) *Coordinator { | ||
| // Determine the source string for logging | ||
| source := configFilePath |
There was a problem hiding this comment.
this is a bit awkward - perhaps it could be modeled differently. What if ConfigLoader required a Source method that exposed this value polymorphically? If we do it that way, the Coordinator doesn't need to know what the concrete implementation of loader actually is.
As it's written now, this is a potential cause of breakage if future changes add new ConfigLoader implementations.
|
|
||
| // Limit response body size to prevent memory issues | ||
| // 10MB should be more than enough for any reasonable configuration | ||
| const maxConfigSize = 10 * 1024 * 1024 // 10MB |
There was a problem hiding this comment.
is this a necessary limit? The file loader doesn't appear to have one. I'd lean towards simplicity and leave the limit out.
@Spaceman1701 Thank you for your valuable feedback & comments. I will review it & try doing necessary fixes |
3 participants
Description
This PR adds support for loading the Alertmanager configuration from an HTTP endpoint as an alternative to the existing local file-based configuration.
A new command-line option is introduced:
--config.http-urland--config.fileare mutually exclusive configuration sources.When
--config.http-urlis configured, Alertmanager retrieves the configuration using an HTTPGETrequest and passes the retrieved configuration through the existing configuration parsing, validation, and application flow.The HTTP configuration is fetched:
POST /-/reloadendpoint is invoked.Motivation
Alertmanager currently expects its configuration to be available on the local filesystem.
In environments where configuration is centrally managed or exposed through a configuration service, this requires an additional mechanism such as a sidecar, shared volume, or synchronization process to materialize the remote configuration as a local file.
Supporting an HTTP configuration source allows Alertmanager to consume configuration directly from a remote configuration service while remaining independent of the underlying storage mechanism.
The intent of this change is therefore not to introduce storage-specific configuration support, but to provide a small, generic HTTP-based configuration source.
Behavior
Existing file-based configuration continues to work unchanged:
HTTP-based configuration can instead be configured using:
When using the HTTP source, Alertmanager performs:
The same configuration source is used when
POST /-/reloadis invoked. For an HTTP configuration source, this results in a fresh HTTP request so that the latest configuration exposed by the endpoint is loaded.If fetching or validating the new configuration fails during reload, the currently active configuration remains in use.
Scope
This change intentionally keeps HTTP configuration loading minimal.
It does not introduce:
These capabilities can be considered independently if needed.
Backward Compatibility
The existing
--config.fileworkflow and configuration reload behavior remain unchanged.This feature is opt-in through
--config.http-urland does not alter the behavior of existing Alertmanager deployments.Testing
Tests have been added for the new HTTP configuration source, including:
POST /-/reload.--config.fileand--config.http-url.HTTP tests use local test HTTP servers and do not depend on external services.
Pull Request Checklist
Please check all the applicable boxes.
benchstatto compare benchmarksWhich user-facing changes does this PR introduce?