sql datastore cleanup #564
Conversation
- consistent use of transactions. - opens sqlite3 databases in WAL mode to improve concurrency performance - enforces a lock over write operations for sqlite3. - improved error handling. - error wrapping for improved diagnostics. - fixed Configure() to be safe in the presence of concurrent callers and to not overwrite the current database unless the new one opened successfully. - converts tests to test suite - has tests test against database on disk in order to properly exercise behavior (in-memory databases have journaling differences) Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
requested review from
amartinezfayo,
evan2645,
MarcosDY,
martincapello and
walmav
as code owners
| defer db.opMu.Unlock() | ||
| } | ||
|
|
||
| // TODO: as soon as GORM supports it, attach the context |
There was a problem hiding this comment.
Is there an issue on gorm tracking this?
There was a problem hiding this comment.
I'll find the root issue and put it in the comment in the follow up PR I'm working on.
| ds.DatabaseType = config.DatabaseType | ||
| ds.ConnectionString = config.ConnectionString | ||
| return resp, ds.restart() | ||
| if len(entry.SpiffeId) == 0 { |
There was a problem hiding this comment.
should we do a more stringent validation check here? registration entries should be workload entries that belong to our trust domain
There was a problem hiding this comment.
Hmm, that's a good question. Is the datastore layer really the proper layer to be doing these kind of checks? It does place a slightly higher burden on implementors that we'd need to document (along with a few other things, .e.g., which Fetch functions should fail if the record doesn't exist, v.s. returning nil). The core is doing this kind of validation in the registration handler. If the code grew to create registration entries from more than one spot, I think I'd rather see the core go through a layer wrapping the datastore plugin that does the validation consistently in a single spot so we didn't have to rely on plugin implementations...
to not overwrite the current database unless the new one opened
successfully.
behavior (in-memory databases have journaling differences)