From 3e868392ba25fc699db63d4eba98d3d7998e2e6b Mon Sep 17 00:00:00 2001 From: Tom <75443136+lvlcn-t@users.noreply.github.com> Date: Tue, 12 Mar 2024 17:40:38 +0100 Subject: [PATCH] feat: load check config on startup (#115) * feat: healthz endpoint * feat: add healthz handler * feat: add health checker component * feat: check metrics endpoint health * feat: check health of all check endpoints * test: add checker tests * test: add healthz handler tests * fix: get runtime config once at sparrow startup * refactor: simplify loader init run * chore: rm formatting helper function * refactor: share http.Client instance * fix: format checker address * refactor: use internal check states instead of api route states * chore: add running state to traceroute check * fix: rm ok handler from unhealthy response * refactor: return http loader cfg by value * revert: healthz endpoint * refactor: test empty config scenario --- .pre-commit-config.yaml | 10 ++--- go.mod | 9 ++-- pkg/api/api.go | 6 +-- pkg/api/api_test.go | 4 +- pkg/checks/dns/dns.go | 2 +- pkg/checks/health/health.go | 2 +- pkg/checks/latency/latency.go | 2 +- pkg/checks/traceroute/traceroute.go | 6 +-- pkg/config/file.go | 18 ++++---- pkg/config/http.go | 64 +++++++++++++++-------------- pkg/config/http_test.go | 48 +++++++++------------- 11 files changed, 82 insertions(+), 89 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index b2d4b585..6f47ff57 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -4,9 +4,9 @@ repos: - id: go-generate-repo name: go generate entry: go - args: [ generate, ./... ] + args: [generate, ./...] language: system - types: [ go ] + types: [go] pass_filenames: false always_run: true - repo: https://github.com/tekwizely/pre-commit-golang @@ -14,12 +14,12 @@ repos: hooks: - id: go-mod-tidy-repo - id: go-test-repo-mod - args: [ -race, -count=1 ] + args: [-race, -count=1, -timeout 30s] - id: go-vet-repo-mod - id: go-fumpt-repo - args: [ -l, -w ] + args: [-l, -w] - id: golangci-lint-repo-mod - args: [ --config, .golangci.yaml, --, --fix ] + args: [--config, .golangci.yaml, --, --fix] - repo: https://github.com/norwoodj/helm-docs rev: v1.13.0 hooks: diff --git a/go.mod b/go.mod index 514224ed..e6875f3c 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,10 @@ module github.com/caas-team/sparrow go 1.22 require ( + github.com/aeden/traceroute v0.0.0-20210211061815-03f5f7cb7908 github.com/getkin/kin-openapi v0.120.0 github.com/go-chi/chi/v5 v5.0.10 + github.com/google/go-cmp v0.6.0 github.com/jarcoal/httpmock v1.3.1 github.com/spf13/cobra v1.8.0 github.com/spf13/viper v1.18.0 @@ -12,12 +14,6 @@ require ( gopkg.in/yaml.v3 v3.0.1 ) -require github.com/google/go-cmp v0.6.0 - -require github.com/aeden/traceroute v0.0.0-20210211061815-03f5f7cb7908 - -require github.com/mitchellh/mapstructure v1.5.0 // indirect - require ( github.com/beorn7/perks v1.0.1 // indirect github.com/cespare/xxhash/v2 v2.2.0 // indirect @@ -34,6 +30,7 @@ require ( github.com/magiconair/properties v1.8.7 // indirect github.com/mailru/easyjson v0.7.7 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.4 // indirect + github.com/mitchellh/mapstructure v1.5.0 // indirect github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 // indirect github.com/pelletier/go-toml/v2 v2.1.0 // indirect github.com/perimeterx/marshmallow v1.1.5 // indirect diff --git a/pkg/api/api.go b/pkg/api/api.go index 5cf1c0d3..6cd94e53 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -131,7 +131,7 @@ func (a *api) RegisterRoutes(ctx context.Context, routes ...Route) error { // Handles requests with simple http ok // Required for global tarMan in checks - a.router.Handle("/", okHandler(ctx)) + a.router.Handle("/", OkHandler(ctx)) return nil } @@ -148,8 +148,8 @@ func (a *api) registerDefaultRoute(route Route) (err error) { return nil } -// okHandler returns a handler that will serve status ok -func okHandler(ctx context.Context) http.Handler { +// OkHandler returns a handler that will serve status ok +func OkHandler(ctx context.Context) http.Handler { log := logger.FromContext(ctx) return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 7d555440..b62fe223 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -212,7 +212,7 @@ func TestAPI_ShutdownWhenContextCanceled(t *testing.T) { } } -func Test_okHandler(t *testing.T) { +func TestAPI_OkHandler(t *testing.T) { ctx := context.Background() req, err := http.NewRequestWithContext(ctx, "GET", "/okHandler", http.NoBody) @@ -221,7 +221,7 @@ func Test_okHandler(t *testing.T) { } rr := httptest.NewRecorder() - handler := okHandler(ctx) + handler := OkHandler(ctx) handler.ServeHTTP(rr, req) diff --git a/pkg/checks/dns/dns.go b/pkg/checks/dns/dns.go index 4cb07cce..c0b1d4db 100644 --- a/pkg/checks/dns/dns.go +++ b/pkg/checks/dns/dns.go @@ -83,8 +83,8 @@ func (d *DNS) Run(ctx context.Context, cResult chan checks.ResultDTO) error { ctx, cancel := logger.NewContextWithLogger(ctx) defer cancel() log := logger.FromContext(ctx) - log.Info("Starting dns check", "interval", d.config.Interval.String()) + log.Info("Starting dns check", "interval", d.config.Interval.String()) for { select { case <-ctx.Done(): diff --git a/pkg/checks/health/health.go b/pkg/checks/health/health.go index 04142857..7e0d8c98 100644 --- a/pkg/checks/health/health.go +++ b/pkg/checks/health/health.go @@ -75,8 +75,8 @@ func (h *Health) Run(ctx context.Context, cResult chan checks.ResultDTO) error { ctx, cancel := logger.NewContextWithLogger(ctx) defer cancel() log := logger.FromContext(ctx) - log.Info("Starting healthcheck", "interval", h.config.Interval.String()) + log.Info("Starting healthcheck", "interval", h.config.Interval.String()) for { select { case <-ctx.Done(): diff --git a/pkg/checks/latency/latency.go b/pkg/checks/latency/latency.go index 27e3c93f..af04096e 100644 --- a/pkg/checks/latency/latency.go +++ b/pkg/checks/latency/latency.go @@ -81,8 +81,8 @@ func (l *Latency) Run(ctx context.Context, cResult chan checks.ResultDTO) error ctx, cancel := logger.NewContextWithLogger(ctx) defer cancel() log := logger.FromContext(ctx) - log.Info("Starting latency check", "interval", l.config.Interval.String()) + log.Info("Starting latency check", "interval", l.config.Interval.String()) for { select { case <-ctx.Done(): diff --git a/pkg/checks/traceroute/traceroute.go b/pkg/checks/traceroute/traceroute.go index 5c45f830..3cc64269 100644 --- a/pkg/checks/traceroute/traceroute.go +++ b/pkg/checks/traceroute/traceroute.go @@ -25,12 +25,12 @@ type Target struct { func NewCheck() checks.Check { return &Traceroute{ - config: Config{}, - traceroute: newTraceroute, CheckBase: checks.CheckBase{ Mu: sync.Mutex{}, DoneChan: make(chan struct{}), }, + config: Config{}, + traceroute: newTraceroute, } } @@ -69,8 +69,8 @@ func (tr *Traceroute) Run(ctx context.Context, cResult chan checks.ResultDTO) er ctx, cancel := logger.NewContextWithLogger(ctx) defer cancel() log := logger.FromContext(ctx) - log.Info("Starting traceroute check", "interval", tr.config.Interval.String()) + log.Info("Starting traceroute check", "interval", tr.config.Interval.String()) for { select { case <-ctx.Done(): diff --git a/pkg/config/file.go b/pkg/config/file.go index 35c66929..7093c844 100644 --- a/pkg/config/file.go +++ b/pkg/config/file.go @@ -53,21 +53,23 @@ func NewFileLoader(cfg *Config, cRuntime chan<- runtime.Config) *FileLoader { // Run gets the runtime configuration from the local file. // The config will be loaded periodically defined by the loader interval configuration. +// If the interval is 0, the configuration is only fetched once and the loader is disabled. func (f *FileLoader) Run(ctx context.Context) error { ctx, cancel := logger.NewContextWithLogger(ctx) defer cancel() log := logger.FromContext(ctx) - if f.config.Interval == 0 { - cfg, err := f.getRuntimeConfig(ctx) - if err != nil { - log.Warn("Could not get local runtime configuration", "error", err) - return fmt.Errorf("could not get local runtime configuration: %w", err) - } + // Get the runtime configuration once on startup + cfg, err := f.getRuntimeConfig(ctx) + if err != nil { + log.Warn("Could not get local runtime configuration", "error", err) + err = fmt.Errorf("could not get local runtime configuration: %w", err) + } + f.cRuntime <- cfg - f.cRuntime <- cfg + if f.config.Interval == 0 { log.Info("File Loader disabled") - return nil + return err } tick := time.NewTicker(f.config.Interval) diff --git a/pkg/config/http.go b/pkg/config/http.go index fdfde80f..aeaffbd8 100644 --- a/pkg/config/http.go +++ b/pkg/config/http.go @@ -20,6 +20,7 @@ package config import ( "context" + "errors" "fmt" "io" "net/http" @@ -51,36 +52,37 @@ func NewHttpLoader(cfg *Config, cRuntime chan<- runtime.Config) *HttpLoader { // Run gets the runtime configuration from the remote file of the configured http endpoint. // The config will be loaded periodically defined by the loader interval configuration. -// Returns an error if the loader is shutdown or the context is done. -func (hl *HttpLoader) Run(ctx context.Context) error { +// If the interval is 0, the configuration is only fetched once and the loader is disabled. +func (h *HttpLoader) Run(ctx context.Context) error { ctx, cancel := logger.NewContextWithLogger(ctx) defer cancel() log := logger.FromContext(ctx) - var runtimeCfg *runtime.Config + var cfg runtime.Config getConfigRetry := helper.Retry(func(ctx context.Context) (err error) { - runtimeCfg, err = hl.getRuntimeConfig(ctx) + cfg, err = h.getRuntimeConfig(ctx) return err - }, hl.cfg.Http.RetryCfg) + }, h.cfg.Http.RetryCfg) - if hl.cfg.Interval == 0 { - err := getConfigRetry(ctx) - if err != nil { - log.Warn("Could not get remote runtime configuration", "error", err) - return fmt.Errorf("could not get remote runtime configuration: %w", err) - } + // Get the runtime configuration once on startup + err := getConfigRetry(ctx) + if err != nil { + log.Warn("Could not get remote runtime configuration", "error", err) + err = fmt.Errorf("could not get remote runtime configuration: %w", err) + } + h.cRuntime <- cfg - hl.cRuntime <- *runtimeCfg + if h.cfg.Interval == 0 { log.Info("HTTP Loader disabled") - return nil + return err } - tick := time.NewTicker(hl.cfg.Interval) + tick := time.NewTicker(h.cfg.Interval) defer tick.Stop() for { select { - case <-hl.done: + case <-h.done: log.Info("HTTP Loader terminated") return nil case <-ctx.Done(): @@ -88,24 +90,24 @@ func (hl *HttpLoader) Run(ctx context.Context) error { case <-tick.C: if err := getConfigRetry(ctx); err != nil { log.Warn("Could not get remote runtime configuration", "error", err) - tick.Reset(hl.cfg.Interval) + tick.Reset(h.cfg.Interval) continue } log.Info("Successfully got remote runtime configuration") - hl.cRuntime <- *runtimeCfg - tick.Reset(hl.cfg.Interval) + h.cRuntime <- cfg + tick.Reset(h.cfg.Interval) } } } // GetRuntimeConfig gets the remote runtime configuration -func (hl *HttpLoader) getRuntimeConfig(ctx context.Context) (*runtime.Config, error) { +func (hl *HttpLoader) getRuntimeConfig(ctx context.Context) (cfg runtime.Config, err error) { log := logger.FromContext(ctx).With("url", hl.cfg.Http.Url) req, err := http.NewRequestWithContext(ctx, http.MethodGet, hl.cfg.Http.Url, http.NoBody) if err != nil { log.Error("Could not create http GET request", "error", err.Error()) - return nil, err + return cfg, err } if hl.cfg.Http.Token != "" { req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", hl.cfg.Http.Token)) @@ -114,34 +116,34 @@ func (hl *HttpLoader) getRuntimeConfig(ctx context.Context) (*runtime.Config, er res, err := hl.client.Do(req) //nolint:bodyclose if err != nil { log.Error("Http get request failed", "error", err.Error()) - return nil, err + return cfg, err } defer func(Body io.ReadCloser) { - err = Body.Close() - if err != nil { - log.Error("Failed to close response body", "error", err.Error()) + cErr := Body.Close() + if cErr != nil { + log.Error("Failed to close response body", "error", cErr) + err = errors.Join(cErr, err) } }(res.Body) if res.StatusCode != http.StatusOK { log.Error("Http get request failed", "status", res.Status) - return nil, fmt.Errorf("request failed, status is %s", res.Status) + return cfg, fmt.Errorf("request failed, status is %s", res.Status) } - body, err := io.ReadAll(res.Body) + b, err := io.ReadAll(res.Body) if err != nil { log.Error("Could not read response body", "error", err.Error()) - return nil, err + return cfg, err } log.Debug("Successfully got response") - runtimeCfg := &runtime.Config{} - if err := yaml.Unmarshal(body, &runtimeCfg); err != nil { + if err := yaml.Unmarshal(b, &cfg); err != nil { log.Error("Could not unmarshal response", "error", err.Error()) - return nil, err + return cfg, err } - return runtimeCfg, nil + return cfg, nil } // Shutdown stops the loader diff --git a/pkg/config/http_test.go b/pkg/config/http_test.go index 1502783f..4ab62ce7 100644 --- a/pkg/config/http_test.go +++ b/pkg/config/http_test.go @@ -50,7 +50,7 @@ func TestHttpLoader_GetRuntimeConfig(t *testing.T) { name string cfg *Config httpResponder httpResponder - want *runtime.Config + want runtime.Config wantErr bool }{ { @@ -65,7 +65,7 @@ func TestHttpLoader_GetRuntimeConfig(t *testing.T) { statusCode: 200, response: httpmock.File("test/data/config.yaml").String(), }, - want: &runtime.Config{ + want: runtime.Config{ Health: &health.Config{ Targets: []string{"http://localhost:8080/health"}, Interval: 1 * time.Second, @@ -88,7 +88,7 @@ func TestHttpLoader_GetRuntimeConfig(t *testing.T) { statusCode: 200, response: httpmock.File("test/data/config.yaml").String(), }, - want: &runtime.Config{ + want: runtime.Config{ Health: &health.Config{ Targets: []string{"http://localhost:8080/health"}, Interval: 1 * time.Second, @@ -161,27 +161,27 @@ func TestHttpLoader_Run(t *testing.T) { tests := []struct { name string interval time.Duration - response *runtime.Config + response runtime.Config code int wantErr bool }{ { name: "non-200 response", interval: 500 * time.Millisecond, - response: nil, + response: runtime.Config{}, code: http.StatusInternalServerError, wantErr: false, }, { name: "empty checks' configuration", interval: 500 * time.Millisecond, - response: &runtime.Config{}, + response: runtime.Config{}, code: http.StatusOK, }, { name: "config with health check", interval: 500 * time.Millisecond, - response: &runtime.Config{ + response: runtime.Config{ Health: &health.Config{ Targets: []string{"http://localhost:8080/health"}, Interval: 1 * time.Second, @@ -192,7 +192,7 @@ func TestHttpLoader_Run(t *testing.T) { { name: "continuous loading disabled", interval: 0, - response: &runtime.Config{ + response: runtime.Config{ Health: &health.Config{ Targets: []string{"http://localhost:8080/health"}, Interval: 1 * time.Second, @@ -224,7 +224,7 @@ func TestHttpLoader_Run(t *testing.T) { }, }, }, - cRuntime: make(chan<- runtime.Config, 1), + cRuntime: make(chan<- runtime.Config, 2), client: &http.Client{ Transport: http.DefaultTransport, }, @@ -341,14 +341,14 @@ func TestHttpLoader_Run_config_sent_to_channel(t *testing.T) { hl.Shutdown(ctx) } -// TestHttpLoader_Run_config_not_sent_to_channel_500 tests if the config is not sent to the channel +// TestHttpLoader_Run_empty_config_sent_to_channel_500 tests if the config is not sent to the channel // when the Run method is called // and the remote endpoint returns a non-200 response -func TestHttpLoader_Run_config_not_sent_to_channel_500(t *testing.T) { +func TestHttpLoader_Run_empty_config_sent_to_channel_500(t *testing.T) { httpmock.Activate() defer httpmock.DeactivateAndReset() - resp, err := httpmock.NewJsonResponder(500, nil) + resp, err := httpmock.NewJsonResponder(http.StatusInternalServerError, nil) if err != nil { t.Fatalf("Failed creating json responder: %v", err) } @@ -384,22 +384,18 @@ func TestHttpLoader_Run_config_not_sent_to_channel_500(t *testing.T) { } }() - // check if the config is sent to the channel - select { - // make sure you wait for at least an interval - case <-time.After(time.Second): - t.Log("Config not sent to channel") - case c := <-cRuntime: - t.Errorf("Config sent to channel: %v", c) + cfg := <-cRuntime + if cfg != (runtime.Config{}) { + t.Errorf("Config sent to channel: %v", cfg) } hl.Shutdown(ctx) } -// TestHttpLoader_Run_config_not_sent_to_channel_client_error tests if the config is not sent to the channel +// TestHttpLoader_Run_empty_config_sent_to_channel_client_error tests if the config is not sent to the channel // when the Run method is called // and the client can't execute the requests -func TestHttpLoader_Run_config_not_sent_to_channel_client_error(t *testing.T) { +func TestHttpLoader_Run_empty_config_sent_to_channel_client_error(t *testing.T) { httpmock.Activate() defer httpmock.DeactivateAndReset() @@ -435,13 +431,9 @@ func TestHttpLoader_Run_config_not_sent_to_channel_client_error(t *testing.T) { } }() - // check if the config is sent to the channel - select { - // make sure you wait for at least an interval - case <-time.After(time.Second): - t.Log("Config not sent to channel") - case c := <-cRuntime: - t.Errorf("Config sent to channel: %v", c) + cfg := <-cRuntime + if cfg != (runtime.Config{}) { + t.Errorf("Config sent to channel: %v", cfg) } hl.Shutdown(ctx)