Handle errors from pricing api (#2783)

Signed-off-by: Sean Holcomb <seanholcomb@gmail.com>

pkg/cloud/gcp/provider.go

@@ -643,6 +643,16 @@ func (gcp *GCP) parsePage(r io.Reader, inputKeys map[string]models.Key, pvKeys m
 		} else if err != nil {
 			return nil, "", fmt.Errorf("error parsing GCP pricing page: %s", err)
 		}
+		if t == "error" {
+			errReader := dec.Buffered()
+			buf := new(strings.Builder)
+			_, err = io.Copy(buf, errReader)
+			if err != nil {
+				return nil, "", fmt.Errorf("error respnse: could not be read %s", err)
+			}
+
+			return nil, "", fmt.Errorf("error respnse: %s", buf.String())
+		}
 		if t == "skus" {
 			_, err := dec.Token() // consumes [
 			if err != nil {

pkg/cloud/gcp/provider_test.go

@@ -7,6 +7,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/google/martian/log"
 	"github.com/opencost/opencost/pkg/cloud/models"
 )
 
@@ -181,171 +182,196 @@ func TestKeyFeatures(t *testing.T) {
 // Load a reader object on a portion of a GCP api response
 // Confirm that the resting *GCP object contains the correctly parsed pricing info
 func TestParsePage(t *testing.T) {
-	// NOTE: SKUs here are copied directly from GCP Billing API. Some of them
-	// are in currency IDR, which relates directly to ticket GTM-52, for which
-	// some of this work was done. So if the prices look huge... don't panic.
-	// The only thing we're testing here is that, given these instance types
-	// and regions and prices, those same prices get set appropriately into
-	// the returned pricing map.
-	skuFilePath := "./test/skus.json"
-	fileBytes, err := os.ReadFile(skuFilePath)
-	if err != nil {
-		t.Fatalf("failed to open file '%s': %s", skuFilePath, err)
-	}
-	reader := bytes.NewReader(fileBytes)
-
-	testGcp := &GCP{}
 
-	inputKeys := map[string]models.Key{
-		"us-central1,a2highgpu,ondemand,gpu": &gcpKey{
-			Labels: map[string]string{
-				"node.kubernetes.io/instance-type": "a2-highgpu-1g",
-				"cloud.google.com/gke-gpu":         "true",
-				"cloud.google.com/gke-accelerator": "nvidia-tesla-a100",
-				"topology.kubernetes.io/region":    "us-central1",
-			},
-		},
-		"us-central1,e2medium,ondemand": &gcpKey{
-			Labels: map[string]string{
-				"node.kubernetes.io/instance-type": "e2-medium",
-				"topology.kubernetes.io/region":    "us-central1",
-			},
-		},
-		"us-central1,e2standard,ondemand": &gcpKey{
-			Labels: map[string]string{
-				"node.kubernetes.io/instance-type": "e2-standard",
-				"topology.kubernetes.io/region":    "us-central1",
-			},
-		},
-		"asia-southeast1,t2dstandard,ondemand": &gcpKey{
-			Labels: map[string]string{
-				"node.kubernetes.io/instance-type": "t2d-standard-1",
-				"topology.kubernetes.io/region":    "asia-southeast1",
-			},
+	testCases := map[string]struct {
+		inputFile      string
+		inputKeys      map[string]models.Key
+		pvKeys         map[string]models.PVKey
+		expectedPrices map[string]*GCPPricing
+		expectedToken  string
+		expectError    bool
+	}{
+		"Error Response": {
+			inputFile:      "./test/error.json",
+			inputKeys:      nil,
+			pvKeys:         nil,
+			expectedPrices: nil,
+			expectError:    true,
 		},
-	}
-
-	pvKeys := map[string]models.PVKey{}
-
-	actualPrices, token, err := testGcp.parsePage(reader, inputKeys, pvKeys)
-	if err != nil {
-		t.Fatalf("got error parsing page: %v", err)
-	}
-
-	const expectedToken = "APKCS1HVa0YpwgyTFbqbJ1eGwzKZmsPwLqzMZPTSNia5ck1Hc54Tx_Kz3oBxwSnRIdGVxXoSPdf-XlDpyNBf4QuxKcIEgtrQ1LDLWAgZowI0ns7HjrGta2s="
-	if token != expectedToken {
-		t.Fatalf("error parsing GCP next page token, parsed %s but expected %s", token, expectedToken)
-	}
-
-	expectedActualPrices := map[string]*GCPPricing{
-		"us-central1,a2highgpu,ondemand,gpu": {
-			Name:        "services/6F81-5844-456A/skus/039F-D0DA-4055",
-			SKUID:       "039F-D0DA-4055",
-			Description: "Nvidia Tesla A100 GPU running in Americas",
-			Category: &GCPResourceInfo{
-				ServiceDisplayName: "Compute Engine",
-				ResourceFamily:     "Compute",
-				ResourceGroup:      "GPU",
-				UsageType:          "OnDemand",
+		"SKU file": {
+			// NOTE: SKUs here are copied directly from GCP Billing API. Some of them
+			// are in currency IDR, which relates directly to ticket GTM-52, for which
+			// some of this work was done. So if the prices look huge... don't panic.
+			// The only thing we're testing here is that, given these instance types
+			// and regions and prices, those same prices get set appropriately into
+			// the returned pricing map.
+			inputFile: "./test/skus.json",
+			inputKeys: map[string]models.Key{
+				"us-central1,a2highgpu,ondemand,gpu": &gcpKey{
+					Labels: map[string]string{
+						"node.kubernetes.io/instance-type": "a2-highgpu-1g",
+						"cloud.google.com/gke-gpu":         "true",
+						"cloud.google.com/gke-accelerator": "nvidia-tesla-a100",
+						"topology.kubernetes.io/region":    "us-central1",
+					},
+				},
+				"us-central1,e2medium,ondemand": &gcpKey{
+					Labels: map[string]string{
+						"node.kubernetes.io/instance-type": "e2-medium",
+						"topology.kubernetes.io/region":    "us-central1",
+					},
+				},
+				"us-central1,e2standard,ondemand": &gcpKey{
+					Labels: map[string]string{
+						"node.kubernetes.io/instance-type": "e2-standard",
+						"topology.kubernetes.io/region":    "us-central1",
+					},
+				},
+				"asia-southeast1,t2dstandard,ondemand": &gcpKey{
+					Labels: map[string]string{
+						"node.kubernetes.io/instance-type": "t2d-standard-1",
+						"topology.kubernetes.io/region":    "asia-southeast1",
+					},
+				},
 			},
-			ServiceRegions: []string{"us-central1", "us-east1", "us-west1"},
-			PricingInfo: []*PricingInfo{
-				{
-					Summary: "",
-					PricingExpression: &PricingExpression{
-						UsageUnit:                "h",
-						UsageUnitDescription:     "hour",
-						BaseUnit:                 "s",
-						BaseUnitConversionFactor: 0,
-						DisplayQuantity:          1,
-						TieredRates: []*TieredRates{
-							{
-								StartUsageAmount: 0,
-								UnitPrice: &UnitPriceInfo{
-									CurrencyCode: "USD",
-									Units:        "2",
-									Nanos:        933908000,
+			pvKeys: map[string]models.PVKey{},
+			expectedPrices: map[string]*GCPPricing{
+				"us-central1,a2highgpu,ondemand,gpu": {
+					Name:        "services/6F81-5844-456A/skus/039F-D0DA-4055",
+					SKUID:       "039F-D0DA-4055",
+					Description: "Nvidia Tesla A100 GPU running in Americas",
+					Category: &GCPResourceInfo{
+						ServiceDisplayName: "Compute Engine",
+						ResourceFamily:     "Compute",
+						ResourceGroup:      "GPU",
+						UsageType:          "OnDemand",
+					},
+					ServiceRegions: []string{"us-central1", "us-east1", "us-west1"},
+					PricingInfo: []*PricingInfo{
+						{
+							Summary: "",
+							PricingExpression: &PricingExpression{
+								UsageUnit:                "h",
+								UsageUnitDescription:     "hour",
+								BaseUnit:                 "s",
+								BaseUnitConversionFactor: 0,
+								DisplayQuantity:          1,
+								TieredRates: []*TieredRates{
+									{
+										StartUsageAmount: 0,
+										UnitPrice: &UnitPriceInfo{
+											CurrencyCode: "USD",
+											Units:        "2",
+											Nanos:        933908000,
+										},
+									},
 								},
 							},
+							CurrencyConversionRate: 1,
+							EffectiveTime:          "2023-03-24T10:52:50.681Z",
 						},
 					},
-					CurrencyConversionRate: 1,
-					EffectiveTime:          "2023-03-24T10:52:50.681Z",
+					ServiceProviderName: "Google",
+					Node: &models.Node{
+						VCPUCost:         "0.031611",
+						RAMCost:          "0.004237",
+						UsesBaseCPUPrice: false,
+						GPU:              "1",
+						GPUName:          "nvidia-tesla-a100",
+						GPUCost:          "2.933908",
+					},
+				},
+				"us-central1,a2highgpu,ondemand": {
+					Node: &models.Node{
+						VCPUCost:         "0.031611",
+						RAMCost:          "0.004237",
+						UsesBaseCPUPrice: false,
+						UsageType:        "ondemand",
+					},
+				},
+				"us-central1,e2medium,ondemand": {
+					Node: &models.Node{
+						VCPU:             "1.000000",
+						VCPUCost:         "327.173848364",
+						RAMCost:          "43.85294978",
+						UsesBaseCPUPrice: false,
+						UsageType:        "ondemand",
+					},
+				},
+				"us-central1,e2medium,ondemand,gpu": {
+					Node: &models.Node{
+						VCPU:             "1.000000",
+						VCPUCost:         "327.173848364",
+						RAMCost:          "43.85294978",
+						UsesBaseCPUPrice: false,
+						UsageType:        "ondemand",
+					},
+				},
+				"us-central1,e2standard,ondemand": {
+					Node: &models.Node{
+						VCPUCost:         "327.173848364",
+						RAMCost:          "43.85294978",
+						UsesBaseCPUPrice: false,
+						UsageType:        "ondemand",
+					},
+				},
+				"us-central1,e2standard,ondemand,gpu": {
+					Node: &models.Node{
+						VCPUCost:         "327.173848364",
+						RAMCost:          "43.85294978",
+						UsesBaseCPUPrice: false,
+						UsageType:        "ondemand",
+					},
+				},
+				"asia-southeast1,t2dstandard,ondemand": {
+					Node: &models.Node{
+						VCPUCost:         "508.934997455",
+						RAMCost:          "68.204999658",
+						UsesBaseCPUPrice: false,
+						UsageType:        "ondemand",
+					},
+				},
+				"asia-southeast1,t2dstandard,ondemand,gpu": {
+					Node: &models.Node{
+						VCPUCost:         "508.934997455",
+						RAMCost:          "68.204999658",
+						UsesBaseCPUPrice: false,
+						UsageType:        "ondemand",
+					},
 				},
 			},
-			ServiceProviderName: "Google",
-			Node: &models.Node{
-				VCPUCost:         "0.031611",
-				RAMCost:          "0.004237",
-				UsesBaseCPUPrice: false,
-				GPU:              "1",
-				GPUName:          "nvidia-tesla-a100",
-				GPUCost:          "2.933908",
-			},
-		},
-		"us-central1,a2highgpu,ondemand": {
-			Node: &models.Node{
-				VCPUCost:         "0.031611",
-				RAMCost:          "0.004237",
-				UsesBaseCPUPrice: false,
-				UsageType:        "ondemand",
-			},
-		},
-		"us-central1,e2medium,ondemand": {
-			Node: &models.Node{
-				VCPU:             "1.000000",
-				VCPUCost:         "327.173848364",
-				RAMCost:          "43.85294978",
-				UsesBaseCPUPrice: false,
-				UsageType:        "ondemand",
-			},
-		},
-		"us-central1,e2medium,ondemand,gpu": {
-			Node: &models.Node{
-				VCPU:             "1.000000",
-				VCPUCost:         "327.173848364",
-				RAMCost:          "43.85294978",
-				UsesBaseCPUPrice: false,
-				UsageType:        "ondemand",
-			},
-		},
-		"us-central1,e2standard,ondemand": {
-			Node: &models.Node{
-				VCPUCost:         "327.173848364",
-				RAMCost:          "43.85294978",
-				UsesBaseCPUPrice: false,
-				UsageType:        "ondemand",
-			},
-		},
-		"us-central1,e2standard,ondemand,gpu": {
-			Node: &models.Node{
-				VCPUCost:         "327.173848364",
-				RAMCost:          "43.85294978",
-				UsesBaseCPUPrice: false,
-				UsageType:        "ondemand",
-			},
-		},
-		"asia-southeast1,t2dstandard,ondemand": {
-			Node: &models.Node{
-				VCPUCost:         "508.934997455",
-				RAMCost:          "68.204999658",
-				UsesBaseCPUPrice: false,
-				UsageType:        "ondemand",
-			},
-		},
-		"asia-southeast1,t2dstandard,ondemand,gpu": {
-			Node: &models.Node{
-				VCPUCost:         "508.934997455",
-				RAMCost:          "68.204999658",
-				UsesBaseCPUPrice: false,
-				UsageType:        "ondemand",
-			},
+			expectedToken: "APKCS1HVa0YpwgyTFbqbJ1eGwzKZmsPwLqzMZPTSNia5ck1Hc54Tx_Kz3oBxwSnRIdGVxXoSPdf-XlDpyNBf4QuxKcIEgtrQ1LDLWAgZowI0ns7HjrGta2s=",
+			expectError:   false,
 		},
 	}
 
-	if !reflect.DeepEqual(actualPrices, expectedActualPrices) {
-		act, _ := json.Marshal(actualPrices)
-		exp, _ := json.Marshal(expectedActualPrices)
-		t.Errorf("error parsing GCP prices: parsed \n%s\n expected \n%s\n", string(act), string(exp))
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			fileBytes, err := os.ReadFile(tc.inputFile)
+			if err != nil {
+				t.Fatalf("failed to open file '%s': %s", tc.inputFile, err)
+			}
+			reader := bytes.NewReader(fileBytes)
+
+			testGcp := &GCP{}
+			actualPrices, token, err := testGcp.parsePage(reader, tc.inputKeys, tc.pvKeys)
+			if err != nil {
+				log.Errorf("got error parsing page: %v", err)
+			}
+			if tc.expectError != (err != nil) {
+				t.Fatalf("Error from result was not as expected. Expected: %v, Actual: %v", tc.expectError, err != nil)
+			}
+
+			if token != tc.expectedToken {
+				t.Fatalf("error parsing GCP next page token, parsed %s but expected %s", token, tc.expectedToken)
+			}
+
+			if !reflect.DeepEqual(actualPrices, tc.expectedPrices) {
+				act, _ := json.Marshal(actualPrices)
+				exp, _ := json.Marshal(tc.expectedPrices)
+				t.Errorf("error parsing GCP prices: parsed \n%s\n expected \n%s\n", string(act), string(exp))
+			}
+		})
 	}
+
 }

pkg/cloud/gcp/test/error.json

@@ -0,0 +1,17 @@
+{
+  "error": {
+    "code": 400,
+    "message": "API key not valid. Please pass a valid API key.",
+    "status": "INVALID_ARGUMENT",
+    "details": [
+      {
+        "@type": "type.googleapis.com/google.rpc.ErrorInfo",
+        "reason": "API_KEY_INVALID",
+        "domain": "googleapis.com",
+        "metadata": {
+          "service": "cloudbilling.googleapis.com"
+        }
+      }
+    ]
+  }
+}