Change overridesPerLimit to GaugeVec; test loadOverrides; add test FIXMEs

Author: jprenken

ratelimits/limit.go

@@ -358,7 +358,7 @@ type limitRegistry struct {
 	stats              prometheus.Registerer
 	overridesTimestamp prometheus.Gauge
 	overridesErrors    prometheus.Gauge
-	overridesPerLimit  map[Name]prometheus.Gauge
+	overridesPerLimit  prometheus.GaugeVec
 
 	logger blog.Logger
 }
@@ -396,27 +396,22 @@ func (l *limitRegistry) loadOverrides(ctx context.Context) error {
 		return err
 	}
 
-	// If it's an empty set, don't replace any current overrides.
 	if len(newOverrides) < 1 {
 		l.logger.Warning("loading overrides: no valid overrides")
+		// If it's an empty set, don't replace any current overrides.
 		return nil
 	}
 
+	l.overrides = newOverrides
+
+	l.overridesTimestamp.SetToCurrentTime()
 	newOverridesPerLimit := make(map[Name]float64)
 	for _, override := range newOverrides {
 		newOverridesPerLimit[override.Name]++
 	}
-
-	l.overrides = newOverrides
-	for name := range l.overridesPerLimit {
-		count, ok := newOverridesPerLimit[name]
-		if ok {
-			l.overridesPerLimit[name].Set(count)
-		} else {
-			l.overridesPerLimit[name].Set(0)
-		}
+	for rlName, rlString := range nameToString {
+		l.overridesPerLimit.WithLabelValues(rlString).Set(newOverridesPerLimit[rlName])
 	}
-	l.overridesTimestamp.SetToCurrentTime()
 
 	return nil
 }

ratelimits/limit_test.go

@@ -1,15 +1,22 @@
 package ratelimits
 
 import (
+	"context"
+	"errors"
 	"net/netip"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/prometheus/client_golang/prometheus"
+	io_prometheus_client "github.com/prometheus/client_model/go"
+
 	"github.com/letsencrypt/boulder/config"
 	"github.com/letsencrypt/boulder/identifier"
+	blog "github.com/letsencrypt/boulder/log"
+	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/test"
 )
 
@@ -26,11 +33,11 @@ func loadAndParseDefaultLimits(path string) (Limits, error) {
 	return parseDefaultLimits(fromFile)
 }
 
-// loadAndParseOverrideLimits is a helper that calls both loadOverrides and
-// parseOverrideLimits to handle a YAML file.
+// loadAndParseOverrideLimitsFromFile is a helper that calls both
+// loadOverridesFromFile and parseOverrideLimits to handle a YAML file.
 //
 // TODO(#7901): Update the tests to test these functions individually.
