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
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds an AWS ChangesAWS credential integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CloudControllerManager
participant CredentialsSecret
participant ServiceAccountToken
CloudControllerManager->>CredentialsSecret: read credentials from mounted secret
CloudControllerManager->>ServiceAccountToken: read projected sts.amazonaws.com token
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
| # Dual-stack / IPv6 NLB support | ||
| - elasticloadbalancing:SetIpAddressType |
There was a problem hiding this comment.
note: I need to investigate this. openshift/cloud-provider-aws seems to use this action (reference) but I don't see it defined in the master node role currently, so we are technically adding it here.
There was a problem hiding this comment.
The feature seem to have been added here: https://github.com/openshift/cloud-provider-aws/pull/135/changes however I do not seem to see any e2e exercising it. It might be a genuinely missing permission.
There was a problem hiding this comment.
The PR points to a slack thread where I see this comment:
I'm uncertain whether this is expected behavior or a bug. Should the cluster's IAM role include the elasticloadbalancing:SetIpAddressType permission by default for dual-stack clusters, or is modifying the ipFamilyPolicy of existing services not a supported operation?AFAIK, we only support "creating a new service" for 4.22. We don't "support" modifying the IP families, which is the use case for day-2 migration from single-to-dual-stack and vice version. Such migration will be in the future!
But thanks for bringing it up! We do need to update cloud-credential-operator to add this new permission if we ever want to support such modification.
Conclusion: the permission is genuinely missing but was by design and adding it was considered out of scope.
Hence I can likely drop it from here.
Summary of test runs executed so farCCCMO-only tests (without installer changes)
CCCMO+Installer tests (includes installer changes)These tests include both the changes in this PR and in the companion one for openshift/installer which removes from the master node IAM roles the permissions that are now defined in the credentialRequest introduced by this PR.
I will be retrying the |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn #495 openshift/installer#10727 |
|
@mfbonfigli: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
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>
2d69730 to
dda7a50
Compare
| - ec2:AttachVolume | ||
| - ec2:CreateVolume | ||
| - ec2:DeleteVolume | ||
| - ec2:DetachVolume | ||
| - ec2:ModifyVolume | ||
| - kms:DescribeKey |
There was a problem hiding this comment.
Volume mgmt perms are ported from master node IAM role in latest rev since they were also listed in the upstream AWS CCM docs, but it is likely that these permissions are actually unused by OpenShift CCM. A permission cleanup is out of scope for this PR for now.
|
@mfbonfigli: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/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/1a0849d0-8b63-11f1-9ec9-69f3ae078cb2-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/1a0849d0-8b63-11f1-9ec9-69f3ae078cb2-0 |
|
/test all |
|
The new execution of periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn with both this and the installer PR passed successfully: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/openshift-cluster-cloud-controller-manager-operator-495-openshift-installer-10727-openshift-cluster-cloud-controller-manager-operator-495-ci-5.0-e2e-aws-ovn/2082490268589756416 |
|
/test unit |
|
/test e2e-aws-ovn |
|
@mfbonfigli: all tests passed! 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. |
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