WIP:OCPSTRAT-3618: Rebase 1.37 - #2408
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
No actionable comments were generated in the recent review. 🎉 WalkthroughThe change updates Go dependencies, adds condition-aware authorization methods, propagates authorization union construction errors, removes obsolete feature-gate initialization, and guards a test channel against repeated closure. ChangesAuthorization and dependency maintenance
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The dependency rebase still leaves Kubernetes beta modules, an affected grpc version, and non-canonical OpenShift replacements in go.mod. This creates bounded production support, security, and supply-chain concerns, so merge should wait for approved dependency updates or documented owner approval. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@go.mod`:
- Around line 41-49: Replace the Kubernetes v0.37.0-beta.0 dependencies in
go.mod, including the modules shown around k8s.io/api and related packages, with
approved stable versions and synchronize vendor/modules.txt. If a beta
dependency is intentionally retained, document the approved exception and ensure
this dependency graph is excluded from production builds.
- Around line 153-155: Remove the non-canonical github.com/openshift/api and
github.com/openshift/client-go replace directives so canonical OpenShift modules
are used. If fork usage is explicitly approved, retain them only with documented
owner approval and verified commits, module declarations, go.sum hashes,
licensing, OSV status, SBOM provenance, and signed artifacts.
- Line 140: Upgrade the google.golang.org/grpc dependency from v1.81.1 to
v1.82.1 or later, ensuring the module graph does not downgrade it. Refresh the
corresponding go.sum entries and vendor/modules.txt metadata to match the
upgraded dependency.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| replace ( | ||
| github.com/openshift/api => github.com/jacobsee/openshift-api v0.0.0-20260723063153-cebda3324a87 | ||
| github.com/openshift/client-go => github.com/jubittajohn/client-go v0.0.0-20260805200440-5e8c00d8a4c0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go list -m -json github.com/openshift/api github.com/openshift/client-go
go mod graph | rg 'github.com/(openshift|jacobsee|jubittajohn)'
test -f go.sum
go mod verifyRepository: openshift/library-go
Length of output: 21922
Security Misconfiguration (CWE-829): Inclusion of Functionality from Untrusted Control Sphere
Remove or formally approve the non-canonical OpenShift module replacements.
These replace directives make builds consume external fork modules while retaining canonical github.com/openshift/... imports. Restore the canonical modules unless explicit owner approval exists. Otherwise, document the justification and verify the pinned commits, module declarations, go.sum hashes, license compatibility, OSV status, SBOM provenance, and signed artifacts. go mod verify does not establish fork ownership or supply-chain provenance.
🤖 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 `@go.mod` around lines 153 - 155, Remove the non-canonical
github.com/openshift/api and github.com/openshift/client-go replace directives
so canonical OpenShift modules are used. If fork usage is explicitly approved,
retain them only with documented owner approval and verified commits, module
declarations, go.sum hashes, licensing, OSV status, SBOM provenance, and signed
artifacts.
Source: Path instructions
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/authorization/hardcodedauthorizer/metrics.go (1)
35-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for conditions-aware authorization.
TestAuthorizeronly callsAuthorize. TestConditionsAwareAuthorizewith allow and no-opinion attributes, and testEvaluateConditionsforDecisionDeny, an empty reason, andauthorizer.ErrorConditionEvaluationNotSupported.🤖 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/authorization/hardcodedauthorizer/metrics.go` around lines 35 - 42, Add regression tests for metricsAuthorizer.ConditionsAwareAuthorize using both allow and no-opinion attributes, verifying the corresponding conditions-aware decisions. Add coverage for EvaluateConditions asserting DecisionDeny, an empty reason, and authorizer.ErrorConditionEvaluationNotSupported.
🤖 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.
Nitpick comments:
In `@pkg/authorization/hardcodedauthorizer/metrics.go`:
- Around line 35-42: Add regression tests for
metricsAuthorizer.ConditionsAwareAuthorize using both allow and no-opinion
attributes, verifying the corresponding conditions-aware decisions. Add coverage
for EvaluateConditions asserting DecisionDeny, an empty reason, and
authorizer.ErrorConditionEvaluationNotSupported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 172763d3-c758-4e40-aabe-3f83abc170b2
📒 Files selected for processing (4)
pkg/authorization/hardcodedauthorizer/metrics.gopkg/controller/controllercmd/builder.gopkg/manifestclient/read_roundtripper.gopkg/manifestclient/testing/list_test.go
💤 Files with no reviewable changes (2)
- pkg/manifestclient/testing/list_test.go
- pkg/manifestclient/read_roundtripper.go
5080e73 to
8dd0468
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
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
♻️ Duplicate comments (2)
go.mod (2)
140-140:⚠️ Potential issue | 🟠 MajorUpgrade
google.golang.org/grpcbefore merge.Line 140 selects
v1.81.1. GO-2026-6061 affects versions beforev1.82.1and covers the HTTP/2 transport and xDS RBAC packages. (osv.dev) Upgrade tov1.82.1or later, then refreshgo.sumand vendored metadata.🤖 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 `@go.mod` at line 140, Upgrade the google.golang.org/grpc dependency from v1.81.1 to v1.82.1 or later, then regenerate go.sum and all vendored metadata to match the selected version.Source: Linters/SAST tools
41-49:⚠️ Potential issue | 🟠 MajorDo not merge the Kubernetes beta dependencies without an approved exception.
The module graph selects
v0.37.0-beta.0for multiple Kubernetes modules, includingk8s.io/streaming. Replace these versions with approved stable releases, or document an approved exception and prevent this graph from entering production builds.Also applies to: 146-146
🤖 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 `@go.mod` around lines 41 - 49, Replace the v0.37.0-beta.0 Kubernetes dependencies in go.mod, including the selected k8s.io/streaming version, with approved stable releases across the module graph. If beta versions are required, document the approved exception and add a production-build safeguard preventing this dependency graph from being used.Source: Path instructions
🤖 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 `@go.mod`:
- Around line 153-155: Remove the non-canonical replace directives for
github.com/openshift/api and github.com/openshift/client-go from the go.mod
replace block; if they are required, obtain formal owner approval and document
the requested provenance, licensing, vulnerability, hash, SBOM, and signature
verification before retaining them.
---
Duplicate comments:
In `@go.mod`:
- Line 140: Upgrade the google.golang.org/grpc dependency from v1.81.1 to
v1.82.1 or later, then regenerate go.sum and all vendored metadata to match the
selected version.
- Around line 41-49: Replace the v0.37.0-beta.0 Kubernetes dependencies in
go.mod, including the selected k8s.io/streaming version, with approved stable
releases across the module graph. If beta versions are required, document the
approved exception and add a production-build safeguard preventing this
dependency graph from being used.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| replace ( | ||
| github.com/openshift/api => github.com/jacobsee/openshift-api v0.0.0-20260806204542-cfe07ec0f95b | ||
| github.com/openshift/client-go => github.com/jubittajohn/client-go v0.0.0-20260806210316-cf531a43afcf |
There was a problem hiding this comment.
Remove or formally approve the non-canonical module replacements.
These directives redirect canonical OpenShift imports to external fork modules. Remove them before merge, or document owner approval and verify commit provenance, module declarations, licenses, OSV status, hashes, SBOM provenance, and artifact signatures.
🤖 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 `@go.mod` around lines 153 - 155, Remove the non-canonical replace directives
for github.com/openshift/api and github.com/openshift/client-go from the go.mod
replace block; if they are required, obtain formal owner approval and document
the requested provenance, licensing, vulnerability, hash, SBOM, and signature
verification before retaining them.
Source: Path instructions
|
/assign |
8dd0468 to
80fb23c
Compare
|
@jubittajohn: This pull request references OCPSTRAT-3618 which is a valid jira issue. 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jubittajohn 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 |
80fb23c to
d6ae509
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. |
|
/test unit |
d6ae509 to
3e419f4
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. |
Signed-off-by: jubittajohn <jujohn@redhat.com>
Signed-off-by: jubittajohn <jujohn@redhat.com>
…nges in k8s 1.37 Signed-off-by: jubittajohn <jujohn@redhat.com>
The feature gate graduated to GA and was removed in k8s 1.37. LabelSelector is now populated unconditionally in RequestInfo. Signed-off-by: jubittajohn <jujohn@redhat.com>
The testControllerWithInformer helper uses defer close(controllerSynced) in the sync function, which panics if sync runs more than once. The k8s 1.37 rebase changed informer event dispatch timing in client-go (shared_informer.go, controller.go), widening the window where a second work item can be processed before the controller fully shuts down. This causes a "close of closed channel" panic that crashes the test binary with exit code 2. Fix by wrapping the channel close in sync.Once so it executes exactly once regardless of how many times sync is called. Signed-off-by: jubittajohn <jujohn@redhat.com>
3e419f4 to
d87bda3
Compare
|
@jubittajohn: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary by CodeRabbit
Chores
Bug Fixes