Handle forward slashes in ruler mapper/manager

CHANGELOG.md

@@ -44,6 +44,7 @@
 * [ENHANCEMENT] Add de-duplicated chunks counter `cortex_chunk_store_deduped_chunks_total` which counts every chunk not sent to the store because it was already sent by another replica. #2485
 * [ENHANCEMENT] query-frontend now also logs the POST data of long queries. #2481
 * [ENHANCEMENT] Experimental WAL: Ingester WAL records now have type header and the custom WAL records have been replaced by Prometheus TSDB's WAL records. Old records will not be supported from 1.3 onwards. Note: once this is deployed, you cannot downgrade without data loss. #2436
+* [BUGFIX] Ruler: Ensure temporary rule files with special characters are properly mapped and cleaned up. #2506
 * [BUGFIX] Fixes #2411, Ensure requests are properly routed to the prometheus api embedded in the query if `-server.path-prefix` is set. #2372
 * [BUGFIX] Experimental TSDB: fixed chunk data corruption when querying back series using the experimental blocks storage. #2400
 * [BUGFIX] Cassandra Storage: Fix endpoint TLS host verification. #2109

pkg/ruler/api_test.go

@@ -70,6 +70,62 @@ func TestRuler_rules(t *testing.T) {
 	require.Equal(t, string(expectedResponse), string(body))
 }
 
