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

Check or emphasize that protobuf endpoints must be rooted at a versioned path #2084

Open
jpeach opened this issue Mar 4, 2022 · 1 comment
Assignees
Labels
type: enhancement New feature or request

Comments

@jpeach
Copy link

jpeach commented Mar 4, 2022

Description

The clutch HTTP muxer checks that gRPC gateway URLs are rooted at the path /v[0-9]+.

var apiPattern = regexp.MustCompile(`^/v\d+/`)

if apiPattern.MatchString(r.URL.Path) || r.URL.Path == "/healthcheck" {
// Serve from the embedded API handler.
a.next.ServeHTTP(w, r)
return
}

This is a trap for young players, and it took me a while to figure out why clutch served a 500 when I used an initial path component of /foo. It would be best if clutch enforced this constraint by inspecting the protobuf types at startup, but otherwise it would be nice to emphasize this in the docs.

@jpeach jpeach added the type: enhancement New feature or request label Mar 4, 2022
@danielhochman danielhochman self-assigned this Mar 10, 2022
@danielhochman
Copy link
Collaborator

danielhochman commented Mar 14, 2022

I threw together a quick prototype for doing this at runtime: https://github.com/lyft/clutch/compare/validate-pattern-at-runtime

But we should probably do it during compilation. Since Buf doesn't allow custom linting rules, we will need to write our own linter using protoparse. It shouldn't be that difficult, we already use protoparse for documentation generation.

Alternatively we could also log a warn at runtime. Maybe that is the best option since everyone might not be linting. For example we don't tell the user to run linting on the amiibo example: https://clutch.sh/docs/development/feature.

@jpeach any thoughts here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants