Repository navigation
fix(terraform): compare full cluster ARNs in the collector duplicate guard and root-derive it correctly - #93
Closed
tim-thacker-nullify wants to merge 1 commit into
Conversation
…guard and root-derive it correctly The guard added in the previous round compared bare cluster names, so two clusters sharing a name in different regions/accounts collided with no override in the v6 root. It also derived collector_cluster_arns from the IRSA trust list, which is a prerequisite for a collector, not evidence one is deployed. - Compare full cluster ARNs instead of names in eks-managed-scan-access (both trees); add a same-name-different-region test row in both. - Add a root collector_cluster_arns override variable (both trees) so root users can correct the IRSA-trust-list proxy and decouple a collector-to-managed-scan cutover from revoking IRSA trust. - Reword the guard's error message to cover the Helm/manifest paths its own variable description already names. - Add ARN-shape validation to collector_cluster_arns, matching cluster_arns. - Remove the dead collector_cluster_arns wiring in both multi-cluster examples: scan_mode's mutual exclusivity makes it always []. - Fix terraform-v6/README.md still documenting the enable_collector + enable_managed_scan_rbac rejection this PR's base commit removed; add the missing collector_cluster_arns doc and Kubernetes-provider-removal upgrade note to match the v5 tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
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.
Round-5 review of #92 found the moved duplicate guard compared bare cluster
names (colliding same-named clusters in different regions/accounts, no
override in the v6 root) and derived its input from a list that means IRSA
trust, not collector deployment.
eks-managed-scan-access(both trees); account/region/name are already parsed off the same ARN on
both sides. Added a same-name-different-region test row in both trees.
collector_cluster_arnsoverride variable (both trees,default
null) so a customer can correct theeks_cluster_arns-derivedguess when trust is granted ahead of a staged rollout, or when cutting a
cluster to managed scan without revoking IRSA trust in the same apply.
manifest paths its own variable description already covers.
collector_cluster_arns, matchingcluster_arns; it previously indexedsplit(...)[1]unguarded.collector_cluster_arnswiring in both multi-clusterexamples:
scan_mode's validation makeslocal.collectorandlocal.managedmutually exclusive, so the expression was provablyalways
[]. Restructuring the example to exercise mixed modes percluster is a larger change; scoped out.
terraform-v6/README.md:117, which still documented theenable_collector+enable_managed_scan_rbacrejection this PR's basecommit removed. Added the missing
collector_cluster_arnsdocs and theKubernetes-provider-removal upgrade note, matching the v5 tree.
Not fixed, by design: a direct caller of
eks-managed-scan-access(bypassing the root) still gets no protection unless it passes
collector_cluster_arnsitself -- the module cannot discover a siblingk8s-resourcesinstance, so no non-empty default is expressible there. Theroot's own derivation is the one automatic path, and it's now correct by
default.
Round-5's F10 (round-4's PR undercounted validate rows removed, four vs
six) is a note about that already-merged PR body; nothing here to correct.
Merge order:
main ← #78 ← #81 ← #83 ← #89 ← #91 ← #92 ← this PR. #82 and#88 (off #78) should be closed: #88 merges clean but leaves two duplicate
README paragraphs; #82 conflicts outright in
k8s-resources/variables.tf(both trees).
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.