ci: publish keyless release attestations (#232)#240
Merged
Conversation
* ci: publish keyless release attestations Signed-off-by: AnouarMohamed <[email protected]> * ci: drop nvcr release attestations Signed-off-by: AnouarMohamed <[email protected]> * ci: narrow release attestation identity Signed-off-by: AnouarMohamed <[email protected]> * ci: gate attestations to release tags Signed-off-by: AnouarMohamed <[email protected]> * docs: show immutable release subjects Signed-off-by: AnouarMohamed <[email protected]> * ci: attest helm chart releases Signed-off-by: AnouarMohamed <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]>
lockwobr
requested review from
ayuskauskas,
mskalka and
rice-riley
as code owners
ayuskauskas
approved these changes
May 19, 2026
Contributor
📝 WalkthroughWalkthroughThis pull request introduces Sigstore keyless signing and attestation infrastructure to the NVIDIA/nodewright CI/CD pipelines. It adds three new reusable GitHub composite actions: Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR introduces substantial new security infrastructure across multiple workflows with varied logic patterns. It includes three new composite actions with distinct validation and orchestration logic, three workflow integrations with conditional signing/verification steps, and comprehensive documentation. The changes affect multiple files and require understanding cosign operations, SBOM generation, attestation formats, and their integration into existing CI pipelines. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/cosign-verify-release/action.yml:
- Around line 51-58: Add a runtime guard that fails fast when the certificate
identity regexp input is empty: check the environment variable
CERTIFICATE_IDENTITY_REGEXP (input name certificate-identity-regexp) and if it
is unset or an empty string echo an error (e.g.
"::error::certificate-identity-regexp is required") and exit 1 before running
any verification commands; place this check near the existing SUBJECT_NAME and
SUBJECT_DIGEST validations so the script validates CERTIFICATE_IDENTITY_REGEXP
alongside SUBJECT_NAME and SUBJECT_DIGEST.
In @.github/workflows/agent-ci.yaml:
- Around line 229-240: The checkout and Docker actions are using mutable tags;
update the steps using actions/checkout, docker/login-action, and
docker/setup-buildx-action to pin each to their full commit SHA and add
persist-credentials: false to the actions/checkout step to disable credential
persistence (since the job has packages: write/attestations: write/id-token:
write); ensure the login and buildx steps reference the exact commit SHAs you
choose for docker/login-action and docker/setup-buildx-action to avoid mutable
tags.
In @.github/workflows/operator-ci.yaml:
- Around line 363-374: Replace floating action tags with pinned commit SHAs for
actions/checkout, docker/login-action, and docker/setup-buildx-action and
disable checkout credential persistence: update the actions referenced as uses:
actions/checkout@v6, uses: docker/login-action@v4, and uses:
docker/setup-buildx-action@v4 to use the corresponding full commit SHAs instead
of version tags, and add persist-credentials: false under the actions/checkout
step to prevent automatic credential export; ensure the docker/login-action and
setup-buildx-action entries remain otherwise unchanged but use their commit SHAs
to eliminate floating-tag supply-chain risk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7a68fd5d-1356-4d1e-a6eb-354bd9b3a66c
📒 Files selected for processing (7)
.github/actions/cosign-sign-sbom/action.yml.github/actions/cosign-verify-release/action.yml.github/actions/resolve-oci-digest/action.yml.github/workflows/agent-ci.yaml.github/workflows/operator-ci.yaml.github/workflows/release.ymldocs/release-process.md
Comment on lines
+51
to
+58
| if [ -z "${SUBJECT_NAME}" ]; then | ||
| echo "::error::subject-name is required" | ||
| exit 1 | ||
| fi | ||
| if ! [[ "${SUBJECT_DIGEST}" =~ ^sha256:[a-f0-9]{64}$ ]]; then | ||
| echo "::error::subject-digest must be in sha256:<64 hex> form" | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
Add a non-empty guard for certificate-identity-regexp.
Line 51 validates subject-name, but there is no equivalent runtime check for CERTIFICATE_IDENTITY_REGEXP. This trust-boundary input can still arrive empty from caller expressions, so fail fast before verification commands.
Suggested patch
if [ -z "${SUBJECT_NAME}" ]; then
echo "::error::subject-name is required"
exit 1
fi
+ if [ -z "${CERTIFICATE_IDENTITY_REGEXP}" ]; then
+ echo "::error::certificate-identity-regexp is required"
+ exit 1
+ fi
if ! [[ "${SUBJECT_DIGEST}" =~ ^sha256:[a-f0-9]{64}$ ]]; then
echo "::error::subject-digest must be in sha256:<64 hex> form"
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -z "${SUBJECT_NAME}" ]; then | |
| echo "::error::subject-name is required" | |
| exit 1 | |
| fi | |
| if ! [[ "${SUBJECT_DIGEST}" =~ ^sha256:[a-f0-9]{64}$ ]]; then | |
| echo "::error::subject-digest must be in sha256:<64 hex> form" | |
| exit 1 | |
| fi | |
| if [ -z "${SUBJECT_NAME}" ]; then | |
| echo "::error::subject-name is required" | |
| exit 1 | |
| fi | |
| if [ -z "${CERTIFICATE_IDENTITY_REGEXP}" ]; then | |
| echo "::error::certificate-identity-regexp is required" | |
| exit 1 | |
| fi | |
| if ! [[ "${SUBJECT_DIGEST}" =~ ^sha256:[a-f0-9]{64}$ ]]; then | |
| echo "::error::subject-digest must be in sha256:<64 hex> form" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/cosign-verify-release/action.yml around lines 51 - 58, Add a
runtime guard that fails fast when the certificate identity regexp input is
empty: check the environment variable CERTIFICATE_IDENTITY_REGEXP (input name
certificate-identity-regexp) and if it is unset or an empty string echo an error
(e.g. "::error::certificate-identity-regexp is required") and exit 1 before
running any verification commands; place this check near the existing
SUBJECT_NAME and SUBJECT_DIGEST validations so the script validates
CERTIFICATE_IDENTITY_REGEXP alongside SUBJECT_NAME and SUBJECT_DIGEST.
Comment on lines
+229
to
+240
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Log in to the Container registry | ||
| uses: docker/login-action@v4 | ||
| with: | ||
| registry: ${{ env.REGISTRY }} | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v4 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the workflow file exists and read the relevant lines
if [ -f ".github/workflows/agent-ci.yaml" ]; then
echo "=== File exists, checking size ==="
wc -l .github/workflows/agent-ci.yaml
echo ""
echo "=== Lines 229-240 ==="
sed -n '229,240p' .github/workflows/agent-ci.yaml
else
echo "File not found, searching for workflow files"
find .github -name "*.yaml" -o -name "*.yml" | head -20
fiRepository: NVIDIA/nodewright
Length of output: 530
🏁 Script executed:
# Also check the broader context around those lines to understand the job permissions
if [ -f ".github/workflows/agent-ci.yaml" ]; then
echo "=== Context around lines 229-240 (200-250) ==="
sed -n '200,250p' .github/workflows/agent-ci.yaml
fiRepository: NVIDIA/nodewright
Length of output: 2173
🏁 Script executed:
# Search for all action uses in the workflow to understand the full scope
if [ -f ".github/workflows/agent-ci.yaml" ]; then
echo "=== All 'uses' statements in the workflow ==="
grep -n "uses:" .github/workflows/agent-ci.yaml
fiRepository: NVIDIA/nodewright
Length of output: 1030
🏁 Script executed:
# Check the job structure to verify permissions mentioned in the review
if [ -f ".github/workflows/agent-ci.yaml" ]; then
echo "=== Looking for permissions section and job structure ==="
grep -B 5 -A 5 "packages: write\|attestations: write\|id-token: write" .github/workflows/agent-ci.yaml | head -30
fiRepository: NVIDIA/nodewright
Length of output: 841
Pin these new privileged actions and disable checkout credential persistence.
This job has packages: write, attestations: write, and id-token: write, but the actions in lines 229-240 use mutable version tags and checkout lacks credential isolation. Pin each action to its full commit SHA and add persist-credentials: false to the checkout step.
Hardening pattern
- name: Checkout repository
- uses: actions/checkout@v6
+ uses: actions/checkout@<full-commit-sha>
+ with:
+ persist-credentials: false
- name: Log in to the Container registry
- uses: docker/login-action@v4
+ uses: docker/login-action@<full-commit-sha>
- name: Set up Docker Buildx
- uses: docker/setup-buildx-action@v4
+ uses: docker/setup-buildx-action@<full-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 229-230: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 230-230: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 233-233: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 240-240: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/agent-ci.yaml around lines 229 - 240, The checkout and
Docker actions are using mutable tags; update the steps using actions/checkout,
docker/login-action, and docker/setup-buildx-action to pin each to their full
commit SHA and add persist-credentials: false to the actions/checkout step to
disable credential persistence (since the job has packages: write/attestations:
write/id-token: write); ensure the login and buildx steps reference the exact
commit SHAs you choose for docker/login-action and docker/setup-buildx-action to
avoid mutable tags.
Comment on lines
+363
to
+374
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Log in to the Container registry | ||
| uses: docker/login-action@v4 | ||
| with: | ||
| registry: ${{ env.REGISTRY }} | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v4 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant lines
head -374 .github/workflows/operator-ci.yaml | tail -50Repository: NVIDIA/nodewright
Length of output: 1904
🏁 Script executed:
# Also check the broader context to find the job definition and permissions
cat -n .github/workflows/operator-ci.yaml | grep -A 50 "363:"Repository: NVIDIA/nodewright
Length of output: 43
🏁 Script executed:
# Search for the permissions section of the relevant job
rg -n "packages: write|attestations: write|id-token: write" .github/workflows/operator-ci.yamlRepository: NVIDIA/nodewright
Length of output: 225
🏁 Script executed:
# Check how actions are referenced throughout the workflow to see if there's a pattern
rg -n "uses:" .github/workflows/operator-ci.yaml | head -20Repository: NVIDIA/nodewright
Length of output: 924
🏁 Script executed:
# Also check if this is a known security concern by searching for recent pinning efforts
git log --oneline --all -- .github/workflows/operator-ci.yaml | head -10Repository: NVIDIA/nodewright
Length of output: 606
Pin these new privileged actions and disable checkout credential persistence.
This job is running with packages: write, attestations: write, and id-token: write permissions. Using floating version tags (@v6, @v4) for external actions increases supply-chain risk—if an upstream action is compromised, malicious code could use these sensitive permissions. Pin each action to a full commit SHA and add persist-credentials: false to actions/checkout to reduce the attack surface.
Hardening pattern
- name: Checkout repository
- uses: actions/checkout@v6
+ uses: actions/checkout@<full-commit-sha>
+ with:
+ persist-credentials: false
- name: Log in to the Container registry
- uses: docker/login-action@v4
+ uses: docker/login-action@<full-commit-sha>
- name: Set up Docker Buildx
- uses: docker/setup-buildx-action@v4
+ uses: docker/setup-buildx-action@<full-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 363-364: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 364-364: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 367-367: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 374-374: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/operator-ci.yaml around lines 363 - 374, Replace floating
action tags with pinned commit SHAs for actions/checkout, docker/login-action,
and docker/setup-buildx-action and disable checkout credential persistence:
update the actions referenced as uses: actions/checkout@v6, uses:
docker/login-action@v4, and uses: docker/setup-buildx-action@v4 to use the
corresponding full commit SHAs instead of version tags, and add
persist-credentials: false under the actions/checkout step to prevent automatic
credential export; ensure the docker/login-action and setup-buildx-action
entries remain otherwise unchanged but use their commit SHAs to eliminate
floating-tag supply-chain risk.
Coverage Report for CI Build 26115378370Warning No base build found for commit Coverage: 81.862%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
lockwobr
added a commit
that referenced
this pull request
May 22, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
added a commit
that referenced
this pull request
Jun 9, 2026
* feat(agent): port enums and steps for agent go rewrite Signed-off-by: Riley Rice <[email protected]> * chore: Cherry pick (#244) * ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]> --------- Signed-off-by: Riley Rice <[email protected]> Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Brian Lockwood <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
cherry picking 2af03eb in main.
Checklist
git commit -s) per the DCO.