fix(filter:drop): add validation to restrict inclusion of single quote to condition

api/observability/v1/filter_types.go

@@ -119,6 +119,7 @@ type DropCondition struct {
 	// Must define only one of matches OR notMatches
 	//
 	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Pattern:=`^[^'\n\r]*$`
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Drop Match Expression"
 	Matches string `json:"matches,omitempty"`
 
@@ -127,6 +128,7 @@ type DropCondition struct {
 	// Must define only one of matches or notMatches
 	//
 	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Pattern:=`^[^'\n\r]*$`
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Keep Match Expression"
 	NotMatches string `json:"notMatches,omitempty"`
 }

bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml

@@ -1115,12 +1115,14 @@ spec:
                                     A regular expression that the field will match.
                                     If the value of the field defined in the DropTest matches the regular expression, the log record will be dropped.
                                     Must define only one of matches OR notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                                 notMatches:
                                   description: |-
                                     A regular expression that the field does not match.
                                     If the value of the field defined in the DropTest does not match the regular expression, the log record will be dropped.
                                     Must define only one of matches or notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                               type: object
                               x-kubernetes-validations:

config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml

@@ -1115,12 +1115,14 @@ spec:
                                     A regular expression that the field will match.
                                     If the value of the field defined in the DropTest matches the regular expression, the log record will be dropped.
                                     Must define only one of matches OR notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                                 notMatches:
                                   description: |-
                                     A regular expression that the field does not match.
                                     If the value of the field defined in the DropTest does not match the regular expression, the log record will be dropped.
                                     Must define only one of matches or notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                               type: object
                               x-kubernetes-validations:

docs/reference/datamodels/viaq/v1.adoc

@@ -518,7 +518,7 @@ Type:: object
 |labels
 |object a|  *(optional)* Labels is a set of common, static labels that were spec'd for log forwarding to be sent with the log Records
 |sequence
-|string a|  Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
+|string a|  Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 |======================
 
 ==== .openshift.cluster_id
@@ -536,7 +536,7 @@ Type:: object
 
 ==== .openshift.sequence
 
-Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 Type:: string
@@ -916,7 +916,7 @@ Type:: object
 |labels
 |object a|  *(optional)* Labels is a set of common, static labels that were spec'd for log forwarding to be sent with the log Records
 |sequence
-|string a|  Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
+|string a|  Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 |======================
 
 ==== .openshift.cluster_id
@@ -934,7 +934,7 @@ Type:: object
 
 ==== .openshift.sequence
 
-Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 Type:: string
@@ -1476,7 +1476,7 @@ Type:: object
 |labels
 |object a|  *(optional)* Labels is a set of common, static labels that were spec'd for log forwarding to be sent with the log Records
 |sequence
-|string a|  Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
+|string a|  Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 |======================
 
 ==== .openshift.cluster_id
@@ -1494,7 +1494,7 @@ Type:: object
 
 ==== .openshift.sequence
 
-Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 Type:: string
@@ -2325,7 +2325,7 @@ Type:: object
 |labels
 |object a|  *(optional)* Labels is a set of common, static labels that were spec'd for log forwarding to be sent with the log Records
 |sequence
-|string a|  Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
+|string a|  Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 |======================
 
 ==== .openshift.cluster_id
@@ -2343,7 +2343,7 @@ Type:: object
 
 ==== .openshift.sequence
 
-Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to establish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 Type:: string

internal/generator/forwarder/generator.go

@@ -13,7 +13,7 @@ import (
 )
 
 type ConfigGenerator struct {
-	conf func(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec, namespace, forwarderName string, resNames factory.ForwarderResourceNames, op utils.Options) *api.Config
+	conf func(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec, namespace, forwarderName string, resNames factory.ForwarderResourceNames, op utils.Options) (*api.Config, error)
 }
 
 func New() *ConfigGenerator {
@@ -24,6 +24,9 @@ func New() *ConfigGenerator {
 }
 
 func (cg *ConfigGenerator) GenerateConf(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec, namespace, forwarderName string, resNames factory.ForwarderResourceNames, op framework.Options) (string, error) {
-	config := cg.conf(secrets, clfspec, namespace, forwarderName, resNames, op)
+	config, err := cg.conf(secrets, clfspec, namespace, forwarderName, resNames, op)
+	if err != nil {
+		return "", err
+	}
 	return toml.Marshal(config)
 }

internal/generator/vector/adapters/pipeline.go

@@ -1,6 +1,7 @@
 package adapters
 
 import (
+	"fmt"
 	"os"
 	"strconv"
 
@@ -23,12 +24,16 @@ type Pipeline struct {
 }
 
 // Transforms creates instances of transforms based upon the pipeline spec
-func (p *Pipeline) Transforms() (tfs api.Transforms) {
-	tfs = api.Transforms{}
+func (p *Pipeline) Transforms() (api.Transforms, error) {
+	tfs := api.Transforms{}
 	for _, pf := range p.Filters {
-		tfs.Add(pf.ID(), pf.Transform())
+		tf := pf.Transform()
+		if tf == nil {
+			return nil, fmt.Errorf("filter %q produced nil transform for pipeline %q", pf.ID(), p.Name())
+		}
+		tfs.Add(pf.ID(), tf)
 	}
-	return tfs
+	return tfs, nil
 }
 
 func NewPipeline(index int, p obs.PipelineSpec, inputs map[string]helpers.InputComponent, outputs map[string]*Output, filters map[string]*InternalFilterSpec, inputSpecs []obs.InputSpec, addPostFilters func(p *Pipeline)) *Pipeline {

internal/generator/vector/adapters/pipeline_test.go

@@ -80,15 +80,49 @@ var _ = Describe("Pipeline adapters", func() {
 				},
 			)
 			Expect(adapter.Filters).To(HaveLen(2), "expected the filter and post-filter to be added to the pipeline")
+			tfs, err := adapter.Transforms()
+			Expect(err).ToNot(HaveOccurred())
 			Expect(api.Transforms{
 				"pipeline_mypipeline_dropfilter_0": transforms.NewRemap("fakeElementVRL", "input_app_in_container_meta"),
 				"pipeline_mypipeline_fakefilter_1": transforms.NewRemap("updatedFromAddPostfilter: mypipeline", "pipeline_mypipeline_dropfilter_0"),
-			}).To(Equal(adapter.Transforms()))
+			}).To(Equal(tfs))
 			Expect(outputMap["referenced"].Inputs()).To(Equal([]string{"pipeline_mypipeline_fakefilter_1"}))
 			Expect(outputMap["notReferenced"].Inputs()).To(BeNil(), "Exp. the unreferenced output to not have the filter as an input")
 		})
 	})
 
