REPLACE-WITH-JIRA-KEY: Bump Kubernetes dependencies to 1.36.2#732
Draft
dfarrell07 wants to merge 9 commits into
Draft
REPLACE-WITH-JIRA-KEY: Bump Kubernetes dependencies to 1.36.2#732dfarrell07 wants to merge 9 commits into
dfarrell07 wants to merge 9 commits into
Conversation
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/api@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 ./.rebase-tmp/k8s-rebase-patched.sh Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
IntOrString no longer satisfies the %q verb directly in k8s 1.36. Use .String() to get the string representation for formatting. 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>
Update kindest/node image to v1.36.1, Go version in generators.Dockerfile to 1.26.0, and fix stale lint version comment. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
golangci-lint v2 removed default exclusions for common false positives. Add .golangci.yml with exclusion presets and rules to suppress errcheck in tests and style suggestions that are not actionable (QF1008, QF1001, QF1003). Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Migrate ioutil.ReadFile to os.ReadFile and ioutil.ReadDir to os.ReadDir. Remove duplicate v1alpha1 import in loader. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Remove duplicate package imports in ebpfsyncer and namespaces. Lowercase error strings per Go conventions (ST1005) and update test assertions that match on the old error text. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Remove dead code building unused interfaces slice (SA4010), replace unnecessary fmt.Sprintf with string literal (S1039), deduplicate corev1 import in reporter (ST1019), and suppress scheme.Builder deprecation (SA1019) with nolint directive. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
WalkthroughChangesThe PR standardizes the project on Go 1.26, refreshes Kubernetes and related dependencies, upgrades golangci-lint to v2, updates Kubernetes test tooling, and applies compatibility and cleanup changes across ingress firewall components. Toolchain and runtime refresh
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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: 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
hack/generators.Dockerfile (2)
6-8: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMerge package installation and cache cleanup.
As per path instructions, there should be no package manager cache in the final layer. Since
dnf clean allruns in a separateRUNinstruction, the cache fromdnf installremains baked into the preceding layer. Combine these into a single command.🛡️ Proposed fix
-RUN dnf install -y git kernel-devel make llvm clang glibc-devel.i686 unzip clang-tools-extra - -RUN dnf clean all +RUN dnf install -y git kernel-devel make llvm clang glibc-devel.i686 unzip clang-tools-extra && \ + dnf clean all🤖 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 `@hack/generators.Dockerfile` around lines 6 - 8, Merge the package installation and cache cleanup steps into a single RUN instruction in the Dockerfile, chaining dnf install and dnf clean all so package-manager cache is removed within the same image layer.Source: Path instructions
33-33: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSpecify a non-root user.
As per path instructions, containers should never run as root. Consider specifying a
USERdirective before theENTRYPOINTif root privileges are not strictly required for the generation steps.🤖 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 `@hack/generators.Dockerfile` at line 33, Configure the generators container to run as a non-root user by adding an appropriate USER directive before the existing ENTRYPOINT. Ensure the selected user has the permissions required by the ebpf-generate target.Source: Path instructions
Dockerfile.daemon (1)
10-10: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSpecify a non-root user.
As per path instructions, containers should never run as root. Specify a
USERdirective in the final stage. If the daemon requires specific system privileges (e.g., for loading eBPF programs), it should run as a non-root user with explicitly granted capabilities in the deployment manifest.🤖 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 10, Update the final Dockerfile stage containing CMD ["/usr/bin/daemon"] to add a USER directive for a dedicated non-root user, ensuring that user exists and can access required daemon resources. Do not run the daemon as root; handle any required elevated capabilities through the deployment manifest.Source: Path instructions
🧹 Nitpick comments (1)
hack/lint.sh (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the
$(pwd)output to prevent word splitting.Using
$(pwd)unquoted can lead to word splitting if the directory path contains spaces, causing the container runtime command to fail.♻️ Proposed fix
-$1 run --security-opt label=disable --rm -v $(pwd):/app -w /app -e GO111MODULE=on golangci/golangci-lint:${VERSION} \ +$1 run --security-opt label=disable --rm -v "$(pwd):/app" -w /app -e GO111MODULE=on golangci/golangci-lint:${VERSION} \ golangci-lint run --verbose \ --modules-download-mode=vendor --timeout=15m0s && \🤖 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 `@hack/lint.sh` around lines 12 - 14, Update the volume mount argument in the lint command to quote the $(pwd) substitution, preventing directory paths containing spaces from being split by the shell while preserving the existing container invocation.Source: Linters/SAST tools
🤖 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 builder image by SHA256 digest instead of the floating
1.26 tag in both Dockerfile lines 2-2 and Dockerfile.daemon lines 1-1,
preserving the golang:1.26 version while using the digest for reproducible
builds.
In `@Dockerfile.daemon`:
- Line 3: Replace the broad COPY . . instruction in Dockerfile.daemon with
explicit COPY directives for only the files and directories required by the
daemon build, excluding sensitive and unnecessary context while preserving the
existing build behavior.
In `@go.mod`:
- Around line 15-32: Address the OSV findings for the k8s.io/kubernetes v1.36.2
dependency by upgrading to a fixed release if available; otherwise add the
project’s supported mitigation or documented upgrade path in the dependency
configuration. Update related Kubernetes module versions consistently and verify
the resulting dependency graph no longer leaves GO-2025-3521 or GO-2025-3547
unaddressed.
In `@test/e2e/k8sreporter/reporter.go`:
- Around line 129-130: Update the external Kubernetes API calls to use bounded
contexts: in test/e2e/k8sreporter/reporter.go lines 129-130, create and pass a
timeout context to the Pods List call; at lines 148-149, use a timeout context
for GetLogs. In test/e2e/namespaces/namespaces.go lines 21-22, pass the provided
ctx to Get; at lines 64-66, create and pass a timeout context to
DeleteCollection, ensuring each newly created context is properly canceled.
- Line 144: Update the file-handling loop in logLogs to close each opened file
explicitly at the end of its iteration instead of deferring f.Close(). Preserve
the existing processing and ensure the close occurs before continuing to the
next pod.
---
Outside diff comments:
In `@Dockerfile.daemon`:
- Line 10: Update the final Dockerfile stage containing CMD ["/usr/bin/daemon"]
to add a USER directive for a dedicated non-root user, ensuring that user exists
and can access required daemon resources. Do not run the daemon as root; handle
any required elevated capabilities through the deployment manifest.
In `@hack/generators.Dockerfile`:
- Around line 6-8: Merge the package installation and cache cleanup steps into a
single RUN instruction in the Dockerfile, chaining dnf install and dnf clean all
so package-manager cache is removed within the same image layer.
- Line 33: Configure the generators container to run as a non-root user by
adding an appropriate USER directive before the existing ENTRYPOINT. Ensure the
selected user has the permissions required by the ebpf-generate target.
---
Nitpick comments:
In `@hack/lint.sh`:
- Around line 12-14: Update the volume mount argument in the lint command to
quote the $(pwd) substitution, preventing directory paths containing spaces from
being split by the shell while preserving the existing container invocation.
🪄 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 | 🟠 Major | ⚡ Quick win
Pin non-Red Hat base images by digest.
As per path instructions for container security, non-Red Hat images must be pinned by digest rather than a floating tag like 1.26 to ensure reproducible builds and prevent supply chain attacks.
Dockerfile#L2-L2: pin thegolang:1.26image by its SHA256 digest.Dockerfile.daemon#L1-L1: pin thegolang:1.26image by its SHA256 digest.
📍 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 builder image by SHA256 digest instead
of the floating 1.26 tag in both Dockerfile lines 2-2 and Dockerfile.daemon
lines 1-1, preserving the golang:1.26 version while using the digest for
reproducible builds.
Source: Path instructions
| FROM golang:1.25 AS builder | ||
| FROM golang:1.26 AS builder | ||
| WORKDIR /go/src/github.com/openshift/ingress-node-firewall | ||
| COPY . . |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Copy specific files instead of the entire context.
As per path instructions, avoid using COPY . .. Instead, explicitly copy only the files necessary for the build. This prevents unintended inclusion of sensitive or unnecessary files and improves build cache efficiency.
🤖 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 3, Replace the broad COPY . . instruction in
Dockerfile.daemon with explicit COPY directives for only the files and
directories required by the daemon build, excluding sensitive and unnecessary
context while preserving the existing build behavior.
Source: Path instructions
Comment on lines
+15
to
+32
| github.com/openshift/api v0.0.0-20260715165912-72066cc9718b | ||
| github.com/openshift/controller-runtime-common v0.0.0-20260428152732-64ee174f5e2e | ||
| github.com/openshift/library-go v0.0.0-20260615113748-bc9d4056464b | ||
| github.com/openshift/library-go v0.0.0-20260716135552-61990a0aa401 | ||
| github.com/pkg/errors v0.9.1 | ||
| github.com/prometheus/client_golang v1.23.2 | ||
| github.com/prometheus/common v0.66.1 | ||
| github.com/vishvananda/netlink v1.3.1-0.20250206174618-62fb240731fa | ||
| golang.org/x/sys v0.42.0 | ||
| github.com/prometheus/common v0.67.5 | ||
| github.com/vishvananda/netlink v1.3.1 | ||
| golang.org/x/sys v0.45.0 | ||
| gopkg.in/mcuadros/go-syslog.v2 v2.3.0 | ||
| k8s.io/api v0.35.2 | ||
| k8s.io/apiextensions-apiserver v0.35.1 | ||
| k8s.io/apimachinery v0.35.2 | ||
| k8s.io/client-go v0.35.2 | ||
| k8s.io/component-base v0.35.1 | ||
| k8s.io/api v0.36.2 | ||
| k8s.io/apiextensions-apiserver v0.36.2 | ||
| k8s.io/apimachinery v0.36.2 | ||
| k8s.io/client-go v0.36.2 | ||
| k8s.io/component-base v0.36.2 | ||
| k8s.io/klog v1.0.0 | ||
| k8s.io/kubernetes v1.32.3 | ||
| k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 | ||
| sigs.k8s.io/controller-runtime v0.23.3 | ||
| k8s.io/kubernetes v1.36.2 | ||
| k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 | ||
| sigs.k8s.io/controller-runtime v0.24.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Scan go.mod for known vulnerabilities.
go install github.com/google/osv-scanner/cmd/osv-scanner@latest
osv-scanner --lockfile=go.modRepository: openshift/ingress-node-firewall
Length of output: 6039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod excerpt =="
nl -ba go.mod | sed -n '1,120p'
echo
echo "== search for k8s.io/kubernetes usage =="
rg -n 'k8s.io/kubernetes|GO-2025-3521|GO-2025-3547' -S .Repository: openshift/ingress-node-firewall
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod excerpt =="
sed -n '1,120p' go.mod | cat -n
echo
echo "== search for k8s.io/kubernetes usage =="
rg -n 'k8s.io/kubernetes|GO-2025-3521|GO-2025-3547' -S .Repository: openshift/ingress-node-firewall
Length of output: 14125
k8s.io/kubernetes v1.36.2 is flagged by OSV (GO-2025-3521, GO-2025-3547) with no fixed version yet; add a mitigation or upgrade path before merging.
🤖 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 15 - 32, Address the OSV findings for the
k8s.io/kubernetes v1.36.2 dependency by upgrading to a fixed release if
available; otherwise add the project’s supported mitigation or documented
upgrade path in the dependency configuration. Update related Kubernetes module
versions consistently and verify the resulting dependency graph no longer leaves
GO-2025-3521 or GO-2025-3547 unaddressed.
Source: Path instructions
Comment on lines
129
to
+130
| func (r *KubernetesReporter) logLogs(filterPods func(*corev1.Pod) bool, dirName string) { | ||
| pods, err := r.clients.Pods(v1.NamespaceAll).List(context.Background(), metav1.ListOptions{}) | ||
| pods, err := r.clients.Pods(corev1.NamespaceAll).List(context.Background(), metav1.ListOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing timeout contexts for external API calls.
As per path instructions, always use a context.Context configured with a timeout or cancellation for external calls to prevent deadlocks if the Kubernetes API server is unresponsive.
test/e2e/k8sreporter/reporter.go#L129-L130: replacecontext.Background()with a timeout context for theListcall.test/e2e/k8sreporter/reporter.go#L148-L149: replacecontext.Background()with a timeout context for theGetLogscall.test/e2e/namespaces/namespaces.go#L21-L22: replacecontext.Background()with the providedctx(which already has a timeout applied) in theGetcall.test/e2e/namespaces/namespaces.go#L64-L66: replacecontext.Background()with a timeout context for theDeleteCollectioncall.
📍 Affects 2 files
test/e2e/k8sreporter/reporter.go#L129-L130(this comment)test/e2e/k8sreporter/reporter.go#L148-L149test/e2e/namespaces/namespaces.go#L21-L22test/e2e/namespaces/namespaces.go#L64-L66
🤖 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 129 - 130, Update the external
Kubernetes API calls to use bounded contexts: in
test/e2e/k8sreporter/reporter.go lines 129-130, create and pass a timeout
context to the Pods List call; at lines 148-149, use a timeout context for
GetLogs. In test/e2e/namespaces/namespaces.go lines 21-22, pass the provided ctx
to Get; at lines 64-66, create and pass a timeout context to DeleteCollection,
ensuring each newly created context is properly canceled.
Source: Path instructions
| @@ -143,11 +142,11 @@ func (r *KubernetesReporter) logLogs(filterPods func(*corev1.Pod) bool, dirName | |||
| return | |||
| } | |||
| defer f.Close() | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Resource leak: defer f.Close() inside a loop.
Using defer f.Close() inside the for loop will delay closing the file descriptors until the entire logLogs function returns. If the cluster has many pods, this can exhaust the process's open file limit.
🐛 Proposed fix
Replace the defer with explicit closing at the end of the loop iteration:
if err != nil {
fmt.Fprintf(os.Stderr, "failed to open pods_logs file: %v\n", dirName)
return
}
- defer f.Close()
containersToLog := make([]corev1.Container, 0)
containersToLog = append(containersToLog, pod.Spec.Containers...)
containersToLog = append(containersToLog, pod.Spec.InitContainers...)
for _, container := range containersToLog {
logs, err := r.clients.Pods(pod.Namespace).GetLogs(pod.Name, &corev1.PodLogOptions{Container: container.Name}).DoRaw(context.Background())
if err == nil {
fmt.Fprintf(f, "-----------------------------------\n")
fmt.Fprintf(f, "Dumping logs for pod %s-%s-%s\n", pod.Namespace, pod.Name, container.Name)
fmt.Fprintln(f, string(logs))
}
}
+ f.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.
Suggested change
| defer f.Close() | |
| f.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 `@test/e2e/k8sreporter/reporter.go` at line 144, Update the file-handling loop
in logLogs to close each opened file explicitly at the end of its iteration
instead of deferring f.Close(). Preserve the existing processing and ensure the
close occurs before continuing to the next pod.
This was referenced Jul 17, 2026
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
1 participant
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
What changed
Commits
Verification
go build ./...passesgo vet ./...passesgolangci-lint runpasses (0 issues)Notes
k8s.io/klogv1 imports remain (pkg/bpf-mgr, pkg/ebpf) — the project intentionally uses klog v1scheme.Builderdeprecation (SA1019) suppressed with nolint — migration to runtime.SchemeBuilder is a separate effortAssisted-by: Claude CodetrailersSummary by CodeRabbit
Compatibility
Bug Fixes
Maintenance