Skip to content

Fix VPN cert rotation flag collision between concurrent slices#389

Open
Ady0333 wants to merge 1 commit into
kubeslice:masterfrom
Ady0333:fix/vpn-rotation-per-slice-flag
Open

Fix VPN cert rotation flag collision between concurrent slices#389
Ady0333 wants to merge 1 commit into
kubeslice:masterfrom
Ady0333:fix/vpn-rotation-per-slice-flag

Conversation

@Ady0333

@Ady0333 Ady0333 commented May 18, 2026

Copy link
Copy Markdown

Description

VpnKeyRotationService is a singleton. The old jobCreationInProgress atomic.Bool was
process-global shared across every slice reconcile. When slice-A triggered cert jobs and set the
flag to true, slice-B's reconcile would skip triggerJobsForCertCreation entirely and fall
through to verifyAllJobsAreCompleted. Since old Jobs from slice-B's prior rotation are never
deleted, they'd return JobStatusComplete, causing the reconciler to advance
CertificateExpiryTime and increment RotationCount for slice-B without generating any new
certs
. No error, no event indicating failure.

Before (broken):

// shared across all slices
jobCreationInProgress atomic.Bool

if !v.jobCreationInProgress.Load() {
    v.triggerJobsForCertCreation(...)  // skipped for slice-B if slice-A set flag
    v.jobCreationInProgress.Store(true)
}
// falls through to verifyAllJobsAreCompleted -- finds stale old jobs marks rotation complete

After (fixed):

  // per-slice key
  jobCreationInProgress sync.Map

  sliceKey := s.Namespace + "/" + s.Name
  inProgress, _ := v.jobCreationInProgress.Load(sliceKey)
  if inProgress != true {
      v.triggerJobsForCertCreation(...)
      v.jobCreationInProgress.Store(sliceKey, true)
  }

How Has This Been Tested?

  • Wrote a unit test (Test_GlobalFlagBug_SkipsCertRotationForSecondSlice) that reconciles two slices
    with expired certs sequentially on the same singleton service instance
  • Before fix: GenerateCerts called once (only slice-A), slice-B's RotationCount incremented silently
  • After fix: GenerateCerts called independently for both slices confirmed with
    wg.AssertNumberOfCalls(t, "GenerateCerts", 2)

Checklist:

  • The title of the PR states what changed and the related issues number (used for the release note).
  • Does this PR requires documentation updates?
  • I've updated documentation as required by this PR.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have tested it for all user roles.
  • I have added all the required unit test cases.

Does this PR introduce a breaking change for other components like worker-operator?

No breaking changes. The fix is internal to the controller's rotation state tracking.


Make jobCreationInProgress per-slice instead of process-global to prevent
one slice's certificate rotation from blocking another's. Replace atomic.Bool
with sync.Map keyed by namespace/sliceName.

Also fixes pre-existing broken test type hints (util.Client -> client.Client).

Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
@Ady0333 Ady0333 requested a review from that-backend-guy as a code owner May 18, 2026 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant