NO-ISSUE: Add failure isolation to Apply configuration workflow - #169
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe manual apply workflow accepts optional state removal and resource exclusion inputs. It serializes all workflow runs, validates addresses, passes exclusions to OpenTofu, and reports apply failures through GitHub issues. ChangesApply workflow controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant OpenTofu
participant GitHubIssues
GitHubActions->>GitHubActions: Read and validate workflow inputs
GitHubActions->>OpenTofu: Remove selected state addresses
GitHubActions->>OpenTofu: Run apply with -exclude arguments
OpenTofu-->>GitHubActions: Return apply failure
GitHubActions->>GitHubActions: Clear AWS credentials
GitHubActions->>GitHubIssues: Report failed run
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/workflows/apply.yaml:
- Around line 90-102: Update the workflow’s third-party action references for
actions/checkout, actions/create-github-app-token, and opentofu/setup-opentofu
to reviewed immutable full commit SHA pins, preserving their current
major-version behavior and configuration.
- Around line 108-112: Update the “File an issue on apply failure” step so it
does not inherit the job-level AWS credentials: either move those credentials to
the OpenTofu steps that require them or explicitly clear the AWS environment
variables for this issue-reporting step, while preserving its GITHUB_TOKEN and
RUN_URL configuration.
- Around line 114-121: The apply workflow’s concurrency group must serialize
runs across all refs and triggers. Update the workflow-level concurrency
configuration associated with the apply job to use one repository-wide group
without github.ref, while preserving any intended cancellation behavior.
- Around line 71-89: The safety description around the manual -exclude escape
hatch is inaccurate: update it to state that tofu apply is not atomic, completed
changes are not rolled back after failure, resources may remain partially
configured or tainted, and excluding an address also removes it and dependent
resources from the plan, leaving dependent changes pending.
- Around line 92-98: Update the EXCLUDE_ARGS parsing in the workflow to preserve
commas inside quoted addresses and trim unintended surrounding spaces, using a
quote-aware or newline-delimited approach with -exclude-file where appropriate.
Pin actions/checkout@v7, actions/create-github-app-token@v3, and
opentofu/setup-opentofu@v2 to their full immutable commit SHAs.
- Around line 90-109: Scope the “File an issue on apply failure” step to the TF
Apply step by assigning that step a unique ID and changing its condition to
require that step’s outcome to be failure, rather than using the broad failure()
status. Keep the existing issue-reporting behavior unchanged for genuine TF
Apply failures.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9b4aefd7-5bc2-453f-90e1-1bb42016f6a1
📒 Files selected for processing (1)
.github/workflows/apply.yaml
| - name: TF Apply | ||
| run: | | ||
| tofu apply -concise -auto-approve | ||
| EXCLUDE_ARGS=() | ||
| if [[ -n "${EXCLUDE_ADDRESSES}" ]]; then | ||
| IFS=',' read -ra ADDRS <<< "${EXCLUDE_ADDRESSES}" | ||
| for addr in "${ADDRS[@]}"; do | ||
| EXCLUDE_ARGS+=("-exclude=${addr}") | ||
| done | ||
| fi | ||
| tofu apply -concise -auto-approve "${EXCLUDE_ARGS[@]}" | ||
| env: | ||
| GITHUB_TOKEN: ${{ steps.generate-token.outputs.token }} | ||
| EXCLUDE_ADDRESSES: ${{ inputs.exclude_addresses }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/workflows/apply.yaml"
printf '%s\n' "Action references:"
rg -n 'uses:' "$file"
printf '%s\n' "Workflow context:"
sed -n '1,115p' "$file"Repository: osac-project/github-config
Length of output: 5458
Security Misconfiguration (CWE-829): Inclusion of Functionality from Untrusted Control Sphere
Reachability: External · Exploitability: Moderate
Pin all third-party actions to full commit SHAs.
Replace actions/checkout@v7, actions/create-github-app-token@v3, and opentofu/setup-opentofu@v2 with reviewed full commit-SHA references.
🤖 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/apply.yaml around lines 90 - 102, Update the workflow’s
third-party action references for actions/checkout,
actions/create-github-app-token, and opentofu/setup-opentofu to reviewed
immutable full commit SHA pins, preserving their current major-version behavior
and configuration.
Source: Path instructions
There was a problem hiding this comment.
Skipping — this is a pre-existing pattern across the entire file (all three uses: lines predate this PR, which doesn't add or touch any). Fixing it here would be inconsistent with the rest of the file; better addressed as its own dedicated hardening PR touching all of them at once.
There was a problem hiding this comment.
@eliorerz, acknowledged. The SHA-pinning finding remains valid, but it is outside the scope of this PR. A dedicated hardening PR that updates all workflow action references consistently is appropriate.
Would you like me to create a follow-up GitHub issue for the action-SHA pinning work?
ᕱ⑅ᕱ
You are interacting with an AI system.
| EXCLUDE_ARGS=() | ||
| if [[ -n "${EXCLUDE_ADDRESSES}" ]]; then | ||
| IFS=',' read -ra ADDRS <<< "${EXCLUDE_ADDRESSES}" | ||
| for addr in "${ADDRS[@]}"; do | ||
| EXCLUDE_ARGS+=("-exclude=${addr}") | ||
| done | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/workflows/apply.yaml"
printf '%s\n' "--- workflow excerpt ---"
sed -n '1,125p' "$file"
printf '%s\n' "--- exclusion references ---"
rg -n -C 2 'EXCLUDE_ADDRESSES|exclude-file|exclude=' "$file" || true
printf '%s\n' "--- action references ---"
rg -n '^[[:space:]]*-?[[:space:]]*uses:' "$file" || true
printf '%s\n' "--- Bash splitting probe ---"
EXCLUDE_ADDRESSES='aws_instance.web["a,b"], aws_instance.other'
IFS=',' read -ra ADDRS <<< "${EXCLUDE_ADDRESSES}"
for addr in "${ADDRS[@]}"; do
printf '<%s>\n' "$addr"
doneRepository: osac-project/github-config
Length of output: 6805
Fix exclusion parsing and pin workflow actions.
IFS=','splits valid addresses such asaws_instance.web["a,b"]and preserves leading spaces in values such asaws_a, aws_b. Use a quote-aware parser or newline-delimited addresses with-exclude-file.- Pin
actions/checkout@v7,actions/create-github-app-token@v3, andopentofu/setup-opentofu@v2to full commit SHAs. This job handles AWS credentials and an App private key.
🤖 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/apply.yaml around lines 92 - 98, Update the EXCLUDE_ARGS
parsing in the workflow to preserve commas inside quoted addresses and trim
unintended surrounding spaces, using a quote-aware or newline-delimited approach
with -exclude-file where appropriate. Pin actions/checkout@v7,
actions/create-github-app-token@v3, and opentofu/setup-opentofu@v2 to their full
immutable commit SHAs.
There was a problem hiding this comment.
Trimming fixed in 0837f15 (handles the common "addr1, addr2" case). Not switching to -exclude-file though: that only changes how I invoke tofu, it doesn't fix the comma-inside-brackets ambiguity in how I split the input string, which would need a different input format entirely (e.g. newline-separated). No address in this repo's actual config contains a literal comma inside a for_each key today, so I don't think that larger change is justified for a manual, human-operated one-time escape hatch -- noted the limitation explicitly in the commit message. Action SHA pinning: same reply as the other thread, pre-existing pattern across the whole file.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Terraform for_each declarations ---'
rg -n -C 3 --glob '*.tf' '\bfor_each\s*=' . || true
printf '%s\n' '--- Literal commas inside quoted Terraform strings ---'
rg -n --glob '*.tf' '(["'\''])[^"'\'']*,[^"'\'']*\1' . || true
printf '%s\n' '--- Indexed references with quoted comma-containing keys ---'
rg -n --glob '*.tf' '\[[^]]*["'\''][^"'\'']*,[^"'\'']*["'\''][^]]*\]' . || trueLength of output: 10140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CSV files ---'
fd --type f --extension csv .
printf '%s\n' '--- CSV fields containing literal commas ---'
python3 - <<'PY'
import csv
from pathlib import Path
for path in sorted(Path(".").rglob("*.csv")):
with path.open(newline="", encoding="utf-8") as source:
for row_number, row in enumerate(csv.DictReader(source), start=2):
for field, value in row.items():
if value is not None and "," in value:
print(f"{path}:{row_number}: field={field!r} value={value!r}")
PY
printf '%s\n' '--- Terraform quoted index expressions containing literal commas ---'
rg -nP --glob '*.tf' '\[[^]\r\n]*["'\''][^"'\'']*,[^"'\'']*["'\''][^]\r\n]*\]' . || trueLength of output: 3179
@eliorerz, verified. The CSV data contains no literal commas. The current for_each key sources therefore do not produce an address with a comma in its quoted index.
I withdraw the parsing portion of the finding. The current comma-separated manual input is adequate for this repository and this one-time use case.
The action SHA pinning concern remains separate and valid.
✏️ Learnings added
Learnt from: eliorerz
Repo: osac-project/github-config PR: 169
File: .github/workflows/apply.yaml:95-113
Timestamp: 2026-08-05T22:33:57.616Z
Learning: In `osac-project/github-config`, the Terraform `for_each` keys used for resource addresses derive from current CSV data or fixed values. The current CSV data contains no literal commas, so `.github/workflows/apply.yaml` can use a comma-separated `exclude_addresses` input for its documented one-time manual escape hatch. The workflow trims surrounding whitespace and rejects newline-separated, empty, and option-like entries.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
eliorerz
changed the title
ed806e5 to
8fe6471
Compare
|
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. |
8fe6471 to
d44aa49
Compare
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workflows/apply.yaml:
- Around line 106-113: Trim leading and trailing whitespace from each addr in
the state-removal loop before validating or passing the values to tofu state rm.
Update the ADDRS entries in the loop around the existing empty and
leading-hyphen checks, preserving validation and removal behavior for
already-clean addresses.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 15bb1865-633a-465c-9ad5-93c17b9f8102
📒 Files selected for processing (1)
.github/workflows/apply.yaml
A single broken/unrefreshable resource aborts the entire tofu apply atomically, blocking every other repo's pending Terraform changes with no signal beyond a red Actions run. This has happened twice: the host-management-openstack archived-repo bug (osac-project#165) and the orphaned github_membership entries for users who left the org (osac-project#162). The CaaS-Netris required-check rename sat un-applied for hours because of the former, discovered only by chance while investigating an unrelated CI queue backlog. Two changes: - File (or comment on) a tracking issue when tofu apply fails, so a stuck apply is never silent again. Uses the same GitHub App token already generated for tofu itself (issues:write is already granted), no new secrets needed. - Add a one-time, manual `exclude_addresses` workflow_dispatch input (mirrors osac-project#162's `state_rm_addresses` pattern) that threads into `tofu apply -exclude=...`, letting a maintainer immediately unblock every other repo's changes when one resource is known-broken, while a permanent fix is prepared -- without giving up single-pass, dependency-complete apply for the normal case. A permanent per-module `-target` loop was considered and rejected: several modules share cross-module resources (e.g. github_team.all["wg-infra"], referenced by ruleset_bypass_team_ids in 5+ repo modules), so looping per module would redundantly re-plan/ re-apply those shared resources on every iteration referencing them, and gives up Terraform's whole-graph dependency ordering for no real isolation benefit in the common case where nothing is broken. Signed-off-by: Elior Erez <eerez@redhat.com>
d44aa49 to
ce79636
Compare
Matches the same trimming already applied to exclude_addresses -- "addr1, addr2" (space after the comma) is the natural way to type this list by hand for either input. Signed-off-by: Elior Erez <eerez@redhat.com>
eliorerz
changed the title
1 participant
Why
A single broken/unrefreshable resource aborts the entire
tofu applyatomically, blocking every other repo's pending Terraform changes with no signal beyond a red Actions run. This has happened twice independently: thehost-management-openstackarchived-repo bug (#165) and orphanedgithub_membershipentries for users who left the org (#162). The CaaS-Netris required-check rename sat un-applied for hours because of the former — only discovered by chance while investigating an unrelated CI queue backlog.What
Two complementary changes, chosen after evaluating (and rejecting) a permanent per-module
-targetloop — see tradeoff writeup below:tofu applyfails, so a stuck apply is never silent again. Uses the same GitHub App token already generated fortofuitself (issues: writeis already granted on this app installation) — no new secrets. Reuses an existing openapply-failure-labeled issue via comment rather than spamming a new one on every scheduled retry.exclude_addressesworkflow_dispatch input, mirroring NO-ISSUE: Add one-time TF state rm support for unrefreshable resources #162'sstate_rm_addressespattern, threaded intotofu apply -exclude=.... Lets a maintainer immediately unblock every other repo's pending changes when one specific resource is known-broken, while a permanent fix is prepared — without giving up single-pass, dependency-complete apply for the normal (nothing-broken) case.Why not a permanent per-module
-targetloopConsidered and rejected. Checked this repo's actual module structure first rather than assuming it's free:
github_team.all["wg-infra"]is referenced viaruleset_bypass_team_idsin 5+ separate repo modules. A per-module loop would redundantly re-plan/re-apply that shared resource on every iteration that references it.-targetmode explicitly gives up Terraform's whole-graph dependency ordering (per OpenTofu's own docs: "not recommended for routine use"). For the common case where nothing is broken, this trades away a real safety property for zero benefit.tofu apply -target=...invocations instead of one materially increases apply time and provider API calls every single run, not just during an incident.run:steps default tobash -eo pipefail) adds real complexity for a benefit (failure isolation) theexclude_addressesescape hatch already covers on-demand, at the moment it's actually needed.Test plan
yamllint --strict -c .yamllint.yaml .github/workflows/apply.yaml— passesshellcheck— clean, no warningsexclude_addressesfor real the next time a resource is known-broken (can't simulate a livetofu applyfailure safely without real state access)Summary by CodeRabbit