From 687e9accf82bc992cc97632386139ee6e3bb976e Mon Sep 17 00:00:00 2001 From: sneak Date: Sat, 3 Oct 2020 22:46:48 -0700 Subject: [PATCH 1/2] cleanups: * middlewares in correct order now, now always throws 500 on panic * only adds metrics middleware if metrics serving auth is configured * only adds metrics serving route if metrics serving auth is configured --- go.mod | 1 + go.sum | 2 ++ httpserver/main.go | 17 +++++++---- httpserver/routes.go | 71 ++++++++++++++++++++++++-------------------- 4 files changed, 54 insertions(+), 37 deletions(-) diff --git a/go.mod b/go.mod index e8258d9..fa3ac2f 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module git.eeqj.de/sneak/gohttpserver go 1.15 require ( + github.com/99designs/basicauth-go v0.0.0-20160802081356-2a93ba0f464d github.com/getsentry/sentry-go v0.7.0 github.com/go-chi/chi v4.1.2+incompatible github.com/joho/godotenv v1.3.0 diff --git a/go.sum b/go.sum index 44e5026..7dcc68b 100644 --- a/go.sum +++ b/go.sum @@ -12,6 +12,8 @@ cloud.google.com/go/pubsub v1.0.1/go.mod h1:R0Gpsv3s54REJCy4fxDixWD93lHJMoZTyQ2k cloud.google.com/go/storage v1.0.0/go.mod h1:IhtSnM/ZTZV8YYJWCY8RULGVqBDmpoyjwiyrjsg+URw= contrib.go.opencensus.io/exporter/prometheus v0.1.0/go.mod h1:cGFniUXGZlKRjzOyuZJ6mgB+PgBcCIa79kEKR8YCW+A= dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= +github.com/99designs/basicauth-go v0.0.0-20160802081356-2a93ba0f464d h1:j6oB/WPCigdOkxtuPl1VSIiLpy7Mdsu6phQffbF19Ng= +github.com/99designs/basicauth-go v0.0.0-20160802081356-2a93ba0f464d/go.mod h1:3cARGAK9CfW3HoxCy1a0G4TKrdiKke8ftOMEOHyySYs= github.com/AndreasBriese/bbloom v0.0.0-20190306092124-e2d15f34fcf9/go.mod h1:bOvUY6CB00SOBii9/FifXqc0awNKxLFCL/+pkDPuyl8= github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= diff --git a/httpserver/main.go b/httpserver/main.go index a6c4656..a3df137 100644 --- a/httpserver/main.go +++ b/httpserver/main.go @@ -65,7 +65,6 @@ func Run(appname, version, buildarch string) int { }) // this does nothing if SENTRY_DSN is unset in env. - s.enableSentry() // TODO remove: if s.sentryEnabled { @@ -75,6 +74,9 @@ func Run(appname, version, buildarch string) int { s.configure() s.setupLogging() + // logging before sentry, because sentry logs + s.enableSentry() + s.databaseURL = viper.GetString("DBURL") s.port = viper.GetInt("PORT") @@ -82,19 +84,21 @@ func Run(appname, version, buildarch string) int { } func (s *server) enableSentry() { - sentryDSN := os.Getenv("SENTRY_DSN") - if sentryDSN == "" { - s.sentryEnabled = false + s.sentryEnabled = false + + if viper.GetString("SENTRY_DSN") == "" { return } err := sentry.Init(sentry.ClientOptions{ - Dsn: sentryDSN, + Dsn: viper.GetString("SENTRY_DSN"), Release: fmt.Sprintf("%s-%s", s.appname, s.version), }) if err != nil { log.Fatal().Err(err).Msg("sentry init failure") + return } + s.log.Info().Msg("sentry error reporting activated") s.sentryEnabled = true } @@ -175,6 +179,9 @@ func (s *server) configure() { viper.SetDefault("MAINTENANCE_MODE", "false") viper.SetDefault("PORT", "8080") viper.SetDefault("DBURL", "") + viper.SetDefault("SENTRY_DSN", "") + viper.SetDefault("METRICS_USERNAME", "") + viper.SetDefault("METRICS_PASSWORD", "") if err := viper.ReadInConfig(); err != nil { if _, ok := err.(viper.ConfigFileNotFoundError); ok { diff --git a/httpserver/routes.go b/httpserver/routes.go index da3660a..f71e32e 100644 --- a/httpserver/routes.go +++ b/httpserver/routes.go @@ -4,10 +4,12 @@ import ( "net/http" "time" + basicauth "github.com/99designs/basicauth-go" sentryhttp "github.com/getsentry/sentry-go/http" "github.com/go-chi/chi" "github.com/go-chi/chi/middleware" "github.com/prometheus/client_golang/prometheus/promhttp" + "github.com/spf13/viper" ) func (s *server) routes() { @@ -20,32 +22,31 @@ func (s *server) routes() { // can .Use() more than one) will be applied to every request into // the service. + s.router.Use(middleware.Recoverer) s.router.Use(middleware.RequestID) s.router.Use(s.LoggingMiddleware()) - s.router.Use(s.MetricsMiddleware()) + + // add metrics middleware only if we can serve them behind auth + if viper.GetString("METRICS_USERNAME") != "" { + s.router.Use(s.MetricsMiddleware()) + } // CHANGEME to suit your needs, or pull from config. - // timeout for request context: your handlers must finish within + // timeout for request context; your handlers must finish within // this window: s.router.Use(middleware.Timeout(60 * time.Second)) // this adds a sentry reporting middleware if and only if sentry is - // enabled via setting of SENTRY_DSN in env. this was at the - // bottom, but chi requires *all* middlewares applied before any - // routes are, so now it's up here. unfortunately this cannot - // coexist with the normal chi Recoverer handler which prints a nice - // colorful stack trace to the console + // enabled via setting of SENTRY_DSN in env. if s.sentryEnabled { // Options docs at // https://docs.sentry.io/platforms/go/guides/http/ - sentryHandler := sentryhttp.New(sentryhttp.Options{}) + // we set sentry to repanic so that all panics bubble up to the + // Recoverer chi middleware above. + sentryHandler := sentryhttp.New(sentryhttp.Options{ + Repanic: true, + }) s.router.Use(sentryHandler.Handle) - // FYI: the sentry panic-catcher seems to set the response - // code to 200. - } else { - // FYI: the chi Recoverer middleware sets the response code - // on panics to 500. - s.router.Use(middleware.Recoverer) } //////////////////////////////////////////////////////////////////////// @@ -58,34 +59,40 @@ func (s *server) routes() { // if you want to use a general purpose middleware (http.Handler // wrapper) on a specific HandleFunc route, you need to take the // .ServeHTTP of the http.Handler to get its HandleFunc, viz: - authMiddleware := s.AuthMiddleware() s.router.Get( "/login", authMiddleware(s.handleLogin()).ServeHTTP, ) + // route that panics for testing + // CHANGEME remove this + s.router.Get( + "/panic", + s.handlePanic(), + ) + s.router.Get( "/.well-known/healthcheck.json", s.handleHealthCheck(), ) - // route that panics for testing - // CHANGEME remove this + // set up authenticated /metrics route: + if viper.GetString("METRICS_USERNAME") != "" { + metricsAuthMiddleware := basicauth.New( + "metrics", + map[string][]string{ + viper.GetString("METRICS_USERNAME"): { + viper.GetString("METRICS_PASSWORD"), + }, + }, + ) - s.router.Get( - "/panic", - s.handlePanic(), - ) - - // CHANGEME you probably want to wrap the following in some kind of - // auth like http basic auth which is easy to set up on your - // rometheus collector - // TODO(sneak): read http basic auth user/pass for /metrics - // out of environment vars - - s.router.Get( - "/metrics", - http.HandlerFunc(promhttp.Handler().ServeHTTP), - ) + s.router.Get( + "/metrics", + metricsAuthMiddleware( + http.HandlerFunc(promhttp.Handler().ServeHTTP), + ).ServeHTTP, + ) + } } -- 2.40.1 From a26c0b2b47ba1606b6a349d43b4cfbe961a857e0 Mon Sep 17 00:00:00 2001 From: sneak Date: Sat, 3 Oct 2020 23:03:00 -0700 Subject: [PATCH 2/2] cleanups: * move metrics endpoint protection middleware to correct file * move /metrics route to Chi route group * update readme --- README.md | 14 +++++++++++--- httpserver/middlewares.go | 15 +++++++++++++++ httpserver/routes.go | 21 +++++---------------- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index f17f281..f13ad5b 100644 --- a/README.md +++ b/README.md @@ -25,15 +25,18 @@ Alternately, even just feedback is great: * Stub Authentication middleware * Helper functions for encoding/decoding json * Healthcheck route -* No global state +* Prometheus metrics endpoint +* No global state of our own + * some deps have some, such as the metrics collector and Sentry # Design Decisions * TLS is terminated somewhere else, like on a sidecar or reverse proxy. * logging: [rs/zerolog](https://github.com/rs/zerolog) * configuration: [spf13/viper](https://github.com/spf13/viper) -* mux/router: [go-chi/chi](https://github.com/go-chi/chi) -* prometheus-style metrics via [slok/go-http-metrics](https://github.com/slok/go-http-metrics) + * used as a wrapper around env vars +* router is Chi: [go-chi/chi](https://github.com/go-chi/chi) +* Prometheus-style metrics via [slok/go-http-metrics](https://github.com/slok/go-http-metrics) * database: TBD (thinking about [go-gorm/gorm](https://github.com/go-gorm/gorm)) * templating: TBD (suggestions welcome) @@ -44,6 +47,11 @@ Alternately, even just feedback is great: * sync.Once example for precompiling templates * Bundling Static Assets Into Binary +# Known Bugs (more TODO) + +* Chi recovery middleware logs non-json when in non-tty stdout mode, + breaking validity of stdout as a json stream + # Author * [sneak@sneak.berlin](mailto:sneak@sneak.berlin) diff --git a/httpserver/middlewares.go b/httpserver/middlewares.go index e5995c2..70267a6 100644 --- a/httpserver/middlewares.go +++ b/httpserver/middlewares.go @@ -5,10 +5,13 @@ import ( "net/http" "time" + basicauth "github.com/99designs/basicauth-go" "github.com/go-chi/chi/middleware" metrics "github.com/slok/go-http-metrics/metrics/prometheus" ghmm "github.com/slok/go-http-metrics/middleware" + "github.com/slok/go-http-metrics/middleware/std" + "github.com/spf13/viper" ) // the following is from @@ -90,3 +93,15 @@ func (s *server) MetricsMiddleware() func(http.Handler) http.Handler { return std.Handler("", mdlw, next) } } + +func (s *server) MetricsAuthMiddleware() func(http.Handler) http.Handler { + return basicauth.New( + "metrics", + map[string][]string{ + viper.GetString("METRICS_USERNAME"): { + viper.GetString("METRICS_PASSWORD"), + }, + }, + ) + +} diff --git a/httpserver/routes.go b/httpserver/routes.go index f71e32e..7ab9ba6 100644 --- a/httpserver/routes.go +++ b/httpserver/routes.go @@ -4,7 +4,6 @@ import ( "net/http" "time" - basicauth "github.com/99designs/basicauth-go" sentryhttp "github.com/getsentry/sentry-go/http" "github.com/go-chi/chi" "github.com/go-chi/chi/middleware" @@ -79,20 +78,10 @@ func (s *server) routes() { // set up authenticated /metrics route: if viper.GetString("METRICS_USERNAME") != "" { - metricsAuthMiddleware := basicauth.New( - "metrics", - map[string][]string{ - viper.GetString("METRICS_USERNAME"): { - viper.GetString("METRICS_PASSWORD"), - }, - }, - ) - - s.router.Get( - "/metrics", - metricsAuthMiddleware( - http.HandlerFunc(promhttp.Handler().ServeHTTP), - ).ServeHTTP, - ) + s.router.Group(func(r chi.Router) { + r.Use(s.MetricsAuthMiddleware()) + r.Get("/metrics", http.HandlerFunc(promhttp.Handler().ServeHTTP)) + }) } + } -- 2.40.1