feat(session): let a session share expire - #2987
Open
QuentinBisson wants to merge 3 commits into
Open
QuentinBisson wants to merge 3 commits into
QuentinBisson wants to merge 3 commits into
Conversation
QuentinBisson
added a commit
to giantswarm/kagent-upstream
that referenced
this pull request
Sep 28, 2026
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
QuentinBisson
marked this pull request as ready for review
September 29, 2026 09:22
QuentinBisson
requested review from
a team,
Charlesthebird and
peterj
as code owners
September 29, 2026 09:22
EItanya
requested changes
Sep 29, 2026
EItanya
left a comment
EItanya
left a comment
Contributor
There was a problem hiding this comment.
This feature is looking great! Do you think you can add a ceiling value to the server so that admins can set a maximum valid time
Contributor
Author
|
I definitely can, thanks for the review :) |
CreateSessionShareRequest takes an optional ttl. The share records expires_at, and token resolution treats an expired share like an unknown token. The owner still lists an expired share so it can be revoked. Signed-off-by: QuentinBisson <quentin@giantswarm.io>
KAGENT_SESSION_SHARE_MAX_TTL (controller.sessionShareMaxTTL) sets the longest ttl a share may request. A longer ttl is refused, and a share created without one receives the maximum. Zero, the default, leaves shares unbounded; a negative value fails controller startup. Signed-off-by: QuentinBisson <quentin@giantswarm.io>
QuentinBisson
force-pushed
the
upstream/session-share-expiry
branch
from
September 29, 2026 15:09
59b1965 to
fd8bcda
Compare
github-actions
Bot
added
enhancement
Contributor
Author
|
I pushed the new maximum valid time :) |
QuentinBisson
added a commit
to giantswarm/kagent-upstream
that referenced
this pull request
Sep 29, 2026
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
EItanya
reviewed
Sep 29, 2026
Comment on lines
+145
to
+150
| func sameExpiry(payload *timestamppb.Timestamp, column *time.Time) bool { | ||
| if payload == nil || column == nil { | ||
| return payload == nil && column == nil | ||
| } | ||
| return payload.AsTime().Equal(*column) | ||
| } |
Contributor
There was a problem hiding this comment.
Do we really need this? How could these get out of sync, we have many times we do something similar and we don't have this check
Contributor
Author
There was a problem hiding this comment.
You are right, this field is only set when we create a share and cannot be updated unless someone does it in the database. I'll simplify this
Contributor
Author
There was a problem hiding this comment.
The expiry is only stored in the column now, so sameExpiry and the truncation are gone.
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
github-actions
Bot
added
enhancement
moezdil
reviewed
Sep 29, 2026
| not_in: 0 | ||
| }]; | ||
| // How long the share's token grants access, from its creation. Unset means | ||
| // until the share is revoked or the session deleted. |
Contributor
There was a problem hiding this comment.
unset gets the server max when one is configured
| if ttl < 0 { | ||
| return nil, "", serviceerrors.NewInvalidArgument("share ttl must be positive", nil) | ||
| } | ||
| if s.shareMaxTTL > 0 { |
Contributor
There was a problem hiding this comment.
enabling the cap later leaves older no-ttl shares unbounded
Contributor
There was a problem hiding this comment.
That's ok, we're pre-alpha
EItanya
approved these changes
Sep 29, 2026
EItanya
enabled auto-merge
September 29, 2026 21:27
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
3 participants
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.
Problem
A session share's token grants access until the share is revoked or the session is deleted:
CreateSessionShareRequesttakes no lifetime, andGetSessionShareByTokenHashchecks nothing but the hash and the session's state. A client that keeps one share per conversation, for the people the owner lets in, has no way to bound it once the conversation is abandoned. AREAD_WRITEshare can also rename, suspend and delete the session.Change
CreateSessionShareRequest.ttlis optional and must be positive when set. The share recordsexpires_atin a nullablesession_share.expires_atcolumn, added to the unreleased000001baseline.ListSessionSharesstill returns an expired share, so the owner can revoke it.KAGENT_SESSION_SHARE_MAX_TTL(controller.sessionShareMaxTTL) lets admins cap share lifetime. A longerttlis refused withInvalidArgument, and a share created withoutttlreceives the maximum. The default0keeps shares unbounded, so a share created withoutttlbehaves as before.The migration immutability check fails because the column is folded into
000001, as its header asks until release (same as #2970).