Skip to content

Fix conditional GET across the API - #5662

Open
warwickschroeder wants to merge 1 commit into
masterfrom
warwick/etag-fix
Open

Fix conditional GET across the API#5662
warwickschroeder wants to merge 1 commit into
masterfrom
warwick/etag-fix

Conversation

@warwickschroeder

Copy link
Copy Markdown
Contributor

ServicePulse polls data from ServiceControl as often as 5s. ServiceControl was built to answer "nothing has changed", but the ETag it sends was a bare token and RFC 9110 requires a quoted string. NotModifiedStatusHttpHandler compares typed header values, an unquoted token fails to parse as EntityTagHeaderValue and yields null, so the comparison never matched and every conditional request got a full payload.

What changed

  • WithEtag quotes the validator, in ServiceControl and in ServiceControl.Audit, which has its own copy of the same code and the same defect. All 37 ETag call sites funnel through it.
  • WithEtag emits no header when there is nothing to validate. An empty validator quoted becomes ETag: "", which is well formed and therefore matches itself, so quoting alone would have answered 304 to a client holding an unrelated payload.
  • Two body endpoints now use WithEtag instead of assigning Response.Headers.ETag directly. Bodies are the largest payloads on the API and immutable once written.
  • ScatterGatherApi reads a remote instance's ETag from the raw header. The typed accessor returns null for an unquoted value, so every remote validator was being discarded.
  • The in-memory audit persister supplies a stable body validator. It returned string.Empty for a body in metadata and a fresh Guid.NewGuid() per fetch for one in an attachment. Both can never match. It now uses the body id.

Impact assessment

  • Reviewed against ServiceControl iteselt, ServicePulse, and ServiceInsight. Nothing breaks.
  • Mixed-version clusters are safe. Instances read each other's validators raw and accept both shapes.

Tests

  • ConditionalGetTests, 7 tests in ServiceControl.UnitTests and 7 in ServiceControl.Audit.UnitTests: the 304 round trip, the mismatch guard, both validator shapes, and the empty case.
  • When_a_request_is_repeated_with_its_etag, 3 cases: /api/customchecks, GET /api/redirects and HEAD /api/redirect end to end through Kestrel, including that Total-Count survives the 304. The HEAD verb is covered nowhere else.
  • When_a_message_body_is_requested_twice: the audit body endpoint

@johnsimons johnsimons left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
But I noticed the monitoring instance does not use etags 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants