-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: use redis as backend for koa-session #2693
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I will need to update this repo's helm values.yaml after they are added to our helm chart repo. Assuming it mirrors our backend, I believe we just need to add the same redis
config that we have for rafiki-backend
.
I already prepared PR for helm charts. So that we will be able to merge it after this one is merged. |
@@ -342,12 +344,26 @@ export class App { | |||
|
|||
koa.use(cors()) | |||
koa.keys = [this.config.cookieKey] | |||
|
|||
const redis = await this.container.use('redis') | |||
koa.use( | |||
session( | |||
{ | |||
key: 'sessionId', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this correspond to what is stored inside redis? The key I see in redis is a UUID -> where does "sessionId" come into play?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/auth/src/app.ts
Outdated
return data ? JSON.parse(data) : null | ||
}, | ||
async set(key, session) { | ||
await redis.set(key, JSON.stringify(session)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does expiry/TTL of the session get saved in redis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the flow showed that it was being expired but it looks like it was just the http cookie. Redis key was not actually expiring. Changed it to make the record expire in redis. 05fcb02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not set expiry on redis key. With set command, that would be done either via EX options of set command or through some expiry command. If we know the TTL of the session I would rewrite this code to be like this:
const op = redis.multi()
op.hmset(key, session)
op.expire(key, expirationInMilliseconds)
await op.exec()
@@ -85,8 +85,10 @@ services: | |||
TRUST_PROXY: ${TRUST_PROXY} | |||
AUTH_DATABASE_URL: postgresql://cloud_nine_wallet_auth:cloud_nine_wallet_auth@shared-database/cloud_nine_wallet_auth | |||
AUTH_SERVER_DOMAIN: ${CLOUD_NINE_AUTH_SERVER_DOMAIN:-http://localhost:3006} | |||
REDIS_URL: redis://shared-redis:6379/0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to distinguish this from the cloud-nine-backend
one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. Not sure how much it matters TBH but I suppose its consistent with what we're currently doing, so I changed it to:
- c9 backend: 0
- c9 auth: 1
- hlb backend: 2
- hlb auth: 3
const data = await redis.get(key) | ||
return data ? JSON.parse(data) : null | ||
}, | ||
async set(key, session) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there is a third parameter maxAge
possible in this function. Do we want to include that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed I would use that then purposefully did not after seeing it could be undefined
or "session"
depending on how we actually configure it. The type here doesnt account for how we actually configure it, so I didn't want to handle scenarios like session
or undefined (which should never happen) when we know we want to expire it. I suppose an alternative would be handling session
and undefined
by defaulting back to expireInMs
, but I figured it was just simpler to control both the session max age and redis ttl from 1 variable. If we did handle it by defaulting back to expireInMs
then it would basically be: set it to maxAge
when maxAge
is a number (which will match our configuration) or set it to match our configuration.
Does that make sense? Open to feedback here if anyone sees a better way.
// Add a delay to cookie age to ensure redis record expires after cookie | ||
const expireInMs = maxAgeMs + 10 * 1000 | ||
await redis.psetex(key, expireInMs, JSON.stringify(session)) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | |
await redis.set(key, JSON.stringify(session), 'PX', expireInMs) |
I think psetex
is deprecated based on the docs. (sorry for the odd diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
psetex
is not deprecated per my knowledge, but it is not wise to use it, since it will be deprecated and removed.
From the docs:
Note: Since the SET command options can replace SETNX, SETEX, PSETEX, GETSET, it is possible that in future versions of Redis these commands will be deprecated and finally removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, no need to do stringify on session object. Instead, you can just use hmset(key, session)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to hmset as suggested except I used hset
. apparently hmset
, like psetex
is marked as deprecated in redis
docs https://redis.io/docs/latest/commands/hmset/
// Add a delay to cookie age to ensure redis record expires after cookie | ||
const expireInMs = maxAgeMs + 10 * 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this is to possibly allow the "final" request with the to-be-expired cookie parameter to complete before deleting it in redis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah basically. could potentially be shorter I guess - just needs to be long enough to cover the time between finding the non-expired koa-session in the server and retrieving the redis record. no harm in having a little extra buffer though I figure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes proposed in this pull request
koa-session
backendContext
fixes #2673
corresponding helm-chart issue created as well: interledger/helm-charts#38
Checklist
fixes #number