DNM: [wip] Use CCO credentials for CCM via CredentialsRequest - #495
DNM: [wip] Use CCO credentials for CCM via CredentialsRequest#495mfbonfigli wants to merge 1 commit into
Conversation
AWS Cloud Controller Manager currently reads credentials from the EC2 instance metadata service (IMDS) via the master node IAM instance role, which is static from install time. This prevents new IAM permissions required by new CCM features from being granted on upgraded clusters (OCPBUGS-98763). This commit migrates CCM to the Cloud Credential Operator (CCO) model: - Add a CredentialsRequest manifest for CCM with the scoped set of EC2 and ELB permissions it actually needs, including elasticloadbalancing:SetSecurityGroups for BYO Security Group NLB support and elasticloadbalancing:SetIpAddressType for dual-stack NLBs. In Mint mode CCO mints a dedicated IAM user; in Manual+STS mode CCO writes role_arn + web_identity_token_file for IRSA. - Mount the resulting credentials secret and a projected ServiceAccount token (audience: sts.amazonaws.com) into the CCM Deployment, and point the AWS SDK at the credentials file via AWS_SHARED_CREDENTIALS_FILE. - Add unit test assertions verifying the new volumes, mounts, and env var are present in the rendered Deployment. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds an AWS ChangesAWS credential integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test all |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cloud/aws/aws_test.go (1)
72-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert each volume’s source configuration, not only its name.
These assertions pass if
aws-credentialspoints to the wrong Secret or ifbound-sa-tokenhas the wrong token path/audience, breaking the CredentialsRequest-to-pod contract.Suggested test expansion
- volumeNames := make([]string, 0, len(deploy.Spec.Template.Spec.Volumes)) + volumes := make(map[string]corev1.Volume, len(deploy.Spec.Template.Spec.Volumes)) for _, v := range deploy.Spec.Template.Spec.Volumes { - volumeNames = append(volumeNames, v.Name) + volumes[v.Name] = v } - assert.Contains(t, volumeNames, "aws-credentials") - assert.Contains(t, volumeNames, "bound-sa-token") + + credentials := volumes["aws-credentials"] + require.NotNil(t, credentials.Secret) + assert.Equal(t, "cloud-controller-manager-credentials", credentials.Secret.SecretName) + + token := volumes["bound-sa-token"] + require.NotNil(t, token.Projected) + require.Len(t, token.Projected.Sources, 1) + tokenSource := token.Projected.Sources[0].ServiceAccountToken + require.NotNil(t, tokenSource) + assert.Equal(t, "token", tokenSource.Path) + assert.Equal(t, "sts.amazonaws.com", tokenSource.Audience)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cloud/aws/aws_test.go` around lines 72 - 77, Strengthen the volume assertions in the deployment test by locating the volumes named “aws-credentials” and “bound-sa-token” and validating their source configurations. Assert that aws-credentials references the expected Secret, and that bound-sa-token uses the required token path and audience, rather than checking names alone.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/cloud/aws/aws_test.go`:
- Around line 72-77: Strengthen the volume assertions in the deployment test by
locating the volumes named “aws-credentials” and “bound-sa-token” and validating
their source configurations. Assert that aws-credentials references the expected
Secret, and that bound-sa-token uses the required token path and audience,
rather than checking names alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4b8ddaa-223c-42c5-9ae1-e05b43e45dde
📒 Files selected for processing (3)
manifests/0000_26_cloud-controller-manager-operator_19_credentialsrequest-aws.yamlpkg/cloud/aws/assets/deployment.yamlpkg/cloud/aws/aws_test.go
This is a pre-existing configuration left untouched by this change.
|
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-ccm periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade |
|
@mfbonfigli: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/32a05960-8a8c-11f1-815a-380111e6c227-0 |
| - name: AWS_SHARED_CREDENTIALS_FILE | ||
| value: /etc/aws-credentials/credentials |
There was a problem hiding this comment.
This variable is read automatically by the AWS SDK and takes precedence over master node IAM credentials retrieved via IMDS.
| - name: bound-sa-token | ||
| mountPath: /var/run/secrets/openshift/serviceaccount | ||
| readOnly: true | ||
| hostNetwork: true |
There was a problem hiding this comment.
I left this one but maybe can it be dropped? Not sure if CCM needs the hostNetwork for something other than IMDS calls which should become obsolete with this PR. Asking for feedback on this one.
|
@mfbonfigli Are you able to demonstrate through an installer PR what the impact of this would be on the node roles? Have you considered what the migration path looks like for existing clusters at all? |
|
@JoelSpeed Working on the installer PR. I tested a IPI Mint and Manual+STS mode installs on a local cluster and both seemed to work just fine, but will try to confirm via e2e jobs with a joint PR. Regarding the migration path, I suppose it's seamless on upgrades, as once CCO deploys credentials for CCM, this should just use them automatically over the instance ones. The part to clarify would be how to, after, remove now unnecessary old permissions from the master instance roles and most likely the answer is to ask the cluster admin to do that manually. |
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-ccm periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade |
|
@mfbonfigli: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/cbf8ff60-8b25-11f1-9ec0-d93ed0dd6ed0-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn #495 openshift/installer#10727 |
|
@mfbonfigli: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/601f4540-8b2c-11f1-9730-6cf5c066fa5a-0 |
|
@mfbonfigli: This PR was included in a payload test run from #495
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/601f4540-8b2c-11f1-9730-6cf5c066fa5a-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade #495 openshift/installer#10727 |
|
@mfbonfigli: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/71101fa0-8b2c-11f1-9169-e7bbc2e6a60b-0 |
|
@mfbonfigli: This PR was included in a payload test run from #495
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/71101fa0-8b2c-11f1-9169-e7bbc2e6a60b-0 |
2 participants
Do not merge - WIP
AWS Cloud Controller Manager currently reads credentials from the EC2 instance metadata service (IMDS) via the master node IAM instance role, which is static from install time. This prevents new IAM permissions required by new CCM features from being granted on upgraded clusters (OCPBUGS-98763).
This commit migrates CCM to the Cloud Credential Operator (CCO) model:
Add a CredentialsRequest manifest for CCM with the scoped set of EC2 and ELB permissions it actually needs, including elasticloadbalancing:SetSecurityGroups for BYO Security Group NLB support and elasticloadbalancing:SetIpAddressType for dual-stack NLBs. In Mint mode CCO mints a dedicated IAM user; in Manual+STS mode CCO writes role_arn + web_identity_token_file for IRSA.
Mount the resulting credentials secret and a projected ServiceAccount token (audience: sts.amazonaws.com) into the CCM Deployment, and point the AWS SDK at the credentials file via AWS_SHARED_CREDENTIALS_FILE.
Add unit test assertions verifying the new volumes, mounts, and env var are present in the rendered Deployment.
Summary by CodeRabbit
New Features
Tests