Răsfoiți Sursa

Fix noisy warnings for custom provider with default pricing config

Addresses #3905, where the custom (on-prem) provider emits recurring
warnings when optional pricing fields are not overridden:

- CustomProvider.LoadBalancerPricing now treats unset pricing fields as
  0 instead of failing with 'strconv.ParseFloat: parsing ""' on every
  metrics emission, falls back to defaultLBPrice when the forwarding
  rule price is unset, and names the offending field in parse errors.
- CustomProvider.ClusterInfo now falls back to CLUSTER_ID and then to a
  static default for the cluster name, matching other providers, so the
  kubecost_cluster_info metric always carries a 'name' label.
- The Prometheus ClusterMap loader no longer drops a cluster whose info
  metric is missing the 'name' label; it falls back to the cluster id
  and dedupes the warning.
- configs/default.json now includes the load balancer pricing keys.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
Claude 1 lună în urmă
părinte
comite
7c70f0d56a

+ 4 - 1
configs/default.json

@@ -11,5 +11,8 @@
     "regionNetworkEgress": "0.01",
     "internetNetworkEgress": "0.12",
     "natGatewayEgress": "0.045",
-    "natGatewayIngress": "0.045"
+    "natGatewayIngress": "0.045",
+    "firstFiveForwardingRulesCost": "0.0",
+    "additionalForwardingRuleCost": "0.0",
+    "LBIngressDataCost": "0.0"
 }

+ 2 - 2
modules/prometheus-source/pkg/prom/clustermap.go

