From 8b3cd40d4a18de410be68d7f7e824f7e36061c5b Mon Sep 17 00:00:00 2001 From: milosjovanovic Date: Thu, 27 Aug 2026 11:52:20 +0200 Subject: [PATCH 1/5] workflows: read protected owners and drift reviewers from deployment environments PROTECTED_OWNERS and DRIFT_REVIEWERS were the only pieces of deployment config still living as repository-level variables in the config repo, handed to the reusable workflows as workflow_call inputs. Everything else -- APP_ID, APP_NAME, WORKSPACE, app_private_key, tfc_token -- already comes from the deployment environment the job runs in. Both inputs defaulted to "", so a config repo that forgot to wire one got a green check and no enforcement at all: validate-org only warns, and the drift PR silently skips --add-reviewer. Reading the values from the environment removes that hand-off, and with it the fail-open. It also puts the protected-owner list behind the same gate as the rule it backs. validate-org already refuses to read its schema or its owner list from the config repo so that a pull request cannot weaken the rule it is validated against; the value itself now sits behind the plan environment's branch policy rather than in an open repository variable. The validate job was the only job in the repo outside any environment, so it joins terraform-plan in 'plan' rather than getting one of its own -- a dedicated environment would be auto-created empty on first use in config repos that had not set it up, reintroducing the silent fail-open. Breaking change for consumers: see the migration section in docs/workflows.md. --- .github/workflows/drift-check.yaml | 14 ++++------ .github/workflows/tf-plan.yaml | 14 ++++++---- docs/renaming-teams-and-members.md | 2 +- docs/workflows.md | 44 ++++++++++++++++++++++++++++-- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/.github/workflows/drift-check.yaml b/.github/workflows/drift-check.yaml index ff255de..3663ec6 100644 --- a/.github/workflows/drift-check.yaml +++ b/.github/workflows/drift-check.yaml @@ -12,11 +12,6 @@ on: type: string description: "Terraform Cloud organization" required: true - reviewers: - type: string - description: "Comma-separated reviewers (users or org/team slugs) to request on the drift PR" - required: false - default: "" secrets: tfc_token: description: "Terraform Cloud API token" @@ -40,9 +35,10 @@ jobs: runs-on: ubuntu-latest # The 'schedule' environment must provide APP_ID + app_private_key (management # app, which needs org-wide "All repositories" access so new repos are visible - # to the importer), plus WORKSPACE for the plan. Because this runs unattended - # on a schedule, the environment MUST NOT have required-reviewer/wait-timer - # protection rules, or every run will stall pending manual approval. + # to the importer), WORKSPACE for the plan, and DRIFT_REVIEWERS for the drift + # PR. Because this runs unattended on a schedule, the environment MUST NOT + # have required-reviewer/wait-timer protection rules, or every run will stall + # pending manual approval. environment: schedule permissions: contents: write @@ -124,7 +120,7 @@ jobs: uses: ./.github/actions/drift-pr with: branch-name: "drift/detected-changes" - reviewers: ${{ inputs.reviewers }} + reviewers: ${{ vars.DRIFT_REVIEWERS }} github-token: ${{ steps.generate-token.outputs.token }} # terraform is the authority on whether drift is real (avoids false # positives from config differences it treats as no-ops). diff --git a/.github/workflows/tf-plan.yaml b/.github/workflows/tf-plan.yaml index f440cfd..06ac5a4 100644 --- a/.github/workflows/tf-plan.yaml +++ b/.github/workflows/tf-plan.yaml @@ -21,11 +21,6 @@ on: description: "Workspace-relative path to the fallback JSON schema for repos/*.yaml validation, used when the config repo provides no override (e.g. 'gcss_config/.schemas/my.schema.json'). When omitted, the schema bundled with github-terraformer is used." required: false default: "" - protected_owners: - type: string - description: "Comma-separated org logins that must stay owners in organisation/members.yaml (never removed or demoted). Sourced from deployment config (e.g. a repo variable), not the config repo." - required: false - default: "" secrets: app_private_key: required: true @@ -35,8 +30,15 @@ on: required: true jobs: + # The 'plan' environment must provide PROTECTED_OWNERS — the org logins that + # must stay owners in organisation/members.yaml. It is deployment config, so a + # pull request cannot weaken the rule it is validated against. The environment + # MUST NOT have required-reviewer/wait-timer protection rules, or every pull + # request waits for a manual approval before its author sees any validation + # feedback at all. validate: runs-on: ubuntu-latest + environment: plan permissions: contents: read steps: @@ -65,7 +67,7 @@ jobs: uses: ./.github/actions/validate-org-configs with: config-path: gcss_config - protected-owners: ${{ inputs.protected_owners }} + protected-owners: ${{ vars.PROTECTED_OWNERS }} terraform-plan: needs: validate diff --git a/docs/renaming-teams-and-members.md b/docs/renaming-teams-and-members.md index b3c0a2a..c4edb18 100644 --- a/docs/renaming-teams-and-members.md +++ b/docs/renaming-teams-and-members.md @@ -139,7 +139,7 @@ Use `terraform state rm` instead. The recreate is safe because the provider crea ### 1. Pre-flight: protected owners -If the member is listed in the `protected_owners` input passed to `tf-plan.yaml` (a deployment-side repository variable, not config-repo content), **update that variable to the new username first**. Otherwise `validate-org` fails the PR because a protected owner is missing from `members.yaml`. +If the member is listed in `PROTECTED_OWNERS` — a variable on the config repo's `plan` deployment environment, not config-repo content — **update that variable to the new username first**. Otherwise `validate-org` fails the PR because a protected owner is missing from `members.yaml`. ### 2. Open the PR and get it approved diff --git a/docs/workflows.md b/docs/workflows.md index af5d63e..f66602a 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -26,6 +26,32 @@ 5. Upon PR merge, Terraform Cloud plans and applies the configuration 6. Configuration file is then sanitized (ids removed) and moved to the appropriate directory `feature/github-repo-provisioning/repo_configs/{branch}/{organization}` +### ✅ `Validate and Plan` Workflow + +- **Trigger**: `workflow_run` from the config repo, on every pull request. +- **Behavior**: + 1. `validate` checks `repos/*.yaml` against the repository schema and `organisation/*.yaml` against the teams/members schemas and cross-file rules — including the protected-owner rule, which fails the PR if a protected identity is removed from or demoted in `members.yaml`. + 2. `terraform-plan` (`needs: validate`, so it is skipped when validation fails) runs `terraform plan` on Terraform Cloud and posts the result as a PR comment and a check-run. + +#### Consumer setup requirements + +Both jobs run in the **`plan`** environment. That environment **must** provide all of: + +| Name | Type | Purpose | +|---|---|---| +| `PROTECTED_OWNERS` | variable | Comma-separated org logins that must stay owners in `organisation/members.yaml` | +| `APP_ID` | variable | GitHub App used to post the plan comment and check-run | +| `APP_NAME` | variable | Name of that App | +| `app_private_key` | secret | Private key for that App | +| `WORKSPACE` | variable | Terraform Cloud workspace | +| `tfc_token` | secret | Terraform Cloud API token | + +> [!IMPORTANT] +> `PROTECTED_OWNERS` is deployment config and is deliberately read from the environment rather than passed in by the caller, so that a pull request cannot weaken the rule it is validated against — and so that a config repo cannot silently disable the check by forgetting to wire it. When it is unset, `validate-org` warns and enforces nothing. + +> [!IMPORTANT] +> The `plan` environment **must not** have required-reviewer or wait-timer protection rules. `validate` runs in it too, so any approval gate means a pull request author waits for a human before seeing any validation feedback at all. + ### 🔍 `Drift Check` Workflow - **Trigger**: Scheduled (cron) from the config repo. @@ -45,12 +71,11 @@ The reusable `drift-check.yaml` runs in the **`schedule`** environment. That env | `app_private_key` | secret | Private key for that App | | `WORKSPACE` | variable | Terraform Cloud workspace | | `tfc_token` | secret | Terraform Cloud API token | +| `DRIFT_REVIEWERS` | variable | Comma-separated users or `org/team` slugs to request as reviewers on the drift PR | > [!IMPORTANT] > The `schedule` environment **must not** have required-reviewer or wait-timer protection rules. The workflow runs unattended on a schedule, so any approval gate makes every run stall forever. -Caller also passes `reviewers` (comma-separated users or `org/team` slugs) to request on the drift PR. - #### Notes / limitations - **Scale**: each run does a **full org import** plus a plan. On large orgs this is the dominant cost (several API calls per repo). Pick a cron interval that comfortably exceeds a run's duration — overlapping runs are queued (`cancel-in-progress: false`), so too-frequent scheduling lags detection. @@ -105,4 +130,17 @@ To import an **existing GitHub repository** into Terraform: - The name of the YAML file will be the same as the repository name - Create a PR against the `prod` branch 4. Review, approve, and merge the PR -5. Terraform Cloud will detect and apply the changes \ No newline at end of file +5. Terraform Cloud will detect and apply the changes + +## 🔀 Migrating from caller-passed `protected_owners` / `reviewers` + +`tf-plan.yaml` and `drift-check.yaml` used to take these values as `workflow_call` inputs, sourced from **repository-level** variables in the config repo. They are now read from the environment the job already runs in. Removing the inputs is a breaking change: a caller that still passes one fails immediately with `Invalid input`. + +Because callers pin a ref, nothing breaks until that ref is bumped. Migrate in this order: + +1. Add `PROTECTED_OWNERS` to the **`plan`** environment and `DRIFT_REVIEWERS` to the **`schedule`** environment of the config repo (Settings → Environments → … → Environment variables). +2. In a **single** commit, bump the pinned ref **and** drop `protected_owners:` / `reviewers:` from the `with:` blocks. +3. Once a run succeeds, delete the repository-level `PROTECTED_OWNERS` and `DRIFT_REVIEWERS` variables. + +> [!NOTE] +> Repository-level `WORKSPACE` and `TFC_TOKEN` stay where they are — the `discover` job of the decommission workflow runs outside any environment and cannot read environment-scoped values. See the decommission section above. From 59848a094b889a46bad560abbc18c371a44df348 Mon Sep 17 00:00:00 2001 From: milosjovanovic Date: Thu, 27 Aug 2026 12:06:00 +0200 Subject: [PATCH 2/5] docs/workflows: remove obsolete migration guidance for `protected_owners` and `reviewers` inputs --- .github/workflows/tf-plan.yaml | 6 ------ docs/workflows.md | 13 ------------- 2 files changed, 19 deletions(-) diff --git a/.github/workflows/tf-plan.yaml b/.github/workflows/tf-plan.yaml index 06ac5a4..a5fc260 100644 --- a/.github/workflows/tf-plan.yaml +++ b/.github/workflows/tf-plan.yaml @@ -30,12 +30,6 @@ on: required: true jobs: - # The 'plan' environment must provide PROTECTED_OWNERS — the org logins that - # must stay owners in organisation/members.yaml. It is deployment config, so a - # pull request cannot weaken the rule it is validated against. The environment - # MUST NOT have required-reviewer/wait-timer protection rules, or every pull - # request waits for a manual approval before its author sees any validation - # feedback at all. validate: runs-on: ubuntu-latest environment: plan diff --git a/docs/workflows.md b/docs/workflows.md index f66602a..9731249 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -131,16 +131,3 @@ To import an **existing GitHub repository** into Terraform: - Create a PR against the `prod` branch 4. Review, approve, and merge the PR 5. Terraform Cloud will detect and apply the changes - -## 🔀 Migrating from caller-passed `protected_owners` / `reviewers` - -`tf-plan.yaml` and `drift-check.yaml` used to take these values as `workflow_call` inputs, sourced from **repository-level** variables in the config repo. They are now read from the environment the job already runs in. Removing the inputs is a breaking change: a caller that still passes one fails immediately with `Invalid input`. - -Because callers pin a ref, nothing breaks until that ref is bumped. Migrate in this order: - -1. Add `PROTECTED_OWNERS` to the **`plan`** environment and `DRIFT_REVIEWERS` to the **`schedule`** environment of the config repo (Settings → Environments → … → Environment variables). -2. In a **single** commit, bump the pinned ref **and** drop `protected_owners:` / `reviewers:` from the `with:` blocks. -3. Once a run succeeds, delete the repository-level `PROTECTED_OWNERS` and `DRIFT_REVIEWERS` variables. - -> [!NOTE] -> Repository-level `WORKSPACE` and `TFC_TOKEN` stay where they are — the `discover` job of the decommission workflow runs outside any environment and cannot read environment-scoped values. See the decommission section above. From 566f77d4c35350b2413e8f5a2df64233c50e53fe Mon Sep 17 00:00:00 2001 From: milosjovanovic Date: Thu, 27 Aug 2026 12:37:24 +0200 Subject: [PATCH 3/5] validate-org: require a protected-owner list whenever organisation config exists An empty --protected-owners warned and returned 0, so a config repo that never wired the value got a green check and no enforcement. Moving the value behind a deployment environment does not fix that on its own: an unset variable still resolves to "" and lands in the same branch. The list is now required, but only when organisation/ config is actually present. An unconditional error would break config repos that manage repositories and nothing else, which TestValidateOrg_NoFilesAndNoProtected OwnersPasses exists to protect. A repo that has organisation config is exactly the one that had the variable set, so the narrower rule closes the hole without a regression. It is reported through the existing failures slice rather than returned early, so schema and cross-file errors are still collected and shown in the same run. Protected owner logins are no longer echoed. Actions logs on a public config repo are world-readable while repository variables are not, so printing the list widened its exposure rather than narrowing it -- the opposite of the reason for moving it behind the environment. The count is enough to confirm the value was read. --- .../github-repo-importer/cmd/validate-org.go | 17 ++++++++--- .../cmd/validate-org_test.go | 28 +++++++++++++++++-- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/feature/github-repo-importer/cmd/validate-org.go b/feature/github-repo-importer/cmd/validate-org.go index c0c9e95..6e558a2 100644 --- a/feature/github-repo-importer/cmd/validate-org.go +++ b/feature/github-repo-importer/cmd/validate-org.go @@ -44,6 +44,11 @@ Files may be absent. An absent members.yaml is treated as an empty member list, matching how Terraform reads it, so protected owners are enforced even when the file is deleted outright. +--protected-owners is required whenever any organisation config is present: +validating organisation membership without it would report success while +enforcing nothing. A config directory with no organisation config at all needs +no list and passes. + Unlike repository config, the schema is never taken from the config repository: both the schema and the protected-owner list are deployment config, so a pull request cannot weaken the rules it is validated against.`, @@ -74,18 +79,22 @@ func runValidateOrg(cmd *cobra.Command, configDir, protectedOwnersCSV, fallbackT ) protectedOwners := splitCSV(protectedOwnersCSV) + hasOrgConfig := len(teamsFiles) > 0 || len(membersFiles) > 0 + if len(protectedOwners) > 0 { - cmd.Printf("Enforcing %d protected owner(s): %s\n", len(protectedOwners), strings.Join(protectedOwners, ", ")) - } else { - cmd.PrintErrln("WARNING: no protected owners configured, owner removal and demotion are not enforced") + cmd.Printf("Enforcing %d protected owner(s)\n", len(protectedOwners)) } - if len(teamsFiles) == 0 && len(membersFiles) == 0 { + if !hasOrgConfig { cmd.Println("No organisation teams.yaml or members.yaml found, validating as an empty organisation config") } var failures []string + if len(protectedOwners) == 0 && hasOrgConfig { + failures = append(failures, "no protected owners configured: organisation config is present, so owner removal and demotion must be enforced. Set PROTECTED_OWNERS on the deployment environment that runs validation") + } + teamNames, teamsOK := validateTeamsFiles(cmd, teamsFiles, fallbackTeams, &failures) validateMembersFiles(cmd, membersFiles, fallbackMembers, teamNames, teamsOK, protectedOwners, &failures) diff --git a/feature/github-repo-importer/cmd/validate-org_test.go b/feature/github-repo-importer/cmd/validate-org_test.go index d08f849..3acc151 100644 --- a/feature/github-repo-importer/cmd/validate-org_test.go +++ b/feature/github-repo-importer/cmd/validate-org_test.go @@ -242,15 +242,37 @@ func TestValidateOrg_ProtectedOwnersCSVParsed(t *testing.T) { assert.Contains(t, out, "Enforcing 2 protected owner(s)") } -func TestValidateOrg_WarnsWhenNoProtectedOwnersConfigured(t *testing.T) { +func TestValidateOrg_FailsWhenOrgConfigPresentAndNoProtectedOwners(t *testing.T) { dir := newOrgConfigDir(t, map[string]string{ "members.yaml": "members:\n - username: alice\n role: owner\n", }) out, err := runValidateOrgCmd(t, dir, "") - assert.NoError(t, err) - assert.Contains(t, out, "WARNING: no protected owners configured") + require.Error(t, err) + assert.Contains(t, out, "no protected owners configured") +} + +func TestValidateOrg_FailsWhenOnlyTeamsPresentAndNoProtectedOwners(t *testing.T) { + dir := newOrgConfigDir(t, map[string]string{ + "teams.yaml": validTeamsYAML, + }) + + _, err := runValidateOrgCmd(t, dir, "") + + require.Error(t, err) +} + +func TestValidateOrg_ProtectedOwnerLoginsAreNotEchoed(t *testing.T) { + dir := newOrgConfigDir(t, map[string]string{ + "members.yaml": "members:\n - username: alice\n role: owner\n", + }) + + out, err := runValidateOrgCmd(t, dir, "alice") + + require.NoError(t, err) + assert.Contains(t, out, "Enforcing 1 protected owner(s)") + assert.NotContains(t, out, "alice") } func TestValidateOrg_FileBasedSchemasAreUsed(t *testing.T) { From c4f2c6f2e5d51b9c25d942921021e9fc6e9e4ee0 Mon Sep 17 00:00:00 2001 From: milosjovanovic Date: Thu, 27 Aug 2026 12:37:36 +0200 Subject: [PATCH 4/5] drift-pr: warn when a drift PR opens with no reviewers The guard around --add-reviewer skipped silently on an empty list, so a drift PR could open unassigned on an otherwise green run with nothing in the logs to say so. Now that the value comes from DRIFT_REVIEWERS on the schedule environment rather than from the caller, a missing variable lands in exactly that branch. Both this and the existing --add-reviewer failure path are raised as ::warning:: so they surface in the run summary instead of only in the log body. --- .github/actions/drift-pr/action.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/actions/drift-pr/action.yaml b/.github/actions/drift-pr/action.yaml index f07f872..52e2273 100644 --- a/.github/actions/drift-pr/action.yaml +++ b/.github/actions/drift-pr/action.yaml @@ -102,7 +102,9 @@ runs: --head "$BRANCH") echo "Opened new drift PR: $pr_url" if [[ -n "${REVIEWERS}" ]]; then - gh pr edit "$pr_url" --add-reviewer "${REVIEWERS}" || echo "Warning: could not request reviewers '${REVIEWERS}'" + gh pr edit "$pr_url" --add-reviewer "${REVIEWERS}" || echo "::warning::Could not request reviewers '${REVIEWERS}' on $pr_url" + else + echo "::warning::No reviewers configured, drift PR $pr_url is unassigned. Set DRIFT_REVIEWERS on the schedule environment." fi else echo "Updated existing drift PR #$existing_pr." From 6699b4e8b1921ea2f79e581bb90f204ca3623840 Mon Sep 17 00:00:00 2001 From: milosjovanovic Date: Thu, 27 Aug 2026 12:37:36 +0200 Subject: [PATCH 5/5] docs/workflows: correct the plan environment setup table Three problems with the table as written: APP_NAME was listed as required but nothing reads it -- it matched only that row in the whole repo. gh_token was missing, so a consumer following "must provide all of" exactly would fail on first run. Splitting the table also fixes a conflation: the variables are read from the environment by the jobs themselves, while the secrets are workflow_call secrets the caller passes in, and the two are not configured in the same place. The note on PROTECTED_OWNERS contradicted itself in consecutive sentences, claiming the check could not be silently disabled and then describing how it silently disabled itself. That is now accurate against the validation change. The warning about protection rules on plan is raised from IMPORTANT to WARNING and states the cost plainly: protection rules apply per job, so a consumer that had required reviewers on plan for terraform-plan now pays two approvals per pull request and sees no validation feedback until the first one lands. --- docs/workflows.md | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/docs/workflows.md b/docs/workflows.md index 9731249..07beb88 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -35,22 +35,27 @@ #### Consumer setup requirements -Both jobs run in the **`plan`** environment. That environment **must** provide all of: +Both jobs run in the **`plan`** environment. Variables are read from that environment by the workflow itself: -| Name | Type | Purpose | +| Name | Read by | Purpose | |---|---|---| -| `PROTECTED_OWNERS` | variable | Comma-separated org logins that must stay owners in `organisation/members.yaml` | -| `APP_ID` | variable | GitHub App used to post the plan comment and check-run | -| `APP_NAME` | variable | Name of that App | -| `app_private_key` | secret | Private key for that App | -| `WORKSPACE` | variable | Terraform Cloud workspace | -| `tfc_token` | secret | Terraform Cloud API token | +| `PROTECTED_OWNERS` | `validate` | Comma-separated org logins that must stay owners in `organisation/members.yaml` | +| `APP_ID` | `terraform-plan` | GitHub App used to post the plan comment and check-run | +| `WORKSPACE` | `terraform-plan` | Terraform Cloud workspace | -> [!IMPORTANT] -> `PROTECTED_OWNERS` is deployment config and is deliberately read from the environment rather than passed in by the caller, so that a pull request cannot weaken the rule it is validated against — and so that a config repo cannot silently disable the check by forgetting to wire it. When it is unset, `validate-org` warns and enforces nothing. +The caller must also pass all three `workflow_call` secrets — these are declared on the reusable workflow, not looked up by the jobs: + +| Name | Purpose | +|---|---| +| `app_private_key` | Private key for the App named by `APP_ID` | +| `gh_token` | Checks the config repo out at the commit under review | +| `tfc_token` | Terraform Cloud API token | > [!IMPORTANT] -> The `plan` environment **must not** have required-reviewer or wait-timer protection rules. `validate` runs in it too, so any approval gate means a pull request author waits for a human before seeing any validation feedback at all. +> `PROTECTED_OWNERS` is deployment config and is deliberately read from the environment rather than passed in by the caller, so that a pull request cannot weaken the rule it is validated against. Leaving it unset is not a way to opt out: whenever any `organisation/` config is present, `validate-org` fails. A config repo with no organisation config at all needs no list and passes. + +> [!WARNING] +> The `plan` environment **must not** have required-reviewer or wait-timer protection rules. Protection rules apply per job, and `validate` now runs in this environment too — so an approval gate costs **two** approvals per pull request, and the author sees no validation feedback until the first one lands. Consumers that had reviewers on `plan` for the `terraform-plan` job need to remove them, or accept that cost. ### 🔍 `Drift Check` Workflow @@ -71,7 +76,7 @@ The reusable `drift-check.yaml` runs in the **`schedule`** environment. That env | `app_private_key` | secret | Private key for that App | | `WORKSPACE` | variable | Terraform Cloud workspace | | `tfc_token` | secret | Terraform Cloud API token | -| `DRIFT_REVIEWERS` | variable | Comma-separated users or `org/team` slugs to request as reviewers on the drift PR | +| `DRIFT_REVIEWERS` | variable | Comma-separated users or `org/team` slugs to request as reviewers on the drift PR. When unset the drift PR still opens, unassigned, and the run logs a warning | > [!IMPORTANT] > The `schedule` environment **must not** have required-reviewer or wait-timer protection rules. The workflow runs unattended on a schedule, so any approval gate makes every run stall forever.