fix: runner token expiration renewal#305
Conversation
BoxBoxJason
force-pushed
the
fix/runner-expiration-renewal
branch
from
82874e5 to
91b0933
Compare
| ) | ||
|
|
||
| if runners.ShouldRotateRunnerToken(&cr.Status.AtProvider.CommonRunnerObservation, cr.Spec.ForProvider.TokenRenewBeforeDays, time.Now().UTC()) { | ||
| return managed.ExternalObservation{ResourceExists: false}, nil |
There was a problem hiding this comment.
Nice improvement overall. One thing worries me here: when renewal is due, Observe() returns ResourceExists: false even though the runner still exists in GitLab. That pushes Crossplane down the create path, and Create() provisions a brand-new runner instead of rotating the token on the existing one. In practice this looks like it would orphan the old runner, change the external name, and leave cleanup attached only to the newest runner. Since the runner client already exposes ResetRunnerAuthenticationToken, would it be safer to keep ResourceExists: true here and handle token renewal in Update() instead? I think the same issue exists in the group/project + cluster mirrors.
| meta.SetExternalName(cr, strconv.FormatInt(runner.ID, 10)) | ||
|
|
||
| now := metav1.NewTime(time.Now().UTC()) | ||
| cr.Status.AtProvider.CommonRunnerObservation.TokenCreatedAt = &now |
There was a problem hiding this comment.
Small concern on persistence here: TokenCreatedAt / TokenExpiresAt are only written during Create(), and the GitLab details API does not return them later. If these status mutations are not reliably persisted by the managed reconciler, the next Observe() can lose the renewal metadata and the auto-renew path will never trigger consistently. Could we move this onto a status-safe persistence path, or add a reconcile-level test that proves the timestamps survive a full create -> observe cycle?
| } | ||
| createdAt = obs.TokenCreatedAt.Time | ||
| } | ||
| t, ok := common.RotationTime(createdAt, obs.TokenExpiresAt.Time, renewBeforeDays) |
There was a problem hiding this comment.
Another edge case worth guarding: tokenRenewBeforeDays only has a minimum bound today. If a user sets a renewal window larger than the actual token lifetime, RotationTime() ends up in the past immediately, so every reconcile treats the token as already due. With the current recreate behavior that would cause perpetual runner churn; even after switching to reset semantics it would still cause endless resets. A validation or runtime guard for renewAt <= createdAt would make this safer.
| }, | ||
| }), | ||
| ), | ||
| result: managed.ExternalObservation{ |
There was a problem hiding this comment.
I think this test is currently locking in the recreate behavior above by expecting ResourceExists: false once the token is expired. If the intended design is to keep the existing runner and rotate its token, this probably wants to assert that the resource still exists but is not up to date, and then verify a reset call in Update(). Might be worth adjusting these expired-token tests now so they encode the intended renewal semantics rather than the current recreate path.
|
@BoxBoxJason left some comments even though it is a draft i would like to have this personally. |
|
Hey there ! Thanks for this pre review with very good insights. I will implement your suggestions and fixes this week end. |
|
@BoxBoxJason could you rebase this one too and also remove the draft status? Because i think we're quite close to merging |
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
BoxBoxJason
force-pushed
the
fix/runner-expiration-renewal
branch
from
a1faeb0 to
7eee0d1
Compare
|
Hello ! I just performed the rebase, I will have more time this thursday to perform all tests and remove the draft status confidently ! |
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
|
Turns out you were right ! Observation did not properly persist renewAt / createdAt fields set in Create Tell me what you think about this revised (and this time properly tested) version |
2 participants
Description of your changes
This PR adds an automatic renewal mechanism on runner token expiration for the project, group & instance runners.
All of the changes are covered by unit tests
Fixes #304
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
I have:
unexpected drift. (should result in recreation of the resource if applicable)
unexpected drift. (should result in an update of the resource if applicable)