Add enhancement: use CCO for AWS CCM credentials - #2073
Conversation
WalkthroughThe proposal defines migration of AWS Cloud Controller Manager authentication from the master node IMDS role to CCO-managed credentials. It covers credential modes, secret and token mounting, IAM permissions, lifecycle behavior, testing, graduation criteria, and support verification. ChangesAWS CCM CCO credential migration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md (1)
241-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every supported CCO mode in the test plan.
The table covers Mint and Manual+STS, but it omits Passthrough and Manual non-STS even though both are listed as supported goals and GA criteria.
Add explicit tests for both modes. Assert that the
CredentialsRequesttarget secret,data.credentials,cloudTokenPath, and Deployment mounts use the same contract.🤖 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 `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md` around lines 241 - 248, Expand the e2e test plan table to include explicit coverage for Passthrough and Manual non-STS CCO modes. For both modes, add assertions covering the CredentialsRequest target secret, data.credentials, cloudTokenPath, and Deployment mounts, matching the existing contract used by the other supported modes.
🤖 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.
Inline comments:
In `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md`:
- Around line 287-289: Update the “Downgrade” guidance to account for new
installs whose master IAM role contains only ec2:Describe* permissions. Require
restoring the legacy node and load-balancer permissions before removing CCM
credentials, preserving a compatible credential path during downgrade, or
explicitly blocking this unsupported downgrade scenario and documenting the
prerequisite.
- Around line 148-150: Update the AWS CCM credentials proposal to scope the
least-privilege claim to new installations unless upgraded clusters include a
supported procedure for removing the legacy master IAM role. Clarify that
retaining the original role leaves its broad node-level access available to
other processes, and revise the “Harmless” risk classification to reflect this
upgrade limitation.
- Around line 100-110: Update the Manual+STS credential-loading guidance to
enable AWS shared-config loading via the required AWS_SDK_LOAD_CONFIG
environment variable or equivalent SDK session option, or instead configure the
role_arn and web_identity_token_file through environment-specific provider
values. Revise the “no binary changes” claim to accurately reflect the selected
approach.
- Around line 78-81: Update the CCM deployment guidance to explicitly disable
the EC2 metadata provider by setting AWS_EC2_METADATA_DISABLED=true in the CCM
container alongside AWS_SHARED_CREDENTIALS_FILE. Remove the claim that the
shared credentials file prevents IMDS fallback, and state that mounted
credentials must be unavailable rather than falling back to IMDS.
---
Nitpick comments:
In `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md`:
- Around line 241-248: Expand the e2e test plan table to include explicit
coverage for Passthrough and Manual non-STS CCO modes. For both modes, add
assertions covering the CredentialsRequest target secret, data.credentials,
cloudTokenPath, and Deployment mounts, matching the existing contract used by
the other supported modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9215a97-3a55-4f4f-ada7-3b992dd314fe
📒 Files selected for processing (1)
enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md
b98429c to
156524c
Compare
|
[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.
Actionable comments posted: 8
🤖 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.
Inline comments:
In `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md`:
- Line 79: Update the Manual (non-STS) prerequisite in the CCO documentation to
require the administrator to pre-create the cloud-controller-manager-credentials
Secret before CCCMO deploys the CCM Deployment, covering both fresh installation
and cluster upgrades.
- Around line 242-251: Expand the documented test matrix to explicitly cover
Passthrough and Manual credential modes in addition to Mint and Manual+STS. For
each mode, include tests for Secret creation and format, missing-Secret
behavior, policy updates, and CCM AWS operations, updating the unit/e2e coverage
references in the affected testing sections.
- Around line 93-95: Update the missing-Secret behavior described near the
CredentialsRequest permissions and the repeated risk statement near the later
risk section: state that the Pod remains pending because kubelet reports
FailedMount events when the Secret is absent, rather than claiming the CCM
container enters CrashLoopBackOff. Ensure both occurrences use the same accurate
behavior.
- Around line 329-334: Update the IAM policy verification command for the master
role to inspect every policy statement and all associated actions, rather than
only PolicyDocument.Statement[0].Action. Ensure the resulting check can detect
any additional statement or broader permission before reporting that the role is
limited to ec2:Describe*.
- Around line 81-84: Update the AWS credential-chain claims in this document to
state that AWS_SHARED_CREDENTIALS_FILE is preferred but does not disable IMDS
fallback when the file is missing, unreadable, or invalid. Revise the related
“unused” and “safe” statements at the referenced sections consistently, and
define a failure mode that preserves CCM’s required non-credential IMDS calls
while preventing unintended role-credential use.
- Around line 321-327: Replace the full Secret decode in the credential
inspection instructions with metadata-only detection: inspect the credential
content for key names to distinguish Mint/Passthrough from Manual+STS, and print
only the detected mode or redacted key metadata. Do not output the decoded
credential values or any complete Secret contents.
- Around line 213-219: Expand the documentation around the pruned master-node
IAM role to include a concrete inventory of master-node components and their AWS
API requirements, covering both the CAPI path and UPI CloudFormation masters.
Demonstrate that non-CCM components require only the installer-generated
ec2:Describe* permission set, and record this evidence before marking the change
ready for GA.
- Around line 74-80: Clarify the Manual+STS credential producer and lifecycle in
the AWS CCM documentation by selecting a single Secret writer and aligning the
workflow description accordingly. Document the exact CredentialsRequest
secretRef and serviceAccountNames, CCM ServiceAccount, projected-token audience
and mount path, and credentials Secret key; add rendered-manifest tests covering
each value and the selected writer’s behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b71dcc3-636d-4c53-9775-ea202785770b
📒 Files selected for processing (1)
enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md
156524c to
57dce5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md (1)
228-228: 🩺 Stability & Availability | 🔵 TrivialGate the cross-repository rollout, not only the merge order.
Merge order does not ensure that a released payload contains both changes. If the installer prunes the master role while CCCMO still deploys CCM without CCO credentials, new clusters can lose CCM access. Add a release-payload compatibility gate and test the unsupported mixed-version combination.
🤖 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 `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md` at line 228, Update the rollout guidance in the installer/CCCMO compatibility section to require a release-payload compatibility gate, ensuring the installer’s master-role pruning and CCCMO’s CCO credential deployment are present in the same released payload rather than relying only on merge order. Add coverage for the unsupported mixed-version combination where pruning is enabled but CCMO lacks CCO credentials, and block that rollout.
🤖 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.
Inline comments:
In `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md`:
- Around line 323-329: Update the credential-mode detection command in the AWS
CCM credentials documentation to ignore the decoded file’s [default] header,
filter for the allowed credential key names, and print only the matching names.
Preserve the existing behavior of not exposing credential values and retain the
distinction between the Mint/Passthrough and Manual+STS key sets.
---
Nitpick comments:
In `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md`:
- Line 228: Update the rollout guidance in the installer/CCCMO compatibility
section to require a release-payload compatibility gate, ensuring the
installer’s master-role pruning and CCCMO’s CCO credential deployment are
present in the same released payload rather than relying only on merge order.
Add coverage for the unsupported mixed-version combination where pruning is
enabled but CCMO lacks CCO credentials, and block that rollout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d930b60-9309-4d53-bfbb-f0935064ccaf
📒 Files selected for processing (1)
enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md
57dce5f to
4d81200
Compare
mtulio
left a comment
mtulio
left a comment
There was a problem hiding this comment.
Thanks for the solid EP. I have a few questions and suggestions.
It's also worth mentioning if cloud-config RoleARN and SourceARN have been evaluated/considered/drawbacks
| - **Manual (non-STS)**: CCO is a no-op. The admin must pre-create the `cloud-controller-manager-credentials` secret before CCCMO deploys the CCM Deployment, both during installation and upgrades. | ||
| - **Manual+STS**: `ccoctl` (run pre-install) creates an OIDC-federated IAM role and pre-populates the secret with `role_arn` and `web_identity_token_file`. |
There was a problem hiding this comment.
considering the modes are expanded here, and both manual are no-op in this step (3), I'd suggest adding a step before 2 mentioning the required config (manual steps, or even mention the following sections that is well expanding the manual* modes) depending on the credentialsMode mode set on install-config.
There was a problem hiding this comment.
Makes sense, will add in next revision an optional first step mentioning the manual actions for the modes that need that (eg manual/manual+sts etc).
| |---|---| | ||
| | Upgraded cluster's master role still has the old fat permissions | CCM stops using these permissions once the credentials file is in place. Upgrade documentation will instruct the cluster admin to prune the role permissions down to `ec2:Describe*`. | | ||
| | CCM pod remains `Pending` (`FailedMount`) during upgrade while CCO creates the credentials secret | Expected and consistent behavior with all other CCO-managed operators. The pod starts automatically once CCO satisfies the `CredentialsRequest` and the secret becomes available. | | ||
| | Manual+STS admin forgets to run `ccoctl` before upgrading to a release with new permissions | CCM continues operating with the existing permission set. Features requiring the new permission produce `AccessDenied` error. Admin can remediate by fixing the permissions post-upgrade. | |
There was a problem hiding this comment.
CCM continues operating with the existing permission set
Will upgraded CCM Deployment points to the secret that may not exist?
There was a problem hiding this comment.
I will clarify, This line was focusing more on upgrades between releases already using CCO for CCM.
If on first upgrade instead the cluster admin forgot to deploy the IAM changes via ccoctl then the secret could fail to be created and thus to mount and the pod could fail to start. I will make this explicit.
There was a problem hiding this comment.
Tried to clarify in last revision by splitting first upgrade from subsequent upgrades.
| | New install, Mint mode — LB creation, node registration | `e2e-aws-ovn` | | ||
| | New install, Manual+STS — IRSA credentials, CCM operates | `periodic-ci-openshift-cloud-credential-operator-release-5.0-periodics-e2e-aws-manual-oidc` | | ||
| | Upgrade 4.22→5.x, Mint — CCO auto-updates IAM user policy | `periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade` | | ||
| | Hypershift regression | `periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-ccm` | |
There was a problem hiding this comment.
How this job tracs Hypershift regression?
I'd consider those items:
e2e-aws-ovn-conformance-ccmtracks whole CCM tests on hypershift conformance, including unstable (TESTS_SKIPS), so it may not be healthy job reference- installed cluster may be a Hosted Cluster, which isn't impacted by this change
- if intents is to validate regressions in the management cluster, it may need to make sure this job will use the stream image to install mgr - my suspicions based in previous observations those clusters are nested HC, it's worth to validation final goal vs job config
There was a problem hiding this comment.
We are stating that Hypershift won't be affected by the change since CCCMO should not be running on it at all I believe, this job is just to ensure this is actually true, but I agree that it is a bit weak / not necessary probably. I can remove it if you prefer.
| | Upgrade 4.22→5.x, Mint — CCO auto-updates IAM user policy | `periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade` | | ||
| | Hypershift regression | `periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-ccm` | | ||
|
|
||
| **Passthrough and Manual (non-STS) — manual validation:** No existing AWS CI job exercises these two CCO modes for any component. Secret creation/format, missing-Secret behavior, policy updates, and CCM AWS operations for these modes will be validated manually prior to GA. |
There was a problem hiding this comment.
No existing AWS CI job exercises these two CCO modes for any component
is it worth introducing it as those are supported modes/scenarios?
There was a problem hiding this comment.
I mean in general yes it's maybe worth having (if we don't already, but I searched and couldn't find them), but this is a gap of the product as a whole and is not strictly related to this EP.
We could assess during implementation phase on effort / feasibility to implement as part of the work or if to branch this out to a separate workstream, but I would not take adding these tests as explicit goals for this EP to avoid scope creep.
There was a problem hiding this comment.
If there is a gap it's worth captured in Jira, even if it is lowered prioritized in the Epic, QA activities are part of acceptance criteria when evaluating impacted scenarios.
There was a problem hiding this comment.
Sure I will capture the gap in Jira and include it in the decomposition plan as part of the EP implementation.
|
|
||
| No API extensions. N/A. | ||
|
|
||
| ## Support Procedures |
There was a problem hiding this comment.
Is it worth adding KCS for those procedures?
There was a problem hiding this comment.
Maybe, but after the switch CCM will work as any other CCO-backed operator, so there should be noting too special about its configuration. We could maybe mention these in the upgrade documentation? Open to suggestions.
Thanks @mtulio , I went through the feedback and replied. Will work on updating the EP implementing your suggestions. Regarding |
|
|
||
| ## Graduation Criteria | ||
|
|
||
| This change ships directly to GA as it replaces an existing credential mechanism rather than introducing optional new functionality and is not gated by a feature flag. |
There was a problem hiding this comment.
We should introduce this behind a feature gate IMO, it's a substantial change and we have many variations that I worry pre-merge testing this feature isn't sufficient
There was a problem hiding this comment.
Makes sense, I will look into this and include a feature gate in the next revision of the EP.
There was a problem hiding this comment.
I have updated the EP mentioning the need for a feature gate in TechPreview before the change can be promoted to GA.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4d81200 to
4274a07
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md`:
- Line 29: Revise the AWS CCM/CCO proposal claims to limit CCO’s role to AWS
credential retrieval: valid CCO credentials should satisfy credential loading,
while CCM may still access IMDS for instance identity, region, and other
non-credential metadata. Update all related statements, including the claims
that IMDS is bypassed entirely and the master role is unused or ignored, to
reflect this distinction.
- Around line 77-81: Resolve the conflicting Manual+STS Secret ownership across
the generic workflow and the Manual+STS upgrade instructions: choose either CCO
or ccoctl as the sole producer, then consistently state which component creates,
updates, and owns the Secret, including who writes the credentials file. Update
all affected sections, including the referenced upgrade guidance, to remove
contradictory lifecycle statements.
- Around line 149-150: Update the installer master IAM role pruning described
for the CAPI and UPI CloudFormation paths so it is conditional on
AWSCCMCCOCredentials being enabled and the new AWS CCM Deployment being in use.
Preserve the legacy CCM permissions when the gate is disabled or versions are
mixed, and add explicit coverage for both gate-disabled and mixed-version
scenarios.
- Around line 142-143: Update the AWS credential documentation around the
manual+STS flow to preserve cloud-config RoleARN/SourceARN handling: document
the required sts:AssumeRole permission and target-role trust relationship, or
explicitly declare RoleARN unsupported and document its rejection behavior.
Ensure the CredentialsRequest guidance and examples match the selected behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 705979cc-0c9e-4231-ab31-ba971d8fce77
📒 Files selected for processing (1)
enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md
| 4. CCO reconciles it and creates the `cloud-controller-manager-credentials` secret in `openshift-cloud-controller-manager`: | ||
| - **Mint**: CCO mints a scoped IAM user with the declared permissions and puts the key pair into the secret. | ||
| - **Passthrough**: CCO copies the root `aws-creds` into the secret. | ||
| - **Manual (non-STS)**: CCO is a no-op. The admin must pre-create the `cloud-controller-manager-credentials` secret before CCCMO deploys the CCM Deployment, both during installation and upgrades. | ||
| - **Manual+STS**: `ccoctl` (run pre-install) creates an OIDC-federated IAM role and pre-populates the secret with `role_arn` and `web_identity_token_file`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one Manual+STS Secret producer.
The workflow says that CCO creates the Secret, then says that ccoctl pre-populates it. The Manual+STS section also says that CCO writes the credentials file. These statements define conflicting ownership and lifecycle behavior.
Select one producer for Manual+STS and update the generic workflow and upgrade instructions to match. State who creates, updates, and owns the Secret.
Also applies to: 104-114
🤖 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 `@enhancements/cloud-integration/aws/aws-ccm-cco-credentials.md` around lines
77 - 81, Resolve the conflicting Manual+STS Secret ownership across the generic
workflow and the Manual+STS upgrade instructions: choose either CCO or ccoctl as
the sole producer, then consistently state which component creates, updates, and
owns the Secret, including who writes the credentials file. Update all affected
sections, including the referenced upgrade guidance, to remove contradictory
lifecycle statements.
There was a problem hiding this comment.
this is a nit, will clarify on next revision if needed.
There was a problem hiding this comment.
@mfbonfigli, acknowledged. The clarification can be included in the next revision. The finding remains open until the Manual+STS Secret producer and ownership are stated consistently.
You are interacting with an AI system.
|
@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. |
3 participants
Summary
This EP proposes migrating AWS Cloud Controller Manager (CCM) authentication from the EC2 Instance Metadata Service (IMDS) / master node IAM instance role to Cloud Credential Operator (CCO)-managed credentials, using the same
CredentialsRequestmodel already used by other OpenShift cloud components.Motivation
The master node IAM instance role is static and defined at install time: there's no mechanism to extend it during upgrades, so new CCM features that need additional IAM permissions silently fail on upgraded clusters. Addditionally, the role is also accessible to any process on the master node, which violates the principle of least-permissions.
Proposal
This EP proposes the following changes:
CredentialsRequestdeclaring the permissions AWS CCM needs.ec2:Describe*permissions, since CCM no longer depends on it.No CCM binary changes are required as the AWS SDK's default credential chain will automatically detect and consume the CCO credentials via the
AWS_SHARED_CREDENTIALS_FILE(and the STS/IRSAweb_identity_token_fileflow) over the master node IAM role.Summary by CodeRabbit
Documentation