From f810ec31d833c422668c3b3212de9b6de713f383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 09:05:43 -0700 Subject: [PATCH 1/8] Don't enable debug mode by default This should only be enabled when needed, it's mentioned in the docs --- Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/Makefile b/Makefile index 9e9343af3..3971f948e 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,6 @@ clean: .PHONY: run run: $(NAME) - DEBUG=true \ ALERTMANAGER_URI=$(ALERTMANAGER_URI) \ COLOR_LABELS_UNIQUE="instance cluster" \ COLOR_LABELS_STATIC="job" \ From fd7d3a35858c69f58bb0436e74da3a69ed5597d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 09:08:43 -0700 Subject: [PATCH 2/8] Move runtime config logging into separate function This should only happen when needed, so moving this to a dedicated function that is called on startup --- config/config.go | 2 ++ main.go | 1 + 2 files changed, 3 insertions(+) diff --git a/config/config.go b/config/config.go index 54e90a9f9..1abff06d4 100644 --- a/config/config.go +++ b/config/config.go @@ -119,7 +119,9 @@ func (config *configEnvs) Read() { if err != nil { log.Fatal(err) } +} +func (config *configEnvs) LogValues() { s := reflect.ValueOf(config).Elem() typeOfT := s.Type() for i := 0; i < s.NumField(); i++ { diff --git a/main.go b/main.go index d032639f6..35074b280 100644 --- a/main.go +++ b/main.go @@ -85,6 +85,7 @@ func main() { log.Infof("Version: %s", version) config.Config.Read() + config.Config.LogValues() transform.ParseRules(config.Config.JiraRegexp) apiCache = cache.New(cache.NoExpiration, 10*time.Second) From cbe15f71af9546a12bb11801c24fd7635beade12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 09:10:09 -0700 Subject: [PATCH 3/8] Add tests for config handling --- config/config_test.go | 63 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 config/config_test.go diff --git a/config/config_test.go b/config/config_test.go new file mode 100644 index 000000000..84c25bb82 --- /dev/null +++ b/config/config_test.go @@ -0,0 +1,63 @@ +package config + +import ( + "os" + "testing" + "time" +) + +type flagNameTest struct { + env string + flag string +} + +var flagNameTests = []flagNameTest{ + flagNameTest{env: "MyEnv", flag: "my.env"}, + flagNameTest{env: "MyENV", flag: "my.env"}, + flagNameTest{env: "MYEnv", flag: "myenv"}, +} + +func TestMakeFlagName(t *testing.T) { + for _, testCase := range flagNameTests { + generatedFlag := makeFlagName(testCase.env) + if generatedFlag != testCase.flag { + t.Errorf("Invalid flag name generated from env '%s', expected '%s', got '%s'", testCase.env, testCase.flag, generatedFlag) + } + } +} + +func stringInSlice(stringArray []string, value string) bool { + for _, s := range stringArray { + if s == value { + return true + } + } + return false +} + +func TestReadConfig(t *testing.T) { + os.Setenv("ALERTMANAGER_TTL", "1s") + os.Setenv("ALERTMANAGER_URI", "http://localhost") + os.Setenv("DEBUG", "true") + os.Setenv("COLOR_LABELS_STATIC", "a bb ccc") + Config.Read() + if Config.AlertmanagerTTL != time.Second { + t.Errorf("Config.AlertmanagerTTL is invalid, expected 1s, got %v", Config.AlertmanagerTTL) + } + if Config.Debug != true { + t.Errorf("Config.Debug is %v with env DEBUG=true set", Config.Debug) + } + if !stringInSlice(Config.ColorLabelsStatic, "a") { + t.Errorf("Config.ColorLabelsStatic is missing value 'a': %v", Config.ColorLabelsStatic) + } + if !stringInSlice(Config.ColorLabelsStatic, "bb") { + t.Errorf("Config.ColorLabelsStatic is missing value 'bb': %v", Config.ColorLabelsStatic) + } + if !stringInSlice(Config.ColorLabelsStatic, "ccc") { + t.Errorf("Config.ColorLabelsStatic is missing value 'ccc': %v", Config.ColorLabelsStatic) + } + if Config.Port != 8080 { + t.Errorf("Config.Port is invalid, expected 8080, got %v", Config.Port) + } + +} From 1ba5228cb17a867adb28a3765b72cdd085b84514 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 09:12:50 -0700 Subject: [PATCH 4/8] Move view setup after all middleware prometheus middleware needs to be initialized first, before any view is created --- main.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/main.go b/main.go index 35074b280..724429605 100644 --- a/main.go +++ b/main.go @@ -70,8 +70,6 @@ func init() { } func setupRouter(router *gin.Engine) { - router.SetHTMLTemplate(loadTemplates("templates")) - router.Use(static.Serve("/static", newBinaryFileSystem("static"))) router.GET("/favicon.ico", favicon) @@ -107,7 +105,7 @@ func main() { } router := gin.New() - setupRouter(router) + router.SetHTMLTemplate(loadTemplates("templates")) prom := ginprometheus.NewPrometheus("gin") prom.Use(router) @@ -121,5 +119,6 @@ func main() { router.Use(sentry.Recovery(raven.DefaultClient, false)) } + setupRouter(router) router.Run() } From d99f7e60d88d0b76be0384f92fdbeaf6ebbcf80b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 09:14:58 -0700 Subject: [PATCH 5/8] Don't create http.FileServer on every favicon request Setup it once and use for all requests --- views.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/views.go b/views.go index bd9d74cc2..3f08a47c4 100644 --- a/views.go +++ b/views.go @@ -4,9 +4,6 @@ import ( "crypto/sha1" "encoding/json" "fmt" - "github.com/cloudflare/unsee/config" - "github.com/cloudflare/unsee/models" - "github.com/cloudflare/unsee/store" "io" "net/http" "sort" @@ -14,10 +11,19 @@ import ( "strings" "time" + "github.com/cloudflare/unsee/config" + "github.com/cloudflare/unsee/models" + "github.com/cloudflare/unsee/store" + log "github.com/Sirupsen/logrus" "github.com/gin-gonic/gin" ) +var ( + // needed for serving favicon from binary assets + faviconFileServer = http.FileServer(newBinaryFileSystem("static")) +) + func boolInSlice(boolArray []bool, value bool) bool { for _, s := range boolArray { if s == value { @@ -262,7 +268,5 @@ func autocomplete(c *gin.Context) { } func favicon(c *gin.Context) { - fs := newBinaryFileSystem("static") - fileserver := http.FileServer(fs) - fileserver.ServeHTTP(c.Writer, c.Request) + faviconFileServer.ServeHTTP(c.Writer, c.Request) } From 54d67cbbaaf9814c32f345d8e484cd3f78291d17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 09:17:02 -0700 Subject: [PATCH 6/8] Add more tests for views We moved static file handling to binary assets, add some tests so we can spot issues with this code --- views_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/views_test.go b/views_test.go index 2983f8312..5023176f8 100644 --- a/views_test.go +++ b/views_test.go @@ -29,6 +29,7 @@ func mockConfig() { func ginTestEngine() *gin.Engine { gin.SetMode(gin.TestMode) r := gin.New() + r.SetHTMLTemplate(loadTemplates("templates")) setupRouter(r) return r } @@ -256,6 +257,14 @@ var acTests = []acTestCase{ "node=localhost", }, }, + // duplicated to test reponse caching + acTestCase{ + Term: "Nod", + Results: []string{ + "node!=localhost", + "node=localhost", + }, + }, } func TestAutocomplete(t *testing.T) { @@ -294,3 +303,45 @@ func TestAutocomplete(t *testing.T) { } } } + +type staticFileTestCase struct { + path string + code int +} + +var staticFileTests = []staticFileTestCase{ + staticFileTestCase{ + path: "/favicon.ico", + code: 200, + }, + staticFileTestCase{ + path: "/static/unsee.js", + code: 200, + }, + staticFileTestCase{ + path: "/static/managed/js/assets.txt", + code: 200, + }, + staticFileTestCase{ + path: "/xxx", + code: 404, + }, + staticFileTestCase{ + path: "/static/abcd", + code: 404, + }, +} + +func TestStaticFiles(t *testing.T) { + mockConfig() + mockAlerts() + r := ginTestEngine() + for _, staticFileTest := range staticFileTests { + req, _ := http.NewRequest("GET", staticFileTest.path, nil) + resp := httptest.NewRecorder() + r.ServeHTTP(resp, req) + if resp.Code != staticFileTest.code { + t.Errorf("Invalid status code for GET %s: %d", staticFileTest.path, resp.Code) + } + } +} From 66710c6020c120ebdc73289107e19ac2b827124f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 11:00:16 -0700 Subject: [PATCH 7/8] Sanitize URLs to hide all passwords when logging runtime config When unsee starts it will print runtime config, but this can contain password for URL keys, replace passwords with 'xxx' string --- config/config.go | 27 +++++++++++++++++++++++++-- config/config_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index 1abff06d4..4d5ecdda6 100644 --- a/config/config.go +++ b/config/config.go @@ -4,6 +4,7 @@ import ( "bytes" "flag" "fmt" + "net/url" "os" "reflect" "strings" @@ -11,6 +12,7 @@ import ( "unicode" log "github.com/Sirupsen/logrus" + "github.com/asaskevich/govalidator" "github.com/kelseyhightower/envconfig" ) @@ -121,12 +123,33 @@ func (config *configEnvs) Read() { } } +func hideURLPassword(s string) (string, error) { + u, err := url.Parse(s) + if err != nil { + return "", err + } + if u.User != nil { + if _, pwdSet := u.User.Password(); pwdSet { + u.User = url.UserPassword(u.User.Username(), "xxx") + } + } + return u.String(), nil +} + func (config *configEnvs) LogValues() { s := reflect.ValueOf(config).Elem() typeOfT := s.Type() for i := 0; i < s.NumField(); i++ { - f := s.Field(i) - log.Infof("%20s => %v", typeOfT.Field(i).Tag.Get("envconfig"), f.Interface()) + env := typeOfT.Field(i).Tag.Get("envconfig") + val := fmt.Sprintf("%v", s.Field(i).Interface()) + if govalidator.IsURL(val) { + var err error + val, err = hideURLPassword(val) + if err != nil { + log.Errorf("Failed to parse url value for %s: %s", env, err.Error()) + } + } + log.Infof("%20s => %v", env, val) } } diff --git a/config/config_test.go b/config/config_test.go index 84c25bb82..bf11fb480 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -61,3 +61,39 @@ func TestReadConfig(t *testing.T) { } } + +type urlSecretTest struct { + raw string + sanitized string +} + +var urlSecretTests = []urlSecretTest{ + urlSecretTest{ + raw: "http://localhost", + sanitized: "http://localhost", + }, + urlSecretTest{ + raw: "http://alertmanager.example.com/path", + sanitized: "http://alertmanager.example.com/path", + }, + urlSecretTest{ + raw: "http://user@alertmanager.example.com/path", + sanitized: "http://user@alertmanager.example.com/path", + }, + urlSecretTest{ + raw: "https://user:password@alertmanager.example.com/path", + sanitized: "https://user:xxx@alertmanager.example.com/path", + }, +} + +func TestUrlSecretTest(t *testing.T) { + for _, testCase := range urlSecretTests { + sanitized, err := hideURLPassword(testCase.raw) + if err != nil { + t.Errorf("Unexpected error when parsing '%s': %s", testCase.raw, err.Error()) + } + if sanitized != testCase.sanitized { + t.Errorf("Invalid sanitized url, expected '%s', got '%s'", testCase.sanitized, sanitized) + } + } +} From e0279ebe5e4b8adbb6d5b0043364e76ff58ab70d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 28 Mar 2017 11:25:27 -0700 Subject: [PATCH 8/8] Add more details about make variable 'DEBUG' Make 'make DEBUG=true ...' work with run-docker target and provide more details of what it will do --- CONTRIBUTING.md | 12 +++++++++++- Makefile | 6 ++++++ README.md | 4 +++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d318b128d..1bae08057 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,9 +35,19 @@ To build and start `unsee` from local branch see `Running` section of the [README](README.md) file. When working with assets (templates, stylesheets and javascript files) `DEBUG` -flag for make can be set, which will recompile binary assets in debug mode, +make variable can be set, which will recompile binary assets in debug mode, meaning that files from disk will be read instead of compiled in assets. See [go-bindata docs](https://github.com/jteeuwen/go-bindata#debug-vs-release-builds) for details. Example: make DEBUG=true run + make DEBUG=true run-docker + +Note that this is not the same as enabling [debug mode](/README.md#debug) for +the [gin web framework](https://github.com/gin-gonic/gin) which is used +internally, but enabling `DEBUG` via this make variable will also enable gin +debug mode. +When running docker image via `make run-docker` with `DEBUG` make variable set +to `true` volume mapping will be added (in read-only mode), so that unsee +instance running inside the docker can read asset files from the sources +directory. diff --git a/Makefile b/Makefile index 3971f948e..1748d4788 100644 --- a/Makefile +++ b/Makefile @@ -10,9 +10,12 @@ SOURCES := $(wildcard *.go) $(wildcard */*.go) ASSET_SOURCES := $(wildcard assets/*/* assets/*/*/*) GO_BINDATA_MODE := prod +GIN_DEBUG := false ifdef DEBUG GO_BINDATA_FLAGS = -debug GO_BINDATA_MODE = debug + GIN_DEBUG = true + DOCKER_ARGS = -v $(CURDIR)/assets:$(CURDIR)/assets:ro endif .DEFAULT_GOAL := $(NAME) @@ -45,6 +48,7 @@ run: $(NAME) ALERTMANAGER_URI=$(ALERTMANAGER_URI) \ COLOR_LABELS_UNIQUE="instance cluster" \ COLOR_LABELS_STATIC="job" \ + DEBUG="$(GIN_DEBUG)" \ PORT=$(PORT) \ ./$(NAME) @@ -57,9 +61,11 @@ run-docker: docker-image @docker rm -f $(NAME) || true docker run \ --name $(NAME) \ + $(DOCKER_ARGS) \ -e ALERTMANAGER_URI=$(ALERTMANAGER_URI) \ -e COLOR_LABELS_UNIQUE="instance cluster" \ -e COLOR_LABELS_STATIC="job" \ + -e DEBUG="$(GIN_DEBUG)" \ -e PORT=$(PORT) \ -p $(PORT):$(PORT) \ $(NAME):$(VERSION) diff --git a/README.md b/README.md index 938952831..1f0a86264 100644 --- a/README.md +++ b/README.md @@ -115,7 +115,9 @@ This variable is required and there is no default value. #### DEBUG -Will enable [gin](https://github.com/gin-gonic/gin) debug mode. Examples: +Will enable [gin](https://github.com/gin-gonic/gin) debug mode. This will +configure to print out more debugging information on startup. +Examples: DEBUG=true DEBUG=false