From 1355c5f8159ff540335d5aaf77e6aed543f4ca42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leandro=20L=C3=B3pez=20=28inkel=29?= Date: Tue, 3 Jan 2023 14:58:32 -0300 Subject: [PATCH 1/4] Add regression test for GCP provider panicking on non-JSON disk desc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This has been reported in #41. Signed-off-by: Leandro López (inkel) --- gcp/provider_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/gcp/provider_test.go b/gcp/provider_test.go index 8da1b02..e46eb65 100644 --- a/gcp/provider_test.go +++ b/gcp/provider_test.go @@ -98,4 +98,46 @@ func TestProviderListUnusedDisks(t *testing.T) { if err != nil { t.Fatalf("metadata doesn't match: %v", err) } + + t.Run("disk without JSON in description", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + // are we requesting the right API endpoint? + if got, exp := req.URL.Path, "/projects/my-project/aggregated/disks"; exp != got { + t.Fatalf("expecting request to %s, got %s", exp, got) + } + + res := &compute.DiskAggregatedList{ + Items: map[string]compute.DisksScopedList{ + "foo": { + Disks: []*compute.Disk{ + {Name: "disk-2", Zone: "eu-west2-b", Description: "some string that isn't JSON"}, + }, + }, + }, + } + + b, _ := json.Marshal(res) + w.Write(b) + })) + defer ts.Close() + + svc, err := compute.NewService(context.Background(), option.WithAPIKey("123abc"), option.WithEndpoint(ts.URL)) + if err != nil { + t.Fatalf("unexpected error creating GCP compute service: %v", err) + } + + p, err := gcp.NewProvider(svc, "my-project", nil) + if err != nil { + t.Fatal("unexpected error creating provider:", err) + } + + disks, err := p.ListUnusedDisks(ctx) + if err != nil { + t.Fatal("unexpected error listing unused disks:", err) + } + + if len(disks) != 1 { + t.Fatalf("expecting 1 unused disk, got %d", len(disks)) + } + }) } From f86b9a9a297b7e9014b6a013e149093b3eb4de3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leandro=20L=C3=B3pez=20=28inkel=29?= Date: Tue, 3 Jan 2023 15:20:00 -0300 Subject: [PATCH 2/4] Pass down logfmt.Logger to GCP provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Leandro López (inkel) --- gcp/provider.go | 5 ++++- gcp/provider_test.go | 12 +++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/gcp/provider.go b/gcp/provider.go index be69eea..47b1acc 100644 --- a/gcp/provider.go +++ b/gcp/provider.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/grafana/unused" + "github.com/inkel/logfmt" "google.golang.org/api/compute/v1" ) @@ -22,6 +23,7 @@ type Provider struct { project string svc *compute.Service meta unused.Meta + logger *logfmt.Logger } // Name returns GCP. @@ -35,7 +37,7 @@ func (p *Provider) Meta() unused.Meta { return p.meta } // A valid GCP compute service must be supplied in order to listed the // unused resources. It also requires a valid project ID which should // be the project where the disks were created. -func NewProvider(svc *compute.Service, project string, meta unused.Meta) (*Provider, error) { +func NewProvider(logger *logfmt.Logger, svc *compute.Service, project string, meta unused.Meta) (*Provider, error) { if project == "" { return nil, ErrMissingProject } @@ -48,6 +50,7 @@ func NewProvider(svc *compute.Service, project string, meta unused.Meta) (*Provi project: project, svc: svc, meta: meta, + logger: logger, }, nil } diff --git a/gcp/provider_test.go b/gcp/provider_test.go index e46eb65..f0edbb0 100644 --- a/gcp/provider_test.go +++ b/gcp/provider_test.go @@ -4,20 +4,25 @@ import ( "context" "encoding/json" "errors" + "io" "net/http" "net/http/httptest" + "regexp" "testing" "github.com/grafana/unused" "github.com/grafana/unused/gcp" "github.com/grafana/unused/unusedtest" + "github.com/inkel/logfmt" "google.golang.org/api/compute/v1" "google.golang.org/api/option" ) func TestNewProvider(t *testing.T) { + l := logfmt.NewLogger(io.Discard) + t.Run("project is required", func(t *testing.T) { - p, err := gcp.NewProvider(nil, "", nil) + p, err := gcp.NewProvider(l, nil, "", nil) if !errors.Is(err, gcp.ErrMissingProject) { t.Fatalf("expecting error %v, got %v", gcp.ErrMissingProject, err) } @@ -32,7 +37,7 @@ func TestNewProvider(t *testing.T) { if err != nil { t.Fatalf("unexpected error creating GCP compute service: %v", err) } - return gcp.NewProvider(svc, "my-provider", meta) + return gcp.NewProvider(l, svc, "my-provider", meta) }) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -42,6 +47,7 @@ func TestNewProvider(t *testing.T) { func TestProviderListUnusedDisks(t *testing.T) { ctx := context.Background() + l := logfmt.NewLogger(io.Discard) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { // are we requesting the right API endpoint? @@ -71,7 +77,7 @@ func TestProviderListUnusedDisks(t *testing.T) { t.Fatalf("unexpected error creating GCP compute service: %v", err) } - p, err := gcp.NewProvider(svc, "my-project", nil) + p, err := gcp.NewProvider(l, svc, "my-project", nil) if err != nil { t.Fatal("unexpected error creating provider:", err) } From 662e3b08b3219a7ce227e65dcd2d025031ef8701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leandro=20L=C3=B3pez=20=28inkel=29?= Date: Tue, 3 Jan 2023 15:20:28 -0300 Subject: [PATCH 3/4] Log error when parsing GCP disk with non-JSON description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #41 Signed-off-by: Leandro López (inkel) --- gcp/provider.go | 6 +++++- gcp/provider_test.go | 12 +++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/gcp/provider.go b/gcp/provider.go index 47b1acc..ce0719d 100644 --- a/gcp/provider.go +++ b/gcp/provider.go @@ -69,7 +69,11 @@ func (p *Provider) ListUnusedDisks(ctx context.Context) (unused.Disks, error) { m, err := diskMetadata(d) if err != nil { - return fmt.Errorf("reading disk metadata: %w", err) + p.logger.Log("cannot parse disk metadata", logfmt.Labels{ + "project": p.project, + "disk": d.Name, + "err": err, + }) } disks = append(disks, &Disk{d, p, m}) } diff --git a/gcp/provider_test.go b/gcp/provider_test.go index f0edbb0..e067d6d 100644 --- a/gcp/provider_test.go +++ b/gcp/provider_test.go @@ -1,6 +1,7 @@ package gcp_test import ( + "bytes" "context" "encoding/json" "errors" @@ -132,7 +133,10 @@ func TestProviderListUnusedDisks(t *testing.T) { t.Fatalf("unexpected error creating GCP compute service: %v", err) } - p, err := gcp.NewProvider(svc, "my-project", nil) + var buf bytes.Buffer + l := logfmt.NewLogger(&buf) + + p, err := gcp.NewProvider(l, svc, "my-project", nil) if err != nil { t.Fatal("unexpected error creating provider:", err) } @@ -145,5 +149,11 @@ func TestProviderListUnusedDisks(t *testing.T) { if len(disks) != 1 { t.Fatalf("expecting 1 unused disk, got %d", len(disks)) } + + // check that we logged about it + m, _ := regexp.MatchString(`msg="cannot parse disk metadata".+disk="disk-2"`, buf.String()) + if !m { + t.Fatal("expecting a log line to be emitted") + } }) } From 6ec5d4febeee33633cba86bde5e3778e1dfa7a86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leandro=20L=C3=B3pez=20=28inkel=29?= Date: Tue, 3 Jan 2023 15:43:27 -0300 Subject: [PATCH 4/4] Pass down logger when creating providers for binaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Leandro López (inkel) --- cmd/internal/providers.go | 5 +++-- cmd/internal/providers_test.go | 11 +++++++---- cmd/unused-exporter/main.go | 2 +- cmd/unused/main.go | 5 ++++- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/cmd/internal/providers.go b/cmd/internal/providers.go index c9e2c0f..b9c5a44 100644 --- a/cmd/internal/providers.go +++ b/cmd/internal/providers.go @@ -14,12 +14,13 @@ import ( "github.com/grafana/unused/aws" "github.com/grafana/unused/azure" "github.com/grafana/unused/gcp" + "github.com/inkel/logfmt" "google.golang.org/api/compute/v1" ) var ErrNoProviders = errors.New("please select at least one provider") -func CreateProviders(ctx context.Context, gcpProjects, awsProfiles, azureSubs []string) ([]unused.Provider, error) { +func CreateProviders(ctx context.Context, logger *logfmt.Logger, gcpProjects, awsProfiles, azureSubs []string) ([]unused.Provider, error) { providers := make([]unused.Provider, 0, len(gcpProjects)+len(awsProfiles)+len(azureSubs)) for _, projectID := range gcpProjects { @@ -27,7 +28,7 @@ func CreateProviders(ctx context.Context, gcpProjects, awsProfiles, azureSubs [] if err != nil { return nil, fmt.Errorf("cannot create GCP compute service: %w", err) } - p, err := gcp.NewProvider(svc, projectID, map[string]string{"project": projectID}) + p, err := gcp.NewProvider(logger, svc, projectID, map[string]string{"project": projectID}) if err != nil { return nil, fmt.Errorf("creating GCP provider for project %s: %w", projectID, err) } diff --git a/cmd/internal/providers_test.go b/cmd/internal/providers_test.go index 2d16c36..0302d1c 100644 --- a/cmd/internal/providers_test.go +++ b/cmd/internal/providers_test.go @@ -12,11 +12,14 @@ import ( "github.com/grafana/unused/azure" "github.com/grafana/unused/cmd/internal" "github.com/grafana/unused/gcp" + "github.com/inkel/logfmt" ) func TestCreateProviders(t *testing.T) { + l := logfmt.NewLogger(io.Discard) + t.Run("fail when no provider is given", func(t *testing.T) { - ps, err := internal.CreateProviders(context.Background(), nil, nil, nil) + ps, err := internal.CreateProviders(context.Background(), l, nil, nil, nil) if !errors.Is(err, internal.ErrNoProviders) { t.Fatalf("expecting error %v, got %v", internal.ErrNoProviders, err) @@ -31,7 +34,7 @@ func TestCreateProviders(t *testing.T) { } t.Run("GCP", func(t *testing.T) { - ps, err := internal.CreateProviders(context.Background(), []string{"foo", "bar"}, nil, nil) + ps, err := internal.CreateProviders(context.Background(), l, []string{"foo", "bar"}, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -47,7 +50,7 @@ func TestCreateProviders(t *testing.T) { }) t.Run("AWS", func(t *testing.T) { - ps, err := internal.CreateProviders(context.Background(), nil, []string{"foo", "bar"}, nil) + ps, err := internal.CreateProviders(context.Background(), l, nil, []string{"foo", "bar"}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -63,7 +66,7 @@ func TestCreateProviders(t *testing.T) { }) t.Run("Azure", func(t *testing.T) { - ps, err := internal.CreateProviders(context.Background(), nil, nil, []string{"foo", "bar"}) + ps, err := internal.CreateProviders(context.Background(), l, nil, nil, []string{"foo", "bar"}) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cmd/unused-exporter/main.go b/cmd/unused-exporter/main.go index 5ec2aec..99b6368 100644 --- a/cmd/unused-exporter/main.go +++ b/cmd/unused-exporter/main.go @@ -45,7 +45,7 @@ func main() { } func realMain(ctx context.Context, cfg config) error { - providers, err := internal.CreateProviders(ctx, cfg.Providers.GCP, cfg.Providers.AWS, cfg.Providers.Azure) + providers, err := internal.CreateProviders(ctx, cfg.Logger, cfg.Providers.GCP, cfg.Providers.AWS, cfg.Providers.Azure) if err != nil { return err } diff --git a/cmd/unused/main.go b/cmd/unused/main.go index 3f7225c..9888248 100644 --- a/cmd/unused/main.go +++ b/cmd/unused/main.go @@ -23,6 +23,7 @@ import ( "github.com/grafana/unused/cmd/internal" "github.com/grafana/unused/cmd/unused/internal/ui" + "github.com/inkel/logfmt" ) func main() { @@ -65,7 +66,9 @@ func main() { ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt) defer cancel() - providers, err := internal.CreateProviders(ctx, gcpProjects, awsProfiles, azureSubs) + logger := logfmt.NewLogger(os.Stderr) + + providers, err := internal.CreateProviders(ctx, logger, gcpProjects, awsProfiles, azureSubs) if err != nil { cancel() fmt.Fprintln(os.Stderr, "creating providers:", err)