NE-2401: Add support for AWS ISO partitions#284
Conversation
Update IAM policy resource ARNs from `arn:aws:` to `arn:*:` to support all AWS partitions. Extend the `stsIAMRoleARN` CRD validation regex to accept `aws-iso`, `aws-iso-b`, `aws-iso-e`, and `aws-iso-f` partitions in addition to the existing `aws`, `aws-cn`, and `aws-us-gov`. The CRD validation serves as the enforcement point for which partitions are supported, while `arn:*:` in the IAM policy resources ensures permissions work in whichever partition the cluster runs in. Note that the minified IAM policy used in non-STS clusters already collapses all resources to `*`, so the partition-specific ARNs only matter for the non-minified STS policy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@alebedev87: This pull request references NE-2401 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request expands AWS partition support across multiple components. The validation pattern for the 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/credentials_test.go (1)
70-99: Add test coverage for all newly supported ISO partitions.Great addition for
aws-iso. Since support was expanded across four ISO partitions, add table entries foraws-iso-b,aws-iso-e, andaws-iso-fas well to lock in the full contract and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/credentials_test.go` around lines 70 - 99, Add three additional table-driven test cases alongside the existing "nominal sts iso partition" entry to cover the other ISO partitions: "aws-iso-b", "aws-iso-e", and "aws-iso-f". For each new case, mirror the existing test case structure (envVars with ROLEARN set to "arn:aws-iso-<X>:iam::123456789012:role/foo", scheme set to test.Scheme, provisionedSecret with Data["credentials"]=[]byte("okiso"), expectedCredReqName matching the current NamespacedName, compareCredReq function asserting providerSpec.STSIAMRoleARN equals the partition-specific ARN and credReq.Spec.CloudTokenPath equals "/var/run/secrets/openshift/serviceaccount/token", and expectedContents "okiso") so that the tests exercise the same assertions used by compareCredReq, provisionedSecret, expectedCredReqName, and expectedContents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/credentials_test.go`:
- Around line 70-99: Add three additional table-driven test cases alongside the
existing "nominal sts iso partition" entry to cover the other ISO partitions:
"aws-iso-b", "aws-iso-e", and "aws-iso-f". For each new case, mirror the
existing test case structure (envVars with ROLEARN set to
"arn:aws-iso-<X>:iam::123456789012:role/foo", scheme set to test.Scheme,
provisionedSecret with Data["credentials"]=[]byte("okiso"), expectedCredReqName
matching the current NamespacedName, compareCredReq function asserting
providerSpec.STSIAMRoleARN equals the partition-specific ARN and
credReq.Spec.CloudTokenPath equals
"/var/run/secrets/openshift/serviceaccount/token", and expectedContents "okiso")
so that the tests exercise the same assertions used by compareCredReq,
provisionedSecret, expectedCredReqName, and expectedContents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 395fd3f8-f713-4bf5-85ff-f9597a2cfd50
📒 Files selected for processing (12)
api/v1/awsloadbalancercontroller_types.goassets/iam-policy.jsonassets/operator-iam-policy.jsonbundle/manifests/networking.olm.openshift.io_awsloadbalancercontrollers.yamlconfig/crd/bases/networking.olm.openshift.io_awsloadbalancercontrollers.yamlhack/controller/controller-credentials-request.yamlhack/operator-credentials-request.yamlhack/operator-permission-policy.jsonpkg/controllers/awsloadbalancercontroller/iam_policy.gopkg/operator/credentials_test.gopkg/operator/iam_policy.gopkg/utils/resource/update/credentials_request_test.go
|
@alebedev87: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Update IAM policy resource ARNs from
arn:aws:toarn:*:to support all AWS partitions. Extend thestsIAMRoleARNCRD validation regex to acceptaws-iso,aws-iso-b,aws-iso-e, andaws-iso-fpartitions in addition to the existingaws,aws-cn, andaws-us-gov.The CRD validation serves as the enforcement point for which partitions are supported, while
arn:*:in the IAM policy resources ensures permissions work in whichever partition the cluster runs in. Note that the minified IAM policy used in non-STS clusters already collapses all resources to*, so the partition-specific ARNs only matter for the non-minified STS policy.