ROSAENG-2057: add pull-secret update, audit, and validate subcommands#921
Open
nephomaniac wants to merge 1 commit into
Open
ROSAENG-2057: add pull-secret update, audit, and validate subcommands#921nephomaniac wants to merge 1 commit into
nephomaniac wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@nephomaniac: This pull request references ROSAENG-2057 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. |
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
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:
WalkthroughIntroduces ChangesPull-Secret Command Group
Sequence Diagram(s)sequenceDiagram
participant User
participant UpdateCmd as pull-secret update
participant OCM
participant HiveAPI as Hive/Infra Cluster
participant TargetCluster as Target Cluster
User->>UpdateCmd: --cluster-id --reason [--dry-run/--force]
UpdateCmd->>UpdateCmd: validate flags, resolve cluster/owner
UpdateCmd->>OCM: FetchAccessTokenOp
UpdateCmd->>HiveAPI: CheckCanI (RBAC pre-flight)
UpdateCmd->>TargetCluster: CheckCanI (RBAC pre-flight)
UpdateCmd->>TargetCluster: PreflightCheck (openshift-config/pull-secret)
alt live mode
UpdateCmd->>HiveAPI: read Hive pull-secret
UpdateCmd->>TargetCluster: read target pull-secret
UpdateCmd->>UpdateCmd: CompareThreeWay, render results
UpdateCmd->>User: prompt if differences detected
end
alt Classic cluster
UpdateCmd->>HiveAPI: UpdateHivePullSecretSSS
HiveAPI-->>UpdateCmd: SyncSet created, polled
UpdateCmd->>TargetCluster: RestartPodsBySelector
else HCP cluster
UpdateCmd->>HiveAPI: UpdateHCPPullSecretViaManifestWork
end
UpdateCmd->>TargetCluster: post-update verification
UpdateCmd->>OCM: PostCmdOptions.SetDryRun().Run() (service log)
UpdateCmd-->>User: result summary
sequenceDiagram
participant User
participant AuditCmd as pull-secret audit
participant OCM
participant TargetCluster as each cluster
User->>AuditCmd: --account-id [--cluster-id] --reason [--validate]
AuditCmd->>OCM: resolve account
AuditCmd->>OCM: GetLatestCredentialUpdate
AuditCmd->>OCM: ListOwnerSubscriptions
alt --validate enabled
AuditCmd->>OCM: FetchOwnerAccessToken
AuditCmd->>User: prompt if token unavailable (registry-only fallback)
end
loop each owned cluster
AuditCmd->>TargetCluster: connect with elevation reason
AuditCmd->>TargetCluster: CompareAccessTokenAuthsToCluster
AuditCmd->>TargetCluster: CompareRegistryCredentialAuthsToCluster
end
AuditCmd-->>User: account header, per-cluster staleness, verify tables, summary counts
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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
|
SOP update PR: https://github.com/openshift/ops-sop/pull/4035 (on hold until this PR merges) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (1)
pkg/controller/pullsecret.go (1)
386-420: ⚡ Quick winSort comparison rows before rendering or asserting sync status.
CompareThreeWayappends results from Go map iteration, so the table order changes between runs. Sortresult.AuthsbyRegistrybefore returning to keep CLI output and expect tests deterministic.Proposed fix
+ sort.Slice(result.Auths, func(i, j int) bool { + return result.Auths[i].Registry < result.Auths[j].Registry + }) + return result, nil }🤖 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/controller/pullsecret.go` around lines 386 - 420, The result slice `result.Auths` is populated by iterating over the `ocmAuths` map, which has randomized iteration order in Go, causing non-deterministic output order across runs. After the for loop completes and all `ThreeWayAuthState` entries have been appended to `result.Auths`, sort the slice by the `Registry` field before returning to ensure deterministic order in CLI output and test assertions.
🤖 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/cluster/pullsecretstatus.go`:
- Around line 247-263: The code logs verification failures for
CompareAccessTokenAuthsToCluster and CompareRegistryCredentialAuthsToCluster but
does not set cr.err when these errors occur, causing the final audit report to
omit the failure details. Set cr.err when verifyErr is not nil in both the
access token verification block (around line 247-255) and the registry
credential verification block (around line 291-300), ensuring that cluster
verification failures are properly surfaced in the audit report instead of
silently continuing with only a warning log.
- Around line 291-303: The code currently skips calling renderPSStatus when
validation succeeds (in the else if hasCheck branch where cr.err is nil), which
causes stale status information to be omitted from the output and prevents
staleCount from being properly tracked. Refactor the conditional logic so that
renderPSStatus is called unconditionally for every cluster before checking
validation results. Move the renderPSStatus call outside the conditional
branches so it executes before the hasCheck condition is evaluated, then proceed
with rendering the validation check tables (renderCheckTable calls for
cr.accessTokenResult and cr.regCredResult) only when validation succeeded.
- Around line 87-91: The PreRunE validation function only checks that at least
one of --cluster-id or --account-id is provided, but does not enforce that they
are mutually exclusive. Replace the current validation logic to use
cmd.Flags().Changed() to detect which flags were explicitly set by the user, and
return an error if both flags are provided simultaneously or if neither flag is
provided. This ensures that exactly one flag is required and prevents the silent
fallback behavior where --account-id is used when both flags are set.
In `@cmd/cluster/replacepullsecret.go`:
- Around line 698-725: The code properly tracks verification results in
`atAllMatch` and `rcAllMatch` boolean flags—they remain false when verification
fails or mismatches are found. However, the `op.PullSecretUpdated` result branch
(around lines 810-815) unconditionally prints "All ACCESS TOKEN and REGISTRY
CREDENTIAL auths now match" without checking these flags, causing false success
reports even when errors or mismatches occurred. Fix this by wrapping the
success message print statement so it only executes when both `atAllMatch` and
`rcAllMatch` are true. This ensures verification errors or mismatches prevent
the success message from being printed.
- Around line 603-615: The code prompts the user to choose whether to use the
target cluster pull secret as the base for Hive secret restoration via
ResolveExistingPullSecret and ConfirmPrompt, but the user's choice and
existingData are discarded without being applied to the actual Hive update.
Store the result of the ConfirmPrompt call (whether the user selected YES or NO)
and use it to determine whether to merge the existingData with the OCM pull
secret or use OCM auths only when performing the Hive secret update operation.
This ensures the selected restoration base is actually plumbed through and
applied to prevent dropping customer/operator-added auths.
- Around line 508-510: When GetRegistryCredentials fails or returns no
credentials in the else block (lines 508-510), the code only logs a warning but
does not update the allInSync flag. This causes the command to incorrectly
report "nothing to update" if allInSync was previously set to true from other
comparisons. Fix this by explicitly setting allInSync to false whenever registry
credential comparison cannot be completed, ensuring that incomplete verification
is not treated as being in sync.
- Line 293: The error returned from the FetchAccessTokenOp function call is
being ignored with the underscore placeholder, but pullSecret is subsequently
used for Hive/ManifestWork updates. Capture the error return value instead of
ignoring it, check if it is not nil, and either return the error or mark the
operation as failed before proceeding. This ensures pullSecret is only used when
the FetchAccessTokenOp operation actually succeeds, rather than relying on
implicit side effects.
- Around line 776-786: The PostCmdOptions.Run() method in servicelog can prompt
for user confirmation and return nil even when the user declines, causing the
code to incorrectly report success. To fix this, add a non-interactive flag
(such as SkipPrompts or similar) to PostCmdOptions in cmd/servicelog/post.go
that prevents any interactive prompts, then set this flag to true when creating
the postCmd in replacepullsecret.go so the internal service log posting fails
explicitly rather than silently declining. Additionally, correct the fallback
command string to use the cluster ID flag as "-C" instead of "-i", and ensure
that any duplicate-service-log prompt in the Post command also respects the
skip-prompts flag.
In `@docs/osdctl_cluster_pull-secret_validate.md`:
- Around line 25-34: The documentation examples are using the deprecated command
name `osdctl cluster validate-pull-secret-ext` instead of the new command name.
Replace all three occurrences of `osdctl cluster validate-pull-secret-ext` with
`osdctl cluster pull-secret validate` in the examples section, keeping all the
command-line flags and arguments (--cluster-id, --reason, --skip-access-token,
--skip-registry-creds, --skip-service-logs) unchanged.
In `@pkg/controller/pullsecret_test.go`:
- Around line 26-64: The tests are using a helper function validateRequiredKeys
that duplicates the logic of the production function ValidateRequiredAuths,
which means the tests would not catch if ValidateRequiredAuths broke. Remove the
validateRequiredKeys helper function and refactor all three test functions
(TestValidateRequiredAuths_AllPresent, TestValidateRequiredAuths_SomeMissing,
and TestValidateRequiredAuths_Empty) to call ValidateRequiredAuths directly
instead. Build the appropriate input type (map[string]*amv1.AccessTokenAuth)
that ValidateRequiredAuths expects, populating it with the necessary test data,
and verify the returned missing keys match the expected values.
In `@pkg/controller/pullsecret.go`:
- Around line 732-738: The BuildPullSecretFromSources function does not accept
an account email parameter, and registry-credential entries are created with
empty Email fields, causing email mismatches during later verification. Add an
account email parameter to the BuildPullSecretFromSources function signature and
pass this account email to populate the Email field when handling registry
credentials throughout the function, including where registry credentials are
processed and added to the pull secret.
- Around line 879-891: The error handling at
pkg/controller/pullsecret.go#L879-L891 treats all Get() errors the same way
(attempting secret creation), but this conflates NotFound with
permission/timeout/API errors. Use apierrors.IsNotFound(err) to distinguish:
only create the secret if the error is NotFound; return the original error for
other failures like RBAC denials. Apply the same pattern at
pkg/controller/pullsecret.go#L550-L554 to report read failures separately from
genuinely missing secrets. At pkg/controller/pullsecretop.go#L160-L163, check
apierrors.IsNotFound(err) before logging "not found" to avoid misreporting RBAC
or transient errors. At pkg/controller/pullsecretop.go#L206-L225, only fall back
to alternative secret sources (Hive to target, or target to OCM-only) when
apierrors.IsNotFound(err) is true; for other errors, return and fail rather than
silently proceeding.
- Around line 858-866: The code attempts to assign values into existing.Auths
without checking if the map is nil, which will panic when valid JSON like {} or
{"auths":null} is unmarshaled. Before the for loop that ranges over
incoming.Auths and assigns values to existing.Auths, add a check to initialize
existing.Auths if it is nil. Ensure that both existing.Auths and incoming.Auths
are non-nil maps before attempting to merge them in the loop that assigns
incoming.Auths entries into existing.Auths.
- Around line 1112-1135: The HostedCluster.spec.pullSecret.name is being
rewritten unconditionally without first verifying that the Secret it references
actually exists and was updated. Refactor the logic to use two passes: first,
before the main loop, extract the current pullSecret.name from the HostedCluster
manifest; then, within the loop where Secret manifests are processed, only
update the Secret that matches the actual pullSecret.name from the HostedCluster
(not just any Secret matching secretNamePrefix); if the referenced Secret is not
found after scanning all manifests, return an error before updating the
HostedCluster reference; only after successfully finding and updating the exact
referenced Secret should you update the HostedCluster.Spec.PullSecret.Name to
newSecretName.
- Around line 608-613: The ListOwnerSubscriptions function currently only
fetches a single page of 100 results without implementing pagination, which
means accounts with more than 100 subscriptions will silently return incomplete
data. Modify the function to loop through all available pages by implementing
pagination using the .Page() method, continuing to fetch results until
response.Size() returns fewer items than the page size (100) or until all
response.Total() items have been collected. Follow the same pagination pattern
already implemented elsewhere in the codebase (e.g., pkg/utils/ocm.go lines
154–159) to accumulate subscriptions from each page into the result slice before
returning.
- Around line 743-760: When parsing the existingSecret using json.Unmarshal in
this code block, the current logic silently ignores unmarshal errors with the
condition err == nil, causing malformed secrets to be dropped entirely rather
than preserved. Change this behavior to return an error when json.Unmarshal
fails instead of silently skipping the merge operation, ensuring that malformed
existing secrets are properly handled and do not result in loss of
customer-added registries. Replace the err == nil condition to detect and return
the error when the existing secret cannot be parsed.
- Around line 985-1036: The SyncSet created by kubeCli.Create is not cleaned up
if errors occur in subsequent operations (like the kubeCli.Get call, the sync
polling loop, or timeout). Register a deferred cleanup operation immediately
after the successful kubeCli.Create call that deletes the syncSet, ensuring it
is removed even if later error paths return early. Then remove or refactor the
delete operation at the end of the function (the kubeCli.Delete call around line
1030) so it is not duplicated. This ensures the created SyncSet is always
cleaned up regardless of which error path is taken, preventing it from blocking
future runs.
- Around line 595-599: The accountID parameter is directly interpolated into OCM
search filter strings via fmt.Sprintf without validation, creating an injection
vulnerability when the value originates from CLI input. Add allow-list
validation of the accountID format (e.g., UUID pattern matching the expected OCM
account ID format) at the trust boundary before any fmt.Sprintf calls. This
validation must be applied at three locations: the search filter construction in
CountOwnerClusters (pkg/controller/pullsecret.go:595), the search filter
construction in ListOwnerSubscriptions (pkg/controller/pullsecret.go:609), and
the search filter construction in GetRegistryCredentials
(pkg/utils/utils.go:297). Consider centralizing the validation in a single
helper function and calling it before each fmt.Sprintf that uses accountID, or
validate it at the entry point where the CLI value is received.
In `@pkg/controller/pullsecretop.go`:
- Around line 131-150: The CheckCanI function only sets op.AllOK = false when
op.DryRun is true, but live operations rely on op.AllOK to gate mutations. Move
the op.AllOK = false assignments outside of the if op.DryRun conditional blocks
so they apply in both dry-run and live modes. Specifically, after the error
check from the SelfSubjectAccessReview creation, set op.AllOK = false
unconditionally when err is not nil (before the dry-run output), and when the
access is denied (when !allowed is true), set op.AllOK = false unconditionally
before or after the dry-run formatting. The output formatting can remain
dry-run-specific; only the flag setting should apply universally.
---
Nitpick comments:
In `@pkg/controller/pullsecret.go`:
- Around line 386-420: The result slice `result.Auths` is populated by iterating
over the `ocmAuths` map, which has randomized iteration order in Go, causing
non-deterministic output order across runs. After the for loop completes and all
`ThreeWayAuthState` entries have been appended to `result.Auths`, sort the slice
by the `Registry` field before returning to ensure deterministic order in CLI
output and test assertions.
🪄 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: 665be379-01d8-4645-9398-edf9c17e87d5
📒 Files selected for processing (17)
cmd/cluster/cmd.gocmd/cluster/pullsecret.gocmd/cluster/pullsecretstatus.gocmd/cluster/replacepullsecret.gocmd/cluster/validatepullsecret.gocmd/cluster/validatepullsecretext.gocmd/servicelog/post.godocs/README.mddocs/osdctl_cluster.mddocs/osdctl_cluster_pull-secret.mddocs/osdctl_cluster_pull-secret_audit.mddocs/osdctl_cluster_pull-secret_update.mddocs/osdctl_cluster_pull-secret_validate.mdgo.modpkg/controller/pullsecret.gopkg/controller/pullsecret_test.gopkg/controller/pullsecretop.go
clcollins
requested changes
Jun 16, 2026
clcollins
left a comment
clcollins
left a comment
Member
There was a problem hiding this comment.
Almost entirely claude-identified issues, but nothing major, if you don't mind taking a look.
| return fmt.Errorf("cannot update pull-secret within ManifestWork: %w", err) | ||
| } | ||
|
|
||
| fmt.Fprintf(out, "ManifestWork updated. Waiting 60 seconds for secret to sync on hosted cluster...\n") |
Member
There was a problem hiding this comment.
Should this just go check/watch rather than just wait 60s? It might get synced sooner and save folks some time just waiting.
Contributor
Author
There was a problem hiding this comment.
✅ Fixed — replaced 60s sleep with a poll loop that checks ManifestWork Applied condition. Prompts user every 60s to continue waiting or abort.
| // | ||
| // If existingSecret is non-nil, registries already present in the cluster that | ||
| // are not in either OCM source are preserved. | ||
| func BuildPullSecretFromSources( |
Member
There was a problem hiding this comment.
Claude thinks this is dead code?
It is defined but never called from any command. The update path only uses FetchAccessTokenOp (access token auths only). This is consistent with transferowner.go which lso only writes access token auths, so it's not a bug — but it's dead code that should either be used or removed.
Contributor
Author
There was a problem hiding this comment.
✅ Fixed — BuildPullSecretFromSources and associated types removed.
| if err := json.Unmarshal(manifest.Raw, secret); err != nil { | ||
| return err | ||
| } | ||
| if strings.Contains(secret.Name, secretNamePrefix) { |
Member
There was a problem hiding this comment.
This says it's a prefix but strings.Contains only matches the existence in the string. Should use strings.HasPrefix?
Contributor
Author
There was a problem hiding this comment.
✅ Fixed — changed to strings.HasPrefix.
| time.Sleep(syncPollInterval) | ||
| } | ||
| if !isSynced { | ||
| return fmt.Errorf("SyncSet %s/%s failed to sync after %d attempts", hiveNamespace, SyncSetName, checkSyncMaxAttempts) |
Member
There was a problem hiding this comment.
Claude says the SyncSet crated a line 985 is never cleaned up if there's a timeout - the delete only executes in the if isSynced, so this would leave a stale SyncSet.
Contributor
Author
There was a problem hiding this comment.
✅ Fixed — SyncSet cleanup now runs unconditionally before the isSynced check.
| op.CheckCanI(ctx, masterKubeClientSet, infraName, "update", "secrets", "", resolvedHiveNS) | ||
| op.CheckCanI(ctx, masterKubeClientSet, infraName, "create", "secrets", "", resolvedHiveNS) | ||
|
|
||
| op.Would("create SyncSet %s/pull-secret-replacement to sync to %s", resolvedHiveNS, cluster.Name()) |
Member
There was a problem hiding this comment.
Claude again: The syncset name here is the old one, and should be pull-secret-update not pull-secret-replacement.
And bonus points if it's not hard-coded, for future proofing.
Contributor
Author
There was a problem hiding this comment.
✅ Fixed — uses controller.SyncSetName constant.
nephomaniac
force-pushed
the
ROSAENG-2057-pull-secret-update
branch
2 times, most recently
from
2af9003 to
d0d96e8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/osdctl_cluster_pull-secret_validate.md (1)
25-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExamples use the deprecated command name instead of the new command.
Lines 27, 30, and 33 show
osdctl cluster validate-pull-secret-ext, but since this documentation page describes the newosdctl cluster pull-secret validatecommand, the examples should use the new command name instead.🔧 Proposed fix for the examples
# Compare OCM Access-Token, OCM Registry-Credentials, and OCM Account Email against cluster's pull secret - osdctl cluster validate-pull-secret-ext --cluster-id ${CLUSTER_ID} --reason "OSD-XYZ" + osdctl cluster pull-secret validate --cluster-id ${CLUSTER_ID} --reason "OSD-XYZ" # Exclude Access-Token, and Registry-Credential checks... - osdctl cluster validate-pull-secret-ext --cluster-id ${CLUSTER_ID} --reason "OSD-XYZ" --skip-access-token --skip-registry-creds + osdctl cluster pull-secret validate --cluster-id ${CLUSTER_ID} --reason "OSD-XYZ" --skip-access-token --skip-registry-creds # Skip sending service logs (useful for testing) - osdctl cluster validate-pull-secret-ext --cluster-id ${CLUSTER_ID} --reason "OSD-XYZ" --skip-service-logs + osdctl cluster pull-secret validate --cluster-id ${CLUSTER_ID} --reason "OSD-XYZ" --skip-service-logs🤖 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 `@docs/osdctl_cluster_pull-secret_validate.md` around lines 25 - 34, The documentation examples use the deprecated command name osdctl cluster validate-pull-secret-ext instead of the new command osdctl cluster pull-secret validate. In all three examples shown (the basic example with cluster-id and reason, the example with skip-access-token and skip-registry-creds flags, and the example with skip-service-logs flag), replace the deprecated command name osdctl cluster validate-pull-secret-ext with the new command name osdctl cluster pull-secret validate while keeping all the flags and options unchanged.
🧹 Nitpick comments (1)
pkg/controller/pullsecret.go (1)
426-428: 💤 Low valueDuplicate docstring line.
Line 427 repeats the docstring from line 426.
Suggested fix
// RenderThreeWayComparison prints the three-way comparison in a readable format. -// RenderThreeWayComparison prints the three-way comparison in a readable format. // sourceLabel identifies the OCM source (e.g. "ACCESS TOKEN AUTHS", "REGISTRY CREDENTIAL AUTHS").🤖 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/controller/pullsecret.go` around lines 426 - 428, The docstring for the RenderThreeWayComparison function contains a duplicate comment line. Remove the second occurrence of the "RenderThreeWayComparison prints the three-way comparison in a readable format." comment line, keeping only one instance of this description before the sourceLabel documentation line.
🤖 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.
Duplicate comments:
In `@docs/osdctl_cluster_pull-secret_validate.md`:
- Around line 25-34: The documentation examples use the deprecated command name
osdctl cluster validate-pull-secret-ext instead of the new command osdctl
cluster pull-secret validate. In all three examples shown (the basic example
with cluster-id and reason, the example with skip-access-token and
skip-registry-creds flags, and the example with skip-service-logs flag), replace
the deprecated command name osdctl cluster validate-pull-secret-ext with the new
command name osdctl cluster pull-secret validate while keeping all the flags and
options unchanged.
---
Nitpick comments:
In `@pkg/controller/pullsecret.go`:
- Around line 426-428: The docstring for the RenderThreeWayComparison function
contains a duplicate comment line. Remove the second occurrence of the
"RenderThreeWayComparison prints the three-way comparison in a readable format."
comment line, keeping only one instance of this description before the
sourceLabel documentation line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4af9a563-d3e6-45ed-81fd-02764838d0e3
📒 Files selected for processing (17)
cmd/cluster/cmd.gocmd/cluster/pullsecret.gocmd/cluster/pullsecretstatus.gocmd/cluster/replacepullsecret.gocmd/cluster/validatepullsecret.gocmd/cluster/validatepullsecretext.gocmd/servicelog/post.godocs/README.mddocs/osdctl_cluster.mddocs/osdctl_cluster_pull-secret.mddocs/osdctl_cluster_pull-secret_audit.mddocs/osdctl_cluster_pull-secret_update.mddocs/osdctl_cluster_pull-secret_validate.mdgo.modpkg/controller/pullsecret.gopkg/controller/pullsecret_test.gopkg/controller/pullsecretop.go
✅ Files skipped from review due to trivial changes (2)
- cmd/cluster/validatepullsecret.go
- docs/osdctl_cluster.md
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/servicelog/post.go
- cmd/cluster/validatepullsecretext.go
- cmd/cluster/cmd.go
- pkg/controller/pullsecret_test.go
- pkg/controller/pullsecretop.go
- go.mod
- cmd/cluster/pullsecretstatus.go
- cmd/cluster/pullsecret.go
- cmd/cluster/replacepullsecret.go
nephomaniac
force-pushed
the
ROSAENG-2057-pull-secret-update
branch
8 times, most recently
from
9075e2f to
f9ca732
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
pkg/controller/pullsecret.go (1)
897-900:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSyncSet not cleaned up if ClusterSync.Get fails.
The return at line 899 exits before reaching the cleanup at line 922, leaving an orphaned
pull-secret-updateSyncSet that can block future runs and continue applying unintended state. Usedeferimmediately after successful creation to ensure cleanup on all exit paths.Proposed fix
if err := kubeCli.Create(ctx, syncSet); err != nil { return fmt.Errorf("failed to create SyncSet: %w", err) } +defer func() { + if delErr := kubeCli.Delete(ctx, syncSet); delErr != nil { + fmt.Fprintf(out, "\n%s failed to delete SyncSet %s/%s: %v\n", psColorWarn("[WARN]"), hiveNamespace, SyncSetName, delErr) + } +}() // Use the server-side creation timestamp for comparison... syncSetCreatedAt := syncSet.CreationTimestamp.TimeThen remove the manual delete at lines 921-924 since defer handles it.
🤖 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/controller/pullsecret.go` around lines 897 - 900, The current code returns early from the kubeCli.Get call at line 899 before reaching the cleanup code at lines 921-924, which leaves an orphaned pull-secret-update SyncSet that blocks future runs. Move the SyncSet cleanup logic (the delete operations currently at lines 921-924) into a defer statement that is placed immediately after the pull-secret-update SyncSet is successfully created, ensuring cleanup happens on all exit paths including error returns. Then remove the manual delete code at lines 921-924 since the defer will now handle it.cmd/cluster/replacepullsecret.go (1)
796-801:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect fallback command syntax for manual service log posting.
The fallback command uses
-i %sbut-iis a boolean flag for internal-only mode. The cluster ID should be passed with-C:- fmt.Fprintf(out, "To send manually: osdctl servicelog post -i %s -p MESSAGE=\"Pull secret replaced for cluster owner %q.\"\n", o.clusterID, ownerUsername) + fmt.Fprintf(out, "To send manually: osdctl servicelog post -C %s -i -p MESSAGE=\"Pull secret replaced for cluster owner %q. Reason: %s\"\n", o.clusterID, ownerUsername, o.reason)🤖 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 `@cmd/cluster/replacepullsecret.go` around lines 796 - 801, The fallback command string in the fmt.Fprintf call within the error handling block (when postCmd.Run() fails) incorrectly uses the `-i` flag followed by a cluster ID format specifier. The `-i` flag is a boolean flag for internal-only mode, not for passing the cluster ID. Replace the `-i %s` flag with `-C %s` in the fallback command string to correctly pass the cluster ID argument to the osdctl servicelog post command.pkg/controller/pullsecretop.go (1)
113-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
CheckCanIfailure paths can bypass operation gating.When clientset is nil or SAR creation errors (Line 113–119, Line 132–138), the method returns
falsebut does not markop.AllOKfalse or append a failure. Live flows that gate onAllOKcan continue farther than intended.Suggested fix
if clientset == nil { + op.AllOK = false + op.Failures = append(op.Failures, fmt.Sprintf("auth can-i %s %s in %s failed: client not available", verb, resource, nsLabel(namespace))) if op.DryRun { fmt.Fprintf(op.Out, "%s %s %s: auth can-i %s %s in %s (client not available)\n", opColorDryRun("[Dry Run]"), opColorWarn("[SKIP]"), systemLabel, verb, resource, nsLabel(namespace)) } return false } @@ result, err := clientset.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, review, metav1.CreateOptions{}) if err != nil { + op.AllOK = false + op.Failures = append(op.Failures, fmt.Sprintf("auth can-i %s %s in %s failed: %v", verb, resource, nsLabel(namespace), err)) if op.DryRun { fmt.Fprintf(op.Out, "%s %s %s: auth can-i %s %s in %s (%v)\n", opColorDryRun("[Dry Run]"), opColorWarn("[SKIP]"), systemLabel, verb, resource, nsLabel(namespace), err) } return false }Also applies to: 132-138
🤖 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/controller/pullsecretop.go` around lines 113 - 119, The CheckCanI method has two failure paths that bypass operation gating by returning false without updating the operation state. When clientset is nil (lines 113-119) and when SAR creation errors occur (lines 132-138), the method should mark op.AllOK as false and append a failure record in addition to returning false. This ensures that operations gating on the AllOK flag will not proceed farther than intended when these failure conditions occur.cmd/cluster/pullsecretstatus.go (1)
88-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse explicit Cobra flag-state checks for mutual exclusivity.
Line 89–94 validates using resolved option values; that can mis-detect explicit user input when values are pre-populated upstream. Use
cmd.Flags().Changed(...)and enforce exactly one flag explicitly set.Suggested fix
PreRunE: func(cmd *cobra.Command, args []string) error { - if ops.clusterID == "" && ops.accountID == "" { - return fmt.Errorf("one of --cluster-id or --account-id is required") - } - if ops.clusterID != "" && ops.accountID != "" { - return fmt.Errorf("--cluster-id and --account-id are mutually exclusive") + clusterSet := cmd.Flags().Changed("cluster-id") + accountSet := cmd.Flags().Changed("account-id") + if clusterSet == accountSet { + return fmt.Errorf("exactly one of --cluster-id or --account-id is required") }Based on learnings, explicit flag usage in
cmd/**/*.goshould be determined with Cobra’sFlags().Changed(...), not derived values.🤖 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 `@cmd/cluster/pullsecretstatus.go` around lines 88 - 94, The PreRunE validation function is checking resolved option values (ops.clusterID and ops.accountID) to enforce mutual exclusivity, but this approach can misdetect explicit user input when values are pre-populated upstream. Replace these value-based checks with Cobra's Flags().Changed() method to explicitly verify which flags were actually set by the user. Specifically, use cmd.Flags().Changed("cluster-id") and cmd.Flags().Changed("account-id") to ensure exactly one of these two flags was explicitly provided, rather than relying on whether the resulting ops fields are empty or populated.Source: Learnings
🧹 Nitpick comments (2)
cmd/cluster/pullsecretstatus_test.go (1)
10-13: ⚡ Quick winUse a fresh command per subtest to avoid cross-case flag state leakage.
cmdis shared across all table cases, so Cobra flag state can carry between runs. This weakens coverage for explicit-flag behavior and can mask/introduce false results if validation shifts toFlags().Changed(...)(the safer pattern for inherited/autopopulated values). CreatenewCmdPullSecretAudit(...)inside eacht.Runand set only flags intended for that case.Based on learnings: when implementing Cobra command logic, explicit flag usage should be detected via Cobra’s
Changed(...)API instead of resolved values that may be auto-populated.Also applies to: 58-68
🤖 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 `@cmd/cluster/pullsecretstatus_test.go` around lines 10 - 13, The `cmd` variable is created once outside the test cases and reused across multiple `t.Run` subtests, allowing Cobra flag state to leak between test runs and weakening test coverage. Move the `newCmdPullSecretAudit(streams, nil)` call inside each `t.Run` block so that every subtest receives a fresh command instance with clean flag state. Apply this same fix to all affected test cases (also noted at lines 58-68), creating a new command for each subtest rather than sharing a single instance across all table cases.Source: Learnings
cmd/cluster/replacepullsecret.go (1)
835-847: 💤 Low valueRedundant YES confirmation prompt when using
--forceon no-op scenario.When
--forceis used and the pull secret is already up-to-date, the user is prompted to type YES twice:
- First at lines 769-773 (no-op check before service log)
- Again at lines 843-847 (result section)
This creates a confusing UX. Since the first prompt already bypassed the no-op check with
--force, the second prompt is unnecessary.Proposed fix
} else if op.PullSecretUpToDate && !o.dryrun { noopLabel := color.New(color.FgBlue, color.Bold).SprintFunc() fmt.Fprintf(out, "%s%s All %s and %s auths match — nothing to update.\n", prefix, colorOK("[OK]"), noopLabel("ACCESS TOKEN"), noopLabel("REGISTRY CREDENTIAL")) - if !o.force { - return nil - } - op.Warn("--force specified — proceeding with update despite no changes needed") - fmt.Fprintf(out, "Type YES to confirm: ") - var response string - if _, scanErr := fmt.Scanln(&response); scanErr != nil || response != "YES" { - return fmt.Errorf("operation aborted by user") - } } else if op.AuthDiffCount > 0 && o.dryrun {🤖 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 `@cmd/cluster/replacepullsecret.go` around lines 835 - 847, The code is prompting the user to type YES twice when using --force with an up-to-date pull secret: once at the initial no-op check (lines 769-773) and again in the result section (lines 843-847). Since the user has already confirmed with --force at the first location, the second confirmation prompt in the else-if block where op.PullSecretUpToDate is true should be removed to avoid confusing the user with a redundant prompt.
🤖 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/cluster/pullsecretstatus.go`:
- Around line 291-325: The summary line reports the validated count using
len(checkResults), which includes all attempted validations even those that
failed with errors or incomplete checks. Instead, track the actual count of
successful validations by adding a validatedCount variable (initialized to 0
alongside staleCount) and incrementing it only when hasCheck is true and cr.err
is nil. Then update the final fprintf output to use validatedCount instead of
len(checkResults) to accurately report only the clusters that passed validation.
- Line 114: The MarkFlagRequired call on the cmd object is discarding its error
return value using a blank identifier, which violates Go's error-handling
requirements. Replace the blank identifier pattern with proper error handling by
checking if the error is not nil and either logging the error or returning it
appropriately to ensure that any failure to mark the "reason" flag as required
is properly handled rather than silently ignored.
In `@pkg/controller/pullsecret_test.go`:
- Around line 20-30: Stop ignoring errors from fixture-construction calls in the
test file. In the function containing json.Marshal (around line 21) and the
buildAuthMap function containing Build (around line 29), capture the error
returns instead of discarding them with blank identifiers. For json.Marshal,
assign the error to a variable and check it with a test assertion like
require.NoError or testing.Fatal. Similarly, for the Build() method call,
capture and validate the error return. Apply the same fix pattern throughout the
file wherever json.Marshal and Build errors are currently ignored.
In `@pkg/controller/pullsecret.go`:
- Around line 1030-1040: The code retrieves oldPullSecret from secret.Data at
the ".dockerconfigjson" key without checking if it exists; when the key is
missing, oldPullSecret is nil and causes MergePullSecretAuths to fail when
trying to parse it as JSON. Before calling MergePullSecretAuths, check if the
".dockerconfigjson" key is present in secret.Data, and if it's missing or nil,
initialize oldPullSecret with a proper empty JSON auths structure (such as a
JSON object with an empty "auths" field) before passing it to
MergePullSecretAuths.
- Around line 562-566: The current code in the secret retrieval logic treats all
errors from the Secrets Get call as NotFound errors, which masks permission
denials and transient API failures. Instead of treating any error as "not
found", use apierrors.IsNotFound(err) to specifically check if the error is a
NotFound error. If the error is not a NotFound error (such as RBAC permission
denial or API transient failures), return the error to surface it to the
operator. Only proceed with the "secret not found" warning message when the
error is specifically a NotFound error.
In `@pkg/controller/pullsecretop.go`:
- Around line 162-165: The current error handling treats all errors from
clientset.CoreV1().Secrets(namespace).Get() as "not found" conditions, masking
actual Kubernetes API failures like permission or timeout errors. Replace the
generic error check with apierrors.IsNotFound(err) to distinguish actual
NotFound errors from other failures. When a NotFound error occurs, keep the
existing op.Warn() call; when a different error occurs, use op.Fail() instead to
properly report the actual API failure. Apply this pattern consistently at all
locations in the code where similar Get() calls are made on Kubernetes
resources, ensuring permission and network failures are logged distinctly from
legitimate missing resources.
---
Duplicate comments:
In `@cmd/cluster/pullsecretstatus.go`:
- Around line 88-94: The PreRunE validation function is checking resolved option
values (ops.clusterID and ops.accountID) to enforce mutual exclusivity, but this
approach can misdetect explicit user input when values are pre-populated
upstream. Replace these value-based checks with Cobra's Flags().Changed() method
to explicitly verify which flags were actually set by the user. Specifically,
use cmd.Flags().Changed("cluster-id") and cmd.Flags().Changed("account-id") to
ensure exactly one of these two flags was explicitly provided, rather than
relying on whether the resulting ops fields are empty or populated.
In `@cmd/cluster/replacepullsecret.go`:
- Around line 796-801: The fallback command string in the fmt.Fprintf call
within the error handling block (when postCmd.Run() fails) incorrectly uses the
`-i` flag followed by a cluster ID format specifier. The `-i` flag is a boolean
flag for internal-only mode, not for passing the cluster ID. Replace the `-i %s`
flag with `-C %s` in the fallback command string to correctly pass the cluster
ID argument to the osdctl servicelog post command.
In `@pkg/controller/pullsecret.go`:
- Around line 897-900: The current code returns early from the kubeCli.Get call
at line 899 before reaching the cleanup code at lines 921-924, which leaves an
orphaned pull-secret-update SyncSet that blocks future runs. Move the SyncSet
cleanup logic (the delete operations currently at lines 921-924) into a defer
statement that is placed immediately after the pull-secret-update SyncSet is
successfully created, ensuring cleanup happens on all exit paths including error
returns. Then remove the manual delete code at lines 921-924 since the defer
will now handle it.
In `@pkg/controller/pullsecretop.go`:
- Around line 113-119: The CheckCanI method has two failure paths that bypass
operation gating by returning false without updating the operation state. When
clientset is nil (lines 113-119) and when SAR creation errors occur (lines
132-138), the method should mark op.AllOK as false and append a failure record
in addition to returning false. This ensures that operations gating on the AllOK
flag will not proceed farther than intended when these failure conditions occur.
---
Nitpick comments:
In `@cmd/cluster/pullsecretstatus_test.go`:
- Around line 10-13: The `cmd` variable is created once outside the test cases
and reused across multiple `t.Run` subtests, allowing Cobra flag state to leak
between test runs and weakening test coverage. Move the
`newCmdPullSecretAudit(streams, nil)` call inside each `t.Run` block so that
every subtest receives a fresh command instance with clean flag state. Apply
this same fix to all affected test cases (also noted at lines 58-68), creating a
new command for each subtest rather than sharing a single instance across all
table cases.
In `@cmd/cluster/replacepullsecret.go`:
- Around line 835-847: The code is prompting the user to type YES twice when
using --force with an up-to-date pull secret: once at the initial no-op check
(lines 769-773) and again in the result section (lines 843-847). Since the user
has already confirmed with --force at the first location, the second
confirmation prompt in the else-if block where op.PullSecretUpToDate is true
should be removed to avoid confusing the user with a redundant prompt.
🪄 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: e2818a22-4925-41df-a161-08b810760fe8
📒 Files selected for processing (18)
cmd/cluster/cmd.gocmd/cluster/pullsecret.gocmd/cluster/pullsecretstatus.gocmd/cluster/pullsecretstatus_test.gocmd/cluster/replacepullsecret.gocmd/cluster/validatepullsecret.gocmd/cluster/validatepullsecretext.gocmd/servicelog/post.godocs/README.mddocs/osdctl_cluster.mddocs/osdctl_cluster_pull-secret.mddocs/osdctl_cluster_pull-secret_audit.mddocs/osdctl_cluster_pull-secret_update.mddocs/osdctl_cluster_pull-secret_validate.mdgo.modpkg/controller/pullsecret.gopkg/controller/pullsecret_test.gopkg/controller/pullsecretop.go
✅ Files skipped from review due to trivial changes (1)
- docs/osdctl_cluster.md
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/servicelog/post.go
- go.mod
- cmd/cluster/validatepullsecret.go
- cmd/cluster/cmd.go
nephomaniac
force-pushed
the
ROSAENG-2057-pull-secret-update
branch
from
f9ca732 to
75fda6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/cluster/replacepullsecret.go (1)
801-801:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMalformed fallback servicelog command:
-iis a boolean flag.The fallback command passes
o.clusterIDas an argument to-i, but-i(--internal) is a boolean flag that doesn't accept arguments. The cluster ID should be passed with-C. This generates an invalid CLI invocation likeosdctl servicelog post -i abc123 -p ....Proposed fix
- fmt.Fprintf(out, "To send manually: osdctl servicelog post -i %s -p MESSAGE=\"Pull secret replaced for cluster owner %q.\"\n", o.clusterID, ownerUsername) + fmt.Fprintf(out, "To send manually: osdctl servicelog post -C %s -i -p MESSAGE=\"Pull secret replaced for cluster owner '%s'. Reason: %s\"\n", o.clusterID, ownerUsername, o.reason)🤖 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 `@cmd/cluster/replacepullsecret.go` at line 801, The fmt.Fprintf statement constructs an invalid osdctl servicelog command by using the boolean flag `-i` to pass the cluster ID, but `-i` does not accept arguments. Replace `-i %s` with `-C %s` in the format string to correctly pass o.clusterID to the servicelog command, as `-C` is the proper flag for specifying the cluster ID.
🤖 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/cluster/replacepullsecret.go`:
- Around line 838-850: The code contains a duplicate --force confirmation
prompt. When op.PullSecretUpToDate is true and the user confirms with YES at the
first prompt (around line 776), the function should return to prevent reaching
this second confirmation block. Remove or comment out the redundant confirmation
check in the else if branch (lines 846-850 in the diff showing the fmt.Fprintf
prompt and fmt.Scanln logic) since the user has already confirmed their intent
to proceed with --force at the earlier location. This ensures the user only
confirms once for a single --force override operation.
---
Duplicate comments:
In `@cmd/cluster/replacepullsecret.go`:
- Line 801: The fmt.Fprintf statement constructs an invalid osdctl servicelog
command by using the boolean flag `-i` to pass the cluster ID, but `-i` does not
accept arguments. Replace `-i %s` with `-C %s` in the format string to correctly
pass o.clusterID to the servicelog command, as `-C` is the proper flag for
specifying the cluster ID.
🪄 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: 4d3b3abc-6ce2-4c5e-aa7e-d702beeed84d
📒 Files selected for processing (18)
cmd/cluster/cmd.gocmd/cluster/pullsecret.gocmd/cluster/pullsecretstatus.gocmd/cluster/pullsecretstatus_test.gocmd/cluster/replacepullsecret.gocmd/cluster/validatepullsecret.gocmd/cluster/validatepullsecretext.gocmd/servicelog/post.godocs/README.mddocs/osdctl_cluster.mddocs/osdctl_cluster_pull-secret.mddocs/osdctl_cluster_pull-secret_audit.mddocs/osdctl_cluster_pull-secret_update.mddocs/osdctl_cluster_pull-secret_validate.mdgo.modpkg/controller/pullsecret.gopkg/controller/pullsecret_test.gopkg/controller/pullsecretop.go
✅ Files skipped from review due to trivial changes (1)
- docs/osdctl_cluster.md
🚧 Files skipped from review as they are similar to previous changes (10)
- cmd/cluster/validatepullsecretext.go
- go.mod
- cmd/cluster/pullsecret.go
- cmd/cluster/validatepullsecret.go
- cmd/cluster/pullsecretstatus_test.go
- cmd/cluster/cmd.go
- pkg/controller/pullsecret_test.go
- pkg/controller/pullsecretop.go
- pkg/controller/pullsecret.go
- cmd/cluster/pullsecretstatus.go
nephomaniac
force-pushed
the
ROSAENG-2057-pull-secret-update
branch
3 times, most recently
from
b9b5f07 to
1a446e7
Compare
nephomaniac
force-pushed
the
ROSAENG-2057-pull-secret-update
branch
from
1a446e7 to
b4dd9f9
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
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
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
Adds
osdctl cluster pull-secretsubcommand group for managing cluster pull secrets:pull-secret update— refresh a cluster's pull secret from OCM without ownership transfer (replacesreplace-pull-secret)pull-secret audit— account-wide pull secret audit across all clusters owned by an accountpull-secret validate— wraps existingvalidate-pull-secret-extunder the new command groupReferences: ROSAENG-2057
Related: openshift/ops-sop#4035 (SOP update, on hold until this PR merges)
Key design decisions
pkg/controller/pullsecret.go, following PR ROSAENG-2066: Add osdctl account aws-creds command #913transferowner.go: cleanly separated code to keep review scope focusedpull-secret-update(distinct from transfer-owner'spull-secret-replacement) with existing SyncSet detection and user promptreplace-pull-secretandvalidate-pull-secret-extstill work with deprecation messagesFlow: HCP pull secret update
flowchart TD A[osdctl pull-secret update] --> B[Fetch access token from OCM] B --> C[Connect to service cluster] C --> D[Compare OCM auths vs target PS] D -->|All match| E[No-op — nothing to update] D -->|Diffs found| F[Get ManifestWork on service cluster] F --> G[Merge OCM auths into ManifestWork PS] G --> H[Rename secret to trigger HCCO sync] H --> I[Update ManifestWork] I --> J[Wait for HCCO to reconcile to target] J --> K[Verify target PS matches OCM] K --> L[Send internal service log] style A fill:#2563eb,color:#fff style E fill:#22c55e,color:#fff style L fill:#22c55e,color:#fffFlow: Classic pull secret update
flowchart TD A[osdctl pull-secret update] --> B[Fetch access token from OCM] B --> C[Connect to hive + target clusters] C --> D[Three-way compare: OCM ↔ Hive ↔ Target] D -->|All match| E[No-op — nothing to update] D -->|Diffs found| F[Merge OCM auths into hive secret] F --> G{Existing SyncSet?} G -->|Yes| H[Prompt user: delete and replace?] G -->|No| I[Create SyncSet pull-secret-update] H -->|Delete| I I --> J[Poll ClusterSync for sync completion] J --> K[Delete SyncSet cleanup] K --> L[Verify target PS matches OCM] L --> M[Restart telemeter-client + ocm-agent pods] M --> N[Send internal service log] style A fill:#2563eb,color:#fff style E fill:#22c55e,color:#fff style N fill:#22c55e,color:#fffChange scope
pullsecret.go,pullsecretstatus.go,replacepullsecret.go,pullsecretop.go)pkg/controller/pullsecret.go)pullsecret_test.go)cmd.go,validatepullsecret.go,validatepullsecretext.go,post.go)Usage
Example Output
osdctl cluster pull-secret update --dry-run(HCP)osdctl cluster pull-secret update --hive-ocm-url prod(Classic — tampered email detected and fixed)osdctl cluster pull-secret update --dry-run --hive-ocm-url prod(Classic)osdctl cluster pull-secret audit --validateTest plan
make lint— 0 issuesmake format— no changesmake generate-docs— docs up to datego build— cleanpkg/controller/pullsecret_test.go(CompareThreeWay, MergePullSecretAuths, extractPullSecretAuth, updateManifestWorkPayloads, PullSecretOp, syncStatus)Jira
ROSAENG-2057
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
osdctl cluster pull-secretgroup withaudit(account-wide status, optional--validate),update(refresh pull secrets from owner OCM data; supports--dry-runand--force), andvalidate(enhanced auth verification).validate-pull-secret-extis deprecated; usepull-secret validateinstead.replace-pull-secretalias for compatibility.pull-secretand subcommands; removedvalidate-pull-secret-extfrom listings.pull-secret auditandpull-secret validate.