@@ -99,8 +99,8 @@ func (pcm *PrometheusClusterMap) loadClusters() (map[string]*clusters.ClusterInf
 
 		name, err := result.GetString("name")
 		if err != nil {
-			log.Warnf("Failed to load 'name' field for ClusterInfo")
-			continue
+			log.DedupedWarningf(10, "ClusterInfo metric is missing 'name' label; falling back to cluster id %s", id)
+			name = id
 		}
 
 		profile, err := result.GetString("clusterprofile")

+ 35 - 6
pkg/cloud/provider/customprovider.go

@@ -137,10 +137,19 @@ func (cp *CustomProvider) ClusterInfo() (map[string]string, error) {
 	if err != nil {
 		return nil, err
 	}
-	m := make(map[string]string)
-	if conf.ClusterName != "" {
-		m["name"] = conf.ClusterName
+	const defaultClusterName = "Custom Cluster"
+	clusterName := conf.ClusterName
+	if clusterName == "" {
+		if clusterName = coreenv.GetClusterID(); clusterName != "" {
+			log.DedupedInfof(5, "Setting cluster name to %s from %s", clusterName, coreenv.ClusterIDEnvVar)
+		} else {
+			clusterName = defaultClusterName
+			log.DedupedInfof(5, "Unable to detect cluster name - using default of %s; set clusterName in the pricing config or via the %s env var", defaultClusterName, coreenv.ClusterIDEnvVar)
+		}
 	}
+
+	m := make(map[string]string)
+	m["name"] = clusterName
 	m["provider"] = opencost.CustomProvider
 	m["region"] = cp.ClusterRegion
 	m["account"] = cp.ClusterAccountID
@@ -325,20 +334,40 @@ func (cp *CustomProvider) NetworkPricing() (*models.Network, error) {
 	}, nil
 }
 
+// parsePriceOrZero parses a string-encoded price from the custom pricing
+// config, treating an unset field as 0 so that configs which omit optional
+// prices do not produce errors.
+func parsePriceOrZero(field, value string) (float64, error) {
+	if value == "" {
+		return 0, nil
+	}
+	price, err := strconv.ParseFloat(value, 64)
+	if err != nil {
+		return 0, fmt.Errorf("invalid custom pricing value %q for %s: %w", value, field, err)
+	}
+	return price, nil
+}
+
 func (cp *CustomProvider) LoadBalancerPricing() (*models.LoadBalancer, error) {
 	cpricing, err := cp.Config.GetCustomPricingData()
 	if err != nil {
 		return nil, err
 	}
-	fffrc, err := strconv.ParseFloat(cpricing.FirstFiveForwardingRulesCost, 64)
+	// Fall back to the generic defaultLBPrice when the granular forwarding
+	// rule price is not set.
+	fffrcStr := cpricing.FirstFiveForwardingRulesCost
+	if fffrcStr == "" {
+		fffrcStr = cpricing.DefaultLBPrice
+	}
+	fffrc, err := parsePriceOrZero("firstFiveForwardingRulesCost", fffrcStr)
 	if err != nil {
 		return nil, err
 	}
-	afrc, err := strconv.ParseFloat(cpricing.AdditionalForwardingRuleCost, 64)
+	afrc, err := parsePriceOrZero("additionalForwardingRuleCost", cpricing.AdditionalForwardingRuleCost)
 	if err != nil {
 		return nil, err
 	}
-	lbidc, err := strconv.ParseFloat(cpricing.LBIngressDataCost, 64)
+	lbidc, err := parsePriceOrZero("LBIngressDataCost", cpricing.LBIngressDataCost)
 	if err != nil {
 		return nil, err
 	}

+ 128 - 0
pkg/cloud/provider/customprovider_test.go

@@ -0,0 +1,128 @@
+package provider
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/opencost/opencost/pkg/cloud/models"
+	"github.com/opencost/opencost/pkg/config"
+)
+
+type fakeProviderConfig struct {
+	customPricing *models.CustomPricing
+}
+
+func (f *fakeProviderConfig) GetCustomPricingData() (*models.CustomPricing, error) {
+	if f.customPricing != nil {
+		return f.customPricing, nil
+	}
+	return nil, fmt.Errorf("no config")
+}
+
+func (f *fakeProviderConfig) Update(func(*models.CustomPricing) error) (*models.CustomPricing, error) {
+	return nil, fmt.Errorf("no config")
+}
+
+func (f *fakeProviderConfig) UpdateFromMap(map[string]string) (*models.CustomPricing, error) {
+	return nil, fmt.Errorf("no config")
+}
+
+func (f *fakeProviderConfig) ConfigFileManager() *config.ConfigFileManager { return nil }
+
+func TestCustomProviderLoadBalancerPricing(t *testing.T) {
+	cases := map[string]struct {
+		pricing      *models.CustomPricing
+		expectedCost float64
+		expectErr    string
+	}{
+		"unset fields default to zero cost": {
+			pricing:      &models.CustomPricing{},
+			expectedCost: 0.0,
+		},
+		"forwarding rule cost is used when set": {
+			pricing: &models.CustomPricing{
+				FirstFiveForwardingRulesCost: "0.025",
+				AdditionalForwardingRuleCost: "0.01",
+				LBIngressDataCost:            "0.008",
+			},
+			expectedCost: 0.025,
+		},
+		"defaultLBPrice is used when forwarding rule cost is unset": {
+			pricing: &models.CustomPricing{
+				DefaultLBPrice: "0.05",
+			},
+			expectedCost: 0.05,
+		},
+		"forwarding rule cost takes precedence over defaultLBPrice": {
+			pricing: &models.CustomPricing{
+				FirstFiveForwardingRulesCost: "0.025",
+				DefaultLBPrice:               "0.05",
+			},
+			expectedCost: 0.025,
+		},
+		"malformed value returns an error naming the field": {
+			pricing: &models.CustomPricing{
+				FirstFiveForwardingRulesCost: "not-a-number",
+			},
+			expectErr: "firstFiveForwardingRulesCost",
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			cp := &CustomProvider{Config: &fakeProviderConfig{customPricing: tc.pricing}}
+			lb, err := cp.LoadBalancerPricing()
+			if tc.expectErr != "" {
+				if err == nil {
+					t.Fatalf("expected error containing %q, got nil", tc.expectErr)
+				}
+				if !strings.Contains(err.Error(), tc.expectErr) {
+					t.Fatalf("expected error containing %q, got %q", tc.expectErr, err.Error())
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("LoadBalancerPricing returned error: %v", err)
+			}
+			if lb.Cost != tc.expectedCost {
+				t.Fatalf("expected cost %f, got %f", tc.expectedCost, lb.Cost)
+			}
+		})
+	}
+}
+
+func TestCustomProviderClusterInfoName(t *testing.T) {
+	cases := map[string]struct {
+		clusterName  string
+		clusterIDEnv string
+		expectedName string
+	}{
+		"configured cluster name is used": {
+			clusterName:  "my-cluster",
+			clusterIDEnv: "cluster-id",
+			expectedName: "my-cluster",
+		},
+		"falls back to CLUSTER_ID when cluster name is unset": {
+			clusterIDEnv: "cluster-id",
+			expectedName: "cluster-id",
+		},
+		"falls back to default when cluster name and CLUSTER_ID are unset": {
+			expectedName: "Custom Cluster",
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			t.Setenv("CLUSTER_ID", tc.clusterIDEnv)
+			cp := &CustomProvider{Config: &fakeProviderConfig{customPricing: &models.CustomPricing{ClusterName: tc.clusterName}}}
+			info, err := cp.ClusterInfo()
+			if err != nil {
+				t.Fatalf("ClusterInfo returned error: %v", err)
+			}
+			if info["name"] != tc.expectedName {
+				t.Fatalf("expected cluster name %q, got %q", tc.expectedName, info["name"])
+			}
+		})
+	}
+}