fix: only generate LoginSet SSH host keys on the create path - #247
Open
giuliocalzo wants to merge 1 commit into
Open
fix: only generate LoginSet SSH host keys on the create path#247giuliocalzo wants to merge 1 commit into
giuliocalzo wants to merge 1 commit into
Conversation
giuliocalzo
requested review from
SkylerMalinowski,
alanmutsch,
catblade,
vivian-hafener and
wickberg
as code owners
August 28, 2026 12:48
giuliocalzo
force-pushed
the
fix/loginset-ssh-hostkeys-lazy-keygen
branch
3 times, most recently
from
September 4, 2026 17:33
1200a0e to
4ee1203
Compare
BuildLoginSshHostKeys generated an RSA, an Ed25519 and an ECDSA keypair before anything checked whether the Secret already existed, so every LoginSet reconcile paid for three keypairs that SyncObject then discarded because the Secret is immutable. Worse, a live Secret that is not marked immutable, one restored from a backup that dropped the field for instance, was patched with the freshly generated keys on every reconcile, rotating the host keys clients had already accepted. Look the Secret up and reuse the keys it holds, generating a set only when there are none. The build then sync pattern of the reconcile step is unchanged. Changelog: Fixed - LoginSet SSH host keys are reused instead of regenerated on every reconcile Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
giuliocalzo
force-pushed
the
fix/loginset-ssh-hostkeys-lazy-keygen
branch
from
September 8, 2026 08:24
4ee1203 to
a043c2d
Compare
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
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
The
SSH Host Keyssync step builds the Secret — generating an RSA, an Ed25519 and an ECDSA keypair — before anything checks whether the Secret already exists, then hands it toSyncObjectwithshouldUpdate=true. Two problems follow:Immutable: trueandSyncObjectskips immutable Secrets, so the three keypairs are generated and immediately discarded on every single LoginSet reconcile. RSA keygen defaults to 4096 bits, measured at 0.12–1.05 s per keypair on an M-series laptop, and worse again in a CPU-limited container.SyncObjecttakes the patch path and overwritesDatawith the freshly generated keys on every reconcile. That rotates the SSH host keys out from under clients that have already accepted them, producing host key mismatch warnings on login nodes.This adds
objectutils.CreateObjectIfNotExists, which defers construction to a build closure that only runs on the create path, and uses it for the SSH host keys step. Net effect: keygen happens exactly once per LoginSet, and an existing host keys Secret is never rewritten regardless of itsimmutablefield.CreateObjectIfNotExistsfollows the existingSyncObjectconventions — same argument order, sameReasonCreateSucceeded/ReasonCreateFailedevents — and additionally treats a losingAlreadyExistscreate race as success.Test plan
TestLoginSetReconciler_syncSshHostKeys— new; thekeeps the keys of a mutable existing Secretcase was verified to fail against the pre-change code (the RSA key was replaced with a freshly generated one) and to pass after.TestCreateObjectIfNotExists— covers absent, already-exists (asserting the build closure is not invoked), build error, failing get, and losing create race.go test ./internal/... ./api/...green, including the loginset envtest suite.golangci-lint run ./internal/...introduces no new findings.