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

[POC] Switch from gorilla mux to standard library router #1457

Closed
3 tasks done
szwedm opened this issue Nov 15, 2024 · 7 comments
Closed
3 tasks done

[POC] Switch from gorilla mux to standard library router #1457

szwedm opened this issue Nov 15, 2024 · 7 comments
Assignees
Labels
kind/enhancement Categorizes issue or PR as related to modifying or improving an existing feature

Comments

@szwedm
Copy link
Contributor

szwedm commented Nov 15, 2024

Description
KEB is now using gorilla/mux as a router library. Due to the router change in brokerapi, we should use go-chi/chi.

Goal
Use latest Broker API library. Remove our fork.

AC

  • KEB is using standard library router
  • KEB is able to use gorilla/mux
  • Discuss a possibility to use std lib router in brokerapi module with the module owners

Possibility to switch back to gorilla/mux

@szwedm szwedm self-assigned this Nov 15, 2024
@szwedm szwedm added the kind/enhancement Categorizes issue or PR as related to modifying or improving an existing feature label Nov 15, 2024
@szwedm
Copy link
Contributor Author

szwedm commented Nov 15, 2024

I would like to check 3 scenarios where Kyma Environment Broker's router is changed to work with the router in brokerapi latest versions:

  1. brokerapi std lib router - keb gorilla mux
  2. brokerapi std lib router - keb std lib router
  3. brokeapi chi router - keb std lib router

@szwedm
Copy link
Contributor Author

szwedm commented Nov 15, 2024

Scenario 1 - brokerapi std lib router - keb gorilla mux
#1443
Requests passed from gorilla mux to std lib router don't have the required data for reading values from path patterns. A middleware is required to populate the request with the required data. The middleware consists of http.ServeMux and an no-op handler to register a HTTP method and path pattern for requests.
Ref:
https://cs.opensource.google/go/go/+/refs/tags/go1.23.3:src/net/http/request.go;drc=29b1a6765fb5f124171d94f157b6d6c3b2687468;l=324
https://cs.opensource.google/go/go/+/refs/tags/go1.23.3:src/net/http/request.go;drc=29b1a6765fb5f124171d94f157b6d6c3b2687468;l=334)

@szwedm
Copy link
Contributor Author

szwedm commented Dec 4, 2024

Scenario 3 - brokerapi chi router - keb std lib ServeMux
#1480
Updating brokerapi module to the latest version and replacing gorilla mux router with std lib ServeMux in KEB results in unrecognisable values in URL paths. chi is unable to retrieve the values from paths where path pattern contains std lib ServeMux wildcards.

@szwedm
Copy link
Contributor Author

szwedm commented Dec 4, 2024

Scenario 2 - brokerapi std lib ServeMux - keb std lib ServeMux
#1525
Modules work well together, but both sides require a lot of code changes to replace current routers (chi in brokerapi and gorilla mux in KEB). This may be troublesome for other projects when there is a dependency to brokerapi with chi.

@szwedm
Copy link
Contributor Author

szwedm commented Dec 4, 2024

Scenario 1 (brokerapi http.ServeMux and KEB gorilla/mux) Scenario 2 (brokerapi http.ServeMux and KEB http.ServeMux) Scenario 3 (brokerapi chi and KEB http.ServeMux)
+ requires the least changes in KEB
+ handlers are not changed
+ dependency only to std lib
+ modules are 100% compatible
+ future-proof
+ no changes required in brokerapi, KEB can import the latest version of the module
- requires a lot of changes in brokerapi
- requires http.ServeMux in middleware func in KEB
- gorilla/mux module is in maintenance mode
- requires a lot of changes on both sides
- even though unit tests are passing, all handlers need to be tested while running like in production environment
- http.ServeMux lacks a lof of features from gorilla/mux, own implementations are required
- chi and ServeMux are not compatible, path values (pattern wildcards) are not recognized by chi

@szwedm
Copy link
Contributor Author

szwedm commented Dec 4, 2024

Since replacing the routing library from gorilla/mux to http.ServeMux requires a lot of code changes, we can try to replace gorilla/mux with go-chi/chi as well.

@szwedm
Copy link
Contributor Author

szwedm commented Dec 5, 2024

I posted a summary comment in the issue in brokerapi repository which concludes POC. Closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Categorizes issue or PR as related to modifying or improving an existing feature
Projects
None yet
Development

No branches or pull requests

1 participant