[sql-37] firewalldb: more preparations for SQL actions store #1070
Conversation
In preparation for using the clock to get an Action's AttemptedAt time in an upcoming commit, we let both the bbolt and SQL impls of the firewalldb take a clock.
ellemouton
added
the
no-changelog
Instead of passing an `Action` to the AddAction method, we introduce an `AddActionReq` type which only holds the fields that are needed to create a new Action. The rest of the info is determined by the DB layer.
To make it very clear what the data is actually derived from. Then also add an optional Session.ID. Our bbolt db wont store this real session ID and will populate it in a best effort manner by casting the persisted MacaroonIdentifier.
We add a new macaroon_identifier field to the Action proto message an populate it in the rpc server.
Before this commit, some of the actions store test were using the same global variables & some tests were also modifying those variables meaning that we could not run the tests in parallel. So here we instead make the variables local to the test and add `t.Parallel` to each test. This will also be useful later on when the session IDs used by the `AddActionReq` structs are instead derived from a sessions store.
| // NOTE: for our BoltDB impl, this is not persisted in any way, and we | ||
| // populate it by casting the macaroon ID to a session.ID and so is not | ||
| // guaranteed to be linked to an existing session. | ||
| SessionID fn.Option[session.ID] |
There was a problem hiding this comment.
I wonder if there's a test anywhere for an intercepted action, where we don't set this (i.e. it's not related to a session request).
There was a problem hiding this comment.
yeah i'll expand on the tests for SQL (where it actually will matter if it is set or not) once we've added the sql impl here 👍
| SessionID session.ID | ||
| // MacaroonIdentifier is a 4 byte identifier created from the last 4 | ||
| // bytes of the root key ID of the macaroon used to perform the action. | ||
| MacaroonIdentifier [4]byte |
There was a problem hiding this comment.
it's probably not worth it to have another type alias for [4]byte?
There was a problem hiding this comment.
currently we dont use this in other places - so i think it's ok for now
| @@ -171,7 +174,7 @@ func TestListActions(t *testing.T) { | |||
| actionIds++ | |||
|
|
|||
| actionReq := &AddActionReq{ | |||
| SessionID: sessionID, | |||
| MacaroonIdentifier: sessionID, | |||
There was a problem hiding this comment.
should we set SessionID here as well?
There was a problem hiding this comment.
want to keep the tests as close to how they are today as possible - then will expand the tests to cover more cases once the SQL impl is added 👍
| @@ -14,11 +14,23 @@ import ( | |||
| var ( | |||
| testTime1 = time.Unix(32100, 0) | |||
| testTime2 = time.Unix(12300, 0) | |||
| ) | |||
|
|
|||
| // TestActionStorage tests that the ActionsListDB CRUD logic. | |||
There was a problem hiding this comment.
thank you for the nice diffs 👍
|
|
||
| action := &Action{ | ||
| AddActionReq: *req, | ||
| AttemptedAt: db.clock.Now().UTC(), |
There was a problem hiding this comment.
Just noting that the time is set to the clocks .Now().UTC(), while the previous version set it to time.Now() (i.e. not UTC). I think the new implementation is probably more correct, but wanted to note that. Could this cause any issues, anddo we need to update previous inserted actions to represent that in some way?
There was a problem hiding this comment.
yeah - similarly to sessions/accounts - we're going with "it may just be off by a couple of hours and that's ok" since mostly these timestamps are just metadata for display to the user - we dont use them (much) in logic.
So yeah - i think not worth a whole migration just to fix this
This PR mainly does some refactors in preparation for adding the SQL version of the Actions store.
AddActionsmethod so that it takes anAddActionsReqinstead of anAction. This is more idiomatic sinceActionincludes fields that should not be required by the caller to set.Action.SessionIDfield toMacaroonIdentifierto more clearly describe what it actually is and then add an optionalSession.IDwhich is meant to represent an actual session the action is linked to. For our bbolt impl: we continue to only read from theaction.MacaroonIdentifierat persistence time (as we did before) and then at read time, we do a best effort attempt at populating the newSessionIDfield.Part of #917