+	Describe("#Transforms with nil-returning factory", func() {
+		It("should return an error when a filter factory produces a nil transform", func() {
+			nilFilterMap := map[string]*adapters.InternalFilterSpec{
+				"nilFilter": {
+					FilterSpec: &obs.FilterSpec{
+						Name: "nilFilter",
+						Type: obs.FilterTypeDrop,
+					},
+					Factory: func(inputs ...string) types.Transform {
+						return nil
+					},
+				},
+			}
+			adapter := adapters.NewPipeline(0, obs.PipelineSpec{
+				Name:       "mypipeline",
+				InputRefs:  []string{"app-in"},
+				FilterRefs: []string{"nilFilter"},
+				OutputRefs: []string{"referenced"},
+			}, inputMap,
+				outputMap,
+				nilFilterMap,
+				inputSpecs,
+				func(p *adapters.Pipeline) {},
+			)
+			Expect(adapter.Filters).To(HaveLen(1))
+			_, err := adapter.Transforms()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("nil transform"))
+			Expect(err.Error()).To(ContainSubstring("mypipeline"))
+		})
+	})
+
 	Describe("#NewPipelineFilter", func() {
 
 		It("should add drop filter when spec'd for the pipeline", func() {
@@ -104,9 +138,11 @@ var _ = Describe("Pipeline adapters", func() {
 				func(p *adapters.Pipeline) {},
 			)
 			Expect(adapter.Filters).To(HaveLen(1), "")
+			tfs, err := adapter.Transforms()
+			Expect(err).ToNot(HaveOccurred())
 			Expect(api.Transforms{
 				"pipeline_mypipeline_dropfilter_0": transforms.NewRemap("fakeElementVRL", "input_app_in_container_meta"),
-			}).To(Equal(adapter.Transforms()))
+			}).To(Equal(tfs))
 			Expect(outputMap["referenced"].Inputs()).To(Equal([]string{"pipeline_mypipeline_dropfilter_0"}))
 			Expect(outputMap["notReferenced"].Inputs()).To(BeNil(), "Exp. the unreferenced output to not have the filter as an input")
 

internal/generator/vector/conf/conf.go

@@ -1,6 +1,7 @@
 package conf
 
 import (
+	"fmt"
 	"sort"
 
 	obs "github.com/openshift/cluster-logging-operator/api/observability/v1"
@@ -64,7 +65,7 @@ const (
        output_mykafka_dedot
 */
 //nolint:govet // using declarative style
-func Conf(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec, namespace, forwarderName string, resNames factory.ForwarderResourceNames, op utils.Options) (config *api.Config) {
+func Conf(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec, namespace, forwarderName string, resNames factory.ForwarderResourceNames, op utils.Options) (*api.Config, error) {
 	op[helpers.CLFSpec] = internalobs.ClusterLogForwarderSpec(clfspec)
 
 	// Init inputs, outputs, pipelines
@@ -90,7 +91,7 @@ func Conf(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec
 		pipelineMap[p.Name] = a
 	}
 
-	config = api.NewConfig(func(c *api.Config) {
+	config := api.NewConfig(func(c *api.Config) {
 		Global(c, namespace, forwarderName)
 		c.Sources[InternalMetricsSourceName] = sources.NewInternalMetrics()
 	})
@@ -100,7 +101,11 @@ func Conf(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec
 		config.AddTransforms(transforms)
 	}
 	for _, p := range sortAdapters(pipelineMap) {
-		config.AddTransforms(p.Transforms())
+		transforms, err := p.Transforms()
+		if err != nil {
+			return nil, fmt.Errorf("generating pipeline transforms: %w", err)
+		}
+		config.AddTransforms(transforms)
 	}
 	for _, o := range sortAdapters(outputMap) {
 		sinks, transforms := output.New(o, o.InputIDs, secrets, op)
@@ -109,7 +114,7 @@ func Conf(secrets map[string]*corev1.Secret, clfspec obs.ClusterLogForwarderSpec
 	}
 	config.Transforms[metrics.AddNodenameToMetricTransformName] = metrics.AddNodeNameToMetric([]string{InternalMetricsSourceName})
 	config.Sinks[metrics.PrometheusOutputSinkName] = metrics.PrometheusOutput([]string{metrics.AddNodenameToMetricTransformName}, op)
-	return config
+	return config, nil
 }
 
 // sortAdapters sorts ClusterLogForwarder adapters to ensure consistent generation of component configs

internal/generator/vector/conf/conf_test.go

@@ -61,7 +61,8 @@ var _ = Describe("Testing Complete Config Generation", func() {
 		if op == nil {
 			op = clusterOptions
 		}
-		conf := Conf(secrets, spec, constants.OpenshiftNS, "my-forwarder", factory.ForwarderResourceNames{CommonName: constants.CollectorName}, op)
+		conf, err := Conf(secrets, spec, constants.OpenshiftNS, "my-forwarder", factory.ForwarderResourceNames{CommonName: constants.CollectorName}, op)
+		Expect(err).ToNot(HaveOccurred())
 		Expect(exp).To(EqualConfigFrom(conf))
 	},
 		Entry("with complex spec",

internal/generator/vector/filter/drop/filter.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	log "github.com/ViaQ/logerr/v2/log/static"
+
 	obs "github.com/openshift/cluster-logging-operator/api/observability/v1"
 	"github.com/openshift/cluster-logging-operator/internal/generator/vector/api/transforms"
 	"github.com/openshift/cluster-logging-operator/internal/generator/vector/api/types"
@@ -29,17 +30,34 @@ func New(spec []obs.DropTest, inputs ...string) types.Transform {
 	return transforms.NewFilter(vrl, inputs...)
 }
 
+func buildMatchCondition(field, pattern string, negate bool) (string, error) {
+	if strings.ContainsAny(pattern, "'\n\r") {
+		return "", fmt.Errorf("match pattern must not contain single quotes, newlines, or carriage returns: %q", pattern)
+	}
+	prefix := ""
+	if negate {
+		prefix = "!"
+	}
+	return fmt.Sprintf(`%smatch(to_string(%s) ?? "", r'%s')`, prefix, field, pattern), nil
+}
+
 func (f *Filter) VRL() (string, error) {
 	vrlTests := []string{}
 	for _, test := range f.tests {
 		condList := []string{}
 		for _, cond := range test.DropConditions {
 			field := fmt.Sprintf("._internal%s", cond.Field)
+			var matchExpr string
+			var err error
 			if cond.Matches != "" {
-				condList = append(condList, fmt.Sprintf(`match(to_string(%s) ?? "", r'%s')`, field, cond.Matches))
+				matchExpr, err = buildMatchCondition(field, cond.Matches, false)
 			} else {
-				condList = append(condList, fmt.Sprintf(`!match(to_string(%s) ?? "", r'%s')`, field, cond.NotMatches))
+				matchExpr, err = buildMatchCondition(field, cond.NotMatches, true)
+			}
+			if err != nil {
+				return "", err
 			}
+			condList = append(condList, matchExpr)
 		}
 		// Concatenate the conditions with ANDs and add Vector's error coalescing.
 		// If any errors arise from the match such as, `cond.Field` not being a string or a field

internal/generator/vector/filter/drop/filter_test.go

@@ -10,6 +10,38 @@ import (
 var _ = Describe("drop filter", func() {
 
 	Context("#VRL", func() {
+		It("should reject matches containing single quotes", func() {
+			spec := []obs.DropTest{
+				{
+					DropConditions: []obs.DropCondition{
+						{
+							Field:   ".kubernetes.namespace_name",
+							Matches: "foo'bar",
+						},
+					},
+				},
+			}
+			_, err := NewFilter(spec).VRL()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("single quotes"))
+		})
+
+		It("should reject notMatches containing single quotes", func() {
+			spec := []obs.DropTest{
+				{
+					DropConditions: []obs.DropCondition{
+						{
+							Field:      ".kubernetes.namespace_name",
+							NotMatches: "x'''[sources.evil]",
+						},
+					},
+				},
+			}
+			_, err := NewFilter(spec).VRL()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("single quotes"))
+		})
+
 		It("should generate valid VRL for dropping", func() {
 			spec := []obs.DropTest{
 				{