ROSAENG-2066: Add osdctl account aws-creds command#913
Open
nephomaniac wants to merge 1 commit into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@nephomaniac: This pull request references ROSAENG-2066 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 story 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. |
WalkthroughAdds an aws-creds CLI with snapshot and rotate subcommands, a diagnostic/reporting library, rotation controller refactors for interactive key creation/retry and credential-secret deletion polling, extensive tests, and documentation updates. ChangesAWS Credential Diagnostics and Rotation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labelslgtm, approved Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nephomaniac 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 |
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@cmd/account/aws-creds-rotate.go`:
- Around line 60-63: The code currently discards Cobra's cancellation context by
creating context.TODO() in callers; update the signatures of runRotate and
runRefreshSecrets to accept a context.Context parameter (e.g., func
runRotate(ctx context.Context, ops *... ) error), replace any use of
context.TODO() with the passed ctx (cmd.Context()) at each call site, and thread
that ctx through into DeleteCredentialSecrets and any AWS/Kubernetes or polling
functions so they all honor cancellation/timeouts; ensure callers in the Cobra
Run function call runRotate(cmd.Context(), ops) and propagate ctx through
intermediate helpers.
In `@cmd/account/aws-creds-snapshot.go`:
- Around line 45-48: The snapshot flow currently uses context.TODO() and needs a
cancelable timed context: obtain cmd.Context(), wrap it with context.WithTimeout
(choose a reasonable timeout such as 30s) and defer cancel; pass that ctx into
runSnapshot instead of using TODO, and propagate it into all backend calls by
updating signatures and call sites for runSnapshot, identifyCluster, any
resolve* functions and Diagnose* functions so they accept context.Context and
use it for OCM/Hive/managed-cluster calls; ensure every function that performs
network/backend operations takes and forwards the ctx to enable cancellation and
timeouts.
In `@cmd/account/aws-creds.go`:
- Around line 661-668: After fetching the Account CR with GetAWSAccount (where
rc.account and rc.accountID are set), add the same ManualSTSMode guard used
earlier (the check around ManualSTSMode/DiganoseCRSecrets path) to reject
STS-mode accounts for the --cr-secrets flow: inspect account.Spec.Mode (the
ManualSTSMode enum/constant) and return an error if it equals the ManualSTSMode
value so that DiagnoseCRSecrets is not run for STS clusters and the misleading
missing-claim-secret warning/report is avoided.
In `@pkg/controller/awscreds.go`:
- Around line 327-331: The remediation command in the appended Finding is
outdated; update the Guidance string in the report.Findings append so it uses
the new aws-creds rotate subcommand. Replace the current guidance
fmt.Sprintf("... aws-creds %s --reason <ticket> --rotate-managed-admin",
report.ClusterID) with the rotate form, e.g. fmt.Sprintf("Credentials are out of
sync. Rotate to fix:\n osdctl account aws-creds rotate --name %s --reason
<ticket> --rotate-managed-admin", report.ClusterID), referencing the same
report.ClusterID in the fmt.Sprintf call used in the report.Findings append.
- Around line 983-1021: The current success message uses optimistic defaults
(rotationPermsOK and credReqPermsOK initialized true) and can report [OK] when
no permissions were evaluated; update the success gating to require actual
evaluation: either (A) use report.AllPermissionsOK instead of rotationPermsOK &&
credReqPermsOK, or (B) add explicit evaluation flags (e.g., rotationEvaluated
and credReqEvaluated set to true inside the switch when encountering "rotation"
and "credreq") and change the success condition to rotationPermsOK &&
credReqPermsOK && rotationEvaluated && credReqEvaluated; also ensure the failure
branches still report missing categories when evaluated false.
- Around line 372-420: The code currently uses Secret.CreationTimestamp to mark
a CredentialRequest secret stale; instead, find the active managed admin key ID
from report.Keys (the same place you already derive the Hive key) and compare
that key ID to the CR secret's aws_access_key_id value. Replace the age-based
logic around managedAdminKeyAge and the block that sets crs.Age/NeedsRecreation
with: extract the active key ID into a variable (e.g. managedAdminKeyID) from
report.Keys (the entry with HiveMatch && UserName != "osdCcsAdmin"), read
secret.Data["aws_access_key_id"] (convert to string), and set
crs.NeedsRecreation = true when they differ (set Exists/Err fields unchanged);
apply the same replacement for the other occurrence mentioned (lines ~728-779)
so both paths compare access key IDs instead of creation times.
In `@pkg/controller/rotatesecret.go`:
- Around line 88-96: The controller struct exposes Out but lacks an In reader,
so createAccessKeyWithRetry() uses fmt.Scanln (process stdin) and ignores
injected IOStreams.In; add an In io.Reader field to the controller (alongside
Report, Log, Out) and update createAccessKeyWithRetry() to read prompts from
that In reader (e.g., using fmt.Fscanln or bufio.Reader against the
controller.In) instead of calling fmt.Scanln so automated tests and injected CLI
IOStreams work correctly.
- Around line 131-135: The managed-admin rotation path is re-running
VerifyRotationPermissions via resolveAdminUsername and blocks the operator's
strict-YES override; fix by threading the precomputed permission simulation
result (or an explicit "operator confirmed despite denied simulation" boolean)
into RotateSecret and resolveAdminUsername so you don't re-run the simulation.
Update the RotateSecret signature to accept the precomputed report/confirmation
flag and change callers (including where adminUsername is resolved) to pass that
value, then alter resolveAdminUsername (or add a wrapper) to skip calling
VerifyRotationPermissions when the confirmation flag/report indicates the
operator already overrode the denial. Ensure references to resolveAdminUsername,
RotateSecret, adminUsername, input.AwsClient and accountIDSuffixLabel are used
to locate and update the logic.
- Around line 235-243: The deferred cleanup currently ignores the return value
of input.AwsClient.DeleteAccessKey (inside the deferred closure that checks
rotationCommitted and uses adminUsername/newKeyID), which can leave an orphaned
key without surfacing the cleanup failure; update that defer to capture the
DeleteAccessKey error, log it at error level with context (including
adminUsername and newKeyID) and propagate it so the caller/operator is notified
— e.g., record the cleanup error into the function's named error return (or
wrap/append it to the primary error) rather than discarding it, ensuring
DeleteAccessKey's error is never ignored.
🪄 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: Enterprise
Run ID: a88be861-94fb-4999-b132-7907e53cd407
📒 Files selected for processing (15)
cmd/account/aws-creds-rotate.gocmd/account/aws-creds-snapshot.gocmd/account/aws-creds.gocmd/account/cmd.gocmd/account/rotate-secret.godocs/README.mddocs/osdctl_account.mddocs/osdctl_account_aws-creds.mddocs/osdctl_account_aws-creds_rotate.mddocs/osdctl_account_aws-creds_snapshot.mddocs/osdctl_account_rotate-secret.mdpkg/controller/awscreds.gopkg/controller/awscreds_test.gopkg/controller/rotatesecret.gopkg/controller/rotatesecret_test.go
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
from
3ec12df to
0d45d32
Compare
Contributor
Author
|
@coderabbitai resolve |
1 similar comment
Contributor
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
1 similar comment
✅ Action performedComments resolved. Approval is disabled; enable |
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
from
0d45d32 to
809f194
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/controller/rotatesecret.go`:
- Around line 792-796: The confirmation prompt currently calls
utils.ConfirmPrompt() which reads from os.Stdin and ignores the injected
RotateSecretInput.In; change ConfirmPrompt (or add a new helper like
ConfirmPromptWithIO) to accept an io.Reader (and optionally an io.Writer) and
read the response from that reader (use bufio.Reader, TrimSpace, check first
rune for 'y'/'Y' and treat other input/EOF as false). Then update the call site
in RotateSecret (the fmt.Fprintf prompt and the ConfirmPrompt call) to pass
RotateSecretInput.In (and RotateSecretInput.Out) so tests/automation can inject
input; ensure the function returns false on EOF or non-affirmative input.
🪄 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: Enterprise
Run ID: ee68c51a-fcba-4cfd-b1b3-ef00378cd2e6
📒 Files selected for processing (15)
cmd/account/aws-creds-rotate.gocmd/account/aws-creds-snapshot.gocmd/account/aws-creds.gocmd/account/cmd.gocmd/account/rotate-secret.godocs/README.mddocs/osdctl_account.mddocs/osdctl_account_aws-creds.mddocs/osdctl_account_aws-creds_rotate.mddocs/osdctl_account_aws-creds_snapshot.mddocs/osdctl_account_rotate-secret.mdpkg/controller/awscreds.gopkg/controller/awscreds_test.gopkg/controller/rotatesecret.gopkg/controller/rotatesecret_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/osdctl_account.md
- docs/osdctl_account_rotate-secret.md
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/account/rotate-secret.go
- cmd/account/aws-creds-snapshot.go
- pkg/controller/awscreds_test.go
- cmd/account/aws-creds-rotate.go
- cmd/account/aws-creds.go
- pkg/controller/awscreds.go
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
from
809f194 to
c899f9c
Compare
Contributor
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
Contributor
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
from
c899f9c to
fe26067
Compare
Contributor
|
@nephomaniac: 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. |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
bergmannf
reviewed
Jun 9, 2026
bergmannf
left a comment
bergmannf
left a comment
Contributor
There was a problem hiding this comment.
Good direction - I think the main issue for me that we don't want to keep an outdated command around and just remove it completely.
Also seems there is some duplication between the two controllers which should be removed.
| without rotating AWS keys or modifying Hive secrets. This is useful when | ||
| CCO needs to re-provision secrets with existing credentials. | ||
|
|
||
| AWS credentials are obtained via backplane by default. Use --aws-profile |
Contributor
There was a problem hiding this comment.
Suggested change
| AWS credentials are obtained via backplane by default. Use --aws-profile | |
| AWS credentials are obtained via backplane by default. Use --aws-profile |
I think the local profiles aren't really used anymore with rh-aws-saml-login.
So the command also has to work without profile and without backplane (which would be the call after logging in via rh-aws-saml-login).
Comment on lines
+205
to
+208
| if !utils.ConfirmPrompt() { | ||
| o.log.Info("Refresh cancelled by user") | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| if !utils.ConfirmPrompt() { | |
| o.log.Info("Refresh cancelled by user") | |
| return nil | |
| } | |
| if !utils.ConfirmPrompt() { | |
| o.log.Info("Refresh cancelled by user") | |
| return nil | |
| } |
Should we really give this option at all instead of stopping immediately?
Any issue will likely result in one or multiple operators on the cluster breaking after the rotation - maybe we should instead call out opening a proactive case / responding in-case with the problems first?
| } | ||
|
|
||
| var ( | ||
| jiraTicketPattern = regexp.MustCompile(`[A-Z]+-\d+`) |
Contributor
There was a problem hiding this comment.
Suggested change
| jiraTicketPattern = regexp.MustCompile(`[A-Z]+-\d+`) | |
| jiraTicketPattern = regexp.MustCompile(`[A-Z]+-\d+`) |
Not a big issue, but this might report some false positives by accident.
"Rotating cluster D-1" would match this one but not be a ticket - I think we could just match for OHSS-*.
Comment on lines
+36
to
+41
| NOTE: Consider using 'osdctl account aws-creds' instead. The newer command: | ||
| - Accepts a cluster ID (no need to look up the account CR name) | ||
| - Provides a diagnostic snapshot before rotation | ||
| - Handles account claim resolution automatically | ||
| - Supports interactive key management and CR secret health verification | ||
| - Validates IAM permissions via SimulatePrincipalPolicy |
Contributor
There was a problem hiding this comment.
Suggested change
| NOTE: Consider using 'osdctl account aws-creds' instead. The newer command: | |
| - Accepts a cluster ID (no need to look up the account CR name) | |
| - Provides a diagnostic snapshot before rotation | |
| - Handles account claim resolution automatically | |
| - Supports interactive key management and CR secret health verification | |
| - Validates IAM permissions via SimulatePrincipalPolicy | |
| NOTE: Consider using 'osdctl account aws-creds' instead. The newer command: | |
| - Accepts a cluster ID (no need to look up the account CR name) | |
| - Provides a diagnostic snapshot before rotation | |
| - Handles account claim resolution automatically | |
| - Supports interactive key management and CR secret health verification | |
| - Validates IAM permissions via SimulatePrincipalPolicy |
The new command is a complete superset - I think it would be better to just remove this one and ensure that only the new command is referenced in any SOPs that might use it.
Keeping these old commands tends to be something that is forgotten to be removed and there seems to be no reason to keep a worse version around.
| @@ -0,0 +1,1171 @@ | |||
| package controller | |||
Contributor
There was a problem hiding this comment.
Suggested change
| package controller | |
| package controller |
Thanks for sticking with the controller having (most) logic - it's something we try to move osdctl to, so we don't have all logic in the cobra commands.
|
|
||
| // isAWSCredReqForDiag is a broader filter for diagnostics — includes all | ||
| // CredentialRequests with AWSProviderSpec regardless of name prefix. | ||
| func isAWSCredReqForDiag(cr *ccov1.CredentialsRequest) bool { |
Contributor
There was a problem hiding this comment.
Suggested change
| func isAWSCredReqForDiag(cr *ccov1.CredentialsRequest) bool { | |
| func isAWSCredReqForDiag(cr *ccov1.CredentialsRequest) bool { |
I think the function name can be 'isAWSCredentialRequestForDiagnostic' - the current one sounds a bit C-like.
Comment on lines
+499
to
+510
| rotationActions := []string{ | ||
| "iam:CreateAccessKey", | ||
| "iam:CreateUser", | ||
| "iam:DeleteAccessKey", | ||
| "iam:DeleteUser", | ||
| "iam:DeleteUserPolicy", | ||
| "iam:GetUser", | ||
| "iam:GetUserPolicy", | ||
| "iam:ListAccessKeys", | ||
| "iam:PutUserPolicy", | ||
| "iam:TagUser", | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| rotationActions := []string{ | |
| "iam:CreateAccessKey", | |
| "iam:CreateUser", | |
| "iam:DeleteAccessKey", | |
| "iam:DeleteUser", | |
| "iam:DeleteUserPolicy", | |
| "iam:GetUser", | |
| "iam:GetUserPolicy", | |
| "iam:ListAccessKeys", | |
| "iam:PutUserPolicy", | |
| "iam:TagUser", | |
| } | |
| rotationActions := []string{ | |
| "iam:CreateAccessKey", | |
| "iam:CreateUser", | |
| "iam:DeleteAccessKey", | |
| "iam:DeleteUser", | |
| "iam:DeleteUserPolicy", | |
| "iam:GetUser", | |
| "iam:GetUserPolicy", | |
| "iam:ListAccessKeys", | |
| "iam:PutUserPolicy", | |
| "iam:TagUser", | |
| } |
Duplicating this list between this controller and the older rotate-secret one.
Let's extract this to a shared const so we can't accidentally forgot to update one occurrence if ever needed.
| } | ||
|
|
||
| // simulateActions runs SimulatePrincipalPolicy for a set of IAM actions and records allow/deny results in the report. | ||
| func simulateActions(awsClient awsprovider.Client, policySourceArn string, actions []string, category string, actionToCRs map[string][]string, report *DiagnosticReport) error { |
Contributor
There was a problem hiding this comment.
Suggested change
| func simulateActions(awsClient awsprovider.Client, policySourceArn string, actions []string, category string, actionToCRs map[string][]string, report *DiagnosticReport) error { | |
| func simulateActions(awsClient awsprovider.Client, policySourceArn string, actions []string, category string, actionToCRs map[string][]string, report *DiagnosticReport) error { |
This one should probably replace the function in the other controller (or vice versa):
func VerifyRotationPermissions(out io.Writer, awsClient awsprovider.Client, accountID string, username string) error {
No need to have two functions doing the same thing really.
| fmt.Fprintf(out, " CR secret staleness detection will be skipped.\n\n") | ||
| } else { | ||
| if !accountSecret.CreationTimestamp.IsZero() { | ||
| lastRotationAge = time.Since(accountSecret.CreationTimestamp.Time) |
Contributor
There was a problem hiding this comment.
Suggested change
| lastRotationAge = time.Since(accountSecret.CreationTimestamp.Time) | |
| lastRotationAge = time.Since(accountSecret.CreationTimestamp.Time) |
The secrets are/were update with updateSecret which would not update the CreationTimestamp so I think this will not work as intended.
An update operation will not update creationTimestamp.
|
|
||
| // renderKeySelectionTable prints an interactive numbered table of IAM keys with | ||
| // Hive secret references, highlighting the recommended deletion candidate. | ||
| func renderKeySelectionTable(keys []KeyStatus, report *DiagnosticReport, out io.Writer) { |
Contributor
There was a problem hiding this comment.
I think a useful piece of information to decide which key to remove is generally seeing when it was last used ( https://docs.aws.amazon.com/cli/latest/reference/iam/get-access-key-last-used.html)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is a continuation of the work originally started in #601 (July 2024) to port the aws-creds-rotate.sh bash script into osdctl. Since then, #828 added multi-environment OCM/backplane support (
--hive-ocm-url), and #878 extracted SimulatePrincipalPolicy,--dry-run,--cluster-id, and SyncSet-based credential syncing intopkg/controller/rotatesecret.go. This PR refactors the original #601 work to incorporate the updates from #828 and #878, adding a diagnostic-firstaws-credscommand group with additional robustness and UX improvements.Note that #878 already addresses the core requirements of ROSAENG-2066 and ROSAENG-2067. Reviewers should assess whether the enhancements below justify the added scope.
References: ROSAENG-2066, ROSAENG-2067
Built on: #601 (original), #828 (multi-env OCM), #878 (SREP-726, SimulatePrincipalPolicy)
What this PR adds beyond #878
Adds
osdctl account aws-credswith two subcommands:snapshot— read-only diagnostic report of AWS IAM credentials, Hive secrets, CredentialRequests, and IAM permission simulationrotate— credential rotation for osdManagedAdmin and/or osdCcsAdmin with pre-flight checks, interactive key management, sequential CR secret recreation with CCO health verification, and post-rotation validationThe existing
rotate-secretcommand remains functional with a runtime warning pointing users toaws-creds.rotate-secret(post-#878)aws-creds(this PR)--cluster-id)-C)--aws-profile)Change Scope
aws-creds.go,aws-creds-rotate.go,aws-creds-snapshot.go,awscreds.go,awscreds_test.go)cmd.go,rotate-secret.go,rotatesecret.go,rotatesecret_test.go)aws-creds.md,aws-creds_rotate.md,aws-creds_snapshot.md)README.md,osdctl_account.md,rotate-secret.md)Usage
Example Output
osdctl account aws-creds snapshotosdctl account aws-creds rotate --dry-runosdctl account aws-creds rotate --managed-admin --ccs-adminTest plan
make lint— 0 issuesmake format— cleango build— compiles--hive-ocm-url prod🤖 Generated with Claude Code