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

feat(multi-tenancy): add support for multi-tenancy in live loader #7414

Merged
merged 13 commits into from
Feb 11, 2021

Conversation

NamanJain8
Copy link
Contributor

@NamanJain8 NamanJain8 commented Feb 9, 2021

Makes live-loader work with multi-tenancy.

  • Adds a flag namespace which complements the flags user and password, used by ACL

  • Adds a flag force-namespace which allows the guardian of the galaxy to load data into other namespaces.

  • User from guardian of galaxy group can load the data into any namespace.

  • Other users can only insert data into the namespace they have access to. They have to specify it via the namespace flag.

The --upsertPredicate flag only works when loading the data into a namespace you logged into.


This change is Reviewable

Copy link
Contributor

@ahsanbarkati ahsanbarkati left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @NamanJain8, @pawanrawal, and @vvbalaji-dgraph)


x/x.go, line 1074 at r1 (raw file):

	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
	defer cancel()
	//TODO(Ahsan): What should be the namespace here?

Remove this TODO

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Defer to @jarifibrahim for final approval.

Reviewed 10 of 12 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NamanJain8, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/live/run.go, line 175 at r2 (raw file):

		"be used to store blank nodes as an xid")
	flag.String("tmp", "t", "Directory to store temporary buffers.")
	flag.Uint64("force-namespace", math.MaxUint64,

Use Super Flags here for namespace, user, password, force as well.

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.

:lgtm: Got some questions.

Reviewable status: 12 of 13 files reviewed, 10 unresolved discussions (waiting on @NamanJain8, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/live/run.go, line 472 at r3 (raw file):

				// Sorts the nquads on basis of their predicates, while keeping the
				// predicates with count index later than those without it.
				// TODO(Naman): Why do we keep count indexes later?

This can be removed now.


dgraph/cmd/live/run.go, line 505 at r3 (raw file):

				l.allocateUids(nqs)
			} else {
				// TODO(Naman): Handle this. Upserts UIDs send a single upsert block for multiple

I don't see anything that would prevent this from running in multiple-namespaces mode. Don't you need to add a check somewhere?


edgraph/access_ee.go, line 1043 at r3 (raw file):

func AuthGuardianOfTheGalaxy(ctx context.Context) error {
	if !x.WorkerConfig.AclEnabled {

Don't you want to return an error if ACL is disabled and this function is called?


query/mutation.go, line 67 at r3 (raw file):

	// Reset the namespace to the original.
	defer func(ns uint64) {
		x.AttachNamespace(ctx, ns)

why do you need to do this? we've only read the namespace from the context, not deleted it.


query/query.go, line 2881 at r3 (raw file):

		typeNamespace, typeName := x.ParseNamespaceAttr(update.TypeName)
		if typeNamespace != namespace {
			continue

Should we log an error here?


query/query.go, line 2904 at r3 (raw file):

		nodeNamespace, attrName := x.ParseNamespaceAttr(node.Predicate)
		if nodeNamespace != namespace {
			continue

Should we log an error here?


x/x.go, line 288 at r3 (raw file):

	}
	ns := md.Get("galaxy-operation")
	return len(ns) > 0 && ns[0] == "true"

We should check for True as well.


x/x.go, line 457 at r3 (raw file):

	}
	md.Set("galaxy-operation", "true")
	ctx = metadata.NewOutgoingContext(ctx, md)

return metadata...

Copy link
Contributor Author

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 13 files reviewed, 10 unresolved discussions (waiting on @ahsanbarkati, @jarifibrahim, @manishrjain, @NamanJain8, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/live/run.go, line 472 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

This can be removed now.

Done.


dgraph/cmd/live/run.go, line 505 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

I don't see anything that would prevent this from running in multiple-namespaces mode. Don't you need to add a check somewhere?

Added that.


edgraph/access_ee.go, line 1043 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Don't you want to return an error if ACL is disabled and this function is called?

This function is also called from non-ACL part of code(Dropall etc), so we should not return an error here.
We can move this check over there I think.


query/mutation.go, line 67 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

why do you need to do this? we've only read the namespace from the context, not deleted it.

We are modifying the context to execute the query in the context of edge few lines below. So, we should reset it here as well.


query/query.go, line 2881 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Should we log an error here?

No. Dgraph fetches the entire schema and then filter out the predicates of the required namespace. So, this is expected and is not an error.


query/query.go, line 2904 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Should we log an error here?

Not needed.


x/x.go, line 1074 at r1 (raw file):

Previously, ahsanbarkati (Ahsan Barkati) wrote…

Remove this TODO

Done.


x/x.go, line 288 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

We should check for True as well.

Done.


x/x.go, line 457 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

return metadata...

Done.

Copy link
Contributor Author

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 15 files reviewed, 10 unresolved discussions (waiting on @ahsanbarkati, @jarifibrahim, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/live/run.go, line 175 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Use Super Flags here for namespace, user, password, force as well.

I have combined user, password and namespace as they are the login credentials. --force-namespace is an unrelated flag.

@NamanJain8 NamanJain8 merged commit e9caa75 into ibrahim/multitenancy Feb 11, 2021
@NamanJain8 NamanJain8 deleted the naman/multitenancy/live-loader branch February 11, 2021 09:10
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.

4 participants