Enterprise - Support reading a signed file with details of Enterprise license #3824
Conversation
…alpha servers from Zero.
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@pawanrawal you can click here to see the review status or cancel the code review job.
pawanrawal
changed the title
There was a problem hiding this comment.
Everything here looks like a good start to adding this feature. Thanks for documenting the TODOs and pointing out places that are still in progress. Left some feedback and questions for you inline.
Reviewed with ❤️ by PullRequest
dgraph/cmd/zero/pgp.go
Outdated
| return errors.Wrapf(err, "while reading public key") | ||
| } | ||
|
|
||
| sf, err := os.Open(signedFile) |
There was a problem hiding this comment.
Do you want to close the sf file handle at some point or possibly defer sf.Close() after checking the error below?
| return errors.Wrapf(err, "while reading body from signed license file") | ||
| } | ||
| if md.Signature == nil { | ||
| return errors.New("invalid signature while trying to verify license file") |
There was a problem hiding this comment.
Not positive, but looks like maybe you could wrap the error from md.SignatureError here if you want extra context on the failure. https://godoc.org/golang.org/x/crypto/openpgp#MessageDetails
dgraph/cmd/zero/zero.go
Outdated
| } | ||
|
|
||
| expiry := time.Unix(s.state.Enterprise.ExpiryTs, 0) | ||
| if time.Now().Before(expiry) { |
There was a problem hiding this comment.
looks like could potentially simplify this to one line since you already have a boolean expression:
s.state.Enterprise.Enabled = time.Now().Before(expiry)
| break | ||
| } | ||
| glog.Errorf("While proposing enterprise state: %v. Retrying...", err) | ||
| time.Sleep(3 * time.Second) |
There was a problem hiding this comment.
Should there be any limit to the number of times this is tried or possibly some sort of exponential backoff? Or maybe worth a TODO for the future.
|
|
||
| go func() { | ||
| for { | ||
| err := n.proposeAndWait(context.Background(), proposal) |
There was a problem hiding this comment.
Just to double check, is the n.proposeAndWait safe to call on other threads/goroutines? I didn't see any documentation on the method whether it is or isn't but looks like it does some things like n.Proposals.Delete(key) within it that could potentially mutate or not be thread safe. I find it can be useful for future readers of your code to document which methods require locks and which don't since it looks like there are some locks being used within these methods.
dgraph/cmd/zero/run.go
Outdated
| @@ -95,6 +95,9 @@ instances to achieve high-availability. | |||
| // about the status of supporting annotation logs through the datadog exporter | |||
| flag.String("datadog.collector", "", "Send opencensus traces to Datadog. As of now, the trace"+ | |||
| " exporter does not support annotation logs and would discard them.") | |||
| flag.String("enterprise_license", "", "Path to the enterprise license file") | |||
| // TODO - Only for testing, remove before shipping. | |||
There was a problem hiding this comment.
Could consider upgrading this to a FIXME if it is supposed to be removed before shipping to prevent it sneaking by if FIXME caries a higher weight on your team.
dgraph/cmd/zero/raft.go
Outdated
| break | ||
| } | ||
| if err == errInvalidProposal { | ||
| break |
There was a problem hiding this comment.
Should anything be logged in this case or is it expected to no-op silently on an invalidProposal?
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, 7 unresolved discussions (waiting on @manishrjain and @pullrequest[bot])
dgraph/cmd/zero/pgp.go, line 41 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Do you want to close the
sffile handle at some point or possiblydefer sf.Close()after checking the error below?
Done in the caller.
dgraph/cmd/zero/pgp.go, line 65 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Not positive, but looks like maybe you could wrap the error from
md.SignatureErrorhere if you want extra context on the failure. https://godoc.org/golang.org/x/crypto/openpgp#MessageDetails
Checking for it separately. In my tests I found it to be empty when when md.Signature was nil. If I use errors.Wrapf and its empty then no error would be returned, hence I check for it separately before.
dgraph/cmd/zero/raft.go, line 524 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Just to double check, is the
n.proposeAndWaitsafe to call on other threads/goroutines? I didn't see any documentation on the method whether it is or isn't but looks like it does some things liken.Proposals.Delete(key)within it that could potentially mutate or not be thread safe. I find it can be useful for future readers of your code to document which methods require locks and which don't since it looks like there are some locks being used within these methods.
Added comments to the function definition. It is thread safe.
dgraph/cmd/zero/raft.go, line 530 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Should anything be logged in this case or is it expected to no-op silently on an invalidProposal?
Done.
dgraph/cmd/zero/raft.go, line 533 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Should there be any limit to the number of times this is tried or possibly some sort of exponential backoff? Or maybe worth a TODO for the future.
Done.
dgraph/cmd/zero/run.go, line 99 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Could consider upgrading this to a FIXME if it is supposed to be removed before shipping to prevent it sneaking by if FIXME caries a higher weight on your team.
Done.
dgraph/cmd/zero/zero.go, line 286 at r1 (raw file):
Previously, pullrequest[bot] wrote…
looks like could potentially simplify this to one line since you already have a boolean expression:
s.state.Enterprise.Enabled = time.Now().Before(expiry)
Done.
There was a problem hiding this comment.
Reviewed 4 of 10 files at r1, 9 of 9 files at r2.
Reviewable status: 10 of 13 files reviewed, 16 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @pullrequest[bot])
dgraph/cmd/zero/http.go, line 237 at r2 (raw file):
} func (st *state) applyEnterpriseLicense(w http.ResponseWriter, r *http.Request) {
Add some comment.
dgraph/cmd/zero/pgp.go, line 29 at r2 (raw file):
) func enterpriseDetails(signedFile, publicKey io.Reader, e *enterprise) error {
verifySignature
dgraph/cmd/zero/pgp.go, line 56 at r2 (raw file):
// below. if md.SignatureError != nil { return errors.Wrapf(md.SignatureError, "signature error while trying to verify license file")
100 chars.
dgraph/cmd/zero/raft.go, line 522 at r2 (raw file):
} if fpath := Zero.Conf.GetString("enterprise_license"); len(fpath) > 0 {
Remove this.
dgraph/cmd/zero/zero.go, line 45 at r2 (raw file):
) type enterprise struct {
type license struct
dgraph/cmd/zero/zero.go, line 46 at r2 (raw file):
type enterprise struct { Entity string `json:"entity"`
User
dgraph/cmd/zero/zero.go, line 776 at r2 (raw file):
return errors.Wrapf(err, "while extracting enterprise details from the license") }
Check whether we can do it before proposing.
edgraph/access_ee.go, line 45 at r2 (raw file):
request *api.LoginRequest) (*api.Response, error) { if !worker.EnterpriseEnabled() { return nil, errors.New("Enterprise features are disabled. You can enable them by " +
Look into this carefully. Ensure that for enterprise binary, but with features disabled, users should be able to use the DB but not be able to set ACLs.
protos/pb.proto, line 140 at r2 (raw file):
} message Enterprise {
License
There was a problem hiding this comment.
Reviewable status: 4 of 13 files reviewed, 16 unresolved discussions (waiting on @manishrjain and @pullrequest[bot])
dgraph/cmd/zero/http.go, line 237 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Add some comment.
Done.
dgraph/cmd/zero/pgp.go, line 29 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
verifySignature
Done.
dgraph/cmd/zero/pgp.go, line 56 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars.
Done.
dgraph/cmd/zero/raft.go, line 522 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Remove this.
Done.
dgraph/cmd/zero/zero.go, line 45 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
type license struct
Done.
dgraph/cmd/zero/zero.go, line 46 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
User
Done.
dgraph/cmd/zero/zero.go, line 776 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Check whether we can do it before proposing.
Done.
edgraph/access_ee.go, line 45 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Look into this carefully. Ensure that for enterprise binary, but with features disabled, users should be able to use the DB but not be able to set ACLs.
Checked with Lucas that Login is an enterprise only feature and hence this check is ok. They can still query and mutations without Login.
protos/pb.proto, line 140 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
License
Done.
|
PR is ready to merge. Waiting for @danielmai to share the signed file that we'll upload to Teamcity and read as an environment variable. |
This change gets rid of the
--enterprise_featuresflag in Zero. Instead users who want to use the enterprise feature, would have to supply a file while starting a Zero node. This file would contain would be signed by our private key and would contain details about the maximum number of nodes they can run and the expiry of the license. This information would automatically be propagated to Alpha nodes along with other membership info.TODO
enterpriseDetailsfunction to check PGP verification is working as expected.This change is