Merge pull request #43 from grafana/gcp/log-error-disk-parsing

cmd/internal/providers.go

@@ -14,20 +14,21 @@ 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 {
 		svc, err := compute.NewService(ctx)
 		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)
 		}

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)
 		}

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
 	}

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)

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
 }
 
@@ -66,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})
 				}

gcp/provider_test.go

@@ -1,23 +1,29 @@
 package gcp_test
 
 import (
+	"bytes"
 	"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 +38,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 +48,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 +78,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)
 	}
@@ -98,4 +105,55 @@ 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)
+		}
+
+		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)
+		}
+
+		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))
+		}
+
+		// 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")
+		}
+	})
 }