ROSAENG-2066: Add osdctl account aws-creds command#913
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an aws-creds CLI with snapshot and rotate subcommands, a DiagnosticReport 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 |
|
[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 |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@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
|
@coderabbitai resolve |
1 similar comment
|
@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
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
from
c899f9c to
fe26067
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
bergmannf
left a comment
bergmannf
left a comment
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 |
There was a problem hiding this comment.
| 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).
There was a problem hiding this comment.
✅ Addressed — added --aws-use-env flag that reads AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN from environment variables directly (bypasses backplane and the proxy-wrapped default credential chain). Help text now includes the full kinit → rh-aws-saml-login --output env → osdctl --aws-use-env workflow.
| if !utils.ConfirmPrompt() { | ||
| o.log.Info("Refresh cancelled by user") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
| 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?
There was a problem hiding this comment.
✅ Addressed — permission failures now hard-stop rotation by default. --force unlocks a strict YES confirmation prompt. Useful when SCP restrictions report false positives, during staging testing with non-production IAM configs, or when investigating known permission bugs.
| } | ||
|
|
||
| var ( | ||
| jiraTicketPattern = regexp.MustCompile(`[A-Z]+-\d+`) |
There was a problem hiding this comment.
| 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-*.
There was a problem hiding this comment.
✅ Narrowed to known project prefixes: (OHSS|SREP|ROSAENG|OSD|SDE|CSSRE|MNGT)-\d+.
| 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 |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
✅ Now uses Cobra's Deprecated field — hidden from --help, prints deprecation warning when invoked, points to aws-creds rotate. Hard removal in a follow-up PR after SOP migration.
| @@ -0,0 +1,1171 @@ | |||
| package controller | |||
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
Thanks! All diagnostic and rotation logic stays in pkg/controller/. The CLI layer in cmd/account/ only handles flag parsing, cluster resolution, and client setup.
|
|
||
| // isAWSCredReqForDiag is a broader filter for diagnostics — includes all | ||
| // CredentialRequests with AWSProviderSpec regardless of name prefix. | ||
| func isAWSCredReqForDiag(cr *ccov1.CredentialsRequest) bool { |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
✅ Renamed to isAWSCredentialRequestForDiagnostic.
| rotationActions := []string{ | ||
| "iam:CreateAccessKey", | ||
| "iam:CreateUser", | ||
| "iam:DeleteAccessKey", | ||
| "iam:DeleteUser", | ||
| "iam:DeleteUserPolicy", | ||
| "iam:GetUser", | ||
| "iam:GetUserPolicy", | ||
| "iam:ListAccessKeys", | ||
| "iam:PutUserPolicy", | ||
| "iam:TagUser", | ||
| } |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
✅ Extracted to RotationRequiredActions package-level var in rotatesecret.go. Referenced by both diagnosePermissions and VerifyRotationPermissions.
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
✅ VerifyRotationPermissions now delegates to simulateActions internally. Single implementation, no duplication.
| fmt.Fprintf(out, " CR secret staleness detection will be skipped.\n\n") | ||
| } else { | ||
| if !accountSecret.CreationTimestamp.IsZero() { | ||
| lastRotationAge = time.Since(accountSecret.CreationTimestamp.Time) |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
✅ Good catch. Staleness detection now reads kube-system/aws-creds on the managed cluster and compares its aws_access_key_id against the Hive account secret. If they match, root credentials are synced and CR secrets are CURRENT. No timestamp comparison. Added 20 unit tests covering all edge cases (missing root secret, read errors, both keys empty, rendering correctness).
|
|
||
| // 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) { |
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)
There was a problem hiding this comment.
✅ Added GetAccessKeyLastUsed to the AWS client interface. LAST USED column shown in both the snapshot credentials table and the interactive key deletion table. The recommended deletion candidate (* marker) considers Hive references and last-used time.
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
from
fe26067 to
05f812f
Compare
Response to review feedback
Agreed. PR has been updated to move
Addressed. The role-chain logic is consolidated into Detailed items addressed:
|
|
Thanks @bergmannf for the thorough review — the staleness detection fix and |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 128-137: When handling o.refreshSecrets after resolveForCRSecrets
and DiagnoseCRSecrets, check the returned report for a missing/unreadable
cluster root credential (e.g., report.ClusterRootKeyID or equivalent field) and
fail closed instead of proceeding: if the root key id is empty or indicates
unreadable/missing state, return an error (or print a clear message) and do not
call controller.RenderCredRequestTable or
runRefreshSecrets/DeleteCredentialSecrets; apply the same guard to the other
refresh-secrets branch that mirrors this logic (the block using
resolveForCRSecrets, DiagnoseCRSecrets, controller.RenderCredRequestTable, and
runRefreshSecrets/DeleteCredentialSecrets).
In `@pkg/controller/awscreds.go`:
- Around line 543-550: extractCredReqActions can swallow ManagedClient.List
errors, causing diagnosePermissions to skip CredentialRequest simulation; change
extractCredReqActions (and similarly extractRotationActions) to return
(map[string][]CredentialRequest, error) so listing errors are bubbled up, then
in diagnosePermissions call them, check for non-nil error and either return that
error or set report.AllPermissionsOK = false and return an error; ensure
simulateActions is only skipped when there is a valid action map, and include
references to extractCredReqActions, extractRotationActions,
diagnosePermissions, simulateActions, and ManagedClient.List when making the
change.
In `@pkg/provider/aws/client.go`:
- Line 66: The GetAccessKeyLastUsed method currently discards context and calls
the SDK with context.TODO(); change the method signature on the client interface
and AwsClient implementation to accept a context.Context (i.e.,
GetAccessKeyLastUsed(ctx context.Context, input *iam.GetAccessKeyLastUsedInput)
(*iam.GetAccessKeyLastUsedOutput, error)), update AwsClient.GetAccessKeyLastUsed
to pass that ctx through to c.iamClient.GetAccessKeyLastUsed(ctx, input), and
update any callers (e.g., pkg/controller/awscreds.go) to supply and propagate
the caller's context when invoking AwsClient.GetAccessKeyLastUsed so
cancellations/timeouts are honored.
🪄 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: fb7e59fb-0c66-46e3-ab01-02c2cd7eaaff
📒 Files selected for processing (17)
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.gopkg/provider/aws/client.gopkg/provider/aws/mock/client.go
✅ Files skipped from review due to trivial changes (1)
- docs/osdctl_account_rotate-secret.md
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/account/aws-creds-snapshot.go
- docs/osdctl_account.md
- pkg/controller/rotatesecret_test.go
- cmd/account/aws-creds.go
- pkg/controller/rotatesecret.go
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
5 times, most recently
from
ee34e8d to
d69a726
Compare
|
Separately, ^ Claude Chris, the human: I think that's right, if I follow along through that path (the nil pointer bits) |
|
The managed-admin path (lines 272-286) has a The full key ID is not printed — ^ Also Claude |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nephomaniac
force-pushed
the
ROSAENG-2066-aws-creds-diagnostic
branch
from
d69a726 to
7850b58
Compare
|
All three fixed in 7850b58 — thanks Chris (and Claude) for the thorough review.
|
|
@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. |
4 participants
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)
Related: openshift/ops-sop#4023 (SOP update, on hold until this PR merges)
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
Summary by CodeRabbit
osdctl account aws-credswithrotate(interactive/full or refresh-only, dry-run, force) andsnapshot(read-only diagnostics, optional CR-only output).osdctl account rotate-secretmarked deprecated; useosdctl account aws-creds rotate.aws-creds,aws-creds rotate, andaws-creds snapshotwith examples and options.