Skip to content

fix(control-room): drop bogus LBC image tag and CR IAM permissions boundary - #357

Merged
stevenolen merged 2 commits into
mainfrom
cluster-fixes-integration
Jul 23, 2026
Merged

fix(control-room): drop bogus LBC image tag and CR IAM permissions boundary#357
stevenolen merged 2 commits into
mainfrom
cluster-fixes-integration

Conversation

@stevenolen

Copy link
Copy Markdown
Contributor

Description

Two related fixes for the control-room cluster step's EKS deploy, both surfaced when running the first Go-based apply on a control room.

Code Flow

aws-load-balancer-controller image tag. WithAwsLbc(version, chartVersion) was always setting values.image.tag from the config default, which was a chart-version string, not a real controller image tag. That produced a non-existent image and ImagePullBackOff. Now:

  • WithAwsLbc only sets values.image.tag when a non-empty explicit image tag is passed; otherwise the key is omitted and the chart supplies its own default image.
  • The cluster step calls WithAwsLbc("", "") (no image-tag pin, latest chart), matching the prior Python behavior.
  • The now-unused aws_lbc_version config field and its default accessor are removed.

Control-room IAM permissions boundary. The step unconditionally set a PositTeamDedicatedAdmin permissions boundary on the control-room EKS cluster/node/IRSA roles. The prior Python control-room path set none, the live roles carry none, and the control-room admin identity cannot call iam:PutRolePermissionsBoundary, so the first Go apply failed with AccessDenied. The step now passes an empty boundary; the builder already guards each role with if boundary != "", so this is a no-op diff against live state.

Regression tests added: TestCRAwsLbcImageTag, TestAWSClusterDeployNoPermissionsBoundary.

Category of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have reviewed my own diff and added inline comments on lines I want reviewers to focus on or that I am uncertain about

…undary

- aws-lbc: omit values.image.tag unless an explicit image tag is given, so
  the chart supplies its own default image (a chart-version string was being
  injected as an image tag, causing ImagePullBackOff). Remove the dead
  aws_lbc_version config field/default.
- control-room EKS: pass an empty IAM permissions boundary, matching Python
  and live state; the CR admin identity cannot call
  iam:PutRolePermissionsBoundary. Builder omits the boundary when empty.
- Add regression tests: TestCRAwsLbcImageTag, TestAWSClusterDeployNoPermissionsBoundary.
# Conflicts:
#	lib/types/controlroom.go
@stevenolen
stevenolen marked this pull request as ready for review July 22, 2026 13:29
@stevenolen
stevenolen requested a review from a team as a code owner July 22, 2026 13:29
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude finished @stevenolen's task in 1m 12s —— View job


Review

  • Read review guidelines
  • Read changed files
  • Analyze diff and verify correctness
  • Post review

Both fixes are correct and well-scoped. The code does what the description says. A few observations:

LBC image tag fix (eks_cluster_cr.go)

The conditional guard is the right approach:

if version != "" {
    values["image"] = pulumi.Map{"tag": pulumi.String(version)}
}

This is a clean, minimal change. The existing test TestCRAwsLbcPolicyAndAttachment still calls WithAwsLbc("v2.7.0", "") — which means the explicit-tag path also has a call site that exercises it correctly.

Permissions boundary fix (cluster.go)

Straightforward: passing "" lets the existing builder guard (if IAMPermissionsBoundary != "") do its job. No new branching, no behavior change against live state.

TestAWSClusterDeployNoPermissionsBoundary

One nit worth flagging: the test accepts roles where permissionsBoundary is present but its .StringValue() is "". In practice the Pulumi mock returns resource.PropertyValue for a role with PermissionsBoundary unset as simply absent (the !ok branch catches it), so the assert.Equalf on line 187 is unlikely to fire for the empty-string case. The test is still correct — it would catch a role that has permissionsBoundary: "arn:..." set — but the comment says "unset/empty" when the builder never writes an empty string, it just omits the field. Minor: no action required.

lib/types/controlroom.go — field and accessor removal

Removing AwsLbcVersion and AWSLbcVersion() is correct since they're now unused. Worth confirming with a quick grep that nothing outside the diff still references AWSLbcVersion or aws_lbc_version — the diff itself doesn't show any remaining callers, and the search confirms the accessor only appeared in the now-replaced cluster_aws.go call site, so this is clean.

Overall

Both fixes are clearly correct, the regression tests are well-targeted, and the PR description accurately reflects the changes. No blocking issues.

@stevenolen
stevenolen enabled auto-merge July 22, 2026 15:24
@stevenolen
stevenolen added this pull request to the merge queue Jul 23, 2026
Merged via the queue into main with commit e863705 Jul 23, 2026
7 checks passed
@stevenolen
stevenolen deleted the cluster-fixes-integration branch July 23, 2026 17:59
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.

2 participants