fix: detect expired preauth keys in status#44
Conversation
The test case shows that the status field does not change even if the key is expired.
There was a problem hiding this comment.
Pull request overview
Updates the HeadscalePreAuthKey controller to track and surface preauth key expiration via status, fixing the long-standing issue where keys remained Ready=True forever after expiring.
Changes:
- Add
status.expiresAttoHeadscalePreAuthKeyand CRD schema. - Reconcile existing keys by marking expired keys
Ready=False(KeyExpired) and requeuing active keys based onExpiresAt. - Add unit tests covering expired and active key status transitions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controller/headscalepreauthkey_controller.go | Adds reconcileExistingKey, sets status.expiresAt on key creation, and requeues based on expiration. |
| internal/controller/headscalepreauthkey_controller_test.go | Adds tests for expired-key readiness and active-key requeue behavior. |
| api/v1beta1/headscalepreauthkey_types.go | Adds ExpiresAt to the CR status type. |
| api/v1beta1/zz_generated.deepcopy.go | Regenerates deep-copy logic to include ExpiresAt. |
| config/crd/bases/headscale.infrado.cloud_headscalepreauthkeys.yaml | Extends CRD OpenAPI schema to include status.expiresAt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if preAuthKey.Status.ExpiresAt != nil && time.Now().After(preAuthKey.Status.ExpiresAt.Time) { | ||
| log.Info("PreAuth key has expired", "keyID", preAuthKey.Status.KeyID, "expiresAt", preAuthKey.Status.ExpiresAt.Time) | ||
| if err := r.updateStatusCondition(ctx, preAuthKey, metav1.Condition{ | ||
| Type: "Ready", | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "KeyExpired", | ||
| Message: fmt.Sprintf("PreAuth key expired at %s", preAuthKey.Status.ExpiresAt.Format(time.RFC3339)), | ||
| }); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
Expiration check uses time.Now().After(expiresAt) and later calls time.Until(expiresAt) with a different time.Now(). If ExpiresAt is equal/very close to now, this can fall through the "active" path, compute RequeueAfter <= 0, and then never reconcile again—leaving Ready=True after the key has actually expired. Consider capturing now := time.Now() once and treating ExpiresAt <= now as expired (e.g., !expiresAt.After(now)).
| if preAuthKey.Status.ExpiresAt != nil && time.Now().After(preAuthKey.Status.ExpiresAt.Time) { | |
| log.Info("PreAuth key has expired", "keyID", preAuthKey.Status.KeyID, "expiresAt", preAuthKey.Status.ExpiresAt.Time) | |
| if err := r.updateStatusCondition(ctx, preAuthKey, metav1.Condition{ | |
| Type: "Ready", | |
| Status: metav1.ConditionFalse, | |
| Reason: "KeyExpired", | |
| Message: fmt.Sprintf("PreAuth key expired at %s", preAuthKey.Status.ExpiresAt.Format(time.RFC3339)), | |
| }); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| return ctrl.Result{}, nil | |
| now := time.Now() | |
| if preAuthKey.Status.ExpiresAt != nil { | |
| expiresAt := preAuthKey.Status.ExpiresAt.Time | |
| if !expiresAt.After(now) { | |
| log.Info("PreAuth key has expired", "keyID", preAuthKey.Status.KeyID, "expiresAt", expiresAt) | |
| if err := r.updateStatusCondition(ctx, preAuthKey, metav1.Condition{ | |
| Type: "Ready", | |
| Status: metav1.ConditionFalse, | |
| Reason: "KeyExpired", | |
| Message: fmt.Sprintf("PreAuth key expired at %s", preAuthKey.Status.ExpiresAt.Format(time.RFC3339)), | |
| }); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| return ctrl.Result{}, nil | |
| } |
| // Key is ready, requeue before expiration if ExpiresAt is set | ||
| var requeueAfter time.Duration | ||
| if preAuthKey.Status.ExpiresAt != nil { | ||
| requeueAfter = time.Until(preAuthKey.Status.ExpiresAt.Time) | ||
| } |
There was a problem hiding this comment.
RequeueAfter is set to time.Until(ExpiresAt). When ExpiresAt is set, this schedules reconciliation exactly at expiration (not before), and can produce 0/negative durations (which controller-runtime treats as "no requeue"). To reliably transition to KeyExpired, clamp to a small positive minimum and/or requeue slightly before expiration (e.g., ExpiresAt minus a small buffer).
| expiresAt := metav1.NewTime(time.Now().Add(expiration)) | ||
| preAuthKey.Status.ExpiresAt = &expiresAt |
There was a problem hiding this comment.
Status.ExpiresAt is always set to time.Now().Add(expiration). This diverges from the actual request semantics in pkg/headscale/client.go where expiration is only sent when expiration > 0; for expiration==0 (e.g. "0s"), Headscale will create a non-expiring key but the controller will record ExpiresAt=now and later mark it expired. Also, recomputing time.Now() here can drift from the timestamp actually sent to Headscale. Consider only setting ExpiresAt when expiration > 0 and using the same computed expiration timestamp that was used in the CreatePreAuthKey request (or returning it from hsclient).
| expiresAt := metav1.NewTime(time.Now().Add(expiration)) | |
| preAuthKey.Status.ExpiresAt = &expiresAt | |
| if expiration > 0 { | |
| expiresAt := metav1.NewTime(time.Now().Add(expiration)) | |
| preAuthKey.Status.ExpiresAt = &expiresAt | |
| } else { | |
| preAuthKey.Status.ExpiresAt = nil | |
| } |
|
I'd need to think about this a little, i guess the copilot comments are valid somehow. i'm unsure how k8s will react if we requeue multiple times with the same target over a long period (assuming the expiration is long) and if updating it rapidly with expire and readiness would cause an infinite loop over update event. |
OK -- I see. Note that this is about preauth keys whose expiration date should be pretty small. We could also set a fixed rescheduling time maximum of say 60 minutes. I don't think k8s is the problem here, as I understand things this is a pattern operators are supposed to do. The other comments I can address and seem valid. |
|
@seletz are you still interested on working on this? |
|
@clwluvw Yes |
Summary
The
HeadscalePreAuthKeycontroller always reportsReady: Trueonce a key is created, even after expiration. This adds expiration tracking and status transitions:ExpiresAtfield toHeadscalePreAuthKeyStatus, computed fromspec.Expirationat key creation timeExpiresAtin the past) transition toReady: Falsewith reasonKeyExpiredExpiresAtin the future) remainReady: Trueand requeue before expirationExpiresAt(created before this change) preserve existing behavior:Ready: True, no requeueNo auto-recreation of expired keys — expiration is intentional.
Fixes #41
Test plan
Ready=Falsewith reasonKeyExpiredReady=TruewithRequeueAfter > 0ExpiresAt(Ready=True, no requeue)make lint-fixpassesmake testpasses