support create-version operation #148
Conversation
oxtoacart
force-pushed
the
percy/putifabsent
branch
2 times, most recently
from
a133b51 to
8f07909
Compare
oxtoacart
force-pushed
the
percy/putifabsent
branch
from
8f07909 to
93cfe6c
Compare
oxtoacart
force-pushed
the
percy/putifabsent
branch
from
93cfe6c to
92b39fc
Compare
creachadair
left a comment
creachadair
left a comment
There was a problem hiding this comment.
Mechanically this LGTM, though I would like to ask for a bit more text and test while we're here.
oxtoacart
force-pushed
the
percy/putifabsent
branch
from
92b39fc to
1db40b0
Compare
acl/acl.go
Outdated
| // ActionCreateVersion ("create-version" in the API) denotes permission to | ||
| // create a specific version of a secret if and only if that version has no | ||
| // current value. |
There was a problem hiding this comment.
Optional simplification:
| // ActionCreateVersion ("create-version" in the API) denotes permission to | |
| // create a specific version of a secret if and only if that version has no | |
| // current value. | |
| // ActionCreateVersion ("create-version" in the API) denotes permission to | |
| // create a specific version of a secret if and only if that version does | |
| // not exist. |
since we do not allow version values to change.
And this actually raises an interesting corner case I hadn't considered: Previously it was not possible to delete and replace a version (because any new version got a previously-unused version counter), but this allows that to happen.
I'm not sure if we care about that, but it makes me worry slightly: Does it matter if someone has a reliance interest on a value that may be now different, i.e., that the ID is no longer a reliable indicator of its content?
I wonder if we should consider making deleting a version leave a tombstone (remove the value, set a flag, perhaps), so that we cannot re-create the same version again later.
There was a problem hiding this comment.
I wonder if we should consider making deleting a version leave a tombstone
This is where my mind went after reading your previous sentence :)
Really great catch!
There was a problem hiding this comment.
Okay, I implemented this. I considered using a sentinel value, but instead I track deletions in their own map. This way we don't have to try to discern between a sentinel value and a real value.
| // UNIX timestamps. This simulates that. | ||
| year2099 := api.SecretVersion(time.Date(2099, 12, 31, 24, 60, 60, 0, time.UTC).Unix()) | ||
|
|
||
| t.Run("create", func(t *testing.T) { |
There was a problem hiding this comment.
Nit: The convention we used in the existing tests was MixedCase or MixedCase_withQualifier.
oxtoacart
force-pushed
the
percy/putifabsent
branch
2 times, most recently
from
60213be to
6d62412
Compare
creachadair
left a comment
creachadair
left a comment
There was a problem hiding this comment.
LGTM! I appreciate your patience talking through it all.
The `create-version` operation Creates a specific version of a secret, sets its value and immediately activates that version. It fails with HTTP status 412 (precondition failed) if this version of the secret was ever set, even if it has since been deleted.. Updates tailscale/corp#34020 Signed-off-by: Percy Wegmann <[email protected]>
oxtoacart
force-pushed
the
percy/putifabsent
branch
from
6d62412 to
74b6c38
Compare
Likewise! |
2 participants
The
create-versionoperation Creates a specific version of a secret, sets its value andimmediately activates that version. It fails with HTTP status 412 (precondition failed) if this
version of the secret already has a value.
Updates tailscale/corp#34020