CORENET-7375 d/s merge Bump Kubernetes to 1.36.2 and Go to 1.26 - #123
CORENET-7375 d/s merge Bump Kubernetes to 1.36.2 and Go to 1.26#123miheer wants to merge 6 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
miheer
changed the title
|
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 project adopts Go 1.26 across CI, Docker, and module metadata, upgrades Prometheus and Kubernetes dependencies, and changes webhook certificate reload detection from content hashing to file metadata comparison with added tests. ChangesToolchain and dependency upgrade
Certificate reload detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@miheer: This pull request references CORENET-7359 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 spike 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build/Dockerfile (1)
15-23: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftHarden the final container image.
golang:1.26is not pinned by digest, and the single-stage image retains compiler tooling. The Dockerfile also runs as root and copies the entire context withADD .. Use a pinned builder stage, a minimal non-root runtime stage, selectiveCOPY, and add aHEALTHCHECK.As per path instructions, non-Red Hat images must be pinned by digest, use multi-stage builds, run as non-root, copy specific files, and define
HEALTHCHECK.🤖 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 `@build/Dockerfile` around lines 15 - 23, Harden the Dockerfile by replacing the single-stage golang:1.26 build with a digest-pinned builder and minimal runtime stage; use selective COPY instead of ADD ., copy only the built webhook and required runtime files, create and switch to a non-root user, and add a HEALTHCHECK for the webhook. Preserve the existing build.sh invocation and executable entrypoint behavior.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 @.github/workflows/build.yml:
- Line 7: Pin every referenced CI action immutably by replacing mutable tags or
images with trusted full commit SHAs or digests: update actions/setup-go and
actions/checkout in .github/workflows/build.yml (line 7), and actions/setup-go,
actions/checkout, the Revive Docker action, and Coveralls in
.github/workflows/test.yml (line 7). Ensure all action references in both
workflows use reviewed immutable identifiers.
In @.github/workflows/test.yml:
- Line 7: Update the coverage flag configuration in the workflow step to
reference the matrix.go-version value defined by the go-version matrix,
replacing the incorrect matrix.go reference and preserving distinct coverage
flags for each Go version.
In `@go.mod`:
- Around line 12-30: Update the indirect golang.org/x/net dependency entry in
go.mod from v0.49.0 to a release that resolves the referenced OSV advisories,
preserving the existing dependency declaration and module formatting.
---
Outside diff comments:
In `@build/Dockerfile`:
- Around line 15-23: Harden the Dockerfile by replacing the single-stage
golang:1.26 build with a digest-pinned builder and minimal runtime stage; use
selective COPY instead of ADD ., copy only the built webhook and required
runtime files, create and switch to a non-root user, and add a HEALTHCHECK for
the webhook. Preserve the existing build.sh invocation and executable entrypoint
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
| strategy: | ||
| matrix: | ||
| go-version: [1.25.x] | ||
| go-version: [1.26.x] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin all CI action references immutably.
Both workflows execute mutable action tags/images, allowing upstream changes to alter CI behavior without a reviewed commit change.
.github/workflows/build.yml#L7-L7: pinactions/setup-goandactions/checkoutto trusted full commit SHAs..github/workflows/test.yml#L7-L7: pinactions/setup-go,actions/checkout, the Revive Docker action, and Coveralls to trusted SHAs/digests.
As per path instructions, CI/CD actions must be pinned by full SHA, not tag.
📍 Affects 2 files
.github/workflows/build.yml#L7-L7(this comment).github/workflows/test.yml#L7-L7
🤖 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 @.github/workflows/build.yml at line 7, Pin every referenced CI action
immutably by replacing mutable tags or images with trusted full commit SHAs or
digests: update actions/setup-go and actions/checkout in
.github/workflows/build.yml (line 7), and actions/setup-go, actions/checkout,
the Revive Docker action, and Coveralls in .github/workflows/test.yml (line 7).
Ensure all action references in both workflows use reviewed immutable
identifiers.
Source: Path instructions
| strategy: | ||
| matrix: | ||
| go-version: [1.25.x] | ||
| go-version: [1.26.x] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'matrix\.(go|go-version)' .github/workflows/test.ymlRepository: openshift/multus-admission-controller
Length of output: 265
🏁 Script executed:
sed -n '1,80p' .github/workflows/test.ymlRepository: openshift/multus-admission-controller
Length of output: 1250
Use matrix.go-version in the coverage flag name. The workflow matrix defines go-version, but this step reads matrix.go, so the flag becomes empty and can collide across versions.
Proposed fix
- flag-name: Go-${{ matrix.go }}
+ flag-name: Go-${{ matrix.go-version }}🤖 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 @.github/workflows/test.yml at line 7, Update the coverage flag configuration
in the workflow step to reference the matrix.go-version value defined by the
go-version matrix, replacing the incorrect matrix.go reference and preserving
distinct coverage flags for each Go version.
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 `@cmd/webhook/main_test.go`:
- Line 90: Handle the cleanup errors at both os.Remove sites in
cmd/webhook/main_test.go:90-90 and cmd/webhook/main_test.go:124-124 instead of
discarding them. Assert removal succeeds at the first site; at the second,
accept os.ErrNotExist after the successful rename while reporting or failing on
other errors.
In `@cmd/webhook/main.go`:
- Around line 151-155: Update certificateFileChanged to detect in-place
certificate content changes that preserve inode, size, and modification time by
comparing a retained content fingerprint (or equivalent file-content state)
during polling. Ensure the polling flow stores and updates that fingerprint when
the certificate is read, and add a regression case covering same-size content
with preserved modification time.
- Around line 132-144: Capture the certificate file metadata immediately before
constructing NewTLSKeypairReloader, handling stat errors consistently, and
initialize previousCertInfo with that baseline before server startup/loading the
keypair. Remove the later first-poll-only initialization so replacements
occurring during startup are detected and trigger SIGHUP.
🪄 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: 50aa060a-5115-401c-afbf-af1b0e926a2c
📒 Files selected for processing (2)
cmd/webhook/main.gocmd/webhook/main_test.go
| certPath = certFile.Name() | ||
| Expect(certFile.Close()).To(Succeed()) | ||
| cleanup = func() { | ||
| _ = os.Remove(certPath) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Handle cleanup failures explicitly.
cmd/webhook/main_test.go#L90-L90: assert or otherwise handle failure to remove the test certificate.cmd/webhook/main_test.go#L124-L124: handleos.Removeerrors, allowingos.ErrNotExistafter the successful rename.
As per path instructions, “Never ignore error returns.”
📍 Affects 1 file
cmd/webhook/main_test.go#L90-L90(this comment)cmd/webhook/main_test.go#L124-L124
🤖 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 `@cmd/webhook/main_test.go` at line 90, Handle the cleanup errors at both
os.Remove sites in cmd/webhook/main_test.go:90-90 and
cmd/webhook/main_test.go:124-124 instead of discarding them. Assert removal
succeeds at the first site; at the second, accept os.ErrNotExist after the
successful rename while reporting or failing on other errors.
Sources: Path instructions, Linters/SAST tools
| var previousCertInfo os.FileInfo | ||
| for { | ||
| hasher := sha512.New() | ||
| certPath, err := filepath.Abs(*cert) | ||
| currentCertInfo, err := os.Stat(*cert) | ||
| if err != nil { | ||
| glog.Fatalf("illegal path %s in certPath: %s: %v", *cert, certPath, err) | ||
| os.Exit(1) | ||
| glog.Fatalf("failed to stat certificate file %s: %v", *cert, err) | ||
| } | ||
| s, err := ioutil.ReadFile(certPath) | ||
| hasher.Write(s) | ||
| if err != nil { | ||
| glog.Fatalf("failed to read file %s: %v", *cert, err) | ||
| os.Exit(1) | ||
| } | ||
| newHashVal := hex.EncodeToString(hasher.Sum(nil)) | ||
| if oldHashVal != "" && newHashVal != oldHashVal { | ||
|
|
||
| if previousCertInfo != nil && certificateFileChanged(previousCertInfo, currentCertInfo) { | ||
| if err := proc.Signal(syscall.SIGHUP); err != nil { | ||
| glog.Fatalf("failed to send certificate update notification: %v", err) | ||
| } | ||
| } | ||
| oldHashVal = newHashVal | ||
| previousCertInfo = currentCertInfo |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Capture the baseline before loading the keypair.
NewTLSKeypairReloader loads the certificate at line 97, but this baseline is only captured after server startup. A replacement in that interval becomes previousCertInfo without sending SIGHUP, leaving the old in-memory certificate active until another update. Stat the certificate immediately before constructing the reloader and seed the polling state from that result.
🤖 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 `@cmd/webhook/main.go` around lines 132 - 144, Capture the certificate file
metadata immediately before constructing NewTLSKeypairReloader, handling stat
errors consistently, and initialize previousCertInfo with that baseline before
server startup/loading the keypair. Remove the later first-poll-only
initialization so replacements occurring during startup are detected and trigger
SIGHUP.
| func certificateFileChanged(previous, current os.FileInfo) bool { | ||
| return !os.SameFile(previous, current) || | ||
| previous.Size() != current.Size() || | ||
| !previous.ModTime().Equal(current.ModTime()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Metadata equality does not prove certificate equality.
An in-place rewrite can retain the inode and size, and can have the same modtime on coarse-resolution filesystems or when timestamps are preserved. This returns false and skips reloading changed TLS material. Retain a content fingerprint for polling, or use filesystem change notifications; add a same-size, preserved-modtime regression case.
🤖 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 `@cmd/webhook/main.go` around lines 151 - 155, Update certificateFileChanged to
detect in-place certificate content changes that preserve inode, size, and
modification time by comparing a retained content fingerprint (or equivalent
file-content state) during polling. Ensure the polling flow stores and updates
that fingerprint when the certificate is read, and add a regression case
covering same-size content with preserved modification time.
There was a problem hiding this comment.
I think the odds of hitting this in a prod env are so low that we can ignore this, if we ever hit this edge case we can re-eval
There was a problem hiding this comment.
@bpickard22 Understood. The edge case is valid but sufficiently low-risk to defer; we can revisit it if it occurs in production.
f61ec5e to
149f0c1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/webhook/main_test.go (1)
90-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHandle
os.Removeerror returns — still unaddressed from prior review.Both
os.Removecall sites discard the error return, violating the Go path instruction "Never ignore error returns." This was flagged in a previous review and remains unfixed.
cmd/webhook/main_test.go#L90-L90: assert that cleanup removal succeeds (or at minimum log unexpected errors).cmd/webhook/main_test.go#L124-L124: after the successfulos.Rename,os.ErrNotExistis acceptable; any other error should be reported or fail the test.🛡️ Proposed fix
cleanup = func() { - _ = os.Remove(certPath) + Expect(os.Remove(certPath)).To(Succeed()) }- defer os.Remove(replacementPath) + defer func() { + err := os.Remove(replacementPath) + Expect(errors.Is(err, os.ErrNotExist) || err == nil).To(BeTrue()) + }()Also applies to: 124-124
🤖 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 `@cmd/webhook/main_test.go` at line 90, Handle the error returns from both os.Remove cleanup calls in the webhook tests instead of discarding them. At the first call site, fail the test or report unexpected removal errors; at the second call site, allow os.ErrNotExist after the successful os.Rename but report or fail for any other error.Sources: Path instructions, 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.
Duplicate comments:
In `@cmd/webhook/main_test.go`:
- Line 90: Handle the error returns from both os.Remove cleanup calls in the
webhook tests instead of discarding them. At the first call site, fail the test
or report unexpected removal errors; at the second call site, allow
os.ErrNotExist after the successful os.Rename but report or fail for any other
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d4238fb-08eb-4335-8067-84ecb2c92253
📒 Files selected for processing (2)
cmd/webhook/main.gocmd/webhook/main_test.go
|
/lgtm |
|
Scheduling required tests: |
openshift-ci
Bot
added
the
approved
|
ci/prow/e2e-aws — Job failed. |
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/approve |
miheer
changed the title
openshift-ci-robot
removed
the
jira/valid-reference
|
@miheer: No Jira issue is referenced in the title of this pull request. 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
I have to yet create a upstream PR for the bump for this in k8snetworkplumbingwg/net-attach-def-admission-controller |
|
just created an upstream PR k8snetworkplumbingwg/net-attach-def-admission-controller#78 |
|
@bpickard22 I think we should consider just a pre-merge testing PR for testing upstream bump k8snetworkplumbingwg/net-attach-def-admission-controller#78 Once the upstream bump is merge then we can a downstream merge sync which might be larger than this. |
|
it looks like right way is to have a proper ds merge. i will update this PR to test the u/s to d/s merge. |
deps: bump Kubernetes to 1.36.2 and Go to 1.26
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.49.0 to 0.55.0. - [Commits](golang/net@v0.49.0...v0.55.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-version: 0.55.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
…go_modules/golang.org/x/net-0.55.0 Bump golang.org/x/net from 0.49.0 to 0.55.0
Sync downstream main with upstream master after upstream PR openshift#78 merged. This brings Kubernetes 1.36.2, Go 1.26, and the latest upstream x/net update. Keep the downstream TLS server changes and regenerate the component-base dependency and vendor files.
The security-openshift-ci-security-snyk-scan test failed with a low path traversal error. The tls-cert-file cli value was passed to ioutil.ReadFile while checking for certificate updates. snyk reported that this could read an arbitrary file. Stop reading the certificate contents in the watcher. Use file identity, size, and modified time to detect certificate changes. This also detects normal file updates and kubernetes secret replacements. Add tests for unchanged files, in-place updates, and atomic file replacements. (cherry picked from commit 149f0c1)
miheer
changed the title
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@bpickard22 PTAL. u/s PR k8snetworkplumbingwg/net-attach-def-admission-controller#78 is merged |
|
/lgtm |
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bpickard22, miheer 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 |
|
@miheer: 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. |
4 participants
Bump Kubernetes to 1.36.2 and Go to 1.26
Summary by CodeRabbit