fix tenant glob match bug (#128)

Author: jnyi

.github/workflows/go.yaml

@@ -147,7 +147,7 @@ jobs:
       matrix:
         parallelism: [8]
         index: [0, 1, 2, 3, 4, 5, 6, 7]
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-24.04
     name: Thanos end-to-end tests
     env:
       GOBIN: /tmp/.bin

pkg/extprom/http/instrument_client.go

@@ -23,7 +23,7 @@ type ClientMetrics struct {
 
 // NewClientMetrics creates a new instance of ClientMetrics.
 // It will also register the metrics with the included register.
-// This ClientMetrics should be re-used for diff clients with the same purpose.
+// This ClientMetrics should be reused for diff clients with the same purpose.
 // e.g. 1 ClientMetrics should be used for all the clients that talk to Alertmanager.
 func NewClientMetrics(reg prometheus.Registerer) *ClientMetrics {
 	var m ClientMetrics

pkg/query/endpointset.go

@@ -460,7 +460,7 @@ func (e *EndpointSet) Update(ctx context.Context) {
 		if er.HasStoreAPI() && (er.ComponentType() == component.Sidecar || er.ComponentType() == component.Rule) &&
 			stats[component.Sidecar.String()][extLset]+stats[component.Rule.String()][extLset] > 0 {
 
-			level.Warn(e.logger).Log("msg", "found duplicate storeEndpoints producer (sidecar or ruler). This is not advices as it will malform data in in the same bucket",
+			level.Warn(e.logger).Log("msg", "found duplicate storeEndpoints producer (sidecar or ruler). This is not advice as it will malform data in in the same bucket",
 				"address", addr, "extLset", extLset, "duplicates", fmt.Sprintf("%v", stats[component.Sidecar.String()][extLset]+stats[component.Rule.String()][extLset]+1))
 		}
 		stats[er.ComponentType().String()][extLset]++

pkg/receive/hashring.go

@@ -244,13 +244,42 @@ func (c ketamaHashring) GetN(tenant string, ts *prompb.TimeSeries, n uint64) (En
 	return c.endpoints[endpointIndex], nil
 }
 
+type tenantSet map[string]tenantMatcher
+
+func (t tenantSet) match(tenant string) (bool, error) {
+	// Fast path for the common case of direct match.
+	if mt, ok := t[tenant]; ok && isExactMatcher(mt) {
+		return true, nil
+	} else {
+		for tenantPattern, matcherType := range t {
+			switch matcherType {
+			case TenantMatcherGlob:
+				matches, err := filepath.Match(tenantPattern, tenant)
+				if err != nil {
+					return false, fmt.Errorf("error matching tenant pattern %s (tenant %s): %w", tenantPattern, tenant, err)
+				}
+				if matches {
+					return true, nil
+				}
+			case TenantMatcherTypeExact:
+				// Already checked above, skipping.
+				fallthrough
+			default:
+				continue
+			}
+
+		}
+	}
+	return false, nil
+}
+
 // multiHashring represents a set of hashrings.
 // Which hashring to use for a tenant is determined
 // by the tenants field of the hashring configuration.
 type multiHashring struct {
 	cache      map[string]Hashring
 	hashrings  []Hashring
-	tenantSets []map[string]tenantMatcher
+	tenantSets []tenantSet
 
 	// We need a mutex to guard concurrent access
 	// to the cache map, as this is both written to
@@ -283,28 +312,10 @@ func (m *multiHashring) GetN(tenant string, ts *prompb.TimeSeries, n uint64) (En
 		if t == nil {
 			found = true
 		} else {
-			// Fast path for the common case of direct match.
-			if mt, ok := t[tenant]; ok && isExactMatcher(mt) {
-				found = true
-			} else {
-				for tenantPattern, matcherType := range t {
-					switch matcherType {
-					case TenantMatcherGlob:
-						matches, err := filepath.Match(tenantPattern, tenant)
-						if err != nil {
-							return Endpoint{}, fmt.Errorf("error matching tenant pattern %s (tenant %s): %w", tenantPattern, tenant, err)
-						}
-						found = matches
-					case TenantMatcherTypeExact:
-						// Already checked above, skipping.
-						fallthrough
-					default:
-						continue
-					}
-
-				}
+			var err error
+			if found, err = t.match(tenant); err != nil {
+				return Endpoint{}, err
 			}
-
 		}
 		if found {
 			m.mu.Lock()

pkg/receive/hashring_test.go

@@ -140,6 +140,63 @@ func TestHashringGet(t *testing.T) {
 				"node6": {},
 			},
 		},
+		{
+			name: "glob hashring match",
+			cfg: []HashringConfig{
+				{
+					Endpoints:         []Endpoint{{Address: "node1"}, {Address: "node2"}, {Address: "node3"}},
+					Tenants:           []string{"prefix*"},
+					TenantMatcherType: TenantMatcherGlob,
+				},
+				{
+					Endpoints: []Endpoint{{Address: "node4"}, {Address: "node5"}, {Address: "node6"}},
+				},
+			},
+			nodes: map[string]struct{}{
+				"node1": {},
+				"node2": {},
+				"node3": {},
+			},
+			tenant: "prefix-1",
+		},
+		{
+			name: "glob hashring not match",
+			cfg: []HashringConfig{
+				{
+					Endpoints:         []Endpoint{{Address: "node1"}, {Address: "node2"}, {Address: "node3"}},
+					Tenants:           []string{"prefix*"},
+					TenantMatcherType: TenantMatcherGlob,
+				},
+				{
+					Endpoints: []Endpoint{{Address: "node4"}, {Address: "node5"}, {Address: "node6"}},
+				},
+			},
+			nodes: map[string]struct{}{
+				"node4": {},
+				"node5": {},
+				"node6": {},
+			},
+			tenant: "suffix-1",
+		},
+		{
+			name: "glob hashring multiple matches",
+			cfg: []HashringConfig{
+				{
+					Endpoints:         []Endpoint{{Address: "node1"}, {Address: "node2"}, {Address: "node3"}},
+					Tenants:           []string{"t1-*", "t2", "t3-*"},
+					TenantMatcherType: TenantMatcherGlob,
+				},
+				{
+					Endpoints: []Endpoint{{Address: "node4"}, {Address: "node5"}, {Address: "node6"}},
+				},
+			},
+			nodes: map[string]struct{}{
+				"node1": {},
+				"node2": {},
+				"node3": {},
+			},
+			tenant: "t2",
+		},
 	} {
 		hs, err := NewMultiHashring(AlgorithmHashmod, 3, tc.cfg)
 		require.NoError(t, err)

pkg/runutil/runutil.go

@@ -45,7 +45,7 @@
 // The rununtil.Exhaust* family of functions provide the same functionality but
 // they take an io.ReadCloser and they exhaust the whole reader before closing
 // them. They are useful when trying to use http keep-alive connections because
-// for the same connection to be re-used the whole response body needs to be
+// for the same connection to be reused the whole response body needs to be
 // exhausted.
 package runutil
 

test/e2e/query_test.go

@@ -1248,46 +1248,49 @@ func TestSidecarQueryEvaluation(t *testing.T) {
 	}
 }
 
-// An emptyCtx is never canceled, has no values, and has no deadline. It is not
-// struct{}, since vars of this type must have distinct addresses.
-type emptyCtx int
+var chromedpAllocator context.Context
 
-func (*emptyCtx) Deadline() (deadline time.Time, ok bool) {
-	return
-}
-
-func (*emptyCtx) Done() <-chan struct{} {
-	return nil
-}
-
-func (*emptyCtx) Err() error {
-	return nil
-}
-
-func (*emptyCtx) Value(key any) any {
-	return nil
-}
-
-func (e *emptyCtx) String() string {
-	return "Context"
+func TestMain(m *testing.M) {
+	execAlloc, execCancel := chromedp.NewExecAllocator(
+		context.Background(),
+	)
+	chromedpAllocator = execAlloc
+	rc := m.Run()
+	execCancel()
+	os.Exit(rc)
 }
 
 func checkNetworkRequests(t *testing.T, addr string) {
-	ctx, cancel := chromedp.NewContext(new(emptyCtx))
+	opts := append(chromedp.DefaultExecAllocatorOptions[:],
+		chromedp.Flag("headless", true),
+		chromedp.Flag("no-sandbox", true),
+		chromedp.Flag("disable-gpu", true),
+	)
+	allocCtx, cancel := chromedp.NewExecAllocator(chromedpAllocator, opts...)
+	defer cancel()
+
+	ctx, cancel := chromedp.NewContext(allocCtx)
 	t.Cleanup(cancel)
 
+	// make sure browser is already started
+	err := chromedp.Run(ctx)
+	testutil.Ok(t, err)
+
 	testutil.Ok(t, runutil.Retry(1*time.Minute, ctx.Done(), func() error {
 		var networkErrors []string
 
+		var newCtx context.Context
+		newCtx, newCancel := chromedp.NewContext(ctx)
+		t.Cleanup(newCancel)
 		// Listen for failed network requests and push them to an array.
-		chromedp.ListenTarget(ctx, func(ev interface{}) {
+		chromedp.ListenTarget(newCtx, func(ev interface{}) {
 			switch ev := ev.(type) {
 			case *network.EventLoadingFailed:
 				networkErrors = append(networkErrors, ev.ErrorText)
 			}
 		})
 
-		err := chromedp.Run(ctx,
+		err := chromedp.Run(newCtx,
 			network.Enable(),
 			chromedp.Navigate(addr),
 			chromedp.WaitVisible(`body`),