Fix the bug that the key manager is not updated during the Rename #227
Conversation
Signed-off-by: Xintao <hunterlxt@live.com>
hunterlxt
force-pushed
the
xt/fix-current
branch
from
0cc810c
to
311bbfb
Compare
|
Please fill in PR summary to describe what is the issue, what is changed. |
env/env_basic_test.cc
Outdated
| SyncPoint::GetInstance()->SetCallBack("Encryption:new_file", [&](void* arg) { | ||
| bool* skip = static_cast<bool*>(arg); | ||
| *skip = false; | ||
| }); |
There was a problem hiding this comment.
You need to call SyncPoint::EnableProcessing() after setting up the callback, and SyncPoint::ClearAllCallbacks() then SyncPoint::DisableProcessing() at the end of the test.
| return target()->RenameFile(src_fname, dst_fname); | ||
| Status s = target()->RenameFile(src_fname, dst_fname); | ||
| // Replacing with plaintext requires deleting the info in the key manager. | ||
| Status delete_status __attribute__((__unused__)) = |
There was a problem hiding this comment.
Let's handle the error by hacking a bit. In KeyManagedEncryptedEnv::NewSequentialFile, check if the file being read is the CURRET file. If so, try to read it if it is encrypted, and if the file doesn't start with "MANIFEST-" and end with "\n", reopen it as plaintext file.
| Status s = target()->RenameFile(src_fname, dst_fname); | ||
| // Replacing with plaintext requires deleting the info in the key manager. | ||
| Status delete_status __attribute__((__unused__)) = | ||
| key_manager_->DeleteFile(dst_fname); |
There was a problem hiding this comment.
This is only needed when ShouldSkipEncryption(dst_fname) returns false once. How can this happen?
There was a problem hiding this comment.
when upgrade from old version like tikv/tikv#9701
There was a problem hiding this comment.
@hunterlxt update the inline comment to mention it is for upgrading from TiKV <= v5.0.0-rc?
Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: Xintao <hunterlxt@live.com>
| Status s = target()->RenameFile(src_fname, dst_fname); | ||
| // Replacing with plaintext requires deleting the info in the key manager. | ||
| Status delete_status __attribute__((__unused__)) = | ||
| key_manager_->DeleteFile(dst_fname); |
There was a problem hiding this comment.
@hunterlxt update the inline comment to mention it is for upgrading from TiKV <= v5.0.0-rc?
encryption/encryption.cc
Outdated
| if (!s.ok()) { | ||
| return s; | ||
| } | ||
| // Hack: sometimes the current file may be read as an encrypted |
There was a problem hiding this comment.
mention it happens when upgrade from older version (i.e. TiKV <= v5.0.0-rc).
encryption/encryption.cc
Outdated
| if (!s.ok()) { | ||
| return s; | ||
| } | ||
| // Hack: sometimes the current file may be read as an encrypted | ||
| // file by mistakes. | ||
| if (ShouldSkipEncryption(fname) && !isValidCurrentFile(result)) { | ||
| s = target()->NewSequentialFile(fname, result, options); | ||
| } else { | ||
| s = encrypted_env_->NewSequentialFile(fname, result, options); | ||
| } |
There was a problem hiding this comment.
Let's change the flow here:
if (s.ok() && ShouldSkipEncryption(fname)) {
if (IsValidCurrentFile(std::move(result)) {
s = target()->NewSequentialFile(...);
} else {
s = encrypted_env_->NewSequentialFile(...);
}
}
basically I would prefer avoid early return, since other cases within the switch...case... block does not immediately return.
file/filename.h
Outdated
| @@ -46,6 +46,10 @@ enum FileType { | |||
| // operations. | |||
| extern bool ShouldSkipEncryption(const std::string& fname); | |||
|
|
|||
| // Determine if the content is read from the valid current file. | |||
| extern bool isValidCurrentFile( | |||
| std::unique_ptr<rocksdb::SequentialFile>* seq_file); | |||
There was a problem hiding this comment.
| std::unique_ptr<rocksdb::SequentialFile>* seq_file); | |
| std::unique_ptr<rocksdb::SequentialFile> seq_file); |
This is to indicate the function will consume seq_file.
encryption/encryption.cc
Outdated
| } | ||
| // Hack: sometimes the current file may be read as an encrypted | ||
| // file by mistakes. | ||
| if (ShouldSkipEncryption(fname) && !isValidCurrentFile(result)) { |
There was a problem hiding this comment.
btw let's have this treatment for the CURRENT file only. Replace ShouldSkipEncryption(fname) with a new IsCurerntFile(fname) here.
Signed-off-by: Xintao <hunterlxt@live.com>
hunterlxt
force-pushed
the
xt/fix-current
branch
from
85a27c0
to
09d9825
Compare
|
I tested with tikv integrated with this fix and found that tikv/tikv#9701 was solved. |
| // Hack: sometimes the current file may be read as an encrypted | ||
| // file by mistakes(when upgrading from TiKV <= v5.0.0-rc). | ||
| if (s.ok() && IsCurrentFile(fname)) { | ||
| if (!IsValidCurrentFile(std::move(*result))) { |
There was a problem hiding this comment.
Add a comment explaining that on certain occasion a plaintext current file is written before stale encryption info is cleaned up.
Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: Xintao <hunterlxt@live.com>
…kv#227) Signed-off-by: Xintao <hunterlxt@live.com> Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: Xintao <hunterlxt@live.com> Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: Xintao <hunterlxt@live.com> Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: Xintao hunterlxt@live.com