[ADDED] SecureDeleteMsg method to JetStreamManager #1025
Conversation
piotrpio
changed the title
jsm.go
Outdated
| } | ||
|
|
||
| // SecureDeleteMsg deletes a message from a stream. | ||
| // The deleted message is overwritten with random data |
There was a problem hiding this comment.
maybe mention here that it is slower as a result?
| DeleteMsg(name string, seq uint64, opts ...JSOpt) error | ||
|
|
||
| // SecureDeleteMsg deletes a message from a stream. The message value is overwritten with random data. | ||
| SecureDeleteMsg(name string, seq uint64, opts ...JSOpt) error |
There was a problem hiding this comment.
Wonder if we should not add an option instead of a new API, as we did for the direct get. For instance, we could have a JSOpt that is called nats.EraseMsg() or nats.SecureDelete() that would set a boolean in *jsOpts (see DirectGet() option).
There was a problem hiding this comment.
I thought about that, but I feel that in this case (having really only 2 options), 2 separate methods are a bit more straightforward to the user. JSOpt is already confusingly large bag of options and finding the one particular option which is applicable for a specific method is a hard job, even with good documentation.
Providing 2 methods on the other hand, should eliminate any confusion. If a user wants to delete message, they will stumble on 2 methods with relevant names: DeleteMsg(), which looks like a good starting point and SecureDeleteMsg(). Both have clear documentation and are not configurable (which is a good thing IMO).
WDYT?
There was a problem hiding this comment.
I started with adding a new API for the "direct get" but was told that it was better to add options instead. I do agree - however - that the JSOpt can be a bit confusing.
To sump up, I don't have objections per-se to add a new API (actually, in the C client I have js_DeleteMsg() and js_EraseMsg() APIs), just felt that the will was to go with options instead of new APIs. If other reviewers in the list are OK with new API, I am ok too.
There was a problem hiding this comment.
Usually I would lean into having an option in this case but after reading the ADR discussion, I reached the same conclusion in that in this case a new SecureDeleteMsg is what would fit better. TL;DR; of the ADR discussion, secure delete might have been a good default but considering the performance impact and the context of security, I think it sounds good to have its own function in this case.
|
|
||
| // SecureDeleteMsg deletes a message from a stream. | ||
| // The deleted message is overwritten with random data | ||
| func (js *js) SecureDeleteMsg(name string, seq uint64, opts ...JSOpt) error { |
There was a problem hiding this comment.
If we go the option route, the implementation will simply have to set NoErase to opts.noErase (or whatever boolean you chose).
jsm.go
Outdated
| return js.deleteMsg(o.ctx, name, msgDeleteRequest{Seq: seq}) | ||
| } | ||
|
|
||
| func (js *js) deleteMsg(ctx context.Context, stream string, req msgDeleteRequest) error { |
There was a problem hiding this comment.
If you go with different APIs, I would make req a pointer to msgDeleteRequest so we don't have the copy of this struct when invoked from the Delete or SecureDelete APIs.
| @@ -2116,6 +2116,113 @@ func TestJetStreamManagement_DeleteMsg(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestJetStreamManagement_SecureDeleteMsg(t *testing.T) { | |||
| s := RunBasicJetStreamServer() | |||
There was a problem hiding this comment.
Since really we don't know - from the client perspective - the difference between a delete and secure delete, what I would do is simply make sure that the client's "delete message request" contains the noErase boolean when it is a simple delete and not (or set to false) when using the secure version. This is done by creating a NATS subscription on the delete API subject and inspecting the content (see TestJetStreamConsumerConfigReplicasAndMemStorage for example)
There was a problem hiding this comment.
Good idea, I'll add that.
There was a problem hiding this comment.
I would have personally JUST done the check of the API request containing the no_erase (or not), the rest is testing server behavior really, but that's ok since code is already done.
jsm.go
Outdated
| } | ||
|
|
||
| func (js *js) deleteMsg(ctx context.Context, stream string, req *msgDeleteRequest) error { | ||
| if stream == _EMPTY_ { |
There was a problem hiding this comment.
You could replace this with that:
if err := checkStreamName(stream); err != nil {
return err
}
Since the above also check for invalid use of . in the same.
| @@ -2116,6 +2116,113 @@ func TestJetStreamManagement_DeleteMsg(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestJetStreamManagement_SecureDeleteMsg(t *testing.T) { | |||
| s := RunBasicJetStreamServer() | |||
There was a problem hiding this comment.
I would have personally JUST done the check of the API request containing the no_erase (or not), the rest is testing server behavior really, but that's ok since code is already done.
piotrpio
force-pushed
the
secure-delete
branch
2 times, most recently
from
95459f4
to
af8f561
Compare
|
@piotrpio Do you need new review on this? Looks like there are some force-push'es made, so not sure if things have changed since last review. |
|
@kozlovic Thanks, I just needed to rebase to resolve conflicts, and now some (not added in this PR) test is failing, I'm looking into it. |
|
That is maybe a flapper, let's recycle the test. |
|
If this is ready, maybe we get another LGTM from @wallyqs and I can merge? |
This addesses nats-io/nats-architecture-and-design#118
As mentioned in the above issue, the old
DeleteMsg()now setsno_erasetotrueby default (for better performance), and the newSecureDeleteMsg()can be used to completely overwrite the deleted message on server.