OCPBUGS-94014: Rebase v3.6.13 on main (5.0/4.23)#392
Open
dusk125 wants to merge 29 commits into
Open
Conversation
Signed-off-by: shenmu.wy <shenmu.wy@antfin.com>
….25.10 [release-3.6] Bump Go to 1.25.10
Signed-off-by: shenmu.wy <shenmu.wy@antfin.com>
…mberupdate-learner [release-3.6] bugfix: MemberUpdate implicitly and unexpectedly promotes a learner
… of panic when given non-existent paths Signed-off-by: shenmu.wy <shenmu.wy@antfin.com>
…nvalid-datadir [release-3.6] etcdutl: validate data file path instead of panic
- replace user.Current().Name == "root" with os.Getuid() == 0. - drop os/user import and user.Current() error path. - backport of etcd-io#21788 - address: etcd-io#21787 Signed-off-by: vivekpatani <9080894+vivekpatani@users.noreply.github.com>
…elease-3.6 [release-3.6] client/pkg/fileutil: use os.Getuid() to skip TestIsDirWriteable as root
Signed-off-by: kunal.behbudzade <kunal.behbudzade@btsgrp.com> Signed-off-by: Ian Chechin <ian00chechin@gmail.com>
[release-3.6] server: allow non-admin maintenance status
Co-authored-by: Benjamin Wang <benjamin.wang@broadcom.com> Signed-off-by: Sahil Patel <smppatel999@gmail.com>
Signed-off-by: Ivan Valdes <iv@a.ki>
…check-for-3.6 [release-3.6] Add `write-only-skip-check` option for `--v2-deprecation` to bypass the v2 content check
Signed-off-by: AR21SM <mahajanashishar21sm@gmail.com>
[release 3.6] Update Go to 1.25.11
Signed-off-by: AR21SM <mahajanashishar21sm@gmail.com>
…lease-3.6 [release-3.6] Bump golang.org/x/crypto to v0.52.0
Bumps distroless/static-debian12 from `20bc6c0` to `9c346e4`. --- updated-dependencies: - dependency-name: distroless/static-debian12 dependency-version: 9c346e4be81b5ca7ff31a0d89eaeade58b0f95cfd3baed1f36083ddb47ca3160 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…se-3.6/distroless/static-debian12-9c346e4be81b5ca7ff31a0d89eaeade58b0f95cfd3baed1f36083ddb47ca3160 build(deps): bump distroless/static-debian12 from `20bc6c0` to `9c346e4`
Signed-off-by: AR21SM <mahajanashishar21sm@gmail.com>
[release-3.6] Bump OpenTelemetry dependencies to v1.43.0
Signed-off-by: Sahil Patel <smppatel999@gmail.com>
…ry-pick-21910-to-release-3.6 [release-3.6] server/auth: accept bearer-prefixed auth tokens
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
…ry-pick-21953-to-release-3.6
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
[release-3.6] Reject the client request if the client certificate has been revoked
Signed-off-by: Ivan Valdes <iv@a.ki>
openshift-ci-robot
added
the
jira/valid-reference
|
@dusk125: This pull request references Jira Issue OCPBUGS-94014, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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-bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dusk125 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 |
openshift-ci
Bot
added
the
approved
WalkthroughThis PR bumps Go toolchain and module dependency versions across all go.mod files and the Dockerfile base image, adds TLS CRL verification support for gRPC-only mode, restricts ChangesVersion and dependency bumps
CRL verification for gRPC-only mode
UpdateRaftAttributes scoped to PeerURLs
Auth token normalization and Status RPC gate
V2 deprecation skip-check option
etcdutl input validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
server/etcdserver/api/membership/cluster.go (1)
622-637: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: non-idiomatic var-block + misleading log message.
Two small nits:
var (m *Member; ok bool)followed bym, ok = c.members[id]is more verbosely written than idiomatic Go'sif m, ok := c.members[id]; !ok {.- The log message "Skipped updating non-existent member in v2store" is misleading — the early return also skips the v3 backend update (
c.be.MustSaveMemberToBackend), not just v2store.✏️ Suggested tweak
- var ( - m *Member - ok bool - ) - - if m, ok = c.members[id]; !ok { - c.lg.Info("Skipped updating non-existent member in v2store", + m, ok := c.members[id] + if !ok { + c.lg.Info("Skipped updating non-existent member",🤖 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 `@server/etcdserver/api/membership/cluster.go` around lines 622 - 637, The member lookup in cluster.go is written with a separate var block and assignment, which is less idiomatic Go; simplify the non-existent-member check in the membership update path to use the usual short variable declaration in the conditional around c.members[id]. Also update the log text in that same branch in the cluster membership update flow to accurately describe that the early return skips both the v2store and the v3 backend save, so the message matches the behavior in the code path around c.be.MustSaveMemberToBackend.
🤖 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 `@client/pkg/transport/listener_tls.go`:
- Around line 169-190: Add a focused unit test for ConfigureCRLVerification in
listener_tls.go that exercises the key branches: no-op when TLSInfo.CRLFile is
empty, existing cfg.VerifyConnection being called first and short-circuiting on
error, nil return when tls.ConnectionState has no PeerCertificates, and
delegation to checkCRL for a connection with a certificate. Use TLSInfo and
ConfigureCRLVerification to locate the code, and assert both revoked and
non-revoked outcomes via a tls.ConnectionState setup.
---
Nitpick comments:
In `@server/etcdserver/api/membership/cluster.go`:
- Around line 622-637: The member lookup in cluster.go is written with a
separate var block and assignment, which is less idiomatic Go; simplify the
non-existent-member check in the membership update path to use the usual short
variable declaration in the conditional around c.members[id]. Also update the
log text in that same branch in the cluster membership update flow to accurately
describe that the early return skips both the v2store and the v3 backend save,
so the message matches the behavior in the code path around
c.be.MustSaveMemberToBackend.
🪄 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: 9e1f67e5-fa6e-428c-9765-b3cf88d54599
⛔ Files ignored due to path filters (12)
api/go.sumis excluded by!**/*.sumclient/pkg/go.sumis excluded by!**/*.sumclient/v3/go.sumis excluded by!**/*.sumetcdctl/go.sumis excluded by!**/*.sumetcdutl/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumpkg/go.sumis excluded by!**/*.sumserver/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumtools/mod/go.sumis excluded by!**/*.sumtools/rw-heatmaps/go.sumis excluded by!**/*.sumtools/testgrid-analysis/go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.go-versionDockerfileapi/go.modapi/version/version.goclient/pkg/fileutil/fileutil_test.goclient/pkg/go.modclient/pkg/transport/listener_tls.goclient/v3/go.modetcdctl/go.modetcdutl/etcdutl/common.goetcdutl/etcdutl/defrag_command.goetcdutl/etcdutl/hashkv_command.goetcdutl/etcdutl/migrate_command.goetcdutl/etcdutl/snapshot_command.goetcdutl/go.modgo.modpkg/go.modserver/auth/store.goserver/auth/store_test.goserver/config/v2_deprecation.goserver/embed/etcd.goserver/embed/serve.goserver/etcdmain/config.goserver/etcdmain/help.goserver/etcdserver/api/membership/cluster.goserver/etcdserver/api/membership/cluster_test.goserver/etcdserver/api/v3rpc/maintenance.goserver/go.modserver/storage/util.gotests/common/maintenance_auth_test.gotests/e2e/ctl_v3_kv_test.gotests/go.modtests/integration/clientv3/cluster_test.gotools/mod/go.modtools/rw-heatmaps/go.modtools/testgrid-analysis/go.mod
Comment on lines
+169
to
+190
| // ConfigureCRLVerification appends a VerifyConnection hook to cfg that | ||
| // rejects any peer certificate whose serial number appears in the CRL file | ||
| // configured on info. It is a no-op when CRLFile is empty. Any existing | ||
| // VerifyConnection hook is called first and its error short-circuits. | ||
| func (info TLSInfo) ConfigureCRLVerification(cfg *tls.Config) { | ||
| if len(info.CRLFile) == 0 { | ||
| return | ||
| } | ||
| crlFile := info.CRLFile | ||
| prev := cfg.VerifyConnection | ||
| cfg.VerifyConnection = func(cs tls.ConnectionState) error { | ||
| if prev != nil { | ||
| if err := prev(cs); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if len(cs.PeerCertificates) == 0 { | ||
| return nil | ||
| } | ||
| return checkCRL(crlFile, cs.PeerCertificates) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a unit test for ConfigureCRLVerification.
The method's logic (no-op when CRLFile empty, prior hook short-circuiting, allowing 0-cert connections, delegating to checkCRL) is a good candidate for a focused unit test (e.g., constructing a tls.ConnectionState with a revoked/non-revoked cert and asserting the returned error). Only an e2e test was added for this new security-relevant path.
As per coding guidelines, "All changes are expected to come with a unit test."
🤖 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 `@client/pkg/transport/listener_tls.go` around lines 169 - 190, Add a focused
unit test for ConfigureCRLVerification in listener_tls.go that exercises the key
branches: no-op when TLSInfo.CRLFile is empty, existing cfg.VerifyConnection
being called first and short-circuiting on error, nil return when
tls.ConnectionState has no PeerCertificates, and delegation to checkCRL for a
connection with a certificate. Use TLSInfo and ConfigureCRLVerification to
locate the code, and assert both revoked and non-revoked outcomes via a
tls.ConnectionState setup.
Source: Coding guidelines
|
@dusk125: The following tests failed, say
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. |
This was referenced Jul 7, 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
9 participants
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 by CodeRabbit
New Features
write-only-skip-check, and documented it in help text.Bug Fixes
Bearerprefix when read from gRPC metadata.