Skip to content

[extended] Fix LeaderElectionRecord.equals null renewTime comparison - #4954

Draft
yue9944882 wants to merge 1 commit into
kubernetes-client:masterfrom
yue9944882:fix/extended-leaderelectionrecord-equals-renewtime
Draft

yue9944882 wants to merge 1 commit into
kubernetes-client:masterfrom
yue9944882:fix/extended-leaderelectionrecord-equals-renewtime

Conversation

@yue9944882

Copy link
Copy Markdown
Member

Bug

LeaderElectionRecord.equals() has a field typo in the final branch of its equals chain:

if (renewTime == null) {
  return that.holderIdentity == null;   // compares the wrong field
}
return renewTime.equals(that.renewTime);

When this.renewTime == null, the method returns whether that.holderIdentity is null instead of whether that.renewTime is null. Consequences:

  • Two otherwise-equal records that both have a null renewTime are reported unequal (because holderIdentity is typically non-null).
  • A record with a null renewTime can be reported equal to one with a non-null renewTime when holderIdentity happens to be null.

This has been present since the original 2019 leader-election commit. equals() is used by LeaderElector.tryAcquireOrRenew() to detect observed-record changes, so a wrong result can skew the observedTime bookkeeping that drives lease-expiry decisions.

Fix

Compare renewTime in that branch (minimal one-line change):

if (renewTime == null) {
  return that.renewTime == null;
}

Verification

Added LeaderElectionRecordTest with two focused cases (equal when both renewTime null; unequal when only one is null). Verified the first test fails before the fix and passes after:

  • Before fix: Tests run: 2, Failures: 1 (equalWhenRenewTimeIsNullOnBoth fails)
  • After fix: Tests run: 2, Failures: 0
  • No regression: existing LeaderElectionTest (6) and LeaderElectorTest (3) still pass.

Ran: ./mvnw -pl extended -am test -Dtest=LeaderElectionRecordTest -Dsurefire.failIfNoSpecifiedTests=false

@kubernetes-prow kubernetes-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow kubernetes-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2026
@linux-foundation-easycla

linux-foundation-easycla Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: yue9944882 / name: Min Jin (1a1ccd9)

@kubernetes-prow kubernetes-prow Bot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 22, 2026
When this.renewTime is null, equals() returned (that.holderIdentity ==
null) instead of (that.renewTime == null), a field typo in the final
branch of the equals chain. This made two otherwise-equal records with a
null renewTime compare as unequal, and could report unequal records as
equal. Compare renewTime in that branch. Adds focused unit tests.
@yue9944882
yue9944882 force-pushed the fix/extended-leaderelectionrecord-equals-renewtime branch from 289f54f to 1a1ccd9 Compare October 22, 2026 03:55
@kubernetes-prow kubernetes-prow Bot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant