CCO-834, CCO-835, CCO-836: Upgrade to Kubernetes 1.36 with CI and e2e tests#1066
CCO-834, CCO-835, CCO-836: Upgrade to Kubernetes 1.36 with CI and e2e tests#1066ericahinkleRH wants to merge 1 commit into
Conversation
|
@ericahinkleRH: This pull request references CCO-834 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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 pull request updates OpenShift and Kubernetes dependencies for Kubernetes 1.36, removes generated no-op default registration, adds Kubernetes 1.36 end-to-end tests, and documents validation, upgrade, and rollback procedures. ChangesKubernetes 1.36 upgrade
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestMain
participant KubernetesAPI
participant CCOPods
TestMain->>KubernetesAPI: resolve kubeconfig and inspect deployment
KubernetesAPI->>CCOPods: identify running CCO pods
TestMain->>CCOPods: fetch recent logs
CCOPods-->>TestMain: return readiness and compatibility log data
KubernetesAPI-->>TestMain: return Available condition
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)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1066 +/- ##
=======================================
Coverage 47.30% 47.30%
=======================================
Files 97 97
Lines 12614 12614
=======================================
Hits 5967 5967
Misses 5987 5987
Partials 660 660 🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
/approve |
openshift-ci
Bot
added
the
approved
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
|
/lgtm |
|
@ericahinkleRH: you cannot LGTM your own PR. 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 kubernetes-sigs/prow repository. |
|
The verify-deps test is still failing even with deps.diff added. The error shows the vendor patch isn't being recognized correctly. Any suggestions on how to fix the deps.diff format? |
|
/lgtm |
|
I think we can get rid of deps.diff entirely and go with the clean version of the library-go test helpers in the vendor folder |
|
Thanks @dlom! Removed the deps.diff and vendor patch. Using the clean version from |
|
/override ci/prow/security |
|
@dlom: Overrode contexts on behalf of dlom: ci/prow/security 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 kubernetes-sigs/prow repository. |
|
@dlom The verify-deps test is still failing after removing deps.diff. It's showing differences in the library-go test_helpers.go file, but running |
|
please rerun |
|
/override ci/prow/security |
|
@dlom: Overrode contexts on behalf of dlom: ci/prow/security 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 kubernetes-sigs/prow repository. |
|
I see... let's wait for openshift/library-go#2171 to merge first before moving forwards with this PR |
|
top level PR we're waiting on is openshift/api#2813 |
|
@ericahinkleRH: This pull request references CCO-834 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. This pull request references CCO-835 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. This pull request references CCO-836 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/K8S_1.36_UPGRADE.md`:
- Around line 89-91: Update the “What It Tests” section associated with
TestK8s136ControllerManagerStarts to state only that the CCO Deployment
condition is “Available=True”; remove the claim about checking startup failures
unless the test is extended to inspect them.
In `@test/e2e/k8s136/k8s_1_36_upgrade_test.go`:
- Around line 123-127: Update the pod log inspection loop around GetLogs so the
assessment cannot succeed when every log retrieval fails. Track whether at least
one running pod’s logs were successfully inspected, and fail the assessment if
none were; preserve the existing warning and continue behavior for individual
GetLogs failures.
- Around line 175-184: Update the Deployment condition check around the loop
over deployment.Status.Conditions to track whether an “Available” condition was
found, and fail after the loop unless it was present with corev1.ConditionTrue.
Preserve the existing failure details for an explicitly unavailable condition
and the success log for a valid condition.
- Line 141: Update the pod-log diagnostic around the t.Logf call to avoid
publishing raw operator logs: log the matched pattern and only a bounded excerpt
of logStr, applying the repository’s existing redaction helper or approach
before output. Preserve enough sanitized context for debugging without emitting
the full log contents.
- Line 27: Apply the existing testTimeout to the contexts used by the e2e
assessments in the upgrade test, ensuring the context passed through Assess and
client-go has a deadline. Wrap each assessment or create the test environment
with context.WithTimeout using testTimeout, and preserve cancellation cleanup
for every derived context.
🪄 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: 2a16887c-3d05-46ea-8b42-365c6a61add1
📒 Files selected for processing (2)
docs/K8S_1.36_UPGRADE.mdtest/e2e/k8s136/k8s_1_36_upgrade_test.go
| **What It Tests:** | ||
| - CCO deployment condition is "Available=True" | ||
| - No startup failures related to K8s 1.36 API changes |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not claim startup failures are checked.
TestK8s136ControllerManagerStarts only reads the Deployment’s Available condition; it does not inspect startup failures. Change this to state that the test verifies Available=True, or extend the test accordingly.
🤖 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/K8S_1.36_UPGRADE.md` around lines 89 - 91, Update the “What It Tests”
section associated with TestK8s136ControllerManagerStarts to state only that the
CCO Deployment condition is “Available=True”; remove the claim about checking
startup failures unless the test is extended to inspect them.
|
|
||
| for _, pattern := range errorPatterns { | ||
| if contains(logStr, pattern) { | ||
| t.Logf("Pod %s logs (last 100 lines):\n%s", pod.Name, logStr) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid publishing full operator logs.
This writes raw pod logs into CI artifacts, which may expose internal hostnames, customer data, or credentials. Log the matched pattern and a bounded, redacted excerpt instead. As per coding guidelines, flag logging that may expose sensitive or internal data.
🤖 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 `@test/e2e/k8s136/k8s_1_36_upgrade_test.go` at line 141, Update the pod-log
diagnostic around the t.Logf call to avoid publishing raw operator logs: log the
matched pattern and only a bounded excerpt of logStr, applying the repository’s
existing redaction helper or approach before output. Preserve enough sanitized
context for debugging without emitting the full log contents.
Source: Coding guidelines
|
/test security |
1 similar comment
|
/test security |
bandrade
left a comment
bandrade
left a comment
There was a problem hiding this comment.
Reviewed the current head (aa194f6d). The Kubernetes/OpenShift dependency update itself looks unblocked now that the upstream PR chain has merged. I found a few correctness and safety issues in the new e2e coverage that should be addressed before merge:
- Apply
testTimeoutto the contexts used for Kubernetes API and pod-log calls. It is currently declared but unused. - Fail the log assessment when every
GetLogscall fails; otherwise it can pass without inspecting any pod logs. - Avoid printing raw pod logs when an error pattern matches. The logs may contain sensitive cluster data; reporting the pod and matched pattern is sufficient.
- Require the Deployment to contain an explicit
Available=Truecondition. The current loop succeeds when the condition is absent. - Restore
os.Exit(testenv.Run(m))inTestMainand remove the documentation claim that the controller-manager test detects startup failures, since it currently checks only Deployment availability.
I applied these changes locally and validated them with:
git diff --check
go test -tags e2e -run '^$' ./test/e2e/k8s136/...
Both checks pass. I have the patch ready and can share it or coordinate the changes with you.
Separately, the current ci/prow/security failure appears unrelated to this PR's source changes: Snyk is flagging Go standard-library vulnerabilities in the CI image's Go 1.26.4, fixed in Go 1.26.5.
E2E Test Fixes ✅Fixed all correctness and safety issues identified in code review:
Tests compile cleanly: Security Test Note
|
|
@dlom The Issue DetailsThe Snyk security scan is flagging Go standard library vulnerabilities in the CI environment's Go 1.26.4, which are fixed in Go 1.26.5:
These vulnerabilities affect 1,884 paths across 1,762 dependencies — all rooted in the Go toolchain version, not application code. Application Code Status✅ 0 application vulnerabilities found RequestThis is a CI infrastructure issue that affects the build environment, not this PR's changes. Could you please override the security check or advise on next steps? The K8s 1.36 upgrade code changes are unrelated to these Go stdlib CVEs. All other tests (unit, e2e, verify) are passing or in progress. Thanks! |
|
/override ci/prow/security |
|
@bandrade: bandrade unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. 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 kubernetes-sigs/prow repository. |
|
Updated toolchain to Go 1.26.5 to fix security scan! ✅ The security failures were caused by Go 1.26.4 stdlib CVEs. Updated - toolchain go1.26.4
+ toolchain go1.26.5This should resolve both CVEs:
/test security |
|
/test e2e-hypershift |
Updated to library-go v0.0.0-20260716104731-fdf18b82797f which includes the HasSyncedChecker method required for K8s 1.36 compatibility. This matches the same library-go version used in CCO-834 PR openshift#1066. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@ericahinkleRH can you squash to one commit please? |
… tests Update Kubernetes dependencies to v0.36.0 and controller-runtime to v0.24.0. Bump openshift/api for compatibility with APIs removed in Kubernetes 1.36. Update to official library-go with HasSyncedChecker support. Update toolchain to Go 1.26.5 to fix security scan. Add e2e tests for K8s 1.36 HasSyncedChecker functionality and controller manager startup verification. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@dlom Done! |
|
/retest e2e-hypershift |
|
/test e2e-hypershift |
2 similar comments
|
/test e2e-hypershift |
|
/test e2e-hypershift |
|
@ericahinkleRH: 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. |
|
Hi @dlom, PR #1066 is ready for final review and merge. This PR completes the K8s 1.36 upgrade for CCO (covers CCO-834, CCO-835, and CCO-836):
All tests are passing, including e2e-hypershift which passed after infrastructure flakes resolved. Since I squashed the commits, the previous approval was invalidated. Could you review and add /lgtm and /approve when you get a chance? Thanks! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bandrade, dlom, ericahinkleRH, nader-ziada 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 |
5 participants
Summary
Upgrades Cloud Credential Operator to Kubernetes 1.36 with complete CI implementation and e2e testing.
Jira Tickets
Changes
CCO-834: Core Upgrade
library-goto v0.0.0-20260716104731-fdf18b82797fCCO-835: CI Implementation
docs/K8S_1.36_UPGRADE.mdCCO-836: E2E Testing
test/e2e/k8s136/k8s_1_36_upgrade_test.goTesting
Documentation
docs/K8S_1.36_UPGRADE.md- Complete upgrade guideRelated PRs
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com