OCPBUGS-99013: inspect: Redact OAuthClient secrets - #2354
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-99013, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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:
WalkthroughThe inspection package now supports ChangesOAuth client inspection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Resource as InspectResource
participant OAuthInspection as inspectOAuthClientInfo
participant Redaction as elideOAuthClient
participant Writer as ResourceWriter
Resource->>OAuthInspection: dispatch oauthclients resource
OAuthInspection->>Redaction: redact client and additional secrets
Redaction-->>OAuthInspection: return redacted OAuthClient
OAuthInspection->>Writer: write inspected YAML resource
Writer-->>Resource: return inspection result
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
|
@tchap: This pull request references Jira Issue OCPBUGS-99013, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tchap: This pull request references Jira Issue OCPBUGS-99013, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@tchap: This pull request references Jira Issue OCPBUGS-99013, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/admin/inspect/oauthclient.go`:
- Around line 29-31: Wrap the OAuth inspection errors with meaningful operation
context: in pkg/cli/admin/inspect/oauthclient.go lines 29-31, wrap the
structured-object conversion error; in lines 46-49, wrap directory creation and
resource-write errors; and in pkg/cli/admin/inspect/resource.go lines 112-115,
wrap the OAuthClient handler error. Preserve the existing return flow while
ensuring aggregate errors identify each failed operation.
- Around line 15-19: Add descriptive Go comments for each declaration: document
oauthClientList at pkg/cli/admin/inspect/oauthclient.go:15-19,
(*oauthClientList).addItem at pkg/cli/admin/inspect/oauthclient.go:19-26,
inspectOAuthClientInfo at pkg/cli/admin/inspect/oauthclient.go:28-50,
elideOAuthClient at pkg/cli/admin/inspect/oauthclient.go:52-59, and
TestElideOAuthClient at pkg/cli/admin/inspect/oauthclient_test.go:10-60. Ensure
each comment clearly describes the corresponding type, function, method, or
test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 33fc2f45-d042-4a24-aa54-154dd3536580
📒 Files selected for processing (3)
pkg/cli/admin/inspect/oauthclient.gopkg/cli/admin/inspect/oauthclient_test.gopkg/cli/admin/inspect/resource.go
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
caea5bb to
9f1fb41
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 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/cli/admin/inspect/resource_test.go`:
- Line 23: Add a concise descriptive comment immediately before the
TestInspectResource function, explaining what the test verifies and beginning
with the function name.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 22065dc6-70cf-42f8-9c3b-bcff0026c12e
📒 Files selected for processing (5)
pkg/cli/admin/inspect/oauthclient.gopkg/cli/admin/inspect/oauthclient_test.gopkg/cli/admin/inspect/resource.gopkg/cli/admin/inspect/resource_test.gopkg/cli/admin/inspect/testdata/oauthclient-console-redacted.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cli/admin/inspect/oauthclient.go
- pkg/cli/admin/inspect/oauthclient_test.go
- pkg/cli/admin/inspect/resource.go
9f1fb41 to
736840d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
736840d to
2e28fd6
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
2e28fd6 to
fc942a2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
fc942a2 to
eb41670
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Two nitpicks, other than that looks great. Thanks
| @@ -0,0 +1,7 @@ | |||
| additionalSecrets: | |||
There was a problem hiding this comment.
This is non standard order. Can you please move apiversion and kind at the top?
There was a problem hiding this comment.
This is just what sigs.k8s.io/yaml does and it's used to generate the expected string. It's the same writer that is being used in production. So I think that this is clear and simple, otherwise I would need to decode into maps and compare the objects. Let me know what you think.
There was a problem hiding this comment.
Does that mean changing the order breaking the test?. Isn't this too fragile?
There was a problem hiding this comment.
Typically in unit tests objects (which is in yaml format) are converted to real objects and they are compared using equality.semantics
There was a problem hiding this comment.
Alright, I was being lazy, amended to parse YAML and compare the objects.
eb41670 to
7403b27
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
OAuthClient.Secret and OAuthClient.AdditionalSecrets were written to disk in plaintext when an OAuthClient appeared in must-gather output (either directly or via a ClusterOperator's relatedObjects). This adds an elideOAuthClient handler that replaces secret values with length stubs, consistent with how Secret data and proxy credentials are already treated.
7403b27 to
9f385f3
Compare
|
Thank you |
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, tchap The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@tchap: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
OAuthClient.Secret and OAuthClient.AdditionalSecrets were written to disk in plaintext when an OAuthClient appeared in must-gather output (either directly or via a ClusterOperator's relatedObjects). This adds an elideOAuthClient handler that replaces secret values with length stubs, consistent with how Secret data and proxy credentials are already treated.
Summary by CodeRabbit
New Features
Bug Fixes
Tests