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(dgraph): making all internal communications with tls configured #6692

Merged
merged 29 commits into from
Oct 28, 2020

Conversation

aman-bansal
Copy link
Contributor

@aman-bansal aman-bansal commented Oct 9, 2020

Fixes DGRAPH-2505, DGRAPH-2506 and DGRAPH-546. All internal communication can now be TLS encrypted.
Three flags have been exposed tls_internal_port_enabled, tls_cert and tls_key to enable internal communication.


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/bulk-loader Issues related to bulk loading. label Oct 9, 2020
@aman-bansal aman-bansal marked this pull request as ready for review October 12, 2020 14:59
@aman-bansal aman-bansal force-pushed the aman/internal_tls branch 2 times, most recently from b54a82a to 282682d Compare October 13, 2020 10:27
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 12 rules errored during the review.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 12 rules errored during the review.

dgraph/cmd/bulk/run.go Outdated Show resolved Hide resolved
dgraph/cmd/zero/run.go Show resolved Hide resolved
dgraph/cmd/zero/zero.go Outdated Show resolved Hide resolved
dgraph/cmd/zero/zero.go Outdated Show resolved Hide resolved
x/tls_helper.go Outdated Show resolved Hide resolved
x/tls_helper.go Outdated Show resolved Hide resolved
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 25 rules errored during the review.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 30 rules errored during the review.

@aman-bansal aman-bansal force-pushed the aman/internal_tls branch 3 times, most recently from dabde39 to 4c18671 Compare October 21, 2020 08:09
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Also, cherry-pick this onto the release branch as this has new flags that we don't want in master.

Reviewed 9 of 59 files at r1, 93 of 105 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @aman-bansal, @manishrjain, @parasssh, and @vvbalaji-dgraph)


conn/pool.go, line 149 at r2 (raw file):

	}

	tlsConfig, err := x.GenerateClientTLSConfig(conf)

Can generate it at startup and then pass it here.nn


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

		x.Checkf(tlsErr, "Unable to generate TLS Cert Pool")
	} else {
		helperConfig, err := x.LoadNodeTLSClientHelperConfig(conf)

these 4 lines can go in one func and be reused everywhere


dgraph/cmd/zero/raft.go, line 216 at r2 (raw file):

	// Create a connection to this server.
	go conn.GetPools().Connect(member.Addr, opts.tlsClientConfig)

can this be part of the node struct


dgraph/cmd/zero/run.go, line 56 at r2 (raw file):

	rebalanceInterval time.Duration

	//tls client config which will be used by zeros to connect internally

// tls...


tlstest/mtls_internal/multi_group/mtls_internal_multi_group_test.go, line 36 at r2 (raw file):

		txn := client.NewTxn()
		_, err := txn.Mutate(ctx, &api.Mutation{SetJson: []byte(setJSON)})

send mutation to one alpha and query the other two alphas

Copy link
Contributor Author

@aman-bansal aman-bansal 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: 79 of 103 files reviewed, 11 unresolved discussions (waiting on @manishrjain, @parasssh, @pawanrawal, and @vvbalaji-dgraph)


conn/pool.go, line 149 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can generate it at startup and then pass it here.nn

Done.


dgraph/cmd/bulk/run.go, line 92 at r1 (raw file):

Previously, parasssh wrote…

ISnt this name confusing? This dir contains the bulk loader client's certs, no?

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

these 4 lines can go in one func and be reused everywhere

Done.


dgraph/cmd/zero/raft.go, line 216 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

can this be part of the node struct

Done.


dgraph/cmd/zero/run.go, line 125 at r1 (raw file):

Previously, aman-bansal (aman bansal) wrote…

No this won't change. cmux will only control zero HTTP port.

Done.


dgraph/cmd/zero/run.go, line 56 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

// tls...

Done.


dgraph/cmd/zero/zero.go, line 231 at r1 (raw file):

Previously, parasssh wrote…

remove vertical spacing.

Done.


dgraph/cmd/zero/zero.go, line 236 at r1 (raw file):

Previously, aman-bansal (aman bansal) wrote…

Yes this was for my understanding to see which connections are made. I will remove these

Done.


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

Previously, parasssh wrote…

we dont need to differentiate client and server names. They can be the same thing.

Done.


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

Previously, parasssh wrote…

spelling ... tzhe

Done.


tlstest/mtls_internal/multi_group/mtls_internal_multi_group_test.go, line 36 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

send mutation to one alpha and query the other two alphas

Done.

Copy link
Contributor Author

@aman-bansal aman-bansal 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: 61 of 111 files reviewed, 13 unresolved discussions (waiting on @manishrjain, @parasssh, @pawanrawal, and @vvbalaji-dgraph)


tlstest/mtls_internal/acl/docker-compose.yml, line 48 at r4 (raw file):

--tls_dir /dgraph-tls

tlstest/mtls_internal/loader/loader_test.go, line 74 at r4 (raw file):

"--tls_enable_inter_node=true"

tls_internal_port_enabled

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

:lgtm: with some comments

Reviewed 12 of 105 files at r2, 47 of 51 files at r3, 4 of 4 files at r4.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @aman-bansal, @manishrjain, @parasssh, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 171 at r3 (raw file):

	flag.String("tls_client_auth", "VERIFYIFGIVEN", "Enable TLS client authentication")
	flag.Bool("tls_enable_inter_node", false, "(optional) enable inter node TLS encryption between cluster nodes.")
	flag.String("tls_cert", "", "(optional) The Cert file provided for the nodes to connect with the cluster.")

connect as a client with other nodes in the cluster.

Similarly below


dgraph/cmd/alpha/run.go, line 671 at r3 (raw file):

		TLSClientConfig:      tlsConf,
		TLSDir:               Alpha.Conf.GetString("tls_dir"),
		TLSInterNodeEnabled: Alpha.Conf.GetBool("tls_enable_inter_node"),

Is gofmt run?


tlstest/mtls_internal/ha_6_node/ha_test.go, line 138 at r4 (raw file):

	dgConn, err := grpc.Dial(":" + port, grpc.WithTransportCredentials(credentials.NewTLS(tlsConf)))
	require.NoError(t, err)
	time.Sleep(time.Second * 6)

Why is this sleep required?

Copy link
Contributor Author

@aman-bansal aman-bansal 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: 98 of 111 files reviewed, 9 unresolved discussions (waiting on @manishrjain, @parasssh, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 171 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

connect as a client with other nodes in the cluster.

Similarly below

Done.


dgraph/cmd/alpha/run.go, line 671 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is gofmt run?

Done.


tlstest/mtls_internal/ha_6_node/ha_test.go, line 138 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why is this sleep required?

this is Done. Actually I write the test case in different way initially that requires wait for the cluster to setup. changed it to docker compose setup later. I missed this refactoring.

@aman-bansal aman-bansal merged commit a63af32 into master Oct 28, 2020
aman-bansal added a commit that referenced this pull request Nov 10, 2020
…6692)

* making all internal communications with tls configured
@aman-bansal aman-bansal deleted the aman/internal_tls branch December 15, 2020 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bulk-loader Issues related to bulk loading.
Development

Successfully merging this pull request may close these issues.

3 participants