REPLACE-WITH-JIRA-KEY: Bump Kubernetes dependencies to v1.36.2#741
REPLACE-WITH-JIRA-KEY: Bump Kubernetes dependencies to v1.36.2#741dfarrell07 wants to merge 10 commits into
Conversation
go get k8s.io/api@v0.36.2 go get k8s.io/apiextensions-apiserver@v0.36.2 go get k8s.io/apimachinery@v0.36.2 go get k8s.io/apiserver@v0.36.2 go get k8s.io/client-go@v0.36.2 go get k8s.io/component-base@v0.36.2 go get sigs.k8s.io/controller-runtime@v0.24.1 go get github.com/openshift/api go get github.com/openshift/library-go go get k8s.io/klog go get k8s.io/klog/v2 go get k8s.io/kube-openapi go get k8s.io/kubernetes go get k8s.io/utils go get sigs.k8s.io/apiserver-network-proxy/konnectivity-client go get sigs.k8s.io/json go get sigs.k8s.io/randfill go get sigs.k8s.io/structured-merge-diff/v6 go get sigs.k8s.io/yaml go mod tidy Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
./Dockerfile ./Dockerfile.daemon Makefile Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
IntOrString changed in k8s 1.36 such that %q no longer works directly on the struct. Use .String() to get the string representation before formatting. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Applied: migrate klog v1 imports to klog/v2 Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Applied: bump golangci-lint version Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
go mod tidy removed the stale k8s.io/klog v1 dependency after the import migration to klog/v2. go mod vendor removes the vendored klog v1 files. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
- Replace io/ioutil with os.ReadFile and os.ReadDir (deprecated since Go 1.16) - Remove duplicate package imports (ST1019) in ebpfsyncer, ebpf loader, k8sreporter, and namespaces packages - Add .golangci.yml with errcheck exclusions for fmt.Fprintf, fmt.Fprintln, and Close methods commonly unchecked in tests and logging code Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Remove dead code building an unused interfaces slice and replace unnecessary fmt.Sprintf with a string literal. Update .golangci.yml to suppress pre-existing style checks (ST1000 package comments, SA1019 deprecations) that existed before this rebase. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Explicitly discard the Close return value to satisfy errcheck. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe pull request upgrades the project to Go 1.26, refreshes Kubernetes and related dependencies, migrates linting to golangci-lint v2, updates standard-library and Kubernetes APIs, and adjusts BPF manager controller behavior and error handling. ChangesToolchain modernization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dfarrell07 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 |
![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
🧹 Nitpick comments (2)
test/e2e/namespaces/namespaces.go (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the error using
%w.Consider using
%winstead of%vto properly preserve the original error context for wrapping and unwrapping upstream.♻️ Proposed refactor
- return fmt.Errorf("failed to delete pods %v", err) + return fmt.Errorf("failed to delete pods: %w", err)🤖 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/namespaces/namespaces.go` at line 68, Update the error construction in the pod-deletion flow to use the wrapping verb %w instead of %v, preserving the underlying error for upstream unwrapping while retaining the existing contextual message.test/e2e/k8sreporter/reporter.go (1)
145-147: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePre-allocate the slice to avoid multiple allocations.
Since the total number of containers is known beforehand, you can pre-allocate the slice with the exact capacity. This avoids potential memory reallocations during
append.♻️ Proposed refactor
- containersToLog := make([]corev1.Container, 0) + containersToLog := make([]corev1.Container, 0, len(pod.Spec.Containers)+len(pod.Spec.InitContainers)) containersToLog = append(containersToLog, pod.Spec.Containers...)🤖 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/k8sreporter/reporter.go` around lines 145 - 147, Update the containersToLog initialization in the reporter flow to pre-allocate capacity for len(pod.Spec.Containers)+len(pod.Spec.InitContainers), while retaining the existing append operations and resulting container order.
🤖 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 `@Dockerfile`:
- Line 2: Pin the golang:1.26 base image by appending its verified sha256 digest
in Dockerfile at lines 2-2 and Dockerfile.daemon at lines 1-1; apply the same
digest-pinned reference at both sites and preserve the existing builder
configuration.
In `@Dockerfile.daemon`:
- Line 1: Add a non-root USER directive to the final stage of the Dockerfile,
using a valid non-root UID and placing it after the final-stage setup so the
container does not run as root by default. Leave the builder stage unchanged.
In `@pkg/webhook/webhook_suite_test.go`:
- Line 148: Update the cleanup logic in the Eventually block around conn.Close
so its error return is handled rather than discarded. Return the error from the
callback when conn.Close fails, while preserving the existing successful
teardown behavior.
---
Nitpick comments:
In `@test/e2e/k8sreporter/reporter.go`:
- Around line 145-147: Update the containersToLog initialization in the reporter
flow to pre-allocate capacity for
len(pod.Spec.Containers)+len(pod.Spec.InitContainers), while retaining the
existing append operations and resulting container order.
In `@test/e2e/namespaces/namespaces.go`:
- Line 68: Update the error construction in the pod-deletion flow to use the
wrapping verb %w instead of %v, preserving the underlying error for upstream
unwrapping while retaining the existing contextual message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| @@ -1,5 +1,5 @@ | |||
| # Build the manager binary | |||
| FROM golang:1.25 as builder | |||
| FROM golang:1.26 as builder | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Pin non-Red Hat base images by digest.
Both files use the floating tag golang:1.26, which violates the container security path instructions. Non-RH images must be pinned by digest to ensure reproducible and secure builds.
Dockerfile#L2-L2: append the@sha256:...digest to thegolang:1.26base image.Dockerfile.daemon#L1-L1: append the@sha256:...digest to thegolang:1.26base image.
📍 Affects 2 files
Dockerfile#L2-L2(this comment)Dockerfile.daemon#L1-L1
🤖 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 `@Dockerfile` at line 2, Pin the golang:1.26 base image by appending its
verified sha256 digest in Dockerfile at lines 2-2 and Dockerfile.daemon at lines
1-1; apply the same digest-pinned reference at both sites and preserve the
existing builder configuration.
Source: Path instructions
| @@ -1,4 +1,4 @@ | |||
| FROM golang:1.25 AS builder | |||
| FROM golang:1.26 AS builder | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Specify a non-root user for the final image.
The final container image runs as root by default, which is flagged by the static analysis tools. As per the container security path instructions, never run as root and ensure the image defines a non-root USER.
Although the FROM declaration for the final stage isn't directly modified in this PR, please ensure a USER <uid> directive is added to the final stage of this multi-stage build to comply with the security guidelines.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 `@Dockerfile.daemon` at line 1, Add a non-root USER directive to the final
stage of the Dockerfile, using a valid non-root UID and placing it after the
final-stage setup so the container does not run as root by default. Leave the
builder stage unchanged.
Sources: Path instructions, Linters/SAST tools
| return err | ||
| } | ||
| conn.Close() | ||
| _ = conn.Close() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not ignore error returns.
As per path instructions, never ignore error returns. Even though this is within an Eventually block, explicitly ignoring the conn.Close() error masks potential teardown failures. Please handle the error appropriately by returning it.
🛠️ Proposed fix
- _ = conn.Close()
- return nil
+ return conn.Close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = conn.Close() | |
| return conn.Close() |
🤖 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/webhook/webhook_suite_test.go` at line 148, Update the cleanup logic in
the Eventually block around conn.Close so its error return is handled rather
than discarded. Return the error from the callback when conn.Close fails, while
preserving the existing successful teardown behavior.
Source: Path instructions
1 participant
Summary
What changed
Commits
First 2 commits are mechanical (dep bump + version refs). The rest are
fixes surfaced by the bumped dependencies and linter.
Verification
go build ./...passes (Go 1.26)go vet ./...passesAll commits carry
Assisted-by: Claude Codetrailers.Summary by CodeRabbit
Build & Compatibility
Developer Experience
Reliability