(#1952) Fix fact filter parsing

With the addition of GJson to describe fact values the old fact parser is no
longer able to correctly identify facts vs operators in examples like
`'storage.#(name=="vda").size'`.

Here we rewrite the ParseFactFilterString function to no longer used complicated
regular expressions. We will now correctly identify GJson facts and parse the
filters correctly.

In addition we're adding a wider range of tests for ParseFactFilterString so
that we're covered if we were to add more fact filter functionality in the
future.

Author: ploubser

filter/facts/facts.go

@@ -173,18 +173,52 @@ func HasFact(fact string, operator string, value string, file string, log Logger
 }
 
 // ParseFactFilterString parses a fact filter string as typically typed on the CLI
-func ParseFactFilterString(f string) (pf [3]string, err error) {
-	if matched := regexp.MustCompile("^([^ ]+?)[ ]*=>[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], ">=", matched[2]}, nil
-	} else if matched := regexp.MustCompile("^([^ ]+?)[ ]*=<[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], "<=", matched[2]}, nil
-	} else if matched := regexp.MustCompile("^([^ ]+?)[ ]*(<=|>=|<|>|!=|==|=~)[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], matched[2], matched[3]}, nil
-	} else if matched := regexp.MustCompile("^(.+?)[ ]*=[ ]*/(.+)/$").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], "=~", "/" + matched[2] + "/"}, nil
-	} else if matched := regexp.MustCompile("^([^= ]+?)[ ]*=[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], "==", matched[2]}, nil
+func ParseFactFilterString(f string) ([3]string, error) {
+	// strip whatspaces before processing
+	f = strings.ReplaceAll(f, " ", "")
+	validOperators := regexp.MustCompile(`<=|>=|=>|=<|<|>|!=|=~|={1,2}`)
+	operatorIndexes := validOperators.FindAllStringIndex(f, -1)
+	var mainOpIndex []int
+
+	if opCount := len(operatorIndexes); opCount > 1 {
+		// This is a special case where the left operand contains a valid operator.
+		// We skip over everything and use the right most operator.
+		mainOpIndex = operatorIndexes[len(operatorIndexes)-1]
+	} else if opCount == 1 {
+		mainOpIndex = operatorIndexes[0]
 	} else {
 		return [3]string{}, fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", f)
 	}
+
+	op := f[mainOpIndex[0]:mainOpIndex[1]]
+	leftOp := f[:mainOpIndex[0]]
+	rightOp := f[mainOpIndex[1]:]
+
+	// validate that the left and right operands are both valid
+	if len(leftOp) == 0 || len(rightOp) == 0 {
+		return [3]string{}, fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", f)
+	}
+
+	lStartString := string(leftOp[0])
+	rEndString := string(rightOp[len(rightOp)-1])
+	if validOperators.MatchString(lStartString) || validOperators.Match([]byte(rEndString)) {
+		return [3]string{}, fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", f)
+	}
+
+	// transform op and value for processing
+	switch op {
+	case "=":
+		op = "=="
+	case "=<":
+		op = "<="
+	case "=>":
+		op = ">="
+	}
+
+	// finally check for old style regex fact matches
+	if rightOp[0] == '/' && rightOp[len(rightOp)-1] == '/' {
+		op = "=~"
+	}
+
+	return [3]string{leftOp, op, rightOp}, nil
 }

filter/facts/facts_test.go

@@ -5,6 +5,7 @@
 package facts
 
 import (
+	"fmt"
 	"io"
 	"os"
 	"strings"
@@ -26,6 +27,7 @@ func Test(t *testing.T) {
 var _ = Describe("Server/Discovery/Facts", func() {
 	var (
 		t func(fact, op, val string) (bool, error)
+		p func(fact string) ([3]string, error)
 		l *logrus.Entry
 	)
 
@@ -37,6 +39,10 @@ var _ = Describe("Server/Discovery/Facts", func() {
 			return HasFact(fact, op, val, "testdata/fact.yaml", l)
 		}
 
+		p = func(fact string) ([3]string, error) {
+			return (ParseFactFilterString(fact))
+		}
+
 		// fw.Config.FactSourceFile = "testdata/fact.yaml"
 	})
 
@@ -133,4 +139,50 @@ var _ = Describe("Server/Discovery/Facts", func() {
 			Expect(t("fbool", "!=", "false")).To(BeTrue())
 		})
 	})
+
+	Describe("ParseFactFilterString", func() {
+		It("Should transform = comparisons to ==", func() {
+			result, err := p("storage.size=64")
+			Expect(err).To(BeNil())
+			Expect(result).To(BeEquivalentTo([3]string{"storage.size", "==", "64"}))
+		})
+
+		It("Should transform *= comparisons to regex matches", func() {
+			result, err := p("storage.size=/64/")
+			Expect(err).To(BeNil())
+			Expect(result).To(BeEquivalentTo([3]string{"storage.size", "=~", "/64/"}))
+		})
+
+		It("Should transform =< comparisons to <=", func() {
+			result, err := p("storage.size=<64")
+			Expect(err).To(BeNil())
+			Expect(result).To(BeEquivalentTo([3]string{"storage.size", "<=", "64"}))
+		})
+
+		It("Should transform => comparisons to <=", func() {
+			result, err := p("storage.size=>64")
+			Expect(err).To(BeNil())
+			Expect(result).To(BeEquivalentTo([3]string{"storage.size", ">=", "64"}))
+		})
+
+		It("Should walk over array indexing from the external parser nested facts", func() {
+			result, err := p("storage.#(name==\"vda\").size=64")
+			Expect(err).To(BeNil())
+			Expect(result).To(BeEquivalentTo([3]string{"storage.#(name==\"vda\").size", "==", "64"}))
+		})
+
+		It("Should fail on invalid fact filters", func() {
+			badFilters := []string{"foobarbaz", "=foo(bar=baz)", "foo=", "foo(bar=baz)>", "=><=="}
+			for _, filterString := range badFilters {
+				_, err := p(filterString)
+				Expect(err).To(Equal(fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", filterString)))
+			}
+		})
+
+		It("Should handle spaces", func() {
+			result, err := p("storage.size =    64")
+			Expect(err).To(BeNil())
+			Expect(result).To(BeEquivalentTo([3]string{"storage.size", "==", "64"}))
+		})
+	})
 })