-func loadAndParseOverrideLimits(path string) (Limits, error) {
+func loadAndParseOverrideLimitsFromFile(path string) (Limits, error) {
 	fromFile, err := loadOverridesFromFile(path)
 	if err != nil {
 		return nil, err
@@ -148,25 +155,25 @@ func TestValidateLimit(t *testing.T) {
 	}
 }
 
-func TestLoadAndParseOverrideLimits(t *testing.T) {
+func TestLoadAndParseOverrideLimitsFromFile(t *testing.T) {
 	// Load a single valid override limit with Id formatted as 'enum:RegId'.
-	l, err := loadAndParseOverrideLimits("testdata/working_override.yml")
+	l, err := loadAndParseOverrideLimitsFromFile("testdata/working_override.yml")
 	test.AssertNotError(t, err, "valid single override limit")
 	expectKey := joinWithColon(NewRegistrationsPerIPAddress.EnumString(), "64.112.117.1")
 	test.AssertEquals(t, l[expectKey].Burst, int64(40))
 	test.AssertEquals(t, l[expectKey].Count, int64(40))
 	test.AssertEquals(t, l[expectKey].Period.Duration, time.Second)
 
 	// Load single valid override limit with a 'domainOrCIDR' Id.
-	l, err = loadAndParseOverrideLimits("testdata/working_override_regid_domainorcidr.yml")
+	l, err = loadAndParseOverrideLimitsFromFile("testdata/working_override_regid_domainorcidr.yml")
 	test.AssertNotError(t, err, "valid single override limit with Id of regId:domainOrCIDR")
 	expectKey = joinWithColon(CertificatesPerDomain.EnumString(), "example.com")
 	test.AssertEquals(t, l[expectKey].Burst, int64(40))
 	test.AssertEquals(t, l[expectKey].Count, int64(40))
 	test.AssertEquals(t, l[expectKey].Period.Duration, time.Second)
 
 	// Load multiple valid override limits with 'regId' Ids.
-	l, err = loadAndParseOverrideLimits("testdata/working_overrides.yml")
+	l, err = loadAndParseOverrideLimitsFromFile("testdata/working_overrides.yml")
 	test.AssertNotError(t, err, "multiple valid override limits")
 	expectKey1 := joinWithColon(NewRegistrationsPerIPAddress.EnumString(), "64.112.117.1")
 	test.AssertEquals(t, l[expectKey1].Burst, int64(40))
@@ -190,7 +197,7 @@ func TestLoadAndParseOverrideLimits(t *testing.T) {
 		identifier.NewDNS("example.com"),
 	})
 
-	l, err = loadAndParseOverrideLimits("testdata/working_overrides_regid_fqdnset.yml")
+	l, err = loadAndParseOverrideLimitsFromFile("testdata/working_overrides_regid_fqdnset.yml")
 	test.AssertNotError(t, err, "multiple valid override limits with 'fqdnSet' Ids")
 	test.AssertEquals(t, l[entryKey1].Burst, int64(40))
 	test.AssertEquals(t, l[entryKey1].Count, int64(40))
@@ -206,46 +213,92 @@ func TestLoadAndParseOverrideLimits(t *testing.T) {
 	test.AssertEquals(t, l[entryKey4].Period.Duration, time.Second*4)
 
 	// Path is empty string.
-	_, err = loadAndParseOverrideLimits("")
+	_, err = loadAndParseOverrideLimitsFromFile("")
 	test.AssertError(t, err, "path is empty string")
 	test.Assert(t, os.IsNotExist(err), "path is empty string")
 
 	// Path to file which does not exist.
-	_, err = loadAndParseOverrideLimits("testdata/file_does_not_exist.yml")
+	_, err = loadAndParseOverrideLimitsFromFile("testdata/file_does_not_exist.yml")
 	test.AssertError(t, err, "a file that does not exist ")
 	test.Assert(t, os.IsNotExist(err), "test file should not exist")
 
 	// Burst cannot be 0.
-	_, err = loadAndParseOverrideLimits("testdata/busted_override_burst_0.yml")
+	_, err = loadAndParseOverrideLimitsFromFile("testdata/busted_override_burst_0.yml")
 	test.AssertError(t, err, "single override limit with burst=0")
 	test.AssertContains(t, err.Error(), "invalid burst")
 
 	// Id cannot be empty.
-	_, err = loadAndParseOverrideLimits("testdata/busted_override_empty_id.yml")
+	_, err = loadAndParseOverrideLimitsFromFile("testdata/busted_override_empty_id.yml")
 	test.AssertError(t, err, "single override limit with empty id")
 	test.Assert(t, !os.IsNotExist(err), "test file should exist")
 
 	// Name cannot be empty.
-	_, err = loadAndParseOverrideLimits("testdata/busted_override_empty_name.yml")
+	_, err = loadAndParseOverrideLimitsFromFile("testdata/busted_override_empty_name.yml")
 	test.AssertError(t, err, "single override limit with empty name")
 	test.Assert(t, !os.IsNotExist(err), "test file should exist")
 
 	// Name must be a string representation of a valid Name enumeration.
-	_, err = loadAndParseOverrideLimits("testdata/busted_override_invalid_name.yml")
+	_, err = loadAndParseOverrideLimitsFromFile("testdata/busted_override_invalid_name.yml")
 	test.AssertError(t, err, "single override limit with invalid name")
 	test.Assert(t, !os.IsNotExist(err), "test file should exist")
 
 	// Multiple entries, second entry has a bad name.
-	_, err = loadAndParseOverrideLimits("testdata/busted_overrides_second_entry_bad_name.yml")
+	_, err = loadAndParseOverrideLimitsFromFile("testdata/busted_overrides_second_entry_bad_name.yml")
 	test.AssertError(t, err, "multiple override limits, second entry is bad")
 	test.Assert(t, !os.IsNotExist(err), "test file should exist")
 
 	// Multiple entries, third entry has id of "lol", instead of an IPv4 address.
-	_, err = loadAndParseOverrideLimits("testdata/busted_overrides_third_entry_bad_id.yml")
+	_, err = loadAndParseOverrideLimitsFromFile("testdata/busted_overrides_third_entry_bad_id.yml")
 	test.AssertError(t, err, "multiple override limits, third entry has bad Id value")
 	test.Assert(t, !os.IsNotExist(err), "test file should exist")
 }
 
+func TestLoadOverrides(t *testing.T) {
+	mockLog := blog.NewMock()
+
+	tb, err := NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "../test/config-next/wfe2-ratelimit-overrides.yml", metrics.NoopRegisterer, mockLog)
+	test.AssertNotError(t, err, "creating TransactionBuilder")
+	overridesData, err := loadOverridesFromFile("../test/config-next/wfe2-ratelimit-overrides.yml")
+	test.AssertNotError(t, err, "loading overrides from file")
+	testOverrides, err := parseOverrideLimits(overridesData)
+	test.AssertNotError(t, err, "parsing overrides")
+
+	test.AssertDeepEquals(t, tb.limitRegistry.overrides, testOverrides)
+
+	// FIXME: Test that testLimitRegistry.overridesPerLimit[name] values were set correctly, including that limits without overrides are set to 0.
+
+	var iom io_prometheus_client.Metric
+	err = tb.limitRegistry.overridesTimestamp.Write(&iom)
+	test.AssertNotError(t, err, "encoding overridesTimestamp metric")
+	test.Assert(t, int64(iom.Gauge.GetValue()) >= time.Now().Unix()-5, "overridesTimestamp too old")
+
+	// A failure loading overrides should log and return an error, and not
+	// overwrite existing overrides.
+	mockLog.Clear()
+	tb.limitRegistry.refreshOverrides = func(context.Context, prometheus.Gauge, blog.Logger) (Limits, error) {
+		return nil, errors.New("mock failure")
+	}
+	err = tb.limitRegistry.loadOverrides(context.Background())
+	test.AssertEquals(t, mockLog.GetAll()[0], "ERR: [AUDIT] loading overrides: mock failure")
+	test.AssertError(t, err, "fail to load overrides")
+	test.AssertDeepEquals(t, tb.limitRegistry.overrides, testOverrides)
+
+	// An empty set of overrides should log a warning, return nil, and not
+	// overwrite existing overrides.
+	mockLog.Clear()
+	tb.limitRegistry.refreshOverrides = func(context.Context, prometheus.Gauge, blog.Logger) (Limits, error) {
+		return Limits{}, nil
+	}
+	err = tb.limitRegistry.loadOverrides(context.Background())
+	test.AssertEquals(t, mockLog.GetAll()[0], "WARNING: loading overrides: no valid overrides")
+	test.AssertNotError(t, err, "load empty overrides")
+	test.AssertDeepEquals(t, tb.limitRegistry.overrides, testOverrides)
+}
+
+// FIXME: TestNewRefresher
+
+// FIXME: TestHydrateOverrideLimit
+
 func TestLoadAndParseDefaultLimits(t *testing.T) {
 	// Load a single valid default limit.
 	l, err := loadAndParseDefaultLimits("testdata/working_default.yml")

ratelimits/transaction.go

@@ -278,19 +278,16 @@ func NewTransactionBuilder(defaults LimitConfigs, overrides OverridesRefresher,
 	})
 	stats.MustRegister(overridesErrors)
 
-	overridesPerLimit := make(map[Name]prometheus.Gauge)
-	i := Unknown + 1
-	for i < Name(len(nameToString)) {
-		overridesPerLimit[i] = prometheus.NewGauge(prometheus.GaugeOpts{
-			Namespace:   "ratelimits",
-			Subsystem:   "overrides",
-			Name:        "active",
-			ConstLabels: prometheus.Labels{"limit": nameToString[i]},
-			Help:        "A gauge with the number of overrides per rate limit",
-		})
-		stats.MustRegister(overridesPerLimit[i])
-		i++
-	}
+	overridesPerLimit := prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Namespace: "ratelimits",
+			Subsystem: "overrides",
+			Name:      "active",
+			Help:      "A gauge with the number of overrides, partitioned by rate limit",
+		},
+		[]string{"limit"},
+	)
+	stats.MustRegister(overridesPerLimit)
 
 	registry := &limitRegistry{
 		defaults:         regDefaults,
@@ -300,7 +297,7 @@ func NewTransactionBuilder(defaults LimitConfigs, overrides OverridesRefresher,
 
 		overridesTimestamp: overridesTimestamp,
 		overridesErrors:    overridesErrors,
-		overridesPerLimit:  overridesPerLimit,
+		overridesPerLimit:  *overridesPerLimit,
 	}
 
 	// Load overrides, wrapped in a retry as a workaround to avoid depending on

ratelimits/transaction_test.go

@@ -228,4 +228,10 @@ func TestNewTransactionBuilder(t *testing.T) {
 	test.AssertEquals(t, newRegDefault.Burst, expectedBurst)
 	test.AssertEquals(t, newRegDefault.Count, expectedCount)
 	test.AssertEquals(t, newRegDefault.Period, expectedPeriod)
+
+	// FIXME: Test that metrics were set properly.
 }
+
+// FIXME: TestNewTransactionBuilderFromDatabase
+
+// FIXME: TestNewTransactionBuilderFromFiles