NETOBSERV-1990: fix NOT filters with multiple values

NOT filters with multiple values should generate AND queries, not OR
E.g. IP=10.0.0.1,10.0.0.2 generates IP=10.0.0.1 OR IP=10.0.0.2, which is
correct

But, IP!=10.0.0.1,10.0.0.2 should not generate IP!=10.0.0.1 OR IP!=10.0.0.2 (which would be always true) but
IP!=10.0.0.1 AND IP!=10.0.0.2 instead

Reasoning with CIDRs is the same:
IP!=10.0.0.0/8,172.0.0.0/8 translates into IP NOT IN 10.0.0.0/8 AND IP NOT IN 172.0.0.0/8

.golangci.yml

@@ -26,5 +26,8 @@ linters-settings:
       - rangeValCopy
       - indexAlloc
       - deprecatedComment
+    settings:
+      ifElseChain:
+        minThreshold: 3
   cyclop:
     max-complexity: 20

pkg/loki/flow_query.go

@@ -156,21 +156,26 @@ func (q *FlowQueryBuilder) addLineFilters(key string, values []string, not bool,
 // addIPFilters assumes that we are searching for that IP addresses as part
 // of the log line (not in the stream selector labels)
 func (q *FlowQueryBuilder) addIPFilters(key string, values []string, not bool) {
+	if not {
+		// NOT IP filters means we don't want any of the values, ie. IP!=A AND IP!=B instead of IP!=A OR IP!=B
+		for _, value := range values {
+			// empty exact matches should be treated as attribute filters looking for empty IP
+			if value == emptyMatch {
+				q.jsonFilters = append(q.jsonFilters, []filters.LabelFilter{filters.NotStringLabelFilter(key, "")})
+			} else {
+				q.jsonFilters = append(q.jsonFilters, []filters.LabelFilter{filters.NotIPLabelFilter(key, value)})
+			}
+		}
+		return
+	}
+	// Positive case
 	filtersPerKey := make([]filters.LabelFilter, 0, len(values))
 	for _, value := range values {
 		// empty exact matches should be treated as attribute filters looking for empty IP
 		if value == emptyMatch {
-			if not {
-				filtersPerKey = append(filtersPerKey, filters.NotStringLabelFilter(key, ""))
-			} else {
-				filtersPerKey = append(filtersPerKey, filters.StringEqualLabelFilter(key, ""))
-			}
+			filtersPerKey = append(filtersPerKey, filters.StringEqualLabelFilter(key, ""))
 		} else {
-			if not {
-				filtersPerKey = append(filtersPerKey, filters.NotIPLabelFilter(key, value))
-			} else {
-				filtersPerKey = append(filtersPerKey, filters.IPLabelFilter(key, value))
-			}
+			filtersPerKey = append(filtersPerKey, filters.IPLabelFilter(key, value))
 		}
 	}
 	q.jsonFilters = append(q.jsonFilters, filtersPerKey)

pkg/model/filters/logql.go

@@ -252,6 +252,7 @@ func NotContainsKeyLineFilter(key string) LineFilter {
 	}
 }
 
+// NumericLineFilter returns a LineFilter and true if it has an empty match
 func NumericLineFilter(key string, values []string, not, moreThan bool) (LineFilter, bool) {
 	return checkExact(
 		LineFilter{
@@ -271,6 +272,7 @@ func ArrayLineFilter(key string, values []string, not bool) LineFilter {
 	return lf
 }
 
+// StringLineFilterCheckExact returns a LineFilter and true if it has an empty match
 func StringLineFilterCheckExact(key string, values []string, not bool) (LineFilter, bool) {
 	return checkExact(LineFilter{key: key, not: not}, values, typeRegexContains)
 }
@@ -364,21 +366,11 @@ func moreThanRegex(sb *strings.Builder, value string) {
 // under construction (contained in the provided strings.Builder)
 func (f *LineFilter) WriteInto(sb *strings.Builder) {
 	if f.not {
-		if !f.allowEmpty {
-			// the record must contains the field if values are specified
-			// since FLP skip empty fields / zeros values
-			if len(f.values) > 0 {
-				sb.WriteString("|~`\"")
-				sb.WriteString(f.key)
-				sb.WriteString("\"`")
-			}
-		}
-		// then we exclude match results
-		sb.WriteString("!~`")
-	} else {
-		sb.WriteString("|~`")
+		f.writeIntoNot(sb)
+		return
 	}
 
+	sb.WriteString("|~`")
 	if len(f.values) == 0 {
 		// match only the end of KEY if not 'strictKey'
 		// no value will be provided here as we only check if key exists
@@ -392,49 +384,85 @@ func (f *LineFilter) WriteInto(sb *strings.Builder) {
 			if i > 0 {
 				sb.WriteByte('|')
 			}
+			f.writeValueInto(sb, v)
+		}
+	}
+	sb.WriteRune('`')
+}
 
-			// match only the end of KEY + regex VALUE if not 'strictKey'
-			// if numeric, KEY":VALUE,
-			// if string KEY":"VALUE"
-			// ie 'Port' key will match both 'SrcPort":"XXX"' and 'DstPort":"XXX"
-			// VALUE can be quoted for exact match or contains * to inject regex any
-			// For numeric values, exact match is implicit
-			// 	(the trick is to match for the ending coma; it works as long as the filtered field
-			// 	is not the last one (they're in alphabetic order); a less performant alternative
-			// 	but more future-proof/less hacky could be to move that to a json filter, if needed)
-			if f.strictKey {
-				sb.WriteByte('"')
-			}
+// WriteInto transforms a LineFilter to its corresponding part of a LogQL query
+// under construction (contained in the provided strings.Builder)
+func (f *LineFilter) writeIntoNot(sb *strings.Builder) {
+	if !f.allowEmpty {
+		// the record must contains the field if values are specified
+		// since FLP skip empty fields / zeros values
+		if len(f.values) > 0 {
+			sb.WriteString("|~`\"")
 			sb.WriteString(f.key)
-			sb.WriteString(`":`)
-			switch v.valueType {
-			case typeNumber, typeRegex:
-				if f.moreThan {
-					moreThanRegex(sb, v.value)
-				} else {
-					sb.WriteString(v.value)
-				}
-				// a number or regex can be followed by } if it's the last property of a JSON document
-				sb.WriteString("[,}]")
-			case typeBool:
-				sb.WriteString(v.value)
-			case typeString, typeIP:
-				// exact matches are specified as just strings
-				sb.WriteByte('"')
-				sb.WriteString(valueReplacer.Replace(v.value))
-				sb.WriteByte('"')
-			// contains-match are specified as regular expressions
-			case typeRegexContains:
-				sb.WriteString(`"(?i)[^"]*`)
-				sb.WriteString(valueReplacer.Replace(v.value))
-				sb.WriteString(`.*"`)
-			// for array, we ensure it starts by [ and ends by ]
-			case typeRegexArrayContains:
-				sb.WriteString(`\[(?i)[^]]*`)
-				sb.WriteString(valueReplacer.Replace(v.value))
-				sb.WriteString(`[^]]*]`)
-			}
+			sb.WriteString("\"`")
 		}
 	}
-	sb.WriteRune('`')
+
+	if len(f.values) == 0 {
+		// then we exclude match results
+		sb.WriteString("!~`")
+
+		// match only the end of KEY if not 'strictKey'
+		// no value will be provided here as we only check if key exists
+		if f.strictKey {
+			sb.WriteByte('"')
+		}
+		sb.WriteString(f.key)
+		sb.WriteString("\"`")
+	} else {
+		for _, v := range f.values {
+			sb.WriteString("!~`")
+			f.writeValueInto(sb, v)
+			sb.WriteRune('`')
+		}
+	}
+}
+
+func (f *LineFilter) writeValueInto(sb *strings.Builder, v lineMatch) {
+	// match only the end of KEY + regex VALUE if not 'strictKey'
+	// if numeric, KEY":VALUE,
+	// if string KEY":"VALUE"
+	// ie 'Port' key will match both 'SrcPort":"XXX"' and 'DstPort":"XXX"
+	// VALUE can be quoted for exact match or contains * to inject regex any
+	// For numeric values, exact match is implicit
+	// 	(the trick is to match for the ending coma; it works as long as the filtered field
+	// 	is not the last one (they're in alphabetic order); a less performant alternative
+	// 	but more future-proof/less hacky could be to move that to a json filter, if needed)
+	if f.strictKey {
+		sb.WriteByte('"')
+	}
+	sb.WriteString(f.key)
+	sb.WriteString(`":`)
+	switch v.valueType {
+	case typeNumber, typeRegex:
+		if f.moreThan {
+			moreThanRegex(sb, v.value)
+		} else {
+			sb.WriteString(v.value)
+		}
+		// a number or regex can be followed by } if it's the last property of a JSON document
+		sb.WriteString("[,}]")
+	case typeBool:
+		sb.WriteString(v.value)
+	case typeString, typeIP:
+		// exact matches are specified as just strings
+		sb.WriteByte('"')
+		sb.WriteString(valueReplacer.Replace(v.value))
+		sb.WriteByte('"')
+	// contains-match are specified as regular expressions
+	case typeRegexContains:
+		sb.WriteString(`"(?i)[^"]*`)
+		sb.WriteString(valueReplacer.Replace(v.value))
+		sb.WriteString(`.*"`)
+	// for array, we ensure it starts by [ and ends by ]
+	case typeRegexArrayContains:
+		sb.WriteString(`\[(?i)[^]]*`)
+		sb.WriteString(valueReplacer.Replace(v.value))
+		sb.WriteString(`[^]]*]`)
+	}
 }

pkg/model/filters/logql_test.go

@@ -74,3 +74,22 @@ func TestWriteInto_7654(t *testing.T) {
 		assert.Equal(t, reg.MatchString(val), i >= 7654, fmt.Sprintf("Value: %d", i))
 	}
 }
+
+func TestMultiStrings(t *testing.T) {
+	lf, ok := StringLineFilterCheckExact("foo", []string{`"a"`, `"b"`}, false)
+	assert.False(t, ok)
+	sb := strings.Builder{}
+	lf.WriteInto(&sb)
+	assert.Equal(t, "|~"+backtick(`foo":"a"|foo":"b"`), sb.String())
+
+	// Repeat with "not" (here we expect foo being neither a nor b)
+	lf, ok = StringLineFilterCheckExact("foo", []string{`"a"`, `"b"`}, true)
+	assert.False(t, ok)
+	sb = strings.Builder{}
+	lf.WriteInto(&sb)
+	assert.Equal(t, "|~"+backtick(`"foo"`)+"!~"+backtick(`foo":"a"`)+"!~"+backtick(`foo":"b"`), sb.String())
+}
+
+func backtick(str string) string {
+	return "`" + str + "`"
+}

pkg/server/server_flows_test.go

@@ -64,6 +64,12 @@ func TestLokiFiltering(t *testing.T) {
 		outputQueries: []string{
 			"?query={app=\"netobserv-flowcollector\"}|~`SrcK8S_Name\":\"(?i)[^\"]*name1.*\"|SrcK8S_Name\":\"(?i)[^\"]*name2.*\"`",
 		},
+	}, {
+		name:      "NOT line filter same key",
+		inputPath: "?filters=" + url.QueryEscape(`SrcK8S_Name!="name1","name2"`),
+		outputQueries: []string{
+			"?query={app=\"netobserv-flowcollector\"}|~`\"SrcK8S_Name\"`!~`SrcK8S_Name\":\"name1\"`!~`SrcK8S_Name\":\"name2\"`",
+		},
 	}, {
 		name:      "OR label filter same key",
 		inputPath: "?filters=" + url.QueryEscape("SrcK8S_Namespace=ns1,ns2"),
@@ -90,6 +96,12 @@ func TestLokiFiltering(t *testing.T) {
 		outputQueries: []string{
 			"?query={app=\"netobserv-flowcollector\"}|json|SrcAddr=ip(\"10.128.0.1\")+or+SrcAddr=ip(\"10.128.0.2\")",
 		},
+	}, {
+		name:      "NOT IP filters",
+		inputPath: "?filters=" + url.QueryEscape(`SrcAddr!=10.128.0.1,10.128.0.2`),
+		outputQueries: []string{
+			"?query={app=\"netobserv-flowcollector\"}|json|SrcAddr!=ip(\"10.128.0.1\")|SrcAddr!=ip(\"10.128.0.2\")",
+		},
 	}, {
 		name:      "Several OR filters",
 		inputPath: "?filters=" + url.QueryEscape("SrcPort=8080|SrcAddr=10.128.0.1|SrcK8S_Namespace=default"),

web/src/model/__tests__/filters.spec.ts

@@ -1,6 +1,7 @@
 import { FilterDefinitionSample } from '../../components/__tests-data__/filters';
 import { findFilter } from '../../utils/filter-definitions';
 import { doesIncludeFilter, Filter, filtersEqual } from '../filters';
+import { filtersToString } from '../flow-query';
 
 describe('doesIncludeFilter', () => {
   const srcNameFilter = findFilter(FilterDefinitionSample, 'src_name')!;
@@ -18,6 +19,11 @@ describe('doesIncludeFilter', () => {
     }
   ];
 
+  it('should encode as', () => {
+    const asString = filtersToString(activeFilters, false);
+    expect(asString).toEqual(encodeURIComponent('SrcK8S_Name=abc,def&DstK8S_Name!=abc,def'));
+  });
+
   it('should not include filter due to different key', () => {
     const isIncluded = doesIncludeFilter(activeFilters, { def: findFilter(FilterDefinitionSample, 'protocol')! }, [
       { v: 'abc' },