REPLACE-WITH-JIRA-KEY: Bump Kubernetes dependencies to v1.36.2#734
Draft
dfarrell07 wants to merge 8 commits into
Draft
REPLACE-WITH-JIRA-KEY: Bump Kubernetes dependencies to v1.36.2#734dfarrell07 wants to merge 8 commits into
dfarrell07 wants to merge 8 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>
Go 1.26 vet catches that IntOrString is a struct, not a string, so %q requires .String() to be called explicitly. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
golangci-lint v1 cannot parse Go 1.26 source code. Update Makefile and hack/lint.sh to use v2. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Two files imported k8s.io/klog (v1) instead of k8s.io/klog/v2. The API surface used (Info, Infof) is identical between versions. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Update kindest/node from v1.27.3 to v1.36.1 to match the k8s 1.36 dependency bump. Convert kubeadm extraArgs from map format to list-of-name-value-pairs format required by v1beta4. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Replace deprecated io/ioutil with os package equivalents. Remove duplicate package imports flagged by staticcheck ST1019. Signed-off-by: Daniel Farrell <dfarrell@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Add .golangci.yml config to exclude style-only suggestions (QF1008, QF1001, QF1003) and dot imports in test files (ST1001). Fix ST1005 capitalized error strings, SA4010 unused append result, S1039 unnecessary Sprintf, and SA1019 scheme.Builder deprecation. 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. |
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 |
WalkthroughThe PR upgrades the project to Go 1.26 and golangci-lint v2, refreshes dependencies and Kubernetes test tooling, modernizes API and filesystem references, normalizes errors, and adjusts BPFMan detach and attach handling. ChangesRuntime and tooling modernization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controllers/ingressnodefirewall_controller.go (1)
107-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not silently ignore the error returned by
isUsingBpfmanManager.Both blocks use an inline condition
yes && err == nilthat evaluates to false if an error is returned (e.g., due to an API error when fetching the configuration). This causes the error to be silently swallowed, resulting in skipped eBPF program detachment/attachment without logging or bubbling up the reconciliation error. As per path instructions, "Never ignore error returns".
controllers/ingressnodefirewall_controller.go#L107-L113: Extract theisUsingBpfmanManagercall, check iferr != nil, and explicitly log and returnctrl.Result{}, errbefore checkingif yes.controllers/ingressnodefirewall_controller.go#L362-L378: Extract theisUsingBpfmanManagercall, explicitly handleerr != nilby logging the error and updatingstate.StatustoSyncError, and then proceed withif yes.🤖 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 `@controllers/ingressnodefirewall_controller.go` around lines 107 - 113, The inline isUsingBpfmanManager checks must not discard errors. In controllers/ingressnodefirewall_controller.go lines 107-113, extract the call, log and return ctrl.Result{}, err when it fails, then check yes before detaching; in lines 362-378, extract the call, log failures, set state.Status to SyncError, and then proceed with the yes branch.Source: Path instructions
test/e2e/k8sreporter/reporter.go (1)
149-154: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDo not ignore error returns.
As per Go security path instructions, you must never ignore error returns. Currently, if an error occurs while fetching the pod logs, it is silently swallowed. Please log the error so that failures during test log gathering are observable.
♻️ Proposed fix to handle the error
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)) + if err != nil { + fmt.Fprintf(os.Stderr, "failed to get logs for pod %s-%s-%s: %v\n", pod.Namespace, pod.Name, container.Name, err) + continue } + 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))🤖 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 149 - 154, Handle the non-nil error returned by the pod log retrieval in the reporter’s log-gathering flow instead of silently skipping it. Add an error log that includes the pod and container context plus the returned error, while preserving the existing output behavior when retrieval succeeds.Source: Path instructions
🧹 Nitpick comments (4)
pkg/utils/utils.go (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the extra space in the error message.
There is an unintended double space between
foundand%q.✏️ Proposed fix
- return 0, 0, fmt.Errorf("invalid ports range. Expected two integers separated by hyphen but found %q", p.Ports.String()) + return 0, 0, fmt.Errorf("invalid ports range. Expected two integers separated by hyphen but found %q", p.Ports.String())🤖 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/utils/utils.go` at line 40, Remove the unintended extra space between “found” and the quoted value in the error message returned by the ports-range validation in the relevant utility function, leaving the rest of the message unchanged.Makefile (1)
389-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnsure
golangci-lintis executable regardless of$PATHconfiguration.The
golangci-lintexecutable is installed to the Go bin directory, but the subsequent command assumes it is available in the system$PATH. If$GOPATH/binor$HOME/go/binis not in$PATH, thelinttarget will fail. Consider invoking it via its explicit path.♻️ Proposed refactor
- GOFLAGS="" GOLANGCI_LINT_CACHE=/tmp/golangci-lint-cache go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) 2>/dev/null && GOLANGCI_LINT_CACHE=/tmp/golangci-lint-cache golangci-lint run --verbose --timeout=15m0s + GOFLAGS="" GOLANGCI_LINT_CACHE=/tmp/golangci-lint-cache go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) 2>/dev/null && GOLANGCI_LINT_CACHE=/tmp/golangci-lint-cache $$(go env GOPATH)/bin/golangci-lint run --verbose --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 `@Makefile` at line 389, Update the lint target’s golangci-lint invocation after installation to use the executable’s explicit Go bin path rather than relying on $PATH, while preserving the existing flags, cache settings, and timeout.test/e2e/namespaces/namespaces.go (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
%wover%vfor wrapping errors.When formatting errors with
fmt.Errorf, it is idiomatic in modern Go to use%wto wrap the original error. This preserves the error type and allows callers to inspect it usingerrors.Isanderrors.As. Consider also adding a colon for better readability.♻️ 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 formatting in the pod-deletion error path to use %w instead of %v and include a colon separator, preserving the original error for errors.Is and errors.As callers. Modify the fmt.Errorf call returning “failed to delete pods” while keeping its existing context and behavior.hack/lint.sh (1)
7-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix shell quoting issues to prevent word splitting.
The script has unquoted variables and arrays that can cause issues if arguments or the current working directory contain spaces (as flagged by Shellcheck).
- Line 8: Mixing a string with
$@expands to multiple arguments. Use$*to print the arguments as a single space-separated string.- Line 12: Unquoted
$(pwd)will cause word splitting if the path contains spaces, breaking the container runtime arguments.♻️ Proposed fixes
if [ "$#" -ne 1 ]; then - echo "Expected command line argument - container runtime (docker/podman) got $# arguments: $@" + echo "Expected command line argument - container runtime (docker/podman) got $# arguments: $*" exit 1 fi -$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 --show-stats \🤖 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 7 - 14, Fix shell quoting in the argument-validation and volume-mount logic of the script: use a single space-separated expansion when reporting all received arguments in the `"$#"` error branch, and quote the `$(pwd)` substitution in the `run` command’s volume mapping so paths containing spaces remain one argument.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: Update Dockerfile line 2 to pin the golang:1.26 image by digest and
add an explicit HEALTHCHECK. Update Dockerfile.daemon lines 1-1 to use the
digest-pinned base image, and its final execution stage at lines 7-10 to define
a HEALTHCHECK and run under a non-root USER.
In `@go.mod`:
- Around line 15-32: Update the k8s.io/kubernetes dependency in go.mod to a
patched release newer than v1.36.2 that resolves GO-2025-3521 and GO-2025-3547;
if the module is only required by tests, remove its direct require instead.
Regenerate dependency metadata as needed while keeping the Kubernetes dependency
set consistent.
---
Outside diff comments:
In `@controllers/ingressnodefirewall_controller.go`:
- Around line 107-113: The inline isUsingBpfmanManager checks must not discard
errors. In controllers/ingressnodefirewall_controller.go lines 107-113, extract
the call, log and return ctrl.Result{}, err when it fails, then check yes before
detaching; in lines 362-378, extract the call, log failures, set state.Status to
SyncError, and then proceed with the yes branch.
In `@test/e2e/k8sreporter/reporter.go`:
- Around line 149-154: Handle the non-nil error returned by the pod log
retrieval in the reporter’s log-gathering flow instead of silently skipping it.
Add an error log that includes the pod and container context plus the returned
error, while preserving the existing output behavior when retrieval succeeds.
---
Nitpick comments:
In `@hack/lint.sh`:
- Around line 7-14: Fix shell quoting in the argument-validation and
volume-mount logic of the script: use a single space-separated expansion when
reporting all received arguments in the `"$#"` error branch, and quote the
`$(pwd)` substitution in the `run` command’s volume mapping so paths containing
spaces remain one argument.
In `@Makefile`:
- Line 389: Update the lint target’s golangci-lint invocation after installation
to use the executable’s explicit Go bin path rather than relying on $PATH, while
preserving the existing flags, cache settings, and timeout.
In `@pkg/utils/utils.go`:
- Line 40: Remove the unintended extra space between “found” and the quoted
value in the error message returned by the ports-range validation in the
relevant utility function, leaving the rest of the message unchanged.
In `@test/e2e/namespaces/namespaces.go`:
- Line 68: Update the error formatting in the pod-deletion error path to use %w
instead of %v and include a colon separator, preserving the original error for
errors.Is and errors.As callers. Modify the fmt.Errorf call returning “failed to
delete pods” while keeping its existing context and behavior.
🪄 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
Address container security path instructions across Dockerfiles.
Both Dockerfile and Dockerfile.daemon are missing key security configurations required by the path instructions for container security, including digest pinning, explicit health checks, and non-root users.
Dockerfile#L2-L2: Pin the non-Red Hatgolang:1.26base image by digest (e.g.,golang:1.26@sha256:...) and define aHEALTHCHECK.Dockerfile.daemon#L1-L1: Pin the non-Red Hatgolang:1.26base image by digest.Dockerfile.daemon#L7-L10: Define aHEALTHCHECKand specify a non-rootUSERfor the final execution stage to prevent it from running as root (as also flagged by Trivy).
📍 Affects 2 files
Dockerfile#L2-L2(this comment)Dockerfile.daemon#L1-L1Dockerfile.daemon#L7-L10
🤖 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, Update Dockerfile line 2 to pin the golang:1.26 image
by digest and add an explicit HEALTHCHECK. Update Dockerfile.daemon lines 1-1 to
use the digest-pinned base image, and its final execution stage at lines 7-10 to
define a HEALTHCHECK and run under a non-root USER.
Sources: Path instructions, Linters/SAST tools
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-20260716164659-7926d144f96a | ||
| 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/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/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/v2 v2.140.0 | ||
| 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 | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for known CVEs in the updated Go dependencies using the OSV API.
deps=(
"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-20260716164659-7926d144f96a"
"github.com/pkg/errors@v0.9.1"
"github.com/prometheus/client_golang@v1.23.2"
"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.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/v2@v2.140.0"
"k8s.io/kubernetes@v1.36.2"
"k8s.io/utils@v0.0.0-20260707023825-cf1189d6abe3"
"sigs.k8s.io/controller-runtime@v0.24.1"
)
echo "Checking updated dependencies against OSV.dev..."
for dep in "${deps[@]}"; do
pkg="${dep%@*}"
ver="${dep#*@}"
# Clean up pseudo-version formats for OSV queries if needed,
# but OSV handles standard module queries.
curl -s -X POST -H "Content-Type: application/json" \
-d "{\"version\": \"$ver\", \"package\": {\"name\": \"$pkg\", \"ecosystem\": \"Go\"}}" \
"https://api.osv.dev/v1/query" | jq -c "{package: \"$pkg\", version: \"$ver\", vulns: .vulns}"
doneRepository: openshift/ingress-node-firewall
Length of output: 3848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant go.mod section with line numbers.
sed -n '1,80p' go.mod | cat -n
# Show whether k8s.io/kubernetes is referenced elsewhere in the repo.
rg -n --hidden --glob '!**/vendor/**' 'k8s\.io/kubernetes' .Repository: openshift/ingress-node-firewall
Length of output: 4562
Bump k8s.io/kubernetes off v1.36.2
go.mod:30 still pulls in GO-2025-3521 / GO-2025-3547 (CVE-2025-1767, CVE-2024-7598). Move to a patched release, or drop the direct require if it is only needed for tests.
🤖 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, Update the k8s.io/kubernetes dependency in
go.mod to a patched release newer than v1.36.2 that resolves GO-2025-3521 and
GO-2025-3547; if the module is only required by tests, remove its direct require
instead. Regenerate dependency metadata as needed while keeping the Kubernetes
dependency set consistent.
Source: Path instructions
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
Bumps all k8s.io/* dependencies from v0.35.2 to v0.36.2 (Kubernetes 1.36.2) and Go from 1.25 to 1.26.
What changed
Commits
Verification
go build ./...— PASSgo vet ./...— PASSgolangci-lint run— PASS (v2.12.2)go test— 6/8 packages pass; 2 failures are pre-existing envtest infrastructure requirements (controllers and pkg/webhook need etcd/kube-apiserver binaries)All commits carry
Assisted-by: Claude Codetrailers.Summary by CodeRabbit