From c4cf556982e454e3d87d636f35341da1412957d5 Mon Sep 17 00:00:00 2001 From: Seila Gamo Date: Tue, 7 Nov 2023 13:31:32 +0100 Subject: [PATCH] all: fix CR issues --- cmd/lava/internal/run/run.go | 22 +- internal/config/config.go | 6 +- internal/metrics/metrics.go | 70 +++++++ internal/metrics/metrics_collector.go | 98 --------- internal/metrics/metrics_collector_test.go | 230 --------------------- internal/metrics/metrics_test.go | 125 +++++++++++ internal/report/report.go | 78 ++++--- internal/report/report_test.go | 6 +- 8 files changed, 243 insertions(+), 392 deletions(-) create mode 100644 internal/metrics/metrics.go delete mode 100644 internal/metrics/metrics_collector.go delete mode 100644 internal/metrics/metrics_collector_test.go create mode 100644 internal/metrics/metrics_test.go diff --git a/cmd/lava/internal/run/run.go b/cmd/lava/internal/run/run.go index 04c2e3d..453c10c 100644 --- a/cmd/lava/internal/run/run.go +++ b/cmd/lava/internal/run/run.go @@ -6,8 +6,6 @@ package run import ( "errors" "fmt" - "log/slog" - "net/url" "os" "path/filepath" "time" @@ -42,9 +40,8 @@ output is disabled in the following cases: } var ( - cfgfile = CmdRun.Flag.String("c", "lava.yaml", "config file") - forceColor = CmdRun.Flag.Bool("forcecolor", false, "force colorized output") - metricsFile = CmdRun.Flag.String("m", "", "metrics output file") + cfgfile = CmdRun.Flag.String("c", "lava.yaml", "config file") + forceColor = CmdRun.Flag.Bool("forcecolor", false, "force colorized output") ) func init() { @@ -74,14 +71,6 @@ func run(args []string) error { } metrics.Collect("lava_version", cfg.LavaVersion) metrics.Collect("targets", cfg.Targets) - if os.Getenv("GITHUB_SERVER_URL") != "" && os.Getenv("GITHUB_REPOSITORY") != "" { - ghRepo, err := url.JoinPath(os.Getenv("GITHUB_SERVER_URL"), os.Getenv("GITHUB_REPOSITORY")) - if err != nil { - slog.Warn("error collecting the GitHub repo URI") - } else { - metrics.Collect("repository_uri", ghRepo) - } - } base.LogLevel.Set(cfg.LogLevel) er, err := engine.Run(cfg.ChecktypesURLs, cfg.Targets, cfg.AgentConfig) @@ -101,12 +90,11 @@ func run(args []string) error { } metrics.Collect("exit_code", exitCode) - finishTime := time.Now() - duration := finishTime.Sub(executionTime) + duration := time.Now().Sub(executionTime) metrics.Collect("duration", duration.String()) - if *metricsFile != "" { - if err = metrics.Write(*metricsFile); err != nil { + if cfg.ReportConfig.Metrics != "" { + if err = metrics.WriteFile(cfg.ReportConfig.Metrics); err != nil { return fmt.Errorf("write metrics: %w", err) } } diff --git a/internal/config/config.go b/internal/config/config.go index f00a859..a23e6ee 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -149,8 +149,10 @@ type ReportConfig struct { // instance, accepted risks, false positives, etc. Exclusions []Exclusion `yaml:"exclusions"` - // Metrics decides is a metrics report is printed. - Metrics bool `yaml:"metrics"` + // Metrics is the file where the metrics will be writen. + // If Metrics is an empty string (or not specified in the yaml file) then, + // the metrics report is not saved. + Metrics string `yaml:"metrics"` } // Target represents the target of a scan. diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go new file mode 100644 index 0000000..bd00eac --- /dev/null +++ b/internal/metrics/metrics.go @@ -0,0 +1,70 @@ +// Copyright 2023 Adevinta + +// Package metrics collects Lava execution metrics. +package metrics + +import ( + "encoding/json" + "fmt" + "io" + "os" + "sync" +) + +// DefaultCollector is the default [Collector]. +var DefaultCollector = NewCollector() + +// Collector represents a metrics collector. +type Collector struct { + mutex sync.Mutex + metrics map[string]any +} + +// NewCollector returns a new metrics collector. +func NewCollector() *Collector { + return &Collector{ + metrics: make(map[string]any), + } +} + +// Collect records a metric with the provided name and value. +func (c *Collector) Collect(name string, value any) { + c.mutex.Lock() + defer c.mutex.Unlock() + + c.metrics[name] = value +} + +// Write writes the metrics to the specified [io.Writer]. +func (c *Collector) Write(w io.Writer) error { + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + if err := enc.Encode(c.metrics); err != nil { + return fmt.Errorf("encode JSON: %w", err) + } + return nil +} + +// Collect records a metric with the provided name and value using +// [DefaultCollector]. +func Collect(name string, value any) { + DefaultCollector.Collect(name, value) +} + +// Write writes the collected metrics to the specified [io.Writer] +// using [DefaultCollector]. +func Write(w io.Writer) error { + return DefaultCollector.Write(w) +} + +// WriteFile writes the collected metrics into the specified file +// using [DefaultCollector]. +func WriteFile(file string) error { + f, err := os.Create(file) + if err != nil { + return fmt.Errorf("create file: %w", err) + } + defer f.Close() + + return Write(f) +} diff --git a/internal/metrics/metrics_collector.go b/internal/metrics/metrics_collector.go deleted file mode 100644 index fe9198d..0000000 --- a/internal/metrics/metrics_collector.go +++ /dev/null @@ -1,98 +0,0 @@ -// Copyright 2023 Adevinta - -// Package metrics collects all kind of Lava metrics. -package metrics - -import ( - "encoding/json" - "fmt" - "io" - "os" - "sync" -) - -// DefaultCollector is the default [Collector] and is used by [Collect] and [Write]. -var DefaultCollector *Collector - -// init initializes the DefaultCollector. -func init() { - realInitialization() -} - -func realInitialization() { - DefaultCollector = &Collector{ - metrics: make(map[string]interface{}), - emitter: NewJSONEmitter(), - } -} - -// Collector represents a metrics collector. -type Collector struct { - mutex sync.Mutex - metrics map[string]interface{} - emitter emitter -} - -// Collect stores the metric. -func (c *Collector) Collect(metric string, value interface{}) { - c.mutex.Lock() - c.metrics[metric] = value - c.mutex.Unlock() -} - -// Write renders the metrics. -func (c *Collector) Write(metricsFile string) error { - if err := c.emitter.Emit(metricsFile, c.metrics); err != nil { - return fmt.Errorf("emit metrics: %w", err) - } - return nil -} - -// A emitter emit metrics. -type emitter interface { - Emit(metricsFile string, metrics map[string]interface{}) error -} - -// JSONEmitter emits metrics in JSON format. -type JSONEmitter struct { - createWriter func(metricsFile string) (io.WriteCloser, error) -} - -// NewJSONEmitter create a metrics emitter. -func NewJSONEmitter() *JSONEmitter { - return &JSONEmitter{ - // For lazy file creation. - createWriter: func(metricsFile string) (io.WriteCloser, error) { - f, err := os.Create(metricsFile) - if err != nil { - return nil, fmt.Errorf("create file: %w", err) - } - return f, nil - }, - } -} - -// Emit renders the metrics in JSON format. -func (c *JSONEmitter) Emit(metricsFile string, metrics map[string]interface{}) error { - w, err := c.createWriter(metricsFile) - if err != nil { - return fmt.Errorf("create writer: %w", err) - } - defer w.Close() - enc := json.NewEncoder(w) - enc.SetIndent("", " ") - if err = enc.Encode(metrics); err != nil { - return fmt.Errorf("encode report: %w", err) - } - return nil -} - -// Collect stores the metric. -func Collect(metric string, value interface{}) { - DefaultCollector.Collect(metric, value) -} - -// Write renders the metrics. -func Write(metricsFile string) error { - return DefaultCollector.Write(metricsFile) -} diff --git a/internal/metrics/metrics_collector_test.go b/internal/metrics/metrics_collector_test.go deleted file mode 100644 index 0da1bbe..0000000 --- a/internal/metrics/metrics_collector_test.go +++ /dev/null @@ -1,230 +0,0 @@ -// Copyright 2023 Adevinta - -package metrics - -import ( - "bytes" - "encoding/json" - "io" - "os" - "path" - "testing" - - "github.com/google/go-cmp/cmp" -) - -type mockWriteClose struct { - *bytes.Buffer -} - -func (mwc *mockWriteClose) Close() error { - return nil -} - -func TestMetrics_JSONCollector(t *testing.T) { - tests := []struct { - name string - metrics map[string]interface{} - want map[string]interface{} - }{ - { - name: "Happy Path", - metrics: map[string]interface{}{ - "metric 1": "metric value 1", - "metric 2": 12345, - "metric 3": 25.5, - "metric 4": map[string]int{ - "key 1": 1, - "key 2": 2, - }, - "metric 5": []string{ - "one", "two", "three", - }, - }, - want: map[string]interface{}{ - "metric 1": "metric value 1", - "metric 2": float64(12345), - "metric 3": 25.5, - "metric 4": map[string]any{ - "key 1": float64(1), - "key 2": float64(2), - }, - "metric 5": []any{ - "one", "two", "three", - }, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - realInitialization() - buf := &bytes.Buffer{} - je := &JSONEmitter{ - createWriter: func(metricsFile string) (io.WriteCloser, error) { - return &mockWriteClose{buf}, nil - }, - } - mc := Collector{ - metrics: make(map[string]interface{}), - emitter: je, - } - for key, value := range tt.metrics { - mc.Collect(key, value) - } - - if err := mc.Write("fakeFile.json"); err != nil { - t.Fatalf("write metrics %v", err) - } - var got map[string]interface{} - if err := json.Unmarshal(buf.Bytes(), &got); err != nil { - t.Errorf("unmarshal json metrics: %v", err) - } - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("metrics mismatch (-want +got):\n%v", diff) - } - }) - } -} - -func TestMetrics_DefaultCollector(t *testing.T) { - tests := []struct { - name string - metrics map[string]interface{} - want map[string]interface{} - }{ - { - name: "Happy Path", - metrics: map[string]interface{}{ - "metric 1": "metric value 1", - "metric 2": 12345, - "metric 3": 25.5, - "metric 4": map[string]int{ - "key 1": 1, - "key 2": 2, - }, - "metric 5": []string{ - "one", "two", "three", - }, - }, - want: map[string]interface{}{ - "metric 1": "metric value 1", - "metric 2": float64(12345), - "metric 3": 25.5, - "metric 4": map[string]any{ - "key 1": float64(1), - "key 2": float64(2), - }, - "metric 5": []any{ - "one", "two", "three", - }, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - realInitialization() - buf := &bytes.Buffer{} - je := &JSONEmitter{ - createWriter: func(metricsFile string) (io.WriteCloser, error) { - return &mockWriteClose{buf}, nil - }, - } - mc := &Collector{ - metrics: make(map[string]interface{}), - emitter: je, - } - DefaultCollector = mc - for key, value := range tt.metrics { - Collect(key, value) - } - - if err := Write("fakeFile.json"); err != nil { - t.Fatalf("write metrics %v", err) - } - var got map[string]interface{} - if err := json.Unmarshal(buf.Bytes(), &got); err != nil { - t.Errorf("unmarshal json metrics: %v", err) - } - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("metrics mismatch (-want +got):\n%v", diff) - } - }) - } -} - -func TestMetrics_OutputFile(t *testing.T) { - tests := []struct { - name string - metrics map[string]interface{} - want map[string]interface{} - }{ - { - name: "Happy Path", - metrics: map[string]interface{}{ - "metric 1": "metric value 1", - "metric 2": 12345, - "metric 3": 25.5, - "metric 4": map[string]int{ - "key 1": 1, - "key 2": 2, - }, - "metric 5": []string{ - "one", "two", "three", - }, - }, - want: map[string]interface{}{ - "metric 1": "metric value 1", - "metric 2": float64(12345), - "metric 3": 25.5, - "metric 4": map[string]any{ - "key 1": float64(1), - "key 2": float64(2), - }, - "metric 5": []any{ - "one", "two", "three", - }, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - realInitialization() - tmpPath, err := os.MkdirTemp("", "") - if err != nil { - t.Fatalf("unable to create a temporary dir") - } - defer os.RemoveAll(tmpPath) - metricsFile := path.Join(tmpPath, "metrics.json") - - for key, value := range tt.metrics { - Collect(key, value) - } - - if err = Write(metricsFile); err != nil { - t.Fatalf("write metrics %v", err) - } - - file, err := os.Open(metricsFile) - if err != nil { - t.Fatalf("open metrics file: %v", err) - } - defer file.Close() - - content, err := io.ReadAll(file) - if err != nil { - t.Fatalf("read metrics file: %v", err) - } - var got map[string]interface{} - if err := json.Unmarshal(content, &got); err != nil { - t.Errorf("unmarshal json metrics: %v", err) - } - - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("metrics mismatch (-want +got):\n%v", diff) - } - }) - } -} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go new file mode 100644 index 0000000..6ab0fd3 --- /dev/null +++ b/internal/metrics/metrics_test.go @@ -0,0 +1,125 @@ +// Copyright 2023 Adevinta + +package metrics + +import ( + "bytes" + "encoding/json" + "io" + "os" + "path" + "testing" + + "github.com/google/go-cmp/cmp" +) + +var testdata = []struct { + name string + metrics map[string]any + want map[string]any +}{ + { + name: "happy path", + metrics: map[string]any{ + "metric 1": "metric value 1", + "metric 2": 12345, + "metric 3": 25.5, + "metric 4": map[string]int{ + "key 1": 1, + "key 2": 2, + }, + "metric 5": []string{ + "one", "two", "three", + }, + }, + want: map[string]any{ + "metric 1": "metric value 1", + "metric 2": float64(12345), + "metric 3": 25.5, + "metric 4": map[string]any{ + "key 1": float64(1), + "key 2": float64(2), + }, + "metric 5": []any{ + "one", "two", "three", + }, + }, + }, +} + +func TestWrite(t *testing.T) { + for _, tt := range testdata { + t.Run(tt.name, func(t *testing.T) { + oldDefaultCollector := DefaultCollector + defer func() { DefaultCollector = oldDefaultCollector }() + + DefaultCollector = NewCollector() + + var buf bytes.Buffer + + for key, value := range tt.metrics { + Collect(key, value) + } + + if err := Write(&buf); err != nil { + t.Fatalf("error writing metrics: %v", err) + } + + var got map[string]any + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Errorf("error decoding json metrics: %v", err) + } + + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("metrics mismatch (-want +got):\n%v", diff) + } + }) + } +} + +func TestWriteFile(t *testing.T) { + for _, tt := range testdata { + t.Run(tt.name, func(t *testing.T) { + oldDefaultCollector := DefaultCollector + defer func() { DefaultCollector = oldDefaultCollector }() + + DefaultCollector = NewCollector() + + tmpPath, err := os.MkdirTemp("", "") + if err != nil { + t.Fatalf("error creating temp dir: %v", err) + } + defer os.RemoveAll(tmpPath) + + file := path.Join(tmpPath, "metrics.json") + + for key, value := range tt.metrics { + Collect(key, value) + } + + if err = WriteFile(file); err != nil { + t.Fatalf("error writing metrics: %v", err) + } + + f, err := os.Open(file) + if err != nil { + t.Fatalf("error opening metrics file: %v", err) + } + defer f.Close() + + data, err := io.ReadAll(f) + if err != nil { + t.Fatalf("error reading metrics file: %v", err) + } + + var got map[string]any + if err := json.Unmarshal(data, &got); err != nil { + t.Errorf("error decoding json metrics: %v", err) + } + + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("metrics mismatch (-want +got):\n%v", diff) + } + }) + } +} diff --git a/internal/report/report.go b/internal/report/report.go index b05fbea..37a67e1 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -22,11 +22,10 @@ import ( // Writer represents a Lava report writer. type Writer struct { - prn printer - w io.WriteCloser - minSeverity config.Severity - exclusions []config.Exclusion - printMetrics bool + prn printer + w io.WriteCloser + minSeverity config.Severity + exclusions []config.Exclusion } // NewWriter creates a new instance of a report writer. @@ -51,11 +50,10 @@ func NewWriter(cfg config.ReportConfig) (Writer, error) { } return Writer{ - prn: prn, - w: w, - minSeverity: cfg.Severity, - exclusions: cfg.Exclusions, - printMetrics: cfg.Metrics, + prn: prn, + w: w, + minSeverity: cfg.Severity, + exclusions: cfg.Exclusions, }, nil } @@ -69,7 +67,7 @@ func (writer Writer) Write(er engine.Report) (ExitCode, error) { return 0, fmt.Errorf("parse report: %w", err) } - sum, err := writer.mkSummary(vulns) + sum, err := mkSummary(vulns) if err != nil { return 0, fmt.Errorf("calculate summary: %w", err) } @@ -199,35 +197,6 @@ func (writer Writer) calculateExitCode(sum summary) ExitCode { return 0 } -// mkSummary counts the number vulnerabilities per severity and the -// number of excluded vulnerabilities. The excluded vulnerabilities are -// not considered in the count per severity. -func (writer Writer) mkSummary(vulns []vulnerability) (summary, error) { - if len(vulns) == 0 { - metrics.Collect("vulnerabilities", make(map[string]int)) - metrics.Collect("excluded", 0) - return summary{}, nil - } - sum := summary{ - count: make(map[config.Severity]int), - } - countMetrics := make(map[string]int) - for _, vuln := range vulns { - if !vuln.Severity.IsValid() { - return summary{}, fmt.Errorf("invalid severity: %v", vuln.Severity) - } - if vuln.excluded { - sum.excluded++ - } else { - sum.count[vuln.Severity]++ - countMetrics[vuln.Severity.String()]++ - } - } - metrics.Collect("vulnerabilities", countMetrics) - metrics.Collect("excluded", sum.excluded) - return sum, nil -} - // vulnerability represents a vulnerability found by a check. type vulnerability struct { report.Vulnerability @@ -267,6 +236,35 @@ type summary struct { excluded int } +// mkSummary counts the number vulnerabilities per severity and the +// number of excluded vulnerabilities. The excluded vulnerabilities are +// not considered in the count per severity. +func mkSummary(vulns []vulnerability) (summary, error) { + if len(vulns) == 0 { + metrics.Collect("vulnerabilities", nil) + metrics.Collect("excluded", 0) + return summary{}, nil + } + sum := summary{ + count: make(map[config.Severity]int), + } + countMetrics := make(map[string]int) + for _, vuln := range vulns { + if !vuln.Severity.IsValid() { + return summary{}, fmt.Errorf("invalid severity: %v", vuln.Severity) + } + if vuln.excluded { + sum.excluded++ + } else { + sum.count[vuln.Severity]++ + countMetrics[vuln.Severity.String()]++ + } + } + metrics.Collect("vulnerabilities", countMetrics) + metrics.Collect("excluded", sum.excluded) + return sum, nil +} + // ExitCode represents an exit code depending on the vulnerabilities found. type ExitCode int diff --git a/internal/report/report_test.go b/internal/report/report_test.go index a6fd621..8bf2906 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -681,11 +681,7 @@ func TestMkSummary(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - w, err := NewWriter(config.ReportConfig{}) - if err != nil { - t.Fatalf("unable to create a report writer: %v", err) - } - got, err := w.mkSummary(tt.vulnerabilities) + got, err := mkSummary(tt.vulnerabilities) if (err == nil) != tt.wantNilErr { t.Errorf("unexpected error value: %v", err) }