Support for encrypted backups/restore #5079
Conversation
parasssh
changed the title
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1.
Reviewable status: 2 of 6 files reviewed, 11 unresolved discussions (waiting on @manishrjain and @parasssh)
systest/backup/filesystem/backup_test.go, line 302 at r3 (raw file):
result := worker.RunRestore("./data/restore", backupLocation, lastDir, "") require.Error(t, result.Err) require.Contains(t, result.Err.Error(), "expected a BackupNum value of 1")
Can this PR be tested? It should be possible to check in a sample keyfile for the tests.
worker/backup_ee.go, line 129 at r3 (raw file):
// If encryption turned off, latest backup should be unencrypted. if latestManifest.Type != "" && latestManifest.Encrypted { err = errors.Errorf("Latest Manifest indicates the last backup was encrypted but this instance has encryption turned off. Try forcing.")
What does try forcing mean. If it is a command line flag / opt, would you mind mentioning the flag / opt to pass?
worker/backup_processor.go, line 161 at r3 (raw file):
} else { // Chain = Handler <- Gzip <- Data (plaintext) gzWriter = gzip.NewWriter(handler)
Can this be refactored to make it a bit easier to read (rather than the comment). Go Writers follow the decorator pattern, so it should be possible to model it something like this.
writer := handler
if Crypto {
writer = buildCryptoWriter(writer, KeyFile)
}
writer = gzip.NewWriter(cryptoWriter)
worker/restore.go, line 85 at r3 (raw file):
return 0, err } } else {
same comment as the writer chain. In fact, if the backup/restore related crypto stuff was put in it's own file, that would be easy to read
There was a problem hiding this comment.
Some minor comments. I'll take a look at the test once it's ready.
Reviewed 3 of 6 files at r1, 4 of 4 files at r4.
Reviewable status: 7 of 10 files reviewed, 15 unresolved discussions (waiting on @manishrjain and @parasssh)
ee/enc/util_ee.go, line 82 at r4 (raw file):
cnt, err := r.Read(iv) if cnt != 16 || err != nil { err = errors.Errorf("Unable to get IV from encrypted backup. Read %v bytes, err %v ", cnt, err)
error string should start with lowercase
worker/backup_ee.go, line 122 at r4 (raw file):
Latest Manifest
"latest manifest"
worker/backup_ee.go, line 123 at r4 (raw file):
Try
Maybe "Create a new full backup with the forceFull option." but it's ok if you think that's too long.
worker/backup_ee.go, line 129 at r4 (raw file):
Latest Manifest
also here should be "latest manifest"
parasssh
changed the title
There was a problem hiding this comment.
Dismissed @martinmr, and @martinmr from 15 discussions.
Reviewable status: 7 of 10 files reviewed, all discussions resolved (waiting on @manishrjain)
ee/enc/util_ee.go, line 82 at r4 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
error string should start with lowercase
Done.
systest/backup/filesystem/backup_test.go, line 302 at r3 (raw file):
Previously, gja (Tejas Dinkar) wrote…
Can this PR be tested? It should be possible to check in a sample keyfile for the tests.
Done.
worker/backup_ee.go, line 123 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 143 characters (from
lll)
Done.
worker/backup_ee.go, line 129 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 140 characters (from
lll)
Done.
worker/backup_ee.go, line 129 at r3 (raw file):
Previously, gja (Tejas Dinkar) wrote…
What does try forcing mean. If it is a command line flag / opt, would you mind mentioning the flag / opt to pass?
Done.
There was a problem hiding this comment.
a couple minor comments but
Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain and @parasssh)
worker/backup_ee.go, line 122 at r5 (raw file):
// If encryption turned on, latest backup should be encrypted. if latestManifest.Type != "" && !latestManifest.Encrypted { err = errors.Errorf("Latest Manifest indicates the last backup was not encrypted " +
latest manifest
worker/backup_ee.go, line 129 at r5 (raw file):
// If encryption turned off, latest backup should be unencrypted. if latestManifest.Type != "" && latestManifest.Encrypted { err = errors.Errorf("Latest Manifest indicates the last backup was encrypted " +
latest manifest
There was a problem hiding this comment.
Dismissed @martinmr from 2 discussions.
Reviewable status: 8 of 10 files reviewed, all discussions resolved (waiting on @manishrjain and @martinmr)
worker/backup_ee.go, line 122 at r5 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
latest manifest
Done.
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 2 of 4 files at r4, 3 of 3 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @parasssh)
ee/enc/util_ee.go, line 82 at r6 (raw file):
cnt, err := r.Read(iv) if cnt != 16 || err != nil { err = errors.Errorf("unable to get IV from encrypted backup. Read %v bytes, err %v ", cnt, err)
100 chars
There was a problem hiding this comment.
Dismissed @manishrjain from a discussion.
Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @manishrjain)
ee/enc/util_ee.go, line 82 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars
Done.
(cherry-picked from commit 669228c)
(cherry-picked from commit 669228c)
Description.
Support encrypted backups/restore feature. Fixes https://dgraph.atlassian.net/browse/DGRAPH-1094
GitHub Issue or Jira number.
https://dgraph.atlassian.net/browse/DGRAPH-1094
Other components or 3rd party tools affected (or regression areas).
Affected releases.
20.07
Changelog tags.
Please indicate if this is a breaking change.
Please indicate if this is an enterprise feature.
Yes
Please indicate if documentation needs to be updated.
https://dgraph.atlassian.net/browse/DGRAPH-1191
Please indicate if end to end testing is needed.
https://dgraph.atlassian.net/browse/DGRAPH-1192
This change is