sqlproxyccl: allow denylist entries that do not expire

Previously, we assumed that all denylist entries have an expiration key. When denylist entries do not specify an expiration key, the entries are marked as expired right away since their values default to the zero instant time. This might be cumbersome for operators to specify an expiration when the intention was to not allow the rule to expire at all. This patch changes the behavior of the denylist such that entries without any expiration keys represent rules that do not expire. Release note: None
jaylim-crl · Jun 15, 2021 · 7b778b3 · 7b778b3
1 parent 3819f5e
commit 7b778b3
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 66 deletions.
diff --git a/pkg/ccl/sqlproxyccl/denylist/file.go b/pkg/ccl/sqlproxyccl/denylist/file.go
@@ -155,7 +155,8 @@ func (dl *Denylist) Denied(entity DenyEntity) (*Entry, error) {
 	dl.mu.RLock()
 	defer dl.mu.RUnlock()
 
-	if ent, ok := dl.mu.entries[entity]; ok && !ent.Expiration.Before(dl.timeSource.Now()) {
+	if ent, ok := dl.mu.entries[entity]; ok &&
+		(ent.Expiration.IsZero() || !ent.Expiration.Before(dl.timeSource.Now())) {
 		return &Entry{ent.Reason}, nil
 	}
 	return nil, nil

diff --git a/pkg/ccl/sqlproxyccl/denylist/file_test.go b/pkg/ccl/sqlproxyccl/denylist/file_test.go
@@ -28,11 +28,10 @@ func TestDenyListFileParsing(t *testing.T) {
 		cases := []struct {
 			t        Type
 			expected string
-		}{{
-			IPAddrType, "ip",
-		}, {
-			ClusterType, "cluster",
-		}}
+		}{
+			{IPAddrType, "ip"},
+			{ClusterType, "cluster"},
+		}
 		for _, tc := range cases {
 			s, err := tc.t.MarshalYAML()
 			require.NoError(t, err)
@@ -44,26 +43,14 @@ func TestDenyListFileParsing(t *testing.T) {
 		cases := []struct {
 			raw      string
 			expected Type
-		}{{
-			"ip", IPAddrType,
-		}, {
-			"IP", IPAddrType,
-		},
-			{
-				"Ip", IPAddrType,
-			},
-			{
-				"Cluster", ClusterType,
-			},
-			{
-				"cluster", ClusterType,
-			},
-			{
-				"CLUSTER", ClusterType,
-			},
-			{
-				"random text", UnknownType,
-			},
+		}{
+			{"ip", IPAddrType},
+			{"IP", IPAddrType},
+			{"Ip", IPAddrType},
+			{"Cluster", ClusterType},
+			{"cluster", ClusterType},
+			{"CLUSTER", ClusterType},
+			{"random text", UnknownType},
 		}
 		for _, tc := range cases {
 			var parsed Type
@@ -85,31 +72,34 @@ func TestDenyListFileParsing(t *testing.T) {
 			expected map[DenyEntity]*DenyEntry
 		}{
 			{"text: ", emptyMap},
-			{"random text\n\n\nmore random text",
-				emptyMap},
+			{"random text\n\n\nmore random text", emptyMap},
 			{defaultEmptyDenylistText, emptyMap},
 			{"SequenceNumber: 7", emptyMap},
 			{
-				// old denylist format, making sure it won't break new denylist code
+				// Old denylist format; making sure it won't break new denylist code.
 				`
-			SequenceNumber: 8
-			1.1.1.1: some reason
-			61: another reason`,
+SequenceNumber: 8
+1.1.1.1: some reason
+61: another reason`,
 				emptyMap,
-			}, {
+			},
+			{
 				fmt.Sprintf(`
-             SequenceNumber: 9
-             denylist:
-              - entity: {"item":"1.2.3.4", "type": "ip"}
-                expiration: %s
-                reason: over quota
-`, expirationTimeString),
-				map[DenyEntity]*DenyEntry{{"1.2.3.4", IPAddrType}: {
-					DenyEntity{"1.2.3.4", IPAddrType},
-					expirationTime,
-					"over quota",
+SequenceNumber: 9
+denylist:
+- entity: {"item":"1.2.3.4", "type": "ip"}
+  expiration: %s
+  reason: over quota`,
+					expirationTimeString,
+				),
+				map[DenyEntity]*DenyEntry{
+					{"1.2.3.4", IPAddrType}: {
+						DenyEntity{"1.2.3.4", IPAddrType},
+						expirationTime,
+						"over quota",
+					},
 				},
-				}},
+			},
 		}
 
 		// use cancel to prevent leaked goroutines from file watches
@@ -162,57 +152,83 @@ func TestDenylistLogic(t *testing.T) {
 		outcome *Entry
 	}
 
-	// This is a time evolution of a denylist
+	// This is a time evolution of a denylist.
 	testCases := []struct {
 		input string
 		time  time.Time
 		specs []denyIOSpec
 	}{
+		// Blocks IP address only.
 		{
 			fmt.Sprintf(`
-             SequenceNumber: 9
-             denylist:
-              - entity: {"item": "1.2.3.4", "type": "IP"}
-                expiration: %s
-                reason: over quota`, expirationTimeString),
+SequenceNumber: 9
+denylist:
+- entity: {"item": "1.2.3.4", "type": "IP"}
+  expiration: %s
+  reason: over quota`,
+				expirationTimeString,
+			),
 			startTime.Add(10 * time.Second),
 			[]denyIOSpec{
 				{DenyEntity{"1.2.3.4", IPAddrType}, &Entry{"over quota"}},
 				{DenyEntity{"61", ClusterType}, nil},
 				{DenyEntity{"1.2.3.5", IPAddrType}, nil},
 			},
 		},
+		// Blocks both IP address and tenant cluster.
 		{
 			fmt.Sprintf(`
-             SequenceNumber: 10
-             denylist:
-              - entity: {"item": "1.2.3.4", "type": "IP"}
-                expiration: %s
-                reason: over quota
-              - entity: {"item": 61, "type": "Cluster"}
-                expiration: %s
-                reason: splunk pipeline`, expirationTimeString, expirationTimeString),
+SequenceNumber: 10
+denylist:
+- entity: {"item": "1.2.3.4", "type": "IP"}
+  expiration: %s
+  reason: over quota
+- entity: {"item": 61, "type": "Cluster"}
+  expiration: %s
+  reason: splunk pipeline`,
+				expirationTimeString,
+				expirationTimeString,
+			),
 			startTime.Add(20 * time.Second),
 			[]denyIOSpec{
 				{DenyEntity{"1.2.3.4", IPAddrType}, &Entry{"over quota"}},
 				{DenyEntity{"61", ClusterType}, &Entry{"splunk pipeline"}},
 				{DenyEntity{"1.2.3.5", IPAddrType}, nil},
-			}},
+			},
+		},
+		// Entry that has expired.
 		{
 			fmt.Sprintf(`
-             SequenceNumber: 11
-             denylist:
-              - entity: {"item": "1.2.3.4", "type": "ip"}
-                expiration: %s
-                reason: over quota`, expirationTimeString),
+SequenceNumber: 11
+denylist:
+- entity: {"item": "1.2.3.4", "type": "ip"}
+  expiration: %s
+  reason: over quota`,
+				expirationTimeString,
+			),
 			futureTime,
 			[]denyIOSpec{
 				{DenyEntity{"1.2.3.4", IPAddrType}, nil},
 				{DenyEntity{"61", ClusterType}, nil},
 				{DenyEntity{"1.2.3.5", IPAddrType}, nil},
-			}},
+			},
+		},
+		// Entry without any expiration.
+		{
+			`
+SequenceNumber: 11
+denylist:
+- entity: {"item": "1.2.3.4", "type": "ip"}
+  reason: over quota`,
+			futureTime,
+			[]denyIOSpec{
+				{DenyEntity{"1.2.3.4", IPAddrType}, &Entry{"over quota"}},
+				{DenyEntity{"61", ClusterType}, nil},
+				{DenyEntity{"1.2.3.5", IPAddrType}, nil},
+			},
+		},
 	}
-	// use cancel to prevent leaked goroutines from file watches
+	// Use cancel to prevent leaked goroutines from file watches.
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	tempDir := t.TempDir()