Osquery config etags - #50288
Osquery config etags#50288
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Adds a Redis-backed ETag store and invalidation hooks to “short circuit” /api/osquery/config requests: when a host presents a matching If-None-Match, Fleet can return 304 Not Modified directly from Redis without building the config (and without DB reads), while preserving correctness via generation invalidation + a write fence.
Changes:
- Introduces
fleet.ConfigETagStore+ Redis implementation with generation counter + write-fence semantics to prevent stale-write poisoning. - Updates the osquery config endpoint to be ETag-aware (
GetClientConfigWithETag) and to render pre-marshaled JSON bodies (with explicitETag+304handling). - Adds datastore decorator hooks to invalidate ETags on config-affecting writes, plus expanded tests (unit + integration) and osquery-perf support for conditional config requests.
Reviewed changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/service.go | Adds injectable ConfigETagStore field + setter on Service. |
| server/service/redis_config_etag/redis_config_etag.go | Implements Redis-backed ETag store with generation invalidation + write fence + gate flag caching. |
| server/service/redis_config_etag/redis_config_etag_test.go | Unit tests for Redis ETag store behavior (gen invalidation, fence, malformed records, gate flag). |
| server/service/osquery.go | Implements GetClientConfigWithETag, adds ETag parsing/matching, and switches response path to hijacked body rendering. |
| server/service/osquery_test.go | Adds tests for deterministic config marshaling, ETag formatting/matching, and hijacked rendering behavior. |
| server/service/osquery_etag_test.go | Adds service-level tests for Redis short-circuit hit/miss behavior and fail-open semantics. |
| server/service/mock/service_osquery.go | Extends TLS mock service with GetClientConfigWithETag. |
| server/service/integration_core_test.go | Adds integration coverage for config ETag/304 behavior. |
| server/mock/service/service_mock.go | Extends main service mock with GetClientConfigWithETag and SetConfigETagStore. |
| server/mock/datastore_mock.go | Adds mock support for HasLabelScopedScheduledQueries. |
| server/fleet/service.go | Extends OsqueryService interface with GetClientConfigWithETag; adds ClientConfigResult and ConfigETagStore interfaces. |
| server/fleet/datastore.go | Adds datastore API HasLabelScopedScheduledQueries for deployment-wide short-circuit gating. |
| server/datastore/mysql/queries.go | Implements HasLabelScopedScheduledQueries with an EXISTS query. |
| server/datastore/mysql/queries_test.go | Adds unit tests for HasLabelScopedScheduledQueries. |
| server/datastore/etag_invalidate/etag_invalidate.go | Adds datastore decorator to invalidate config ETags + reset gate flag on relevant writes. |
| server/datastore/etag_invalidate/etag_invalidate_test.go | Tests that decorator fires on success, not on failure, and swallows Redis errors. |
| server/config/config.go | Adds osquery.redis_config_etags config option (default true). |
| cmd/osquery-perf/README.md | Documents osquery-perf support for config ETag/304 lifecycle simulation. |
| cmd/osquery-perf/osquery_perf/stats.go | Adds stats counters + logging for config 200/304, conditional requests, savings, and drift. |
| cmd/osquery-perf/agent.go | Adds optional If-None-Match behavior + 304 handling in osquery-perf agent config loop. |
| cmd/osquery-perf/agent_config_test.go | Adds unit tests for osquery-perf ETag/304 behavior and stats concurrency. |
| cmd/fleet/serve.go | Wires store injection behind osquery.redis_config_etags and logs enable/disable state. |
| cmd/fleet/redis.go | Extends Redis init to create ConfigETagStore and wrap datastore with invalidation decorator. |
| .gitignore | Ignores osquery-perf binary output at repo root. |
| changes/50157-osquery-config-etags | (Excluded from diff) Changes entry for release notes. |
| docs/Configuration/fleet-server-configuration.md | (Excluded from diff) Documents new config option(s). |
Files excluded by content exclusion policy (2)
- changes/50157-osquery-config-etags
- docs/Configuration/fleet-server-configuration.md
Suppressed comments (4)
server/service/integration_core_test.go:10012
- These requests are passing
body1(the config response body) as the POST payload to/api/osquery/config. The endpoint expects a JSON request containing the node key, so this will fail authentication/decoding and won't actually test ETag behavior. Use the node-key request body (e.g.reqBodyfrom above) for these requests.
// 2. Second request with matching ETag returns 304
resp2 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusNotModified, map[string]string{
"If-None-Match": expectedETag,
})
t.Cleanup(func() { resp2.Body.Close() })
require.Equal(t, http.StatusNotModified, resp2.StatusCode)
body2, err := io.ReadAll(resp2.Body)
require.NoError(t, err)
require.Empty(t, body2, "304 response must have no body")
require.Equal(t, expectedETag, resp2.Header.Get("ETag"))
// 3. Request with mismatched ETag returns 200
resp3 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusOK, map[string]string{
"If-None-Match": `"wrong-etag"`,
})
t.Cleanup(func() { resp3.Body.Close() })
require.Equal(t, http.StatusOK, resp3.StatusCode)
body3, err := io.ReadAll(resp3.Body)
require.NoError(t, err)
require.Equal(t, body1, body3, "body should be unchanged")
server/service/integration_core_test.go:10042
- Same issue here:
body1is the previous config response body, not a valid request payload for/api/osquery/config. Use the node-key request body (e.g.reqBody) so these cases actually exercise If-None-Match parsing.
// 5. If-None-Match: * returns 304
resp5 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusNotModified, map[string]string{
"If-None-Match": "*",
})
t.Cleanup(func() { resp5.Body.Close() })
require.Equal(t, http.StatusNotModified, resp5.StatusCode)
// 6. Weak tag does not match, returns 200
resp6 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusOK, map[string]string{
"If-None-Match": `W/"` + expectedETag[1:len(expectedETag)-1] + `"`,
})
t.Cleanup(func() { resp6.Body.Close() })
require.Equal(t, http.StatusOK, resp6.StatusCode)
// 7. Comma-separated list containing the ETag returns 304
resp7 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusNotModified, map[string]string{
"If-None-Match": `"other", ` + expectedETag,
})
t.Cleanup(func() { resp7.Body.Close() })
require.Equal(t, http.StatusNotModified, resp7.StatusCode)
server/service/integration_core_test.go:10049
- The
/api/v1/osquery/configalias request also sendsbody1(a prior response) as the POST body. Use the node-key request body (e.g.reqBody) so this actually exercises the alias route with a valid request payload.
// 8. Test /api/v1/osquery/config alias
resp8 := s.DoRawWithHeaders("POST", "/api/v1/osquery/config", body1, http.StatusNotModified, map[string]string{
"If-None-Match": expectedETag,
})
t.Cleanup(func() { resp8.Body.Close() })
require.Equal(t, http.StatusNotModified, resp8.StatusCode)
server/service/integration_core_test.go:10076
- After the config change, these follow-up requests again pass prior responses (
body1/body9) as the POST body to/api/osquery/config. They should continue to send the node-key request payload (e.g.reqBody) and only vary the If-None-Match header.
// Old ETag no longer matches
resp10 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusOK, map[string]string{
"If-None-Match": expectedETag,
})
t.Cleanup(func() { resp10.Body.Close() })
require.Equal(t, http.StatusOK, resp10.StatusCode)
// New ETag matches
newETag := resp9.Header.Get("ETag")
resp11 := s.DoRawWithHeaders("POST", "/api/osquery/config", body9, http.StatusNotModified, map[string]string{
"If-None-Match": newETag,
})
t.Cleanup(func() { resp11.Body.Close() })
require.Equal(t, http.StatusNotModified, resp11.StatusCode)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hosts := s.createHosts(t) | ||
| nodeKey := *hosts[0].NodeKey | ||
|
|
||
| // Helper to make a raw config request and return the response | ||
| makeConfigReq := func(ifNoneMatch string) *http.Response { | ||
| body, _ := json.Marshal(map[string]string{"node_key": nodeKey}) | ||
| headers := map[string]string{} | ||
| if ifNoneMatch != "" { | ||
| headers["If-None-Match"] = ifNoneMatch | ||
| } | ||
| return s.DoRawWithHeaders("POST", "/api/osquery/config", body, http.StatusOK, headers) | ||
| } |
There was a problem hiding this comment.
Resolved in 10b623c: the test now builds a dedicated configRequestBody ({node_key: ...}) once and every conditional request sends it, instead of reusing the response body.
| if svc != nil && config.Osquery.RedisConfigETags && configETagStore != nil { | ||
| svc.SetConfigETagStore(configETagStore) | ||
| logger.InfoContext(ctx, "osquery config ETag short circuit ENABLED: matching config check-ins are served 304 from Redis without building the config", | ||
| "component", "config-etag", "flag", "osquery.redis_config_etags") | ||
| } else { | ||
| logger.InfoContext(ctx, "osquery config ETag short circuit disabled (osquery.redis_config_etags is off); all config requests take the full-build path", | ||
| "component", "config-etag", "flag", "osquery.redis_config_etags") | ||
| } |
There was a problem hiding this comment.
Fixed in 4d3f8dd: the disabled-branch log message is now neutral ("short circuit disabled; all config requests take the full-build path") and carries flag_enabled and store_configured structured fields, so the actual reason (flag off vs. store not configured vs. nil service) is in the data instead of asserted incorrectly in the text.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50288 +/- ##
==========================================
- Coverage 69.70% 69.21% -0.49%
==========================================
Files 4059 4038 -21
Lines 264113 265147 +1034
Branches 13997 13698 -299
==========================================
- Hits 184104 183528 -576
- Misses 63924 65467 +1543
- Partials 16085 16152 +67
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 23 out of 26 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (2)
- changes/50157-osquery-config-etags
- docs/Configuration/fleet-server-configuration.md
Suppressed comments (3)
server/service/integration_core_test.go:10027
- Same issue as above: these requests should POST the node_key JSON body (not body1, which is the prior response body). Otherwise the test won't authenticate and won't validate If-None-Match parsing.
// 5. If-None-Match: * returns 304
resp5 := s.DoRawWithHeaders("POST", "/api/osquery/config", configRequestBody, http.StatusNotModified, map[string]string{
"If-None-Match": "*",
})
server/service/integration_core_test.go:10049
- This alias request also needs to send the node_key request body (reqBody), not the previous response body (body1), otherwise it won't authenticate and can't return 304.
// 8. Test /api/v1/osquery/config alias
resp8 := s.DoRawWithHeaders("POST", "/api/v1/osquery/config", configRequestBody, http.StatusNotModified, map[string]string{
"If-None-Match": expectedETag,
})
t.Cleanup(func() { resp8.Body.Close() })
server/service/integration_core_test.go:10067
- These final ETag validation requests are also posting response bodies (body1/body9) instead of the node_key JSON body. The request body should remain the node_key payload; only If-None-Match changes.
// Old ETag no longer matches
resp10 := s.DoRawWithHeaders("POST", "/api/osquery/config", configRequestBody, http.StatusOK, map[string]string{
"If-None-Match": expectedETag,
})
| // 2. Second request with matching ETag returns 304 | ||
| resp2 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusNotModified, map[string]string{ | ||
| "If-None-Match": expectedETag, | ||
| }) | ||
| t.Cleanup(func() { resp2.Body.Close() }) | ||
| require.Equal(t, http.StatusNotModified, resp2.StatusCode) | ||
| body2, err := io.ReadAll(resp2.Body) | ||
| require.NoError(t, err) | ||
| require.Empty(t, body2, "304 response must have no body") | ||
| require.Equal(t, expectedETag, resp2.Header.Get("ETag")) | ||
|
|
||
| // 3. Request with mismatched ETag returns 200 | ||
| resp3 := s.DoRawWithHeaders("POST", "/api/osquery/config", body1, http.StatusOK, map[string]string{ | ||
| "If-None-Match": `"wrong-etag"`, | ||
| }) | ||
| t.Cleanup(func() { resp3.Body.Close() }) |
There was a problem hiding this comment.
Resolved in 10b623c — same fix as the sibling thread: all conditional/304/mismatch requests now send the configRequestBody node-key payload, so they authenticate and genuinely exercise the ETag paths.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 23 out of 26 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (2)
- changes/50157-osquery-config-etags
- docs/Configuration/fleet-server-configuration.md
Suppressed comments (1)
cmd/osquery-perf/agent.go:2254
- When config ETag support is enabled, the code overwrites the stored validator with
response.Header.Get("ETag")even if the header is missing. That can clear a previously valid ETag and stop subsequent conditional requests, skewing osquery-perf’s ETag/304 simulation (and stats) during transient/misconfigured responses. Prefer only updatinga.configETagwhen the response provides a non-empty ETag, while still recording the body size.
// Only commit the ETag and body size after successfully parsing and installing the config.
// Use the server's ETag header as the authoritative validator.
if a.configTLSETag {
a.configETag = response.Header.Get("ETag")
a.lastConfigBodyBytes = int64(len(body))
}
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 23 out of 26 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (2)
- changes/50157-osquery-config-etags
- docs/Configuration/fleet-server-configuration.md
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 27 out of 30 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (2)
- changes/50157-osquery-config-etags
- docs/Configuration/fleet-server-configuration.md
Suppressed comments (1)
server/service/redis_config_etag/redis_config_etag.go:722
parseRecordAndGen’s implementation doesn’t match its docstring: the comment says a missing record yields( "", gen, nil ), but the function currently returns( "", "", nil )whenevervals[0] == nil.
Even though current callers ignore currentGen on a miss, returning the actual generation is safer and keeps the helper consistent with its contract (and future-proof if callers start using currentGen for logging/metrics).
// parseRecordAndGen extracts the record value and the current generation from
// an MGET(recordKey, genKey) reply. A missing record yields ("", gen, nil); a
// missing generation key means no invalidation ever ran and reads as "0".
func parseRecordAndGen(ctx context.Context, vals []any) (record, currentGen string, err error) {
if len(vals) != 2 || vals[0] == nil {
return "", "", nil
}
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 27 out of 30 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (2)
- changes/50157-osquery-config-etags
- docs/Configuration/fleet-server-configuration.md
Suppressed comments (2)
cmd/fleet/serve.go:647
- The else-branch log message says "osquery.redis_config_etags is off", but this branch also runs when the flag is ON and the store/service isn't available (e.g. Redis not configured). This makes the log misleading for operators trying to understand why the short circuit is disabled.
} else {
logger.InfoContext(ctx, "osquery config ETag short circuit disabled (osquery.redis_config_etags is off); all config requests take the full-build path",
"component", "config-etag", "flag", "osquery.redis_config_etags")
}
server/service/redis_config_etag/redis_config_etag.go:808
- parseRecordAndGen's docstring says a missing record should return ("", gen, nil), but the implementation returns ("", "", nil) whenever vals[0] is nil. This contradicts the comment and could cause callers to treat the current generation as "" (and makes the helper misleading for any future use that needs gen even on a miss).
func parseRecordAndGen(ctx context.Context, vals []any) (record, currentGen string, err error) {
if len(vals) != 2 || vals[0] == nil {
return "", "", nil
}
record, err = redigo.String(vals[0], nil)
osquery-perf reported savings that were slightly too high. An unchanged response counted the whole prior config body as avoided and its own body as nothing, so the percentage — the number this tool exists to report — was overstated on every unchanged response. RecordConfigNotModified now takes the sent size too, counts it as sent, and adds only the difference as avoided, floored at zero. The tests now also pin the identity that makes the percentage meaningful: sent + avoided equals what a non-conditional agent would have downloaded. The integration test's "validator should have changed" assertion could be vacuous. It patched logger_tls_period to 10, which is already the default, so whether the PATCH changed anything depended on what an earlier test in this shared suite had left behind. The new value is now derived from the current one, and the test asserts the rendered config actually changed before comparing validators. That same PATCH replaced deployment-wide agent options and never restored them, leaking into every later test that reads agent options or the osquery config. It now restores the previous value in t.Cleanup, via the datastore rather than the API: the stored blob is not necessarily accepted by the write validator, since the suite's defaults carry command-line flags that PATCH rejects inside config.options. Two documentation fixes: - The DefaultFenceTTL derivation cited server/datastore/mysql/cached_mysql, which does not exist. It is server/datastore/cached_mysql, in both the package doc and the constant's comment. The derivation now also names defaultTeamAgentOptionsExpiration, since AgentOptionsForHost feeds the build too and a reader re-checking the arithmetic needs all the inputs. - The gate-state paragraph claimed there was no stampede protection by design, while both loaders implement non-blocking leader election via CAS flags and return fleet.ErrConfigETagGateLoading to losers, pinned by TestGateLoaderLeaderElection. Rewritten to describe what the code does.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Three conflicts, all from main gaining code in the same spots. Two were purely additive; one needed the two changes combined. cmd/osquery-perf/osquery_perf/stats.go and server/service/service.go: both sides appended methods / struct fields at the same location, so both were kept. In stats.go the two blocks also shared a trailing closing brace, so ours needed one added explicitly. server/service/osquery.go, buildClientConfig: the WebSocket transport PR (#51427) added a block that strips distributed_plugin from the agent options when websocket.transport_enabled is set, and it calls getPackConfig on the way past. This branch changed getPackConfig's signature to take the host's packs so the publish guard can see them without a second query. Kept the new websocket behavior with the new signature. The strip runs inside buildClientConfig, before the validator is computed, so the validator covers the post-strip body. It keys off a server-level config flag rather than anything per-host, so it does not make the shared cache mode host-incorrect. Nothing else the WebSocket PR added sits on the config path: its other two changes to this file are in AuthenticateHost, which runs before the endpoint, and a distributed/read wrapper. Verified: build and vet clean; the etag unit tests, etag_invalidate, osquery-perf, and redis_config_etag standalone tests pass; main's own TestGetClientConfigStripsDistributedPluginWhenWebSocketTransportEnabled passes in both directions; and TestIntegrations/TestOsqueryConfigETag passes alongside the TestAppConfig tests.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| - Added conditional request (etag) support to the osquery config endpoint (`/api/osquery/config`): agents that send an `etag` field in the request body now receive the minimal `{"etag":"ok"}` response when their configuration is unchanged, reducing agent config bandwidth. Agents that don't send the field see no change. An `osquery.config_etags` server option (`FLEET_OSQUERY_CONFIG_ETAGS`, default on) is the escape hatch: set it to false to disable the feature entirely and serve every config request exactly as before. | ||
| - Added an `osquery.redis_config_etags` server option (`FLEET_OSQUERY_REDIS_CONFIG_ETAGS`, default on): config check-ins with a matching etag are answered directly from a Redis-backed ETag store, skipping the config build and its database reads entirely. Fleets with uniform configs share one ETag per fleet and platform; fleets with label-scoped reports use isolated per-host ETags invalidated whenever a host's label results are recorded. The short circuit fails open (any Redis error falls back to a full build) and is bypassed automatically for deployments with 2017 packs. | ||
| - Added conditional config request support to `osquery-perf` simulated hosts, including stats for conditional requests and estimated bandwidth saved, for load-testing the above. |
There was a problem hiding this comment.
The feature should be disabled by default (to allow for progressive rollout?)
| // Update membership for the label. The changed host IDs are consumed | ||
| // by the config ETag invalidation decorator wrapping ds; nothing to | ||
| // do with them here. |
There was a problem hiding this comment.
(IMO) These comments make a lot of sense on PRs, but adds a lot of confusion for future reader of this cron job.
| if err != nil { | ||
| return nil, newOsqueryError("internal error: encode config: " + err.Error()) | ||
| } | ||
| etag := clientConfigETag(body) |
There was a problem hiding this comment.
Are we calculating this even if the feature is off or the client hasn't reported a etag?
| // stripping the key — so the re-marshal happens after hashing. | ||
| if clientETag != nil { | ||
| config["etag"] = etag | ||
| bodyWithETag, err := marshalClientConfig(config) |
There was a problem hiding this comment.
If the cache is working properly then this should be no problem AFAICS.
PS: Maybe it can avoid a marshal and inject the etag using string concatenation? (not sure if worth it because of the cache)
Related issue: Resolves #50157
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
New Fleet configuration settings
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes