OCPBUGS-85258: 5.0 rebase 3.6.11#375
OCPBUGS-85258: 5.0 rebase 3.6.11#375openshift-merge-bot[bot] merged 19 commits intoopenshift:mainfrom
Conversation
In CI, the TestGateway and TestMixVersionsSnapshotByAddingMember are flaky due to the TestMixVersionsSnapshotByAddingMember test sometimes not closing the second etcd process. This happens if the second process has not had enough time to become healthy according to the logic in EtcdServer.mayRemoveMember. Fix this by retrying member removal for twice the etcdserver.HealthInterval in EtcdProcessCluster.CloseProc. Signed-off-by: Jonathan Albrecht <jonathan.albrecht@ibm.com>
…ck-of-#20840-upstream-release-3.6 Automated cherry pick of etcd-io#20840
Signed-off-by: Wei Fu <fuweid89@gmail.com>
[release-3.6] *: bump go to 1.25.9
…r is down Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
…g member is down Assume the new member is unavailable and check whether quorum is still preserved. Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
[release-3.6] Bump golang.org/x/image to v0.39.0 to resolve GO-2026-4962
[release-3.6] Fix the issue that cannot add a new member when one member is down, even if quorum is still satisfied
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
[release-3.6] Refactor auth check for Put requests in TXN
…rbac check issue Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
…XN bypass RBAC check Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
…ck issue Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
[release-3.6] Fix read access via PrevKv or lease attachment in a Put request in etcd transactions bypass RBAC authorization checks
Signed-off-by: Ivan Valdes <iv@a.ki>
WalkthroughUpgrades Go toolchain/dependencies and OpenShift base images; refactors txn authorization by moving auth checks into the apply layer (adding exported CheckTxnAuth); and relaxes member-add quorum gating from “connected to all peers” to “connected to a majority” with related test and helper changes. ChangesToolchain & Build Infrastructure
Authorization & Transaction Handling (apply-layer takeover)
Member Addition & Quorum Logic
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as EtcdServer
participant Apply as apply.CheckTxnAuth
participant AuthStore
participant Lessor
Client->>Server: Txn(request)
Server->>Apply: CheckTxnAuth(authStore, authInfo, lessor, txnReq)
Apply->>AuthStore: Validate compare key permissions
AuthStore-->>Apply: ok / denied
alt compares authorized
Apply->>Apply: checkTxnReqsPermission(successOps)
Apply->>AuthStore: IsRangePermitted / IsPutPermitted per op
AuthStore-->>Apply: ok / denied
end
alt lease-attached puts present
Apply->>Lessor: Lookup(leaseID)
Lessor-->>Apply: lease keys
Apply->>AuthStore: IsPutPermitted for lease keys
AuthStore-->>Apply: ok / denied
end
alt permission denied
Apply-->>Server: ErrPermissionDenied
Server-->>Client: PERMISSION_DENIED
else all checks pass
Apply-->>Server: nil
Server->>Server: Execute Txn
Server-->>Client: Txn result
end
sequenceDiagram
participant Admin
participant Server as EtcdServer
participant MemberAdd as mayAddMember
participant Util as util.isConnectedToQuorumAfterAddingNewMemberSince
participant Transporter
Admin->>Server: AddMember(newMember)
Server->>MemberAdd: mayAddMember(newMember)
MemberAdd->>Util: isConnectedToQuorumAfterAddingNewMemberSince(transport, since, self, members)
Util->>Util: compute quorum(currentMembers + 1)
Util->>Transporter: check connectivity to peers
Transporter-->>Util: active peers list
alt connected to majority after add
Util-->>MemberAdd: true
MemberAdd-->>Server: proceed
Server-->>Admin: Member added
else not connected to majority
Util-->>MemberAdd: false
MemberAdd-->>Server: error - would break active quorum
Server-->>Admin: FAILED - not connected to majority
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci-robot
added
jira/valid-reference
|
@dusk125: This pull request references Jira Issue OCPBUGS-85258, which is valid. The bug has been moved to the POST state. 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
Bot
added
the
approved
|
@dusk125: This pull request references Jira Issue OCPBUGS-85258, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
etcdutl/go.mod (1)
73-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpgrade
go.opentelemetry.io/otelto v1.41.0 to address HIGH severity DoS (GHSA-mh2q-q3fh-2475 / CVE-2026-29181).
go.opentelemetry.io/otelversions v1.36.0–v1.40.0 are affected; the fix is v1.41.0. The vulnerability allows attackers to amplify CPU and allocations by sending manybaggage:header lines, even when each individual value is within the per-value parse limit. CVSS score is7.5 HIGH(AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H).This dependency appears at v1.40.0 in
etcdutl/go.mod,go.mod,server/go.mod, andtests/go.mod. All affectedgo.modfiles should be updated together to v1.41.0 or above.🤖 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 `@etcdutl/go.mod` around lines 73 - 81, The import entry for go.opentelemetry.io/otel is pinned to v1.40.0 (e.g., the line "go.opentelemetry.io/otel v1.40.0 // indirect") and must be upgraded to v1.41.0 to fix CVE-2026-29181; update that module line in all affected go.mod files (etcdutl/go.mod, root go.mod, server/go.mod, tests/go.mod) to v1.41.0 (or later), run "go get go.opentelemetry.io/otel@v1.41.0" and "go mod tidy" in each module to refresh transitive deps, then run the project's tests/build to ensure nothing breaks.
🧹 Nitpick comments (2)
tests/e2e/ctl_v3_member_test.go (2)
660-660: ⚡ Quick win
time.SleepafterKill()does not wait for a new leader to be elected.If the killed member(s) include the current leader, the remaining cluster must hold a new election before
MemberAddAsLearnercan succeed.etcdserver.HealthInterval + 2*time.Second(≈ 2.5 s) is usually enough, but it's unconditional and timing-sensitive. AWaitLeadercall on the surviving members before proceeding would make the test deterministic without being slower on average.🔧 Suggested fix
time.Sleep(etcdserver.HealthInterval + 2*time.Second) + epc.WaitLeader(t)🤖 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 `@tests/e2e/ctl_v3_member_test.go` at line 660, Replace the unconditional time.Sleep after Kill() with a deterministic wait for a new leader by invoking WaitLeader on the surviving member(s) before calling MemberAddAsLearner; specifically, remove the time.Sleep(etcdserver.HealthInterval + 2*time.Second) and call the cluster/peer helper like survivingMember.WaitLeader(ctx, timeout) (or the existing test helper used elsewhere) so the test only proceeds once a leader is elected and MemberAddAsLearner is invoked reliably.
649-651: 💤 Low valueConsider logging or propagating
epc.Close()errors.Silently discarding the close error with
_ = epc.Close()can hide resource-cleanup failures between sub-test iterations. Other defers in this file userequire.NoError(t, epc.Close()). At minimum, at.Logfon error would help diagnose flaky teardowns.🔧 Suggested fix
- defer func() { - _ = epc.Close() - }() + defer func() { + require.NoError(t, epc.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 `@tests/e2e/ctl_v3_member_test.go` around lines 649 - 651, The defer is silently discarding epc.Close() errors; replace the anonymous defer using `_ = epc.Close()` with an explicit error check so teardown failures are surfaced—e.g., in the defer func() call epc.Close(), capture its error and call require.NoError(t, err) (or if outside a subtest context use t.Logf("epc.Close error: %v", err) then fail as appropriate). Update the defer that wraps epc.Close() to use that explicit check instead of silencing the return value.
🤖 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 `@server/etcdserver/apply/apply_auth_test.go`:
- Around line 1049-1052: The test hardcodes auth.AuthInfo{Username: "foo",
Revision: 8} which can mismatch the authorizer's current revision and produce
ErrAuthOldRevision; update the table test to use the authorizer's current
revision by replacing the literal revision with as.Revision() when calling
CheckTxnAuth (i.e., pass &auth.AuthInfo{Username: "foo", Revision:
as.Revision()}), so CheckTxnAuth and the permission logic are validated against
the current auth revision.
In `@server/etcdserver/server.go`:
- Around line 1431-1432: The quorum-safety check currently always treats the new
member as a voting member by calling
isConnectedToQuorumAfterAddingNewMemberSince(s.r.transport, ..., s.MemberID(),
s.cluster.VotingMembers()); change this so the "post-add quorum bump" is only
applied when the member being added is a voting member — for learner additions
do NOT call the "AfterAddingNewMember" variant and instead check quorum against
the current voting set (i.e., use the non-post-add quorum check such as
isConnectedToQuorumSince or the equivalent check with s.cluster.VotingMembers()
that does not include the new member). Ensure you detect learner vs voter using
the member type information available in the add path and switch between
isConnectedToQuorumAfterAddingNewMemberSince(...) for voters and the non-bumped
quorum check for learners.
In `@tests/framework/e2e/cluster.go`:
- Around line 849-863: The loop that calls memberCtl.MemberRemove is treating
only the "member not found" error as success and ignores the successful case
(err == nil); update the loop in the function containing memberCtl.MemberRemove
so that if MemberRemove returns nil you set memberRemoved = true and break
immediately, otherwise keep the existing check for strings.Contains(err.Error(),
"member not found") to mark success; this change ensures both a successful
removal (nil error) and an already-removed state mark memberRemoved and stop
retrying.
In `@tests/integration/v3_auth_test.go`:
- Around line 384-390: The test currently silently falls back to READWRITE when
user.perm is non-empty but not a valid key in authpb.Permission_Type_value;
change the logic in the test setup so that when len(user.perm) > 0 and the
lookup into authpb.Permission_Type_value yields ok == false, the test fails
immediately (e.g., call t.Fatalf or require.FailNow) with a clear message
referencing the invalid user.perm, instead of assigning permType :=
authpb.READWRITE; otherwise keep the default behavior when user.perm is empty.
---
Outside diff comments:
In `@etcdutl/go.mod`:
- Around line 73-81: The import entry for go.opentelemetry.io/otel is pinned to
v1.40.0 (e.g., the line "go.opentelemetry.io/otel v1.40.0 // indirect") and must
be upgraded to v1.41.0 to fix CVE-2026-29181; update that module line in all
affected go.mod files (etcdutl/go.mod, root go.mod, server/go.mod, tests/go.mod)
to v1.41.0 (or later), run "go get go.opentelemetry.io/otel@v1.41.0" and "go mod
tidy" in each module to refresh transitive deps, then run the project's
tests/build to ensure nothing breaks.
---
Nitpick comments:
In `@tests/e2e/ctl_v3_member_test.go`:
- Line 660: Replace the unconditional time.Sleep after Kill() with a
deterministic wait for a new leader by invoking WaitLeader on the surviving
member(s) before calling MemberAddAsLearner; specifically, remove the
time.Sleep(etcdserver.HealthInterval + 2*time.Second) and call the cluster/peer
helper like survivingMember.WaitLeader(ctx, timeout) (or the existing test
helper used elsewhere) so the test only proceeds once a leader is elected and
MemberAddAsLearner is invoked reliably.
- Around line 649-651: The defer is silently discarding epc.Close() errors;
replace the anonymous defer using `_ = epc.Close()` with an explicit error check
so teardown failures are surfaced—e.g., in the defer func() call epc.Close(),
capture its error and call require.NoError(t, err) (or if outside a subtest
context use t.Logf("epc.Close error: %v", err) then fail as appropriate). Update
the defer that wraps epc.Close() to use that explicit check instead of silencing
the return value.
🪄 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: 577368ab-5095-4462-b0cf-12e5afe14f82
⛔ 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 (29)
.ci-operator.yaml.go-versionDockerfile.art-cachi2Dockerfile.installerDockerfile.installer.art-cachi2Dockerfile.rhelapi/go.modapi/version/version.goclient/pkg/go.modclient/v3/go.modetcdctl/go.modetcdutl/go.modgo.modpkg/go.modserver/etcdserver/apply/apply_auth.goserver/etcdserver/apply/apply_auth_test.goserver/etcdserver/server.goserver/etcdserver/txn/txn.goserver/etcdserver/txn/txn_test.goserver/etcdserver/util.goserver/etcdserver/v3_server.goserver/go.modtests/e2e/ctl_v3_member_test.gotests/framework/e2e/cluster.gotests/go.modtests/integration/v3_auth_test.gotools/mod/go.modtools/rw-heatmaps/go.modtools/testgrid-analysis/go.mod
💤 Files with no reviewable changes (2)
- server/etcdserver/txn/txn.go
- server/etcdserver/txn/txn_test.go
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := CheckTxnAuth(as, &auth.AuthInfo{Username: "foo", Revision: 8}, &lease.FakeLessor{}, tt.txnRequest) | ||
| assert.Equal(t, tt.err, err) |
There was a problem hiding this comment.
Use the current auth revision in the table test.
Line 1051 hardcodes Revision: 8, so any extra setup write will flip these cases to ErrAuthOldRevision and stop validating the permission logic you actually care about. Use as.Revision() here instead.
Suggested fix
- err := CheckTxnAuth(as, &auth.AuthInfo{Username: "foo", Revision: 8}, &lease.FakeLessor{}, tt.txnRequest)
+ err := CheckTxnAuth(as, &auth.AuthInfo{Username: "foo", Revision: as.Revision()}, &lease.FakeLessor{}, tt.txnRequest)📝 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.
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| err := CheckTxnAuth(as, &auth.AuthInfo{Username: "foo", Revision: 8}, &lease.FakeLessor{}, tt.txnRequest) | |
| assert.Equal(t, tt.err, err) | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| err := CheckTxnAuth(as, &auth.AuthInfo{Username: "foo", Revision: as.Revision()}, &lease.FakeLessor{}, tt.txnRequest) | |
| assert.Equal(t, tt.err, 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 `@server/etcdserver/apply/apply_auth_test.go` around lines 1049 - 1052, The
test hardcodes auth.AuthInfo{Username: "foo", Revision: 8} which can mismatch
the authorizer's current revision and produce ErrAuthOldRevision; update the
table test to use the authorizer's current revision by replacing the literal
revision with as.Revision() when calling CheckTxnAuth (i.e., pass
&auth.AuthInfo{Username: "foo", Revision: as.Revision()}), so CheckTxnAuth and
the permission logic are validated against the current auth revision.
| // Treat the new member as unavailable when checking quorum safety. | ||
| if !isConnectedToQuorumAfterAddingNewMemberSince(s.r.transport, time.Now().Add(-HealthInterval), s.MemberID(), s.cluster.VotingMembers()) { |
There was a problem hiding this comment.
Don't apply the post-add quorum bump to learner additions.
Line 1432 uses the “after adding a new member” quorum calculation for every add. That is correct for voting members, but not for learners: adding a learner does not increase the voting quorum. In a 3-voter cluster with one voter down, this now rejects an add-as-learner even though the current voting quorum is still intact.
Suggested fix
- // Treat the new member as unavailable when checking quorum safety.
- if !isConnectedToQuorumAfterAddingNewMemberSince(s.r.transport, time.Now().Add(-HealthInterval), s.MemberID(), s.cluster.VotingMembers()) {
+ // Treat a new voting member as unavailable when checking quorum safety.
+ since := time.Now().Add(-HealthInterval)
+ connectedToRequiredQuorum := isConnectedToQuorumSince(s.r.transport, since, s.MemberID(), s.cluster.VotingMembers())
+ if !memb.IsLearner {
+ connectedToRequiredQuorum = isConnectedToQuorumAfterAddingNewMemberSince(s.r.transport, since, s.MemberID(), s.cluster.VotingMembers())
+ }
+ if !connectedToRequiredQuorum {🤖 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/server.go` around lines 1431 - 1432, The quorum-safety
check currently always treats the new member as a voting member by calling
isConnectedToQuorumAfterAddingNewMemberSince(s.r.transport, ..., s.MemberID(),
s.cluster.VotingMembers()); change this so the "post-add quorum bump" is only
applied when the member being added is a voting member — for learner additions
do NOT call the "AfterAddingNewMember" variant and instead check quorum against
the current voting set (i.e., use the non-post-add quorum check such as
isConnectedToQuorumSince or the equivalent check with s.cluster.VotingMembers()
that does not include the new member). Ensure you detect learner vs voter using
the member type information available in the add path and switch between
isConnectedToQuorumAfterAddingNewMemberSince(...) for voters and the non-bumped
quorum check for learners.
| sleepDuration := 500 * time.Millisecond | ||
| maxRetries := int((2 * etcdserver.HealthInterval) / sleepDuration) | ||
| memberRemoved := false | ||
| for i := 0; i < 10; i++ { | ||
| for i := 0; i < maxRetries; i++ { | ||
| _, err := memberCtl.MemberRemove(ctx, memberID) | ||
| if err != nil && strings.Contains(err.Error(), "member not found") { | ||
| memberRemoved = true | ||
| break | ||
| } | ||
|
|
||
| time.Sleep(500 * time.Millisecond) | ||
| time.Sleep(sleepDuration) | ||
| } | ||
|
|
||
| if !memberRemoved { | ||
| return errors.New("failed to remove member after 10 tries") | ||
| return fmt.Errorf("failed to remove member after %d tries", maxRetries) |
There was a problem hiding this comment.
Mark successful member removal as success immediately.
At Line 853, a successful MemberRemove (err == nil) does not set memberRemoved, so the code retries unnecessarily and can fail later despite already removing the member.
💡 Proposed fix
sleepDuration := 500 * time.Millisecond
maxRetries := int((2 * etcdserver.HealthInterval) / sleepDuration)
memberRemoved := false
for i := 0; i < maxRetries; i++ {
_, err := memberCtl.MemberRemove(ctx, memberID)
- if err != nil && strings.Contains(err.Error(), "member not found") {
+ if err == nil || (err != nil && strings.Contains(err.Error(), "member not found")) {
memberRemoved = true
break
}
time.Sleep(sleepDuration)
}📝 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.
| sleepDuration := 500 * time.Millisecond | |
| maxRetries := int((2 * etcdserver.HealthInterval) / sleepDuration) | |
| memberRemoved := false | |
| for i := 0; i < 10; i++ { | |
| for i := 0; i < maxRetries; i++ { | |
| _, err := memberCtl.MemberRemove(ctx, memberID) | |
| if err != nil && strings.Contains(err.Error(), "member not found") { | |
| memberRemoved = true | |
| break | |
| } | |
| time.Sleep(500 * time.Millisecond) | |
| time.Sleep(sleepDuration) | |
| } | |
| if !memberRemoved { | |
| return errors.New("failed to remove member after 10 tries") | |
| return fmt.Errorf("failed to remove member after %d tries", maxRetries) | |
| sleepDuration := 500 * time.Millisecond | |
| maxRetries := int((2 * etcdserver.HealthInterval) / sleepDuration) | |
| memberRemoved := false | |
| for i := 0; i < maxRetries; i++ { | |
| _, err := memberCtl.MemberRemove(ctx, memberID) | |
| if err == nil || (err != nil && strings.Contains(err.Error(), "member not found")) { | |
| memberRemoved = true | |
| break | |
| } | |
| time.Sleep(sleepDuration) | |
| } | |
| if !memberRemoved { | |
| return fmt.Errorf("failed to remove member after %d tries", maxRetries) | |
| } |
🤖 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 `@tests/framework/e2e/cluster.go` around lines 849 - 863, The loop that calls
memberCtl.MemberRemove is treating only the "member not found" error as success
and ignores the successful case (err == nil); update the loop in the function
containing memberCtl.MemberRemove so that if MemberRemove returns nil you set
memberRemoved = true and break immediately, otherwise keep the existing check
for strings.Contains(err.Error(), "member not found") to mark success; this
change ensures both a successful removal (nil error) and an already-removed
state mark memberRemoved and stop retrying.
| permType := authpb.READWRITE | ||
| if len(user.perm) > 0 { | ||
| val, ok := authpb.Permission_Type_value[strings.ToUpper(user.perm)] | ||
| if ok { | ||
| permType = authpb.Permission_Type(val) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail fast on invalid permission strings in test setup.
Line [386] currently falls back to READWRITE when user.perm is invalid. That can accidentally over-grant and hide auth test mistakes; make invalid values fail the test instead.
Proposed fix
permType := authpb.READWRITE
if len(user.perm) > 0 {
val, ok := authpb.Permission_Type_value[strings.ToUpper(user.perm)]
- if ok {
- permType = authpb.Permission_Type(val)
- }
+ require.Truef(t, ok, "invalid permission type %q for user %s", user.perm, user.name)
+ permType = authpb.Permission_Type(val)
}📝 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.
| permType := authpb.READWRITE | |
| if len(user.perm) > 0 { | |
| val, ok := authpb.Permission_Type_value[strings.ToUpper(user.perm)] | |
| if ok { | |
| permType = authpb.Permission_Type(val) | |
| } | |
| } | |
| permType := authpb.READWRITE | |
| if len(user.perm) > 0 { | |
| val, ok := authpb.Permission_Type_value[strings.ToUpper(user.perm)] | |
| require.Truef(t, ok, "invalid permission type %q for user %s", user.perm, user.name) | |
| permType = authpb.Permission_Type(val) | |
| } |
🤖 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 `@tests/integration/v3_auth_test.go` around lines 384 - 390, The test currently
silently falls back to READWRITE when user.perm is non-empty but not a valid key
in authpb.Permission_Type_value; change the logic in the test setup so that when
len(user.perm) > 0 and the lookup into authpb.Permission_Type_value yields ok ==
false, the test fails immediately (e.g., call t.Fatalf or require.FailNow) with
a clear message referencing the invalid user.perm, instead of assigning permType
:= authpb.READWRITE; otherwise keep the default behavior when user.perm is
empty.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dusk125, tjungblu 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 |
|
/retest |
|
/verified by @tjungblu |
openshift-ci-robot
added
the
verified
|
@tjungblu: This PR has been marked as verified by 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. |
|
/retest-required |
|
@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. |
7 participants
Rebase handled by Claude
❯ ./bin/etcd --version
etcd Version: 3.6.11
Git SHA: 821d95e
Go Version: go1.25.9
Go OS/Arch: darwin/arm64
Summary by CodeRabbit
New Features
Bug Fixes
Chores