Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(raftwal): Pass the encryption key instead of reading from WorkerConfig #7013

Merged
merged 6 commits into from
Dec 7, 2020

Conversation

ahsanbarkati
Copy link
Contributor

@ahsanbarkati ahsanbarkati commented Nov 27, 2020

We should not read the encryption key from the worker config, rather we can pass it as an argument to the Init() method which initializes the diskStorage for the raftwal.


This change is Reviewable

@netlify
Copy link

netlify bot commented Nov 27, 2020

Deploy preview for dgraph-docs ready!

Built with commit e4d34ae

https://deploy-preview-7013--dgraph-docs.netlify.app

@ahsanbarkati ahsanbarkati marked this pull request as ready for review November 27, 2020 17:04
Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we had a func called InitEncrypted(dir string, encKey x.SensitiveByteSlice) *DiskStorage alongside the Init func? That way the change isn't as invasive as adding nil everywhere for Init(dir, nil). And, given that all the error checks just panic/stop anyway if raftwal isn't init'd when returning an error value, is returning an error helpful?

Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)

@ahsanbarkati
Copy link
Contributor Author

ahsanbarkati commented Nov 27, 2020

Yes, having InitEncrypted makes sense. The idea for returning the error was mainly because of the tests, we need to check against the errors in the tests. But yes, I can make Init a wrapper around InitEncrypted and handle the error there itself.

Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the return parameter from InitEncrypted function and this PR should be good to merge.

raftwal/storage.go Show resolved Hide resolved
@ahsanbarkati ahsanbarkati merged commit cf4c796 into master Dec 7, 2020
@ahsanbarkati ahsanbarkati deleted the ahsan/raftwal-cleanup branch December 7, 2020 17:16
NamanJain8 pushed a commit that referenced this pull request Dec 7, 2020
…onfig (#7013)

Pass the encryption key to `raftwal.InitEncrypted()` instead of directly reading from the workerConfig.
Fix the debug tool and raftwal migrate, to allow using them with encryption enabled.

(cherry picked from commit cf4c796)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants