diff --git a/api/observability/v1/filter_types.go b/api/observability/v1/filter_types.go index 3134e5ff46..d257633159 100644 --- a/api/observability/v1/filter_types.go +++ b/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"` } diff --git a/bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml b/bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml index 5e392c1a0f..8b70d1cde2 100644 --- a/bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml +++ b/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: diff --git a/config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml b/config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml index 736efb4dcd..d44bc893a5 100644 --- a/config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml +++ b/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: diff --git a/docs/reference/datamodels/viaq/v1.adoc b/docs/reference/datamodels/viaq/v1.adoc index c4bbde6ef8..d410a5f999 100644 --- a/docs/reference/datamodels/viaq/v1.adoc +++ b/docs/reference/datamodels/viaq/v1.adoc @@ -973,7 +973,7 @@ to be sent with the log Records |string -a| Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +a| Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. |====================== @@ -1003,7 +1003,7 @@ to be sent with the log Records ===== Description -Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. ===== Type @@ -1664,7 +1664,7 @@ to be sent with the log Records |string -a| Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +a| Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. |====================== @@ -1694,7 +1694,7 @@ to be sent with the log Records ===== Description -Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. ===== Type @@ -2648,7 +2648,7 @@ to be sent with the log Records |string -a| Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +a| Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. |====================== @@ -2678,7 +2678,7 @@ to be sent with the log Records ===== Description -Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. ===== Type @@ -4267,7 +4267,7 @@ to be sent with the log Records |string -a| Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +a| Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. |====================== @@ -4297,7 +4297,7 @@ to be sent with the log Records ===== Description -Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline +Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. ===== Type diff --git a/internal/generator/vector/filter/drop/filter.go b/internal/generator/vector/filter/drop/filter.go index b26073d35d..5261063b85 100644 --- a/internal/generator/vector/filter/drop/filter.go +++ b/internal/generator/vector/filter/drop/filter.go @@ -15,17 +15,34 @@ func NewFilter(dropTestsSpec []obs.DropTest) *Filter { return &Filter{dropTestsSpec} } +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 diff --git a/internal/generator/vector/filter/drop/filter_test.go b/internal/generator/vector/filter/drop/filter_test.go index ca8405b9a8..350de68fdc 100644 --- a/internal/generator/vector/filter/drop/filter_test.go +++ b/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{ { diff --git a/internal/pkg/generator/forwarder/generator.go b/internal/pkg/generator/forwarder/generator.go index e5b8ca8208..6b4770cc67 100644 --- a/internal/pkg/generator/forwarder/generator.go +++ b/internal/pkg/generator/forwarder/generator.go @@ -6,11 +6,14 @@ import ( obs "github.com/openshift/cluster-logging-operator/api/observability/v1" "github.com/openshift/cluster-logging-operator/internal/api/initialize" + internalobs "github.com/openshift/cluster-logging-operator/internal/api/observability" "github.com/openshift/cluster-logging-operator/internal/factory" forwardergenerator "github.com/openshift/cluster-logging-operator/internal/generator/forwarder" "github.com/openshift/cluster-logging-operator/internal/generator/framework" "github.com/openshift/cluster-logging-operator/internal/utils" + filtervalidation "github.com/openshift/cluster-logging-operator/internal/validations/observability/filters" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/yaml" log "github.com/ViaQ/logerr/v2/log/static" @@ -43,6 +46,19 @@ func Generate(clfYaml string, debugOutput bool, client client.Client) (string, e //} forwarder = initialize.ClusterLogForwarder(forwarder, utils.NoOptions) log.V(3).Info("Initialized ClusterLogForwarder", "cr", forwarder) + + filterMap := internalobs.FilterMap(forwarder.Spec) + var filterErrors []error + for _, filter := range filterMap { + cond := filtervalidation.ValidateFilter(*filter) + if cond.Status == metav1.ConditionFalse { + filterErrors = append(filterErrors, errors.New(cond.Message)) + } + } + if len(filterErrors) > 0 { + return "", fmt.Errorf("invalid filter spec: %w", errors.Join(filterErrors...)) + } + // TODO: enable secrets //secrets := internalobs.FetchSecrets(forwarder.Spec.Outputs, client) secrets := map[string]*corev1.Secret{} diff --git a/internal/validations/observability/filters/validate_filters.go b/internal/validations/observability/filters/validate_filters.go index d372b5bb79..b529d57ef5 100644 --- a/internal/validations/observability/filters/validate_filters.go +++ b/internal/validations/observability/filters/validate_filters.go @@ -54,11 +54,14 @@ func validateDropFilter(filterSpec obs.FilterSpec) (results []string) { if testCondition.Matches != "" && testCondition.NotMatches != "" { testErrors = append(testErrors, "only one of matches or notMatches can be defined at once") } + if strings.ContainsAny(testCondition.Matches, "'\n\r") || strings.ContainsAny(testCondition.NotMatches, "'\n\r") { + testErrors = append(testErrors, "matches/notMatches must not contain single quotes, newlines, or carriage returns") + } // Validate provided regex if testCondition.Matches != "" { _, err = regexp.Compile(testCondition.Matches) } else if testCondition.NotMatches != "" { - _, err = regexp.Compile(testCondition.Matches) + _, err = regexp.Compile(testCondition.NotMatches) } if err != nil { testErrors = append(testErrors, "matches/notMatches must be a valid regular expression.") diff --git a/internal/validations/observability/filters/validate_filters_test.go b/internal/validations/observability/filters/validate_filters_test.go index 8011f11636..b9949dcbc2 100644 --- a/internal/validations/observability/filters/validate_filters_test.go +++ b/internal/validations/observability/filters/validate_filters_test.go @@ -95,6 +95,45 @@ var _ = Describe("[internal][validations][observability][filters]", func() { }, "[matches/notMatches must be a valid regular expression.]", ), + Entry("should fail validation if notMatches contains an invalid regular expression", + []obs.DropTest{ + { + DropConditions: []obs.DropCondition{ + { + Field: ".kubernetes.namespace_name", + NotMatches: "[invalid", + }, + }, + }, + }, + "[matches/notMatches must be a valid regular expression.]", + ), + Entry("should fail validation if matches contains a single quote", + []obs.DropTest{ + { + DropConditions: []obs.DropCondition{ + { + Field: ".kubernetes.namespace_name", + Matches: "foo'bar", + }, + }, + }, + }, + "[matches/notMatches must not contain single quotes, newlines, or carriage returns]", + ), + Entry("should fail validation if notMatches contains a single quote", + []obs.DropTest{ + { + DropConditions: []obs.DropCondition{ + { + Field: ".kubernetes.namespace_name", + NotMatches: "x'''[sources.evil]", + }, + }, + }, + }, + "[matches/notMatches must not contain single quotes, newlines, or carriage returns]", + ), ) DescribeTable("valid drop filter spec", func(dropTests []obs.DropTest) { diff --git a/test/e2e/collection/apivalidations/api_validations_test.go b/test/e2e/collection/apivalidations/api_validations_test.go index 052b124a76..5a692f6f0f 100644 --- a/test/e2e/collection/apivalidations/api_validations_test.go +++ b/test/e2e/collection/apivalidations/api_validations_test.go @@ -126,5 +126,16 @@ var _ = Describe("", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(MatchRegexp("azureMonitor.logType: Required value")) }), + Entry("should pass for drop filter with valid matches", "drop-filter-valid.yaml", func(out string, err error) { + Expect(err).ToNot(HaveOccurred()) + }), + Entry("should fail for drop filter with single quote in matches", "drop-filter-single-quote-matches.yaml", func(out string, err error) { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Invalid value")) + }), + Entry("should fail for drop filter with single quote in notMatches", "drop-filter-single-quote-notmatches.yaml", func(out string, err error) { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Invalid value")) + }), ) }) diff --git a/test/e2e/collection/apivalidations/drop-filter-single-quote-matches.yaml b/test/e2e/collection/apivalidations/drop-filter-single-quote-matches.yaml new file mode 100644 index 0000000000..83ce3beb24 --- /dev/null +++ b/test/e2e/collection/apivalidations/drop-filter-single-quote-matches.yaml @@ -0,0 +1,35 @@ +apiVersion: observability.openshift.io/v1 +kind: ClusterLogForwarder +metadata: + name: clf-validation-test +spec: + filters: + - name: my-drop-filter + type: drop + drop: + - test: + - field: .kubernetes.namespace_name + matches: "foo'bar" + managementState: Managed + outputs: + - name: splunk-aosqe + splunk: + authentication: + token: + key: hecToken + secretName: to-splunk-secret-54980 + index: main + tuning: + compression: none + url: http://to-nowhere.svc:8088 + type: splunk + pipelines: + - filterRefs: + - my-drop-filter + inputRefs: + - application + name: forward-log-splunk + outputRefs: + - splunk-aosqe + serviceAccount: + name: clf-validation-test diff --git a/test/e2e/collection/apivalidations/drop-filter-single-quote-notmatches.yaml b/test/e2e/collection/apivalidations/drop-filter-single-quote-notmatches.yaml new file mode 100644 index 0000000000..8dde61bcb3 --- /dev/null +++ b/test/e2e/collection/apivalidations/drop-filter-single-quote-notmatches.yaml @@ -0,0 +1,35 @@ +apiVersion: observability.openshift.io/v1 +kind: ClusterLogForwarder +metadata: + name: clf-validation-test +spec: + filters: + - name: my-drop-filter + type: drop + drop: + - test: + - field: .kubernetes.namespace_name + notMatches: "x'''[sources.evil]" + managementState: Managed + outputs: + - name: splunk-aosqe + splunk: + authentication: + token: + key: hecToken + secretName: to-splunk-secret-54980 + index: main + tuning: + compression: none + url: http://to-nowhere.svc:8088 + type: splunk + pipelines: + - filterRefs: + - my-drop-filter + inputRefs: + - application + name: forward-log-splunk + outputRefs: + - splunk-aosqe + serviceAccount: + name: clf-validation-test diff --git a/test/e2e/collection/apivalidations/drop-filter-valid.yaml b/test/e2e/collection/apivalidations/drop-filter-valid.yaml new file mode 100644 index 0000000000..6b1b2980ad --- /dev/null +++ b/test/e2e/collection/apivalidations/drop-filter-valid.yaml @@ -0,0 +1,35 @@ +apiVersion: observability.openshift.io/v1 +kind: ClusterLogForwarder +metadata: + name: clf-validation-test +spec: + filters: + - name: my-drop-filter + type: drop + drop: + - test: + - field: .kubernetes.namespace_name + matches: busybox + managementState: Managed + outputs: + - name: splunk-aosqe + splunk: + authentication: + token: + key: hecToken + secretName: to-splunk-secret-54980 + index: main + tuning: + compression: none + url: http://to-nowhere.svc:8088 + type: splunk + pipelines: + - filterRefs: + - my-drop-filter + inputRefs: + - application + name: forward-log-splunk + outputRefs: + - splunk-aosqe + serviceAccount: + name: clf-validation-test diff --git a/test/functional/filters/prune/prune_filter_test.go b/test/functional/filters/prune/prune_filter_test.go index eb8785e5dd..92f87b80e0 100644 --- a/test/functional/filters/prune/prune_filter_test.go +++ b/test/functional/filters/prune/prune_filter_test.go @@ -186,6 +186,7 @@ var _ = Describe("[Functional][Filters][Prune] Prune filter", func() { ".log_type", ".log_source", ".k8s_audit_level", + ".message", }, } }).ToHttpOutput() diff --git a/test/helpers/types/types.go b/test/helpers/types/types.go index 89bd54b095..8b0c6a7ecf 100644 --- a/test/helpers/types/types.go +++ b/test/helpers/types/types.go @@ -165,7 +165,7 @@ type OpenshiftMeta struct { //+optional Labels map[string]string `json:"labels,omitempty"` - //Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline + //Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline //of log records. This was added as a workaround for logstores that do not have nano-second precision. Sequence OptionalInt `json:"sequence,omitempty"` }