feat: Support Accept: application/json on /config - #24557
darrenjaneczek wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Address the content-negotiation issues and update the API documentation before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds JSON content negotiation for /config while preserving YAML by default.
Changes:
- Supports JSON for base and
q-scoped responses. - Preserves YAML field names through normalization.
- Adds negotiation tests.
File summaries
| File | Summary |
|---|---|
pkg/loki/config_handler.go |
Adds YAML/JSON response selection. Findings: set Vary: Accept (moderate, 1 vote); properly parse media ranges and q values (moderate, 3 votes); update API documentation (nit, 2 votes). |
pkg/loki/config_handler_test.go |
Adds tests for JSON responses and Accept headers. |
Review details
Suppressed comments (1)
pkg/loki/config_handler.go:198
- Because the representation now varies on the request's
Acceptheader, this handler needs to sendVary: Accept. Without it, an intermediary may cache a YAML response and serve it to a JSON client (or the reverse), since the header is not part of the cache key; setVarybefore both branches.
if !strings.Contains(r.Header.Get("Accept"), "application/json") {
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
💻 Deploy preview available (feat: Support Accept: application/json on /config): |
Adds Accept-header content negotiation to /config: YAML by default, JSON if the client asks for it. Same pattern as dskit's ring status handler. Applies to both the full config response and the q-scoped response.
strings.Contains treated Accept: application/json;q=0 (explicitly "not acceptable" per RFC 7231 §5.3.2) as a request for JSON, and could false-positive on lookalike types like application/json-seq. acceptsJSON now parses each media range and checks the type and q value properly.
6ada769 to
5b25d06
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Address the Vary header, invalid q-value handling, regression coverage, and contradictory documentation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
pkg/loki/config_handler.go:200
- Because this handler now selects the representation from
Accept, it must emitVary: Accept. Without that response metadata, an intermediary can cache the first YAML or JSON response and serve it to requests with the otherAcceptvalue. Add the Vary header before this branch while preserving any existing Vary values.
func writeConfigResponse(w http.ResponseWriter, r *http.Request, v any) {
pkg/loki/config_handler.go:230
- When
qis present but malformed or outside the RFC q-value range, this code silently keeps the defaultq = 1and selects JSON (for example,Accept: application/json;q=bogusorq=2). A media range with an invalid q-value should not be treated as an explicit positive preference; reject/skip the range on parse or range errors and add a regression test.
if raw, ok := params["q"]; ok {
if parsed, err := strconv.ParseFloat(raw, 64); err == nil {
q = parsed
}
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
The q-parameter paragraph still said "The response is YAML" unconditionally, contradicting the negotiation note added just above it.
There was a problem hiding this comment.
🔵 Needs a closer look
Address invalid q-values, repeated Accept headers, and missing Vary: Accept handling.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
pkg/loki/config_handler.go:202
Header.Getreturns only the first value, but HTTP permits repeatedAcceptfields. A request withAccept: text/htmlfollowed byAccept: application/jsontherefore falls back to YAML even though JSON was requested; joinHeader.Values("Accept")before parsing rather than dropping later values.
pkg/loki/config_handler.go:231- A malformed or out-of-range q value is treated as acceptable here: parse errors leave q at 1, while values such as q=2 remain positive. Thus
Accept: application/json;q=bogusorq=2selects JSON even though those are not valid qvalues; reject invalid values instead of defaulting them to 1.
pkg/loki/config_handler.go:202
- This response now varies by the
Acceptrequest header, but it never emitsVary: Accept. A shared HTTP cache can consequently reuse a JSON response for a YAML request (or the reverse); setVarybefore selecting the representation.
func writeConfigResponse(w http.ResponseWriter, r *http.Request, v any) {
// Only the first Accept header occurrence is honored; subsequent Accept headers are ignored.
if !acceptsJSON(r.Header.Get("Accept")) {
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Without Vary: Accept, an intermediary cache could serve a cached YAML response to a JSON client or vice versa, since Accept wasn't part of the cache key. acceptsJSON also silently treated a malformed q (e.g. q=bogus) or one outside the valid 0-1 range (e.g. q=2) as the default q=1, i.e. accepted. Such media ranges are now skipped instead.
There was a problem hiding this comment.
🔵 Needs a closer look
Resolve the two outstanding Accept-header parsing issues before approval.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/loki/config_handler.go:206
Header.Getreturns only the first field value, butAcceptis a list-valued header and may legally be sent on multiple field lines. For example, separateAccept: text/plainandAccept: application/jsonlines currently fall back to YAML even though JSON is explicitly accepted; combine/iterate overr.Header.Values("Accept")before parsing.
pkg/loki/config_handler.go:227
- Splitting the header with
strings.Splitdoes not correctly parse all valid Accept media ranges: commas inside a quoted parameter value are not separators. For example,application/json;profile="a,b"is split into invalid fragments and incorrectly falls back to YAML. Use quote-aware list parsing (or a dedicated Accept parser) before callingmime.ParseMediaType.
for _, part := range strings.Split(accept, ",") {
mediaType, params, err := mime.ParseMediaType(part)
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
What this PR does / why we need it:
/configreturns YAML by default; this addsAccept: application/jsoncontent negotiation on top, for both the base response and theq-scoped response (#24230). A caller that sendsAccept: application/jsongets the same data as JSON instead of YAML; everything else is unchanged.Which issue(s) this PR fixes:
Special notes for your reviewer:
vendor/github.com/grafana/dskit/ring/ring_http.go).yamltags, notjsonones).Acceptdoesn't ask for JSON — this is purely additive.Checklist
CONTRIBUTING.mdguidedocs/sources/reference/loki-http-api.mdif desired