Skip to content

fix(helm): accept KMS alias ARNs in the collector and keep the role ARN commercial - #72

Closed
tim-thacker-nullify wants to merge 1 commit into
fix/helm-collector-reviewfrom
fix/helm-collector-review-r2
Closed

tim-thacker-nullify wants to merge 1 commit into
fix/helm-collector-reviewfrom
fix/helm-collector-review-r2

Conversation

@tim-thacker-nullify

@tim-thacker-nullify tim-thacker-nullify commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Claude

Round-2 review of #68: the collector still rejected the KMS alias ARN the configure page shows, and the role-arn guard accepted GovCloud and China partitions that cannot reach Nullify's commercial bucket and key.

Merge order: this PR into fix/helm-collector-review (#68), then #68 into #63. The alias grant this relies on ships in connector CloudFormation #64/#69.

  • R2-68-1: collector.kms.keyArn accepts arn:aws:kms:<region>:<12 digits>:alias/<name> and key ARNs including key/mrk-…, using the connector CloudFormation AllowedPattern. The region default and mismatch check take the region from either form.
  • R2-68-1: NOTES prints a warning when an alias is used. IAM matches the key ARN, so the role needs the connector stack version that also grants key/*; the Terraform module grants only the given ARN. README and values.yaml say the same.
  • R2-68-1 CI: ci/eks-kms-alias.yaml and ci/eks-kms-mrk.yaml must render. Must-fail: bare alias/…, a GovCloud alias ARN, and an alias region mismatch.
  • R2-68-2: the role-arn guard accepts arn:aws:iam:: only. serviceAccount.requireRoleArn=false is kept. ci/eks-govcloud-role.yaml is replaced by must-fail role-arn-govcloud.yaml, and the partition mentions are removed from README and values.yaml.

CI: this branch's base has no pull-request Helm trigger, so the chart was validated in a temporary draft PR that merges it onto #65's follow-up; the run is linked in a comment below.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…RN commercial

- collector.kms.keyArn accepts arn:aws:kms:<region>:<account>:alias/<name> and
  key ARNs including multi-Region key/mrk-..., matching the connector
  CloudFormation AllowedPattern. The region default and check take the region
  from either form.
- NOTES warns when an alias is used: IAM matches the key ARN, so the role needs
  the connector stack version that also grants key/*; the Terraform module
  grants only the given ARN.
- The role-arn guard is back to arn:aws:iam:: only; Nullify's bucket and key are
  commercial-partition. serviceAccount.requireRoleArn=false stays the escape
  hatch. ci/eks-govcloud-role.yaml is removed.
- CI: ci/eks-kms-alias.yaml and ci/eks-kms-mrk.yaml must render; a bare alias,
  a GovCloud alias ARN, an alias region mismatch and a GovCloud role ARN must
  fail.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tim-thacker-nullify

Copy link
Copy Markdown
Member Author

Claude

CI for this PR ran in throwaway draft #73 (closed, branch deleted), since this base has no pull-request Helm trigger.

  • Run 34886190776, head b2b04b0 = fix/helm-publish-review-r2 (fix(helm): verify recovered chart assets, tag before deploying and gate on pre-publish hooks #70) merged with this branch at 91786b1, no conflicts. charts, validate (nullify-k8s-collector) and publish-dry-run passed.
  • publish-dry-run lints and renders every ci/*.yaml under set -e, including the new eks-kms-alias.yaml and eks-kms-mrk.yaml.
  • All 12 must-fail cases were rejected with their # expect: text, including kms-bare-alias, kms-govcloud-alias, region-mismatch-alias and role-arn-govcloud.
  • Dry run: kept nullify-k8s-collector 0.2.0, new nullify-k8s-collector 0.4.0.

@tim-thacker-nullify

Copy link
Copy Markdown
Member Author

Claude

Collapsed into #63 at 1740f37 after passing adversarial review; all of this PR's commits are now in #63.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant