Add transaction-like behavior for Transit persists. #3959
Conversation
This ensures that in-memory policy matches the state of on-disk policy if a storage error occurs.
jefferai
changed the title
| } | ||
| lock.RUnlock() |
There was a problem hiding this comment.
This should be unlocked right away. In theory in the rest of the test we should be grabbing a write lock on the policy, but since it's test-scoped and not in parallel we don't.
helper/keysutil/policy.go
Outdated
| // roll back keys, but better safe than sorry and this doesn't happen | ||
| // enough to worry about the speed tradeoff. | ||
| priorArchiveVersion := p.ArchiveVersion | ||
| priorKeys := keyEntryMap(nil) |
There was a problem hiding this comment.
Could this use copystructure.Copy(p.Keys) so that we can just do p.Keys = priorKeys.(keyEntryMap) similar do what is being done in Upgrade()?
There was a problem hiding this comment.
Discussed internally, removed copystructure instead.
helper/keysutil/policy.go
Outdated
| // roll back keys, but better safe than sorry and this doesn't happen | ||
| // enough to worry about the speed tradeoff. | ||
| priorArchiveVersion := p.ArchiveVersion | ||
| priorKeys := keyEntryMap(nil) |
There was a problem hiding this comment.
FMI, is there a difference between this and var priorKeys keyEntryMap?
There was a problem hiding this comment.
I'll convert to the other way to be a bit more canonical.
helper/keysutil/policy.go
Outdated
| priorLatestVersion := p.LatestVersion | ||
| priorMinDecryptionVersion := p.MinDecryptionVersion | ||
| priorConvergentVersion := p.ConvergentVersion | ||
| priorKeys, err := copystructure.Copy(p.Keys) |
There was a problem hiding this comment.
Is there a reason for using copystructure here vs doing a manual copy like in the Persist() and Rotate() functions? The only difference I see is that of the nil check on p.Keys.
This ensures that in-memory policy matches the state of on-disk policy
if a storage error occurs.
Ref: #2705