GCSS-1391: validate organisation/*.yaml before plan - #67
Merged
Conversation
Adds a validate-org command that checks organisation/teams.yaml and organisation/members.yaml against their JSON schema and the cross-file rules the schema cannot express (unknown team references, duplicate teams, and a protected owner being removed or demoted). The protected-owner list is passed in from deployment config rather than read from the config repo, so a PR cannot weaken its own guard. Wired into tf-plan as an extra step in the existing validate job, reusing its checkouts, so an invalid org config fails fast before Terraform runs. The schema-compilation block in validate.go is factored into a shared compileSchema helper used by the repository, teams, and members schemas.
Deleting members.yaml plans the same org-wide member wipe as emptying it, because Terraform reads an absent file as an empty list, but only the empty list was rejected: skipping the check when the file was missing let the easier path through. The members check now always runs, loading an absent file as an empty config. Validation also only looked at organisation/, while both .tf files merge in staged content from importer_tmp_dir/organisation/. That left the bootstrap import, which first hands over ownership of the org, as the one plan with no checks at all. Both locations are now read and merged the way Terraform merges them, so team references resolve across the two and the protected-owner check sees the effective config.
An empty protected-owner list made the guard a silent no-op: the output was an unqualified pass, so an operator could not tell an enforced run from an unconfigured one. It now names the owners it enforces, and warns when there are none. Each file was also read and parsed twice, once per validation layer, which reported a parse failure twice and then blamed every member team reference on the teams file it had failed to load. Parsing now happens once and is shared by both layers, and the member checks are skipped while the teams config is unparsable. A schema that fails to compile no longer discards the failures already collected. Names are constrained to a non-empty string: presence alone let name: "" through, which yields a resource keyed by the empty string.
Merging the two members files before validating meant a duplicate username was reported or not depending on what the other file happened to contain: when the promoted file overrode that username, both staged entries were dropped and the duplicate disappeared. Terraform keys the staged members on their own, so that is precisely the plan error the check exists to pre-empt. Duplicates and team references are per-file rules and now run per file, as the teams checks already did. Only the protected-owner rule is genuinely cross-file and keeps running on the merged set. Splitting them also means an unparsable teams file no longer hides an owner being removed, since that rule never needed the team names.
GitHub treats logins as case-insensitive and lowercases team names into slugs, but the checks compared bytes. alice and Alice passed as two members of one account, which Terraform then keys separately so two resources manage a single membership and each plan shows the other's write; platform and Platform passed as two teams that resolve to one. A protected owner spelled with different case was reported missing, which is the misleading direction of the same fault. Team references are deliberately still matched exactly, because Terraform resolves them as a map key and a folded match would only move the failure to plan time. The error now names the defined spelling instead.
dev-milos
marked this pull request as ready for review
July 28, 2026 09:24
The why behind these decisions is already in the commit messages that made them, and repeating it inline only creates a second copy to keep in sync. Godoc is trimmed to a single sentence per function, matching how validate.go already documents itself.
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
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.
Implements the schema-validation half of GCSS-1391. The separate workspace/state half is a follow-up PR.
What this does
Validates the organisation
teams.yamlandmembers.yamlon every PR, before Terraform plan, so a bad org config fails fast with a clear message instead of erroring at apply — or worse, planning a member wipe that reads as a clean plan.Two layers, mirroring how
repos/*.yamlis already validated:visibilityisvisible|secret,roleisowner|member, names are non-empty), reusing the existingvalidateFilemachinery.TeamsConfig.Validate()/MembersConfig.Validate(), which until now were called from nowhere but tests:teams.yamlBoth config locations are validated
Terraform reads org config from two places and
merge()s them —organisation/and the bootstrap importer's stagedimporter_tmp_dir/organisation/. Validation reads and merges both the same way, so:Without this, the bootstrap import PR — the one that first hands GCSS ownership of the org, and the highest-stakes plan in the feature — would be the single plan receiving no checks at all.
An absent members.yaml is not a bypass
Terraform reads a missing
members.yamlasmembers: [], so deleting the file plans the same org-wide member wipe as emptying it. Both are now rejected identically; validation loads an absent file as an empty config rather than skipping the check.git rm organisation/members.yamldoes not sail through.Protected owners and schemas both come from deployment config
--protected-ownersis threaded from aprotected_ownersworkflow input (intended to come from a repo/org Actions variable) and is never read from a file inside the config repo — otherwise a PR could edit its own guard away.For the same reason, and unlike repository config, the org path deliberately does not honour a config-repo schema override (
resolveOrgSchema). Both the rules and the list of who they protect are deployment config, so a pull request cannot weaken what it is validated against.When no protected owners are configured the run warns rather than passing silently, so an operator can tell an enforced run from an unconfigured one.
What's included
validate-orgcommand — orchestrates both layers over both locations.compileSchema+loadYAMLDocument/validateInstance— factored out ofvalidate.goso the repository, teams and members paths share one schema compiler and one YAML parse, rather than each file being read and parsed once per layer.validate-org-configscomposite action — mirrorsvalidate-repo-configs.validatejob intf-plan.yaml, reusing its checkouts;terraform-planstillneeds: validate.minLength: 1on team name, member username and team-membership name —requiredonly checks presence, soname: ""previously produced a resource keyed by the empty string.teams.yamlfails to parse, and the file-based schema path the composite action actually uses in CI.Testing
go build/go vet/go test ./...pass; schemas regenerate with no drift; workflow and action YAML parse.ok -- organisation config(the log line confirms it validated the staged files rather than skipping) → plan1 to import, 0 to add, 0 to change, 0 to destroyPROTECTED_OWNERSset →protected owner "…" is missing from members.yaml, validate job failed,terraform-planskippedA conscious sequencing call
Org validation is added to the shared
validatejob thatterraform-plandepends on. Until the workspace split lands, that means a brokenorganisation/members.yamlblocks the plan for a PR touching onlyrepos/*.yaml— the shared blast radius #1391 exists to remove, present at the CI layer.This is deliberate rather than overlooked. While the workspace is shared, the same plan applies org config, so allowing a plan to proceed against config known to be invalid would be the worse failure. Making the step conditional on diff scope would add machinery the workspace split then deletes. The coupling goes away with the split, not before it.
Rollout / follow-ups
PROTECTED_OWNERSvariable andprotected_owners: ${{ vars.PROTECTED_OWNERS }}on the tf-plan caller for enforcement to take effect. Note that setting it before the org config is bootstrapped will fail every PR, correctly — it is effectively the "org management adopted" switch.import-orgcaller workflow; there is currently no way to trigger the bootstrap importer from it.main.tfmergesimporter_tmp_dir/*.yamlinto the applied set, butglobRepoConfigsonly globsrepos/. This PR closes that gap for org and leaves it open for repos — same class of bug, pre-existing, and worth its own change since it can make a currently-passing import PR start failing.