feat: add X-Permit-Consistent-Update header on facts proxy requests#306
feat: add X-Permit-Consistent-Update header on facts proxy requests#306
Conversation
🔍 Vulnerabilities of
|
| digest | sha256:6434e121c5fcf51c63f670e47555127f5871518f18a378ae720cbf9c4e08d84d |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 214 MB |
| packages | 252 |
📦 Base Image python:3.10-alpine3.22
| also known as |
|
| digest | sha256:c8f94b3bb77e6ea9015ccd091b7f8aec1b1fcbca95159675235d9a93788797cd |
| vulnerabilities |  |
Description | ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description |
🔍 Vulnerabilities of
|
| digest | sha256:6434e121c5fcf51c63f670e47555127f5871518f18a378ae720cbf9c4e08d84d |
| vulnerabilities |  |
| platform | linux/amd64 |
| size | 214 MB |
| packages | 252 |
📦 Base Image python:3.10-alpine3.22
| also known as |
|
| digest | sha256:c8f94b3bb77e6ea9015ccd091b7f8aec1b1fcbca95159675235d9a93788797cd |
| vulnerabilities | |
Description | ||||||||||||
Description
|
The backend opal-interface uses this header to skip sending the control-plane delta update back to PDPs, since the PDP already propagates the update via OPAL Server pubsub after a successful facts proxy write. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the X-Permit-Consistent-Update header injection behind an explicit is_consistent_update kwarg so the fallback proxy route (forward_remaining_requests) does not falsely mark generic passthrough requests as consistent updates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
omer9564
force-pushed
the
omer/per-14392-consistent-updates-pdp-header
branch
from
af90b4f to
74bf9ed
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in request header to facts-service proxy calls made via the “wait-for-local-sync” path, allowing the backend to skip emitting a redundant control-plane delta update when the PDP is already publishing an OPAL pubsub update.
Changes:
- Introduce
CONSISTENT_UPDATE_HEADERand anis_consistent_updatekwarg onFactsClient.build_forward_request()/send_forward_request(), addingX-Permit-Consistent-Update: truewhen enabled. - Set
is_consistent_update=Truefor the consistent-update proxy flow (forward_request_then_wait_for_update). - Add unit tests asserting the header is present when requested and absent by default.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
horizon/facts/client.py |
Adds the consistent-update header constant and gating logic in forwarded request construction/sending. |
horizon/facts/router.py |
Enables the consistent-update header for the wait-for-local-sync proxy flow. |
horizon/tests/test_facts_client.py |
Adds tests covering header inclusion/exclusion behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| def test_consistent_update_header_constant(): | ||
| assert CONSISTENT_UPDATE_HEADER == "X-Permit-Consistent-Update" |
There was a problem hiding this comment.
Tautological constant test
This asserts the constant against its own literal value — if the header is renamed, a developer updates constant and test in lockstep, so the test catches nothing. The other two tests in this file use forward_request.headers.get(CONSISTENT_UPDATE_HEADER) which also references the constant, so neither guards against an accidental rename of the outgoing header spelling.
Suggestion: Delete this test, or replace with an assertion that checks the literal header name is present on the built request:
Example:
async def test_build_forward_request_uses_exact_header_spelling():
...
forward_request = await client.build_forward_request(request, "/users", is_consistent_update=True)
assert "X-Permit-Consistent-Update" in forward_request.headersThere was a problem hiding this comment.
Replaced with a literal-header-spelling assertion in test_build_forward_request_adds_header_when_consistent_update — now checks "X-Permit-Consistent-Update" in forward_request.headers and value == "true". Tautological constant test removed. Fixed in d02054a.
| ) -> Response: | ||
| _update_id = update_id or uuid4() | ||
| response = await client.send_forward_request(request, path, query_params=query_params) | ||
| response = await client.send_forward_request(request, path, query_params=query_params, is_consistent_update=True) |
There was a problem hiding this comment.
Router integration is untested
The only place is_consistent_update=True is wired in is here, and nothing tests that the wait-for-update path passes True while the fallback at forward_remaining_requests (router.py:391-392) does not. A future refactor could drop the kwarg silently and reintroduce the exact duplicate-update bug this PR prevents — all unit tests would still pass.
Suggestion: Add a router-level test that mocks FactsClient.send_forward_request, exercises forward_request_then_wait_for_update, and asserts the call received is_consistent_update=True. Add the mirror test for forward_remaining_requests asserting the header is absent on the built request.
There was a problem hiding this comment.
Added router-level coverage in horizon/tests/test_facts_router.py: test_forward_request_then_wait_for_update_sets_consistent_update_flag pins is_consistent_update=True on the wait-for-update path, and test_forward_remaining_requests_does_not_set_consistent_update_header asserts the header is absent on the fallback proxy's built request. Fixed in d02054a.
| forward_request = await self.build_forward_request(request, path, query_params=query_params) | ||
| forward_request = await self.build_forward_request( | ||
| request, path, query_params=query_params, is_consistent_update=is_consistent_update | ||
| ) |
There was a problem hiding this comment.
send_forward_request kwarg passthrough is untested
The kwarg plumbing from send_forward_request into build_forward_request has no direct test. The PR's purpose is propagating this flag across that boundary, so the boundary itself should be covered.
Suggestion: Add a test that patches FactsClient.send and asserts the built request carries X-Permit-Consistent-Update: true when send_forward_request(..., is_consistent_update=True) is called.
There was a problem hiding this comment.
Added test_send_forward_request_propagates_consistent_update_kwarg which patches FactsClient.send, calls send_forward_request(..., is_consistent_update=True), and asserts the built HttpxRequest carries X-Permit-Consistent-Update: true. Fixed in d02054a.
| key: value for key, value in request.headers.items() if key.lower() in {"authorization", "content-type"} | ||
| } | ||
| if is_consistent_update: | ||
| forward_headers[CONSISTENT_UPDATE_HEADER] = "true" |
There was a problem hiding this comment.
Header value is a magic string literal
"true" is a lowercase string literal the backend likely compares directly. If a future well-intentioned edit changes it to "True" or "1", and the backend does a case-sensitive string compare (e.g. value == "true"), consistent-update routing silently breaks with no test catching it.
Suggestion: Extract the value next to the header name so both are coupled, and document that the backend expects this exact literal:
Example:
CONSISTENT_UPDATE_HEADER = "X-Permit-Consistent-Update"
CONSISTENT_UPDATE_HEADER_VALUE = "true" # backend compares case-sensitively
...
forward_headers[CONSISTENT_UPDATE_HEADER] = CONSISTENT_UPDATE_HEADER_VALUEThere was a problem hiding this comment.
Extracted CONSISTENT_UPDATE_HEADER_VALUE = "true" alongside the header-name constant in horizon/facts/client.py, with a comment noting the backend expects this exact literal (case-sensitive compare). Fixed in d02054a.
Address review feedback on PR #306: - Extract CONSISTENT_UPDATE_HEADER_VALUE constant for the "true" literal - Replace tautological constant test with literal-header-spelling assertion - Add send_forward_request kwarg passthrough test - Add router-level tests pinning is_consistent_update on wait-for-update path and asserting the fallback proxy does not set the header Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
X-Permit-Consistent-Update: trueheader on facts requests that the PDP proxies throughforward_request_then_wait_for_update(the wait-for-local-sync flow)forward_remaining_requests) does NOT set the header, so generic passthrough requests continue to rely on the standard control-plane delta pathDetails
The header is gated behind a new
is_consistent_update: bool = Falsekwarg onFactsClient.build_forward_requestandFactsClient.send_forward_request. Onlyforward_request_then_wait_for_update(called by the explicit consistent-update routes: users, tenants, role_assignments, resource_instances, relationship_tuples) passesTrue.Paired with backend changes in permit-backend (branch:
omer/per-14392-consistent-updates-duplicated-updates) which:is_consistent_update: Trueinto the DB session's AMQP headersTest plan
FactsClient.build_forward_request:is_consistent_update=True🤖 Generated with Claude Code