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: openapi request and response validation middleware #116

Merged
merged 4 commits into from
Jan 14, 2022

Conversation

cmars
Copy link
Contributor

@cmars cmars commented Jan 11, 2022

This introduces a simple validation middleware which makes use of the
recently added openapi3filter.Validator (see
https://pkg.go.dev/github.com/getkin/kin-openapi/openapi3filter#example-Validator)

@cmars cmars requested a review from a team as a code owner January 11, 2022 18:55
return nil, err
}
v.versions[i] = *version
router, err := gorillamux.NewRouter(docs[i])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not doing what you think this is doing. We made the v3-go-libs router to explicitly NOT use their router for very valid reasons. Mainly the fact that other middlewares cannot be used with their implementation. Please do not use this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not completely sure I understand what you mean by middlewares conflicting with the gorillamux implementation of router.Router... If it's about non-API routes alongside the API at the top-level (like /metrics and healthchecks) and sub-route matching, I think that can be accommodated by applying the middleware only to the top-level handler/subrouter for the API, and then possibly using the server URL override to match the request path if it gets stripped.

Validating such a scenario with an example test seems in order, can do!

Copy link
Contributor

@genslein genslein Jan 12, 2022

Choose a reason for hiding this comment

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

https://github.com/getkin/kin-openapi/blob/master/routers/gorillamux/router.go#L67 the getkin router explicitly does not expose the actual muxRouter. Gorilla Mux requires a call to Use function https://github.com/gorilla/mux#middleware, so trying to use a prom-client for example outside of this is not possible in a clean way.

We went down this road while investing time into the v3-go-libs. @Todai88 and myself can chat about this.

@cmars cmars force-pushed the feat/versionware-validation branch 3 times, most recently from 6a58371 to acbb7ea Compare January 14, 2022 00:38
@cmars
Copy link
Contributor Author

cmars commented Jan 14, 2022

@genslein I'm going to try and recap points from our conversation yesterday about this PR, to capture the callouts and points raised, which may be instructive for future improvement as the need arises. Please add anything I've missed, or correct anything I've misunderstood.

  • Including versionware in vervet has value in providing a concrete example of how to build an actual service with our open source versioning scheme and tooling.
  • OpenAPI request and response middleware needs to conform to the Go stdlib net/http interfaces, specifically being able to wrap any http.Handler. If we cannot do this, introducing versioning to a Go service will require a significant restructuring that seems to be blocking adoption so far (my feedback from stakeholders).
  • However we're also aware that the gorillamux implementation of the kin-router/router.Router has suboptimal performance. It performs an O(N) search on route matching, which may see a degradation in performance as the number of operations in the vervet-compiled OpenAPI spec grows.
  • This can be mitigated by providing an alternative implementation of kin-openapi/router.Router. Instead of using gorillamux, it could use a trie for fast lookups. We could add it here, or contribute it upstream.
  • The Chi and Gorilla example versioned services are expressed as Example tests, which ensures that they will be continually tested in CI, avoiding drift and rot.

Looking out even longer term, request and response middleware may be something that moves up the stack into an edge proxy -- so this code may be adapted into such a thing, with the caveats above called out well in advance (esp. the perf issues).

CC @jjmschofield @Todai88 @jcsackett

@cmars cmars force-pushed the feat/versionware-validation branch 3 times, most recently from 1cb71eb to 3a03c04 Compare January 14, 2022 17:55
@jcsackett
Copy link
Contributor

It's not clear to me from the comments you've posted, @cmars, if the code that @genslein highlighted is going to be modified or is ok as it is presently written? It sounds as though the problem of not working with other middleware is still a concern?

Setting that part aside, the rest of this looks good to me. It's a little odd to me that the examples are here rather than part of the same example repo that is coming out of the v3-go work elsewhere; I don't think it's wrong to have it here, but I worry about having too many examples of the way to do versioning and our api design in too many places. However, since I don't have any solution to that, I don't want to block this on a "maybe this will confuse someone, somewhere, sometime". 😄

I really like that the chi and gorilla examples are tests; adopting that idea for other places where we have example implementations seems like a really solid idea. 🎉

@cmars
Copy link
Contributor Author

cmars commented Jan 14, 2022

@genslein Landing to unblock stakeholders who are needing the functionality in this PR, but please do let me know if I've missed any concerns above, I'd be happy to followup. 🙏

@cmars cmars force-pushed the feat/versionware-validation branch from 3a03c04 to 1963fca Compare January 14, 2022 20:58
@cmars cmars merged commit 25fb58e into snyk:main Jan 14, 2022
@genslein
Copy link
Contributor

@cmars thanks for picking up the comments from my sick day.

@jcsackett The issue is that we are initializing a router without actually using it for anything other than the middleware and it's missing some of the implementation improvements from the first pass of the v3-libs middleware attempt at making a router with the request and response middlewares.

While we achieved some of those things, including better route lookups and validation running times, @cmars did point out that it's not standard enough interface to use. This makes using the v3 libs router and the current iteration of the golang sample app cludgy for users to reason about. This implementation hides some non-standard workflows from end-users at least so we've opted for ease-of-use rather than potential performance for now.

The change here we agreed on Thursday was best to unblock customers but that we will have to consolidate on the golang sample app as well reusing some of the test case items here. There was also an issue with the port-over of the v3 sample app to its own repo which we've marked in work items. The follow-up plan here is to reimplement the sample app using versionware to integration test a bit more effectively for some newer features being worked on this cycle.

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.

None yet

3 participants