Repository navigation
feat(session): let a session share expire - #2987
Conversation
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
EItanya
left a comment
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
|
I definitely can, thanks for the review :) |
59b1965 to
fd8bcda
Compare
|
I pushed the new maximum valid time :) |
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
| func sameExpiry(payload *timestamppb.Timestamp, column *time.Time) bool { | ||
| if payload == nil || column == nil { | ||
| return payload == nil && column == nil | ||
| } | ||
| return payload.AsTime().Equal(*column) | ||
| } |
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
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
There was a problem hiding this comment.
The expiry is only stored in the column now, so sameExpiry and the truncation are gone.
| 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. |
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 { |
There was a problem hiding this comment.
enabling the cap later leaves older no-ttl shares unbounded
There was a problem hiding this comment.
That's ok, we're pre-alpha
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
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>
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
Head branch was pushed to by a user without write access
50235d3 to
f9ca91b
Compare
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
|
Rebased onto main and pushed one change:
I also added a note to the PR body: the controller's clock sets @EItanya, one question before this merges: |
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
## Problem A session share's token grants access until the share is revoked or the session is deleted: `CreateSessionShareRequest` takes no lifetime, and `GetSessionShareByTokenHash` checks 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. A `READ_WRITE` share can also rename, suspend and delete the session. ## Change - `CreateSessionShareRequest.ttl` is optional and must be positive when set. The share records `expires_at` in a nullable `session_share.expires_at` column, added to the unreleased `000001` baseline. - Token resolution treats an expired share like an unknown token, so both A2A transports refuse it. `ListSessionShares` still returns an expired share, so the owner can revoke it. - `KAGENT_SESSION_SHARE_MAX_TTL` (`controller.sessionShareMaxTTL`) lets admins cap share lifetime. A longer `ttl` is refused with `InvalidArgument`, and a share created without `ttl` receives the maximum. The default `0` keeps shares unbounded, so a share created without `ttl` behaves as before. - The controller's clock sets `expires_at` and the database clock checks it. The skew between the two (NTP, milliseconds) is accepted. The migration immutability check fails because the column is folded into `000001`, as its header asks until release (same as kagent-dev#2970). --------- Signed-off-by: QuentinBisson <quentin@giantswarm.io>
| 2026-10-05 20:30 | `dd3b240` | `bf8afa5` (2026-10-05) | `a5186dd` → `f7bf3da` | 102 own, as kagent-dev#3019/kagent-dev#3020/kagent-dev#3018/kagent-dev#3066 have them where the line had its own form (`Harness.spec.substrate.egress` and its wildcard fix → `Agent.spec.egress`; the four limits commits → kagent-dev#3020; the caller credential and its path fix → kagent-dev#3018, the two forwarder fixes kept on top; #136/#137 shared turn → kagent-dev#3066); the Substrate pin on the line's 1.4.0 (kagent-dev/substrate v0.3.0-alpha3, giantswarm/substrate `e27ae953`); the 2026-09-30 align-files commit (auto-release, PR title check, cliff.toml) re-carried after the re-pin dropped it, with the candidate counter of giantswarm/devctl#2498 applied by hand | FORK.md "Dropped at the 2026-10-05 re-pin": the discovery-label commits (kagent-dev#2752), AUTH_MODE wiring (kagent-dev#2971), pair-collection comparison (kagent-dev#2952), gRPC log trace ids (kagent-dev#2929), the grpc pin, the ActorTemplate rejection pair (kagent-dev#2994), the AgentInstance share expiry (kagent-dev#2987), the three gateway dispatch fixes (kagent-dev#2939), the 1.0 schema migration and its CI upgrade check (replaced by the 1.x refusal); owed: the boot-bound golden retry (`ebe49f0f`) and the not-resumable directive reader (`7eabf64f`) | by hand — HeraldBot's hand-over [#159](#159) updated in place; [CI Build 37430773899](https://github.com/giantswarm/kagent-upstream/actions/runs/37430773899) on the candidate, [37434519929](https://github.com/giantswarm/kagent-upstream/actions/runs/37434519929) on the pushed branch; release `v1.3.0` (2026-10-06T08:24Z, promoted from `v1.3.0-rc.16`, which the force-push cut at 08:12Z) |
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.expires_atand the database clock checks it. The skew between the two (NTP, milliseconds) is accepted.The migration immutability check fails because the column is folded into
000001, as its header asks until release (same as #2970).