Allow multiple matchers for NetObserv multi-tenancy (NETOBSERV-171) #440

jotak · 2023-01-18T11:33:42Z

JIRA: https://issues.redhat.com/browse/NETOBSERV-171

In short, without this PR, only one matcher injection for multi-tenancy is allowed: e.g. namespace=foo
NetObserv is an observability solution dedicated to the network, that stores flow logs in Loki. Implementing multi-tenancy for NetObserv requires injecting more complex matchers for handling flow logs sources and destinations: e.g.
src-namespace=foo OR dst-namespace=foo (as a single query injection).

Changes:

Updated result data from OPA Authz (OPA breaking change)
Query injection needs to handle several matchers, and logical OR
Fix string escaping issue in Loki reencoder
Add a few tests
As an aside, allow building with podman

Related PRs:

jotak · 2023-01-18T11:33:55Z

/cc @periklis

periklis · 2023-01-18T11:48:06Z

api/logs/v1/labels_enforcer.go

 	expr.Walk(func(expr interface{}) {
 		switch le := expr.(type) {
 		case *logqlv2.StreamMatcherExpr:


FWIW considering the observatorium/opa-openshift#15 draft PR it looks like the opa.matcher (considering we can only do vector or vector with stream label matcher in LogQL) is overloaded. The label.Matchers should maybe include a field that tells if they are going to be appended as label selectors in StreamMatcherExpr or as line filters., e.g.:

expr.Walk(func(expr interface{}) { switch le := expr.(type) { case *logqlv2.StreamMatcherExpr: le.AppendMatchers(lm) case *logqlv2.LineFilterExpr: // I believe our syntax does not support this case yet if mInfo.LogicalOp == logicalOr { le.AppendORMatchers(mInfo.Matchers) } else { le.AppendMatchers(mInfo.Matchers) } default: // Do nothing } })

yes, or we do line filters anyway, regardless if it's vectors or not.

Looking on the StreamMatchersExpr.String() implementation below I see that you circuit break with a closing curly brace in case of an or-Matcher and move forward with a line filter. This looks very to me. In the above sample I propose to clearly render or-Matchers as line filters, e.g. by appending a new line filter with or-s at the end of the query.

PS: I mention Line Filter above, but I am not sure if it the right expression. IIRC logical expressions on matchers are supported only on label filters and not line filters. Please consider the parser tests for the right expression.

I tried your suggested approach at first but I hit an issue: if I remember correctly, Line filters "walker" is only invoked if there are actually line filters in the initial expression, it isn't invoked at all if there are none. Or maybe I didn't proceed like what you were thinking about... I can retry and let you see the issue if you want

I think the approach you suggest would work but requires a little more refactoring of the traversal pattern

logql/v2/ast_test.go

logql/v2/parser_test.go

JIRA: https://issues.redhat.com/browse/NETOBSERV-171 - Updated result data from OPA Authz - Query injection needs to handle several matchers, and logical OR (this is WIP) - Update tests

It produces valid logql, however the parser is currently failing to parse it :(

- Use strconv.Quote to preserve string escaping - Add test - Makefile: allow building with podman

jotak · 2023-02-17T15:29:25Z

@periklis this one is ready for review

periklis

In general the approach looks good to me. However I suggest we keep rendering rendering stream matchers independently from label filters with logical expressions.

periklis · 2023-02-23T07:13:33Z

api/logs/v1/labels_enforcer.go

 	expr.Walk(func(expr interface{}) {
 		switch le := expr.(type) {
 		case *logqlv2.StreamMatcherExpr:


Looking on the StreamMatchersExpr.String() implementation below I see that you circuit break with a closing curly brace in case of an or-Matcher and move forward with a line filter. This looks very to me. In the above sample I propose to clearly render or-Matchers as line filters, e.g. by appending a new line filter with or-s at the end of the query.

PS: I mention Line Filter above, but I am not sure if it the right expression. IIRC logical expressions on matchers are supported only on label filters and not line filters. Please consider the parser tests for the right expression.

- Grammar has to be updated so that LogQueryExpr doesn't translate into a simple StreamMatcherExpr when there is no lof pipeline - New func LogQueryExpr.AppendPipelineMatchers that injects "or" matchers - update tests

periklis

In general looks good to me. I will add my approve once I have tested this in companion with observatorium-api on our Loki Operator release branches.

periklis · 2023-02-28T13:46:01Z

logql/v2/ast.go

@@ -61,17 +61,15 @@ func (s *StreamMatcherExpr) Walk(fn WalkFn) {
 func (s *StreamMatcherExpr) String() string {
 	var sb strings.Builder

-	sb.WriteString("{")
-
+	sb.WriteRune('{')


What is the value of this? I am just curious.

I assume it's supposed to be more optimized but I admit I have no certitude I could not find a clear statement on that while googling, and doing a quick benchmark isn't conclusive. I can revert then ...

Let's keep it the old way

logql/v2/ast.go

periklis · 2023-02-28T13:49:02Z

logql/v2/parser_test.go

+func TestQuotesEncode(t *testing.T) {
+	expr, err := ParseExpr(`{app="test"}`)
+	require.NoError(t, err)
+	require.Equal(t, `{app="test"}`, expr.String())
+
+	expr, err = ParseExpr("{app=\"test\"}|~`key\":\"val\"`")
+	require.NoError(t, err)
+	require.Equal(t, `{app="test"} |~ "key\":\"val\""`, expr.String())
+}


Can we eliminate testify usage here? This repo is under shared maintenance with many teams and we don't have any vote on changing the way we write tests.

btw: I tend to write flat tests rather than test cases arrays (aka table-driven tests), that's a personal preference, but since the norm here seems to be test cases arrays, I can definitely comply with it.

periklis · 2023-02-28T13:49:28Z

logql/v2/parser_test.go

+		{
+			input: `{first="value"} |~ "key\":\"val\""`,
+			expr: &LogQueryExpr{
+				filter: LogPipelineExpr{
+					&LogFilterExpr{
+						filter: "|~",
+						value:  "key\":\"val\"",
+					},
+				},
+				left: &StreamMatcherExpr{
+					matchers: []*labels.Matcher{
+						{
+							Type:  labels.MatchEqual,
+							Name:  "first",
+							Value: "value",
+						},
+					},
+				},
+			},
+		},


What is the value added by this test case not covered by any of the existing?

It's indeed similar to TestQuotesEncode below except this one tests double-quotes whereas the other one tests backquotes. I can move it to the TestQuotesEncode test, maybe it will be more clear & consistent

periklis · 2023-02-28T13:53:19Z

logql/v2/parser_test.go

+		{
+			input: `{first="value"} | second=~"foo|bar" or third=~"foo|bar"`,
+			expr: &LogQueryExpr{
+				filter: LogPipelineExpr{
+					&LogLabelFilterExpr{
+						labelName:    "second",
+						comparisonOp: "=~",
+						labelValue:   "foo|bar",
+						right: []*LogLabelFilterExpr{
+							{
+								labelName:    "third",
+								comparisonOp: "=~",
+								labelValue:   "foo|bar",
+								isNested:     true,
+								chainOp:      "or",
+							},
+						},
+					},
+				},
+				left: &StreamMatcherExpr{
+					matchers: []*labels.Matcher{
+						{
+							Type:  labels.MatchEqual,
+							Name:  "first",
+							Value: "value",
+						},
+					},
+				},
+			},
+		},


Isn't this test case already covered by the next one? Or is this all about testing the strconv.Quote? If yes, on the last question then we can drop this test as per strconv.Quote having enough tests in the Golang std library.

I think I wrote this test because it was the closest I had from the actual netobserv queries with tenancy matchers. So it helped me while doing the implementation. But you're right that it overlaps with other tests. I can remove it.

periklis

Still looks good to me

periklis

Approving this PR as is right now. Waiting for merge until I get a green light from OpenShift Logging release manager as per main branch being used for multiple releases right now (i.e. OpenShift Logging 5.5, 5.6 and upcoming 5.7).

jotak mentioned this pull request Jan 18, 2023

Allow multiple matchers for NetObserv multi-tenancy (NETOBSERV-171) observatorium/opa-openshift#15

Merged

periklis reviewed Jan 18, 2023

View reviewed changes

jotak mentioned this pull request Jan 18, 2023

operator: Allow multiple matchers for multi-tenancy with Network tenant grafana/loki#8192

Merged

5 tasks

jotak force-pushed the poc-mt branch from aa2cdb2 to 4c21f17 Compare January 31, 2023 10:58

jotak commented Jan 31, 2023

View reviewed changes

logql/v2/ast_test.go Show resolved Hide resolved

jotak commented Jan 31, 2023

View reviewed changes

logql/v2/parser_test.go Outdated Show resolved Hide resolved

jotak force-pushed the poc-mt branch from 4c21f17 to 7eaed78 Compare January 31, 2023 13:05

jotak added 3 commits February 17, 2023 16:06

NETOBSERV-171 PoC multi-tenancy (WIP)

0a86a45

JIRA: https://issues.redhat.com/browse/NETOBSERV-171 - Updated result data from OPA Authz - Query injection needs to handle several matchers, and logical OR (this is WIP) - Update tests

Use LogFilter for "or" matchers

aec0b31

It produces valid logql, however the parser is currently failing to parse it :(

revert change on parser_test

0374b9d

jotak force-pushed the poc-mt branch from 7eaed78 to d18abd8 Compare February 17, 2023 15:06

Fix quotes re-encoding

5c9e872

- Use strconv.Quote to preserve string escaping - Add test - Makefile: allow building with podman

jotak force-pushed the poc-mt branch from d18abd8 to 5c9e872 Compare February 17, 2023 15:17

jotak changed the title ~~NETOBSERV-171 PoC multi-tenancy (WIP)~~ Allow multiple matchers for NetObserv multi-tenancy (NETOBSERV-171) Feb 17, 2023

jotak marked this pull request as ready for review February 17, 2023 15:26

periklis requested changes Feb 23, 2023

View reviewed changes

jotak added 2 commits February 28, 2023 13:04

Update grammar / append OR matchers via LogQueryExpr traversal

bd68805

- Grammar has to be updated so that LogQueryExpr doesn't translate into a simple StreamMatcherExpr when there is no lof pipeline - New func LogQueryExpr.AppendPipelineMatchers that injects "or" matchers - update tests

Update OPA model; renamed matcherOp

bd27b1d

periklis reviewed Feb 28, 2023

View reviewed changes

jotak added 2 commits February 28, 2023 16:40

Remove some overlaping tests

39a01d3

go mod cleanup

48721e9

periklis reviewed Feb 28, 2023

View reviewed changes

Revert using WriteRune

51be12c

periklis approved these changes Mar 1, 2023

View reviewed changes

periklis merged commit 37022fd into observatorium:main Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple matchers for NetObserv multi-tenancy (NETOBSERV-171) #440

Allow multiple matchers for NetObserv multi-tenancy (NETOBSERV-171) #440

jotak commented Jan 18, 2023 •

edited

jotak commented Jan 18, 2023

periklis Jan 18, 2023

jotak Jan 18, 2023

periklis Feb 23, 2023

jotak Feb 27, 2023

jotak Feb 27, 2023

jotak commented Feb 17, 2023

periklis left a comment

periklis Feb 23, 2023

periklis left a comment

periklis Feb 28, 2023

jotak Feb 28, 2023 •

edited

periklis Feb 28, 2023

periklis Feb 28, 2023

jotak Feb 28, 2023

periklis Feb 28, 2023

jotak Feb 28, 2023

periklis Feb 28, 2023

jotak Feb 28, 2023

periklis left a comment

periklis left a comment

Allow multiple matchers for NetObserv multi-tenancy (NETOBSERV-171) #440

Allow multiple matchers for NetObserv multi-tenancy (NETOBSERV-171) #440

Conversation

jotak commented Jan 18, 2023 • edited

jotak commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak commented Feb 17, 2023

periklis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak Feb 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis left a comment

Choose a reason for hiding this comment

periklis left a comment

Choose a reason for hiding this comment

jotak commented Jan 18, 2023 •

edited

jotak Feb 28, 2023 •

edited