Repository navigation
blueprints: re-apply when a file referenced by !File changes - #26402
Conversation
The blueprint hash only covered the blueprint file itself, so a change to a file referenced through a `!File` tag - such as a Kubernetes Secret mounted into the container and then rotated - was never detected and the blueprint was not re-applied. Hash the referenced files' paths and contents alongside the blueprint's own content, in both places the hash is computed, so discovery and apply stay in agreement. Closes goauthentik#26289
`File.__init__` assigns `self.path` from `loader.construct_object()` when the tag is built from a sequence node, so `!File [!Env P, default]` carries a tag object rather than a string. `Path()` raises `TypeError` on it, which the surrounding `except OSError` does not catch, and `blueprints_find` has no per-file guard - so one such blueprint would abort discovery for all of them. Skip those tags: their own content is already part of the blueprint content hashed above, and resolving them needs an entry and a blueprint that the hashing path does not have.
…overy The previous commit's guard reads `tag.path` to decide whether to skip the tag, but `File.__init__` assigns `self.path` only for scalar and sequence nodes; `path` is otherwise just a class annotation, which creates no attribute. A `!File` built from a mapping node takes neither branch, so the guard raises `AttributeError` before it can guard anything. Read the attribute with `getattr()` so the check cannot be what fails. `Path(...).read_bytes()` also raises `ValueError`, not `OSError`, for a path no syscall can accept, such as one containing a null byte, so widen the except to cover it. Both inputs parse cleanly and hash without complaint before this series, and neither `blueprints_find` nor `apply_blueprint` catches either exception, so a single such blueprint would stop every other blueprint from being discovered. Drop the separate digest of the path while here: the path of a tag that is hashed is a substring of the blueprint content already hashed above, so it cannot contribute anything the content digest does not.
YAML anchors and aliases let a node be its own descendant, and PyYAML builds
collections in two steps precisely so such documents load. `iter_file_tags`
recurses with no bound, so hashing a blueprint like
context:
secret: &anchor
- *anchor
raises `RecursionError`. `blueprints_find` has no per-file guard around
`blueprint_hash` and `apply_blueprint` does not catch `RecursionError`, so one
such blueprint stops every other blueprint from being discovered - and the
document hashes without complaint before this series, so this would be a
regression rather than a pre-existing limitation.
Skip a node already on the path from the root. A node shared by two disjoint
routes - an ordinary blueprint reusing one anchor - is not its own ancestor, so
it is still walked from each route and its digest is unchanged; suppressing
those instead would silently move the hash of every aliased blueprint in the
wild and re-apply each one once.
Spell the widened `except` as `black` at the version pinned in pyproject.toml
formats it.
A node reached by two routes through one anchor is walked from each of them, so the file it references is folded into the digest twice, exactly as it is when the tag is written out twice. Bounding the walk by the ancestor chain keeps that; suppressing every node seen anywhere would fold it in once and move the hash of every blueprint that reuses an anchor. Compute the expected digest from the blueprint's own content rather than comparing two runs, so the test pins the count and not just stability.
The stability cases read as annotations of a patch rather than as tests of the hash, which is not how the rest of the file reads.
…d cycle fold count
The tests covering the `!File` hashing added cases one per input, which made the test diff several times the size of the change it covers. Group the ones that differ only in the reference they hash into subtests, keeping every input, and drop the docstrings from the test-local helpers.
…able `!File` path
✅ Deploy Preview for authentik-integrations ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26402 +/- ##
========================================
Coverage 92.33% 92.34%
========================================
Files 1175 1178 +3
Lines 77789 78262 +473
Branches 4129 4129
========================================
+ Hits 71827 72269 +442
- Misses 5896 5927 +31
Partials 66 66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
blueprint_hash skipped an unreadable reference entirely, so [missing, X] and [X, missing] both hashed as the content plus the digest of X: a file disappearing while another appeared with the same contents left the hash unchanged, and discovery would not re-apply the blueprint. An unreadable reference now contributes a one-byte marker, where a readable one contributes its 64-byte digest, so its position counts. Blueprints whose references all resolve hash as before.
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@askalf Could you move this new hashing logic to its own file, blueprints/v1/hash.py? I tried to make this change but wasn't able to push to your fork |
|
Moved to |
Summary
.yamlfile, not the files it references through!Filetags. Rotating a Kubernetes Secret mounted into the container therefore left the hash identical, socheck_blueprint_v1_filenever dispatchedapply_blueprintand authentik kept serving the old value.blueprint_hash(content)inauthentik/blueprints/v1/tasks.py: the blueprint's own content, then the contents of every file it references through a!Filetag, each folded in as a fixed-length sha512 digest.iter_file_tags(value, ancestors), which walks the loaded blueprint and finds!Filetags including ones passed as arguments to another tag (!Format ["client-%s", !File /x]) — a flat scan of the top level would miss those. The walk skips a node already on the path from the root, because YAML anchors let a node contain itself; a node merely shared by two routes is not its own ancestor and is still walked from each, so no existing blueprint's digest moves.!Filewhose path is not a usable string, reading the attribute withgetattr(tag, "path", None).File.__init__assignsself.pathonly for scalar and sequence nodes, so a!Filebuilt from a mapping node has nopathattribute at all, and a sequence-node path built from a nested tag (!File [!Env P, default]) is a tag object. Resolving those needs an entry and a blueprint the hashing path does not have, and their own content is already in the digest.blueprints_find, which produces the value compared against the DB, andapply_blueprint, which stores it). They must agree or every discovery run would re-apply forever; that symmetry is why this is one shared helper.!Filetag hashes exactly as before when it has LF line endings, so the two literal sha512 digests pinned by the pre-existing tests do not move.test_valid_crlf: discovery used to hash the raw file bytes whileapply_blueprinthashed the text-mode read thatretrieve_file()returns, so a blueprint saved with CRLF line endings never matched its ownlast_applied_hashand was re-applied on every discovery run. Both sides now hash the same text, so they agree.Head of this branch:
e518be0aab1737e609e15735e43d6640bfa98ad1. The production fileauthentik/blueprints/v1/tasks.pyis byte-identical from1c67effb1through3ad347d1b,e0bdb02a4,c4c91590aand2453510bc; the tenth commita3c2c6e81changes only its comments and docstrings (the module's AST with docstrings blanked is identical before and after, and every statement is unchanged), the eleventh18479bddcis a test, and the twelfthe518be0aa(a fifth verification round) adds one expected-value assertion to that test. Transcripts below that name3ad347d1b,e0bdb02a4orc4c91590awere run against that identical source and are labelled with the sha they were captured at. The probe transcripts in## Reproand## Test evidencewere re-run at this head. The issue's own scenario, both arms, run at this head:On base the three discriminating cases print the same digest twice — that identity is the bug.
Commits three to five on this branch answer two rounds of adversarial verification, the seventh (
e0bdb02a4) adds the three tests a third round asked for, the eighth (c4c91590a) folds the accumulated tests into the surrounding file's idiom at a gating review's request, the ninth (2453510bc) adds the two discovery inputs a fourth verification round found unpinned, the tenth and eleventh (a3c2c6e81,18479bddc) answer a second-opinion review: comments trimmed to the runtime contract, and a CRLF discovery/apply test, and the twelfth (e518be0aa) pins that test's discovery hash to an expected digest so it no longer asserts agreement alone (round one, round two); see## Reworkat the end of this body for exactly what changed in each and why one line of the fix was deleted rather than tested.Decisions
!Filecontents in the change-detection hash while preserving the content-only digest for blueprints without file tags.All targeted verification described above was run; the upstream full CI matrix was not run locally because it requires the project's full containerized environment.
AI assistance: this bug was found and the fix and tests were drafted with AI tooling in my workflow; the tests and checks above were executed as pasted. I'm responsible for the change and will handle review feedback.
closes #26289