+func TestRuler_rules_special_characters(t *testing.T) {
+	cfg, cleanup := defaultRulerConfig(newMockRuleStore(mockSpecialCharRules))
+	defer cleanup()
+
+	r, rcleanup := newTestRuler(t, cfg)
+	defer rcleanup()
+	defer services.StopAndAwaitTerminated(context.Background(), r) //nolint:errcheck
+
+	req := httptest.NewRequest("GET", "https://localhost:8080/api/prom/api/v1/rules", nil)
+	req.Header.Add(user.OrgIDHeaderName, "user1")
+	w := httptest.NewRecorder()
+	r.PrometheusRules(w, req)
+
+	resp := w.Result()
+	body, _ := ioutil.ReadAll(resp.Body)
+
+	// Check status code and status response
+	responseJSON := response{}
+	err := json.Unmarshal(body, &responseJSON)
+	require.NoError(t, err)
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+	require.Equal(t, responseJSON.Status, "success")
+
+	// Testing the running rules for user1 in the mock store
+	expectedResponse, _ := json.Marshal(response{
+		Status: "success",
+		Data: &RuleDiscovery{
+			RuleGroups: []*RuleGroup{
+				{
+					Name: ")(_+?/|group1+/?",
+					File: ")(_+?/|namespace1+/?",
+					Rules: []rule{
+						&recordingRule{
+							Name:   "UP_RULE",
+							Query:  "up",
+							Health: "unknown",
+							Type:   "recording",
+						},
+						&alertingRule{
+							Name:   "UP_ALERT",
+							Query:  "up < 1",
+							State:  "inactive",
+							Health: "unknown",
+							Type:   "alerting",
+							Alerts: []*Alert{},
+						},
+					},
+					Interval: 60,
+				},
+			},
+		},
+	})
+
+	require.Equal(t, string(expectedResponse), string(body))
+}
+
 func TestRuler_alerts(t *testing.T) {
 	cfg, cleanup := defaultRulerConfig(newMockRuleStore(mockRules))
 	defer cleanup()

pkg/ruler/mapper.go

@@ -2,6 +2,8 @@ package ruler
 
 import (
 	"crypto/md5"
+	"net/url"
+	"path/filepath"
 	"sort"
 
 	"github.com/go-kit/kit/log"
@@ -33,16 +35,18 @@ func (m *mapper) MapRules(user string, ruleConfigs map[string][]legacy_rulefmt.R
 	anyUpdated := false
 	filenames := []string{}
 
-	// user rule files will be stored as `/<path>/<userid>/filename`
-	path := m.Path + "/" + user + "/"
+	// user rule files will be stored as `/<path>/<userid>/<encoded filename>`
+	path := filepath.Join(m.Path, user)
 	err := m.FS.MkdirAll(path, 0777)
 	if err != nil {
 		return false, nil, err
 	}
 
 	// write all rule configs to disk
 	for filename, groups := range ruleConfigs {
-		fullFileName := path + filename
+		// Store the encoded file name to better handle `/` characters
+		encodedFileName := url.PathEscape(filename)
+		fullFileName := filepath.Join(path, encodedFileName)
 
 		fileUpdated, err := m.writeRuleGroupsIfNewer(groups, fullFileName)
 		if err != nil {
@@ -59,8 +63,16 @@ func (m *mapper) MapRules(user string, ruleConfigs map[string][]legacy_rulefmt.R
 	}
 
 	for _, existingFile := range existingFiles {
-		fullFileName := path + existingFile.Name()
-		ruleGroups := ruleConfigs[existingFile.Name()]
+		fullFileName := filepath.Join(path, existingFile.Name())
+
+		// Ensure the namespace is decoded from a url path encoding to see if it is still required
+		decodedNamespace, err := url.PathUnescape(existingFile.Name())
+		if err != nil {
+			level.Warn(m.logger).Log("msg", "unable to remove rule file on disk", "file", fullFileName, "err", err)
+			continue
+		}
+
+		ruleGroups := ruleConfigs[string(decodedNamespace)]
 
 		if ruleGroups == nil {
 			err = m.FS.Remove(fullFileName)

pkg/ruler/mapper_test.go

@@ -1,6 +1,7 @@
 package ruler
 
 import (
+	"net/url"
 	"os"
 	"testing"
 
@@ -15,8 +16,14 @@ import (
 var (
 	testUser = "user1"
 
+	fileOneEncoded = url.PathEscape("file /one")
+	fileTwoEncoded = url.PathEscape("file /two")
+
+	fileOnePath = "/rules/user1/" + fileOneEncoded
+	fileTwoPath = "/rules/user1/" + fileTwoEncoded
+
 	initialRuleSet = map[string][]legacy_rulefmt.RuleGroup{
-		"file_one": {
+		"file /one": {
 			{
 				Name: "rulegroup_one",
 				Rules: []legacy_rulefmt.Rule{
@@ -39,7 +46,7 @@ var (
 	}
 
 	outOfOrderRuleSet = map[string][]legacy_rulefmt.RuleGroup{
-		"file_one": {
+		"file /one": {
 			{
 				Name: "rulegroup_two",
 				Rules: []legacy_rulefmt.Rule{
@@ -62,7 +69,7 @@ var (
 	}
 
 	updatedRuleSet = map[string][]legacy_rulefmt.RuleGroup{
-		"file_one": {
+		"file /one": {
 			{
 				Name: "rulegroup_one",
 				Rules: []legacy_rulefmt.Rule{
@@ -107,10 +114,10 @@ func Test_mapper_MapRules(t *testing.T) {
 		updated, files, err := m.MapRules(testUser, initialRuleSet)
 		require.True(t, updated)
 		require.Len(t, files, 1)
-		require.Equal(t, "/rules/user1/file_one", files[0])
+		require.Equal(t, fileOnePath, files[0])
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
 	})
@@ -121,7 +128,7 @@ func Test_mapper_MapRules(t *testing.T) {
 		require.Len(t, files, 1)
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
 	})
@@ -132,7 +139,7 @@ func Test_mapper_MapRules(t *testing.T) {
 		require.Len(t, files, 1)
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
 	})
@@ -141,18 +148,18 @@ func Test_mapper_MapRules(t *testing.T) {
 		updated, files, err := m.MapRules(testUser, updatedRuleSet)
 		require.True(t, updated)
 		require.Len(t, files, 1)
-		require.Equal(t, "/rules/user1/file_one", files[0])
+		require.Equal(t, fileOnePath, files[0])
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
 	})
 }
 
 var (
 	twoFilesRuleSet = map[string][]legacy_rulefmt.RuleGroup{
-		"file_one": {
+		"file /one": {
 			{
 				Name: "rulegroup_one",
 				Rules: []legacy_rulefmt.Rule{
@@ -172,7 +179,7 @@ var (
 				},
 			},
 		},
-		"file_two": {
+		"file /two": {
 			{
 				Name: "rulegroup_one",
 				Rules: []legacy_rulefmt.Rule{
@@ -186,7 +193,7 @@ var (
 	}
 
 	twoFilesUpdatedRuleSet = map[string][]legacy_rulefmt.RuleGroup{
-		"file_one": {
+		"file /one": {
 			{
 				Name: "rulegroup_one",
 				Rules: []legacy_rulefmt.Rule{
@@ -206,7 +213,7 @@ var (
 				},
 			},
 		},
-		"file_two": {
+		"file /two": {
 			{
 				Name: "rulegroup_one",
 				Rules: []legacy_rulefmt.Rule{
@@ -220,7 +227,7 @@ var (
 	}
 
 	twoFilesDeletedRuleSet = map[string][]legacy_rulefmt.RuleGroup{
-		"file_one": {
+		"file /one": {
 			{
 				Name: "rulegroup_one",
 				Rules: []legacy_rulefmt.Rule{
@@ -256,10 +263,10 @@ func Test_mapper_MapRulesMultipleFiles(t *testing.T) {
 		updated, files, err := m.MapRules(testUser, initialRuleSet)
 		require.True(t, updated)
 		require.Len(t, files, 1)
-		require.Equal(t, "/rules/user1/file_one", files[0])
+		require.Equal(t, fileOnePath, files[0])
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
 	})
@@ -268,14 +275,14 @@ func Test_mapper_MapRulesMultipleFiles(t *testing.T) {
 		updated, files, err := m.MapRules(testUser, twoFilesRuleSet)
 		require.True(t, updated)
 		require.Len(t, files, 2)
-		require.True(t, sliceContains(t, "/rules/user1/file_one", files))
-		require.True(t, sliceContains(t, "/rules/user1/file_two", files))
+		require.True(t, sliceContains(t, fileOnePath, files))
+		require.True(t, sliceContains(t, fileTwoPath, files))
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
-		exists, err = afero.Exists(m.FS, "/rules/user1/file_two")
+		exists, err = afero.Exists(m.FS, fileTwoPath)
 		require.True(t, exists)
 		require.NoError(t, err)
 	})
@@ -284,14 +291,14 @@ func Test_mapper_MapRulesMultipleFiles(t *testing.T) {
 		updated, files, err := m.MapRules(testUser, twoFilesUpdatedRuleSet)
 		require.True(t, updated)
 		require.Len(t, files, 2)
-		require.True(t, sliceContains(t, "/rules/user1/file_one", files))
-		require.True(t, sliceContains(t, "/rules/user1/file_two", files))
+		require.True(t, sliceContains(t, fileOnePath, files))
+		require.True(t, sliceContains(t, fileTwoPath, files))
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
-		exists, err = afero.Exists(m.FS, "/rules/user1/file_two")
+		exists, err = afero.Exists(m.FS, fileTwoPath)
 		require.True(t, exists)
 		require.NoError(t, err)
 	})
@@ -300,19 +307,72 @@ func Test_mapper_MapRulesMultipleFiles(t *testing.T) {
 		updated, files, err := m.MapRules(testUser, twoFilesDeletedRuleSet)
 		require.True(t, updated)
 		require.Len(t, files, 1)
-		require.Equal(t, "/rules/user1/file_one", files[0])
+		require.Equal(t, fileOnePath, files[0])
 		require.NoError(t, err)
 
-		exists, err := afero.Exists(m.FS, "/rules/user1/file_one")
+		exists, err := afero.Exists(m.FS, fileOnePath)
 		require.True(t, exists)
 		require.NoError(t, err)
-		exists, err = afero.Exists(m.FS, "/rules/user1/file_two")
+		exists, err = afero.Exists(m.FS, fileTwoPath)
 		require.False(t, exists)
 		require.NoError(t, err)
 	})
 
 }
 
+var (
+	specialCharFile        = "+A_/ReallyStrange<>NAME:SPACE/?"
+	specialCharFileEncoded = url.PathEscape(specialCharFile)
+	specialCharFilePath    = "/rules/user1/" + specialCharFileEncoded
+
+	specialCharactersRuleSet = map[string][]legacy_rulefmt.RuleGroup{
+		specialCharFile: {
+			{
+				Name: "rulegroup_one",
+				Rules: []legacy_rulefmt.Rule{
+					{
+						Record: "example_rule",
+						Expr:   "example_expr",
+					},
+				},
+			},
+		},
+	}
+)
+
+func Test_mapper_MapRulesSpecialCharNamespace(t *testing.T) {
+	l := log.NewLogfmtLogger(os.Stdout)
+	l = level.NewFilter(l, level.AllowInfo())
+	m := &mapper{
+		Path:   "/rules",
+		FS:     afero.NewMemMapFs(),
+		logger: l,
+	}
+
+	t.Run("create special characters rulegroup", func(t *testing.T) {
+		updated, files, err := m.MapRules(testUser, specialCharactersRuleSet)
+		require.NoError(t, err)
+		require.True(t, updated)
+		require.Len(t, files, 1)
+		require.Equal(t, specialCharFilePath, files[0])
+
+		exists, err := afero.Exists(m.FS, specialCharFilePath)
+		require.NoError(t, err)
+		require.True(t, exists)
+	})
+
+	t.Run("delete special characters rulegroup", func(t *testing.T) {
+		updated, files, err := m.MapRules(testUser, map[string][]legacy_rulefmt.RuleGroup{})
+		require.NoError(t, err)
+		require.True(t, updated)
+		require.Len(t, files, 0)
+
+		exists, err := afero.Exists(m.FS, specialCharFilePath)
+		require.NoError(t, err)
+		require.False(t, exists)
+	})
+}
+
 func sliceContains(t *testing.T, find string, in []string) bool {
 	t.Helper()
 

pkg/ruler/ruler.go

@@ -530,10 +530,17 @@ func (r *Ruler) getLocalRules(userID string) ([]*GroupStateDesc, error) {
 
 	for _, group := range groups {
 		interval := group.Interval()
+
+		// The mapped filename is url path escaped encoded to make handling `/` characters easier
+		decodedNamespace, err := url.PathUnescape(strings.TrimPrefix(group.File(), prefix))
+		if err != nil {
+			return nil, errors.Wrap(err, "unable to decode rule filename")
+		}
+
 		groupDesc := &GroupStateDesc{
 			Group: &rules.RuleGroupDesc{
 				Name:      group.Name(),
-				Namespace: strings.TrimPrefix(group.File(), prefix),
+				Namespace: string(decodedNamespace),
 				Interval:  interval,
 				User:      userID,
 			},