Skip to content

feat: Live Loader Now Supports Slash GraphQL #6117

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

Merged
merged 4 commits into from
Jul 31, 2020
Merged

Conversation

gja
Copy link
Contributor

@gja gja commented Jul 30, 2020

Live Loader can now support Slash GraphQL

Here is an example:

./dgraph live --slash_grpc_endpoint=defiant-curve-2926.grpc.us-east-1.aws.thegaas.com:443 -f /path/to/file.json.gz -t api-token

Why introduce a new flag, --slash_grpc_endpoint?

  • Connections to zero didn't support TLS, so we'd need a new flag to indicate this
  • There was no way to start TLS, but use the system CA pool (--tls_ca_cert had to be a valid path). This is not needed for public auth
  • Passing in the --alpha --zero and --tls_server_name with the same value felt clunky
  • In Slash GraphQL, the GRPC authentication is passed in via Authentication metadata to all endpoints, as opposed to only passing in X-Dgraph-AuthToken to alter alone

This change is Reviewable

@gja gja self-assigned this Jul 30, 2020
@github-actions github-actions bot added the area/live-loader Issues related to live loading. label Jul 30, 2020
@gja gja requested review from martinmr and pawanrawal July 30, 2020 06:35
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.

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gja, @manishrjain, @martinmr, and @vvbalaji-dgraph)


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

	dialOpts := []grpc.DialOption{}
	if conf.GetString("slash_grpc_endpoint") != "" && conf.IsSet("auth_token") {

We should also be modifying the description of auth_token so that it says that the token can also be the Slash API key now?


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

}

// SlashTLSConfig returns the TLS config appropriate for SlashGraphQL

Please add a comment that this function assumed endpoint != ""


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

	pool, err := generateCertPool("", true)
	hostWithoutPort := strings.Split(endpoint, ":")[0]
	if err != nil {

The error is usually checked right after it is declared. So it should be checked after the poll, err line

@gja gja merged commit b8c83e5 into master Jul 31, 2020
@gja gja deleted the tejas/slash-live-loader branch July 31, 2020 11:11
gja added a commit that referenced this pull request Jul 31, 2020
Live Loader can now support Slash GraphQL

Here is an example: 
```
./dgraph live --slash_grpc_endpoint=defiant-curve-2926.grpc.us-east-1.aws.thegaas.com:443 -f /path/to/file.json.gz -t api-token
```

Why introduce a new flag, `--slash_grpc_endpoint`?
* Connections to zero didn't support TLS, so we'd need a new flag to indicate this
* There was no way to start TLS, but use the system CA pool (--tls_ca_cert had to be a valid path). This is not needed for public auth
* Passing in the --alpha --zero and --tls_server_name with the same value felt clunky
* In Slash GraphQL, the GRPC authentication is passed in via `Authentication` metadata to all endpoints, as opposed to only passing in `X-Dgraph-AuthToken` to alter alone
gja added a commit that referenced this pull request Aug 19, 2020
Live Loader can now support Slash GraphQL

Here is an example: 
```
./dgraph live --slash_grpc_endpoint=defiant-curve-2926.grpc.us-east-1.aws.thegaas.com:443 -f /path/to/file.json.gz -t api-token
```

Why introduce a new flag, `--slash_grpc_endpoint`?
* Connections to zero didn't support TLS, so we'd need a new flag to indicate this
* There was no way to start TLS, but use the system CA pool (--tls_ca_cert had to be a valid path). This is not needed for public auth
* Passing in the --alpha --zero and --tls_server_name with the same value felt clunky
* In Slash GraphQL, the GRPC authentication is passed in via `Authentication` metadata to all endpoints, as opposed to only passing in `X-Dgraph-AuthToken` to alter alone
@rstorr
Copy link

rstorr commented Oct 2, 2020

@CodeLingoBot capture errors should be checked immediately after they are declared

@codelingo
Copy link

codelingo bot commented Oct 2, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/live-loader Issues related to live loading.
Development

Successfully merging this pull request may close these issues.

4 participants