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: Improve messaging for common cloud config and rpc errors #2885

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

andrewmbenton
Copy link
Collaborator

This turned out to have more changes than I expected. A high-level list follows. There are definitely some style choices I made here that I'm not at all attached to, so feel free to recommend changes.

  1. Add a UnaryClientInterceptor for all gRPC client connections to catch and rewrite unauthenticated error messages

  2. Put the sqlc auth token in the config struct, and add simple validation

  3. Add more explanatory error messages in cases where users try to use cloud features without the proper configuration

  4. Prevent sending the SQLC_AUTH_TOKEN env var to plugins

Resolves #2881

1. Add a UnaryClientInterceptor for all gRPC client connections to
catch and rewrite unauthenticated error messages

2. Put the sqlc auth token in the config struct, and add simple validation

3. Add more explanatory error messages in cases where users try to use
cloud features without the proper configuration

4. Prevent sending the SQLC_AUTH_TOKEN env var to plugins

Resolves #2881

Resolves #2881
Comment on lines +9 to +17
func (c *Config) addEnvVars() error {
authToken := os.Getenv("SQLC_AUTH_TOKEN")
if authToken != "" && !strings.HasPrefix(authToken, "sqlc_") {
return fmt.Errorf("$SQLC_AUTH_TOKEN doesn't start with \"sqlc_\"")
}
c.Cloud.AuthToken = authToken

return nil
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to get away from calling os.Getenv() all over the place, but I'm not sure this (adding fields to Config struct and then populating after parse) is the best way to do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think as long as we only do this in one place, here in the config package, it's okay.

Comment on lines +40 to +42
if key == "SQLC_AUTH_TOKEN" {
continue
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't really need this, it just seemed like a reasonable thing to do (prevent users from sending their auth tokens to plugins).

@kyleconroy kyleconroy merged commit 4f875c1 into main Oct 19, 2023
8 checks passed
@kyleconroy kyleconroy deleted the andrew/better-rpc-error-messages branch October 19, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More useful error messages for RPC failures
2 participants