Index arbitrary labels #317

ericpromislow · 2024-11-07T01:17:39Z

Related to #46333

pkg/resources/virtual/common/common.go

pkg/stores/sqlpartition/listprocessor/processor_test.go

pkg/stores/sqlpartition/queryparser/labels.go

bigkevmcd

Mostly fine, I would much prefer to see some real tests against SQLite rather than the continued use of mocks.

pkg/stores/sqlpartition/listprocessor/processor.go

pkg/resources/virtual/common/common.go

pkg/stores/sqlpartition/listprocessor/processor.go

pkg/stores/sqlpartition/listprocessor/processor_test.go

pkg/stores/sqlpartition/queryparser/selector.go

ericpromislow · 2025-01-06T22:18:08Z

"I would much prefer to see some real tests against SQLite rather than the continued use of mocks."

This is covered in tech-debt item
rancher/rancher#48263
(I edited the subject to include steve)

moio

Thanks @ericpromislow, I do not see big issues either. Most comments from my part are really suggestions and should be quick to address.

Ready to approve as soon as they are dealt with.

👀 from @tomleb would also be helpful

pkg/stores/sqlpartition/listprocessor/processor.go

moio · 2025-01-16T10:04:16Z

pkg/stores/sqlpartition/listprocessor/processor.go

+		return false
+	}
+	s1 := values[0][0:1]
+	if !strings.Contains(`"'`, s1) {


When filtering for exact matches, do we want the string to be single quoted, double quoted, or support both? I am asking because the current behavior seem to be single quote:

https://github.com/rancher/steve/blame/d05a48f9cfa6a43a5fbb840cc5da00a92b229009/pkg/stores/sqlpartition/listprocessor/processor.go#L80

and IIUC this change would make it either single or double quote. Alas, we never documented the choice on the README.

@richard-cox do you have a preference?

@ericpromislow can you amend the README so that whatever we decide is documented please?

Currently

' denotes exact match

no quotes denotes partial match

From my interpretation of docs the proposal would be the following?

' denotes exact match

" denotes partial match

no quotes syntax has been removed?

I think ideally we would continue as currently. all other params, include those that contain text, do not require "

No, quotes are still optional. And I've implemented single quotes indicate an exact match, and double quotes don't (they're just used to delimit the contents, but because of the syntax of k8s labels, they're never required because characters like spaces aren't allowed).

Do we need double quotes at all?

My first thought was it should probably be the operator that decides if its an exact match equality or a substring match. Something like: = is exact (stay closer to k8s), =~ is substring. !~ is not equal substring.

Imo we should have only one way per matching strategy, I don't see a reason to have both double quotes and no quotes denote substring match. That should simplify the logic a bit too.

ok, double quotes are gone and trigger a syntax error

pkg/stores/sqlpartition/queryparser/labels.go

pkg/stores/sqlpartition/queryparser/selector.go

pkg/stores/sqlproxy/proxy_store.go

pkg/stores/sqlpartition/listprocessor/processor.go

pkg/stores/sqlpartition/queryparser/selector.go

moio

Looks good from my perspective.

We definitely need:

@richard-cox's perspective on https://github.com/rancher/steve/pull/317/files?w=0#r1918171835 / https://github.com/rancher/steve/pull/317/files?w=0#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R120-R136
another look from somebody in Frameworks (@tomleb ?) especially from a maintenance perspective

pkg/stores/sqlpartition/queryparser/selector.go

tomleb

Mostly nits / styling, one question about the deepcopy, but overall LGTM

tomleb · 2025-01-20T21:19:48Z

pkg/stores/sqlpartition/queryparser/zz_generated.deepcopy.go

@@ -0,0 +1,48 @@
+//go:build !ignore_autogenerated


WDYT stripping this out as well? We're unlikely to be able to regenerate it if we make changes to Requirements. Do we need it to be deepcopy?

I don't have any opinion on this. All this code came as-is from kubectl.

So imo if we don't have anything to regenerate the file we shouldn't add the autogenerated file ourselves if we don't need it. In this case I don't think we need the deepcopy code (I might be wrong, you'll have to try to remove it see if things still compile), so I'd suggest just removing it.

pkg/stores/sqlproxy/proxy_store.go

tomleb · 2025-01-21T13:20:47Z

pkg/stores/sqlpartition/queryparser/selector.go

+	SingleQuoteToken
+	DoubleQuoteToken


I'm not sure those are necessary as they aren't used anywhere. For example, the tests passed even with them removed.

Yes, dropped in an earlier commit

But I still see them so it looks like they were brought back? Unless I misunderstand. (eg: I'm fairly sure we can remove both SingleQuoteToken and DoubleQuoteToken here.

tomleb · 2025-01-21T13:20:54Z

pkg/stores/sqlpartition/queryparser/selector.go

+	"'":     SingleQuoteToken,
+	"\"":    DoubleQuoteToken,


I'm not sure those are necessary as they aren't used anywhere. For example, the tests passed even with them removed.

Yup, dropped

pkg/stores/sqlpartition/queryparser/selector.go

tomleb · 2025-01-21T13:30:12Z

pkg/stores/sqlpartition/listprocessor/processor.go

+		return false
+	}
+	s1 := values[0][0:1]
+	if !strings.Contains(`"'`, s1) {


My first thought was it should probably be the operator that decides if its an exact match equality or a substring match. Something like: = is exact (stay closer to k8s), =~ is substring. !~ is not equal substring.

Imo we should have only one way per matching strategy, I don't see a reason to have both double quotes and no quotes denote substring match. That should simplify the logic a bit too.

The UI team wasn't sure whether the event fields should go in the empty-string group or in 'events.k8s.io', so let's go with both until/unless specified otherwise.

I see both types of operators being bandied about -- it's easy to support the aliases.

Also reduce the case where there's no namespace function.

…ence.

- Stricter processing of m.l.name keys: Require ending close-bracket for a start-bracket - Comment fix - Moving sql processing from lasso to steve: some changes missed in rebase

For now on only single-quotes (or none where possible) are allowed.

ericpromislow · 2025-01-21T22:05:16Z

My first thought was it should probably be the operator that decides if its an exact match equality or a substring match. Something like: = is exact (stay closer to k8s), =~ is substring. !~ is not equal substring.

Imo we should have only one way per matching strategy, I don't see a reason to have both double quotes and no quotes denote substring match. That should simplify the logic a bit too.

The requirements come form the dashboard, and they're the main consumer of this. So they'll need to weigh in on these operators. But I added the double-quotes as a sort of symmetric thing and they're easily dropped.

moio · 2025-01-22T08:06:37Z

My first thought was it should probably be the operator that decides if its an exact match equality or a substring match. Something like: = is exact (stay closer to k8s), =~ is substring. !~ is not equal substring.

Imo we should have only one way per matching strategy, I don't see a reason to have both double quotes and no quotes denote substring match. That should simplify the logic a bit too.

The requirements come form the dashboard, and they're the main consumer of this. So they'll need to weigh in on these operators. But I added the double-quotes as a sort of symmetric thing and they're easily dropped.

From an architectural perspective “defaulting to Kubernetes standards and extending them when needed” seems like the most sensible thing to do, but I’ll leave the final decision to the UI team (cc @richard-cox).

moio · 2025-01-22T07:50:28Z

pkg/stores/sqlpartition/queryparser/selector.go

+		string(selection.GreaterThan), string(selection.LessThan),
+	}
+	validRequirementOperators = append(binaryOperators, unaryOperators...)
+	labelSelectorRegex        = regexp.MustCompile(`^metadata.labels(?:\.\w[-a-zA-Z0-9_./]*|\[.*])$`)


Per previous discussion:

Suggested change

labelSelectorRegex = regexp.MustCompile(`^metadata.labels(?:\.\w[-a-zA-Z0-9_./]*|\[.*])$`)

labelSelectorRegex = regexp.MustCompile(`^metadata\.labels((\.\w[-a-zA-Z0-9_./]*)|(\[.*\]))$`)

quotes the first literal dot

separates the "dot + identifier" case from the "square brackets with anything inside" case in two parenthesized groups for readability

quotes the ] for readability

https://go.dev/play/p/ybN1HHir7oY

Does that make sense?

richard-cox · 2025-01-22T10:31:16Z

My first thought was it should probably be the operator that decides if its an exact match equality or a substring match. Something like: = is exact (stay closer to k8s), =~ is substring. !~ is not equal substring.

Imo we should have only one way per matching strategy, I don't see a reason to have both double quotes and no quotes denote substring match. That should simplify the logic a bit too.

The requirements come form the dashboard, and they're the main consumer of this. So they'll need to weigh in on these operators. But I added the double-quotes as a sort of symmetric thing and they're easily dropped.

From an architectural perspective “defaulting to Kubernetes standards and extending them when needed” seems like the most sensible thing to do, but I’ll leave the final decision to the UI team (cc @richard-cox).

Dashboard side as long as we can execute equals or not equals operators and exact or partial matching of the value we're pretty free on the syntax. Implementation side the usage is abstracted away from the api syntax, so it's an easy update either way

Changing the operator will make the type of match much more obvious. I had some minor concerns with tilde (this used to be a reserved character relating to the home directory and used when locating assets), though looks fine now https://datatracker.ietf.org/doc/html/rfc3986#section-2.3

For url brevity i would go with the following

= exact
~ partial
!= not exact
!~ not partial

ericpromislow requested a review from a team as a code owner November 7, 2024 01:17

ericpromislow marked this pull request as draft November 7, 2024 01:17

ericpromislow mentioned this pull request Nov 7, 2024

Support database queries on arbitrary labels rancher/lasso#117

Merged

bigkevmcd reviewed Nov 7, 2024

View reviewed changes

ericpromislow requested a review from bigkevmcd November 8, 2024 23:25

bigkevmcd reviewed Nov 14, 2024

View reviewed changes

pkg/stores/sqlpartition/queryparser/labels.go Show resolved Hide resolved

ericpromislow force-pushed the 46333-index-arbitrary-labels branch 2 times, most recently from 697fdee to 3bbfa55 Compare December 7, 2024 00:04

moio requested a review from bigkevmcd December 17, 2024 14:57

bigkevmcd requested changes Dec 18, 2024

View reviewed changes

ericpromislow force-pushed the 46333-index-arbitrary-labels branch from 3bbfa55 to cfdeafd Compare January 6, 2025 21:50

ericpromislow force-pushed the 46333-index-arbitrary-labels branch 2 times, most recently from 607d208 to 3ea8184 Compare January 16, 2025 00:16

ericpromislow marked this pull request as ready for review January 16, 2025 00:16

ericpromislow requested a review from bigkevmcd January 16, 2025 00:16

tomleb mentioned this pull request Jan 16, 2025

Move lasso SQL cache in Steve #452

Merged

moio previously approved these changes Jan 16, 2025

View reviewed changes

ericpromislow dismissed moio’s stale review via 21a391d January 16, 2025 18:47

ericpromislow requested a review from moio January 16, 2025 18:47

ericpromislow force-pushed the 46333-index-arbitrary-labels branch 2 times, most recently from 87eea56 to 7dd7067 Compare January 17, 2025 21:49

moio reviewed Jan 20, 2025

View reviewed changes

pkg/stores/sqlpartition/listprocessor/processor.go Outdated Show resolved Hide resolved

pkg/stores/sqlpartition/queryparser/selector.go Show resolved Hide resolved

moio previously approved these changes Jan 20, 2025

View reviewed changes

pkg/stores/sqlpartition/queryparser/selector.go Outdated Show resolved Hide resolved

ericpromislow dismissed moio’s stale review via 4075bc5 January 20, 2025 20:56

ericpromislow force-pushed the 46333-index-arbitrary-labels branch from 4075bc5 to 3f6a70a Compare January 20, 2025 20:59

moio requested changes Jan 21, 2025

View reviewed changes

pkg/stores/sqlpartition/queryparser/selector.go Outdated Show resolved Hide resolved

tomleb reviewed Jan 21, 2025

View reviewed changes

ericpromislow added 2 commits January 21, 2025 10:32

Add more fields to index when sql-caching is on.

2b35811

Restore the gvkKey helper, add event fields.

ef66767

The UI team wasn't sure whether the event fields should go in the empty-string group or in 'events.k8s.io', so let's go with both until/unless specified otherwise.

ericpromislow added 24 commits January 21, 2025 10:32

Parse less-than and greater-than ops

14c2a69

Lasso's CacheFor now takes a watchable argument.

9c90d37

Support 'gt' and 'lt' as synonyms for '<' and '>'.

1eb72a3

I see both types of operators being bandied about -- it's easy to support the aliases.

typo fix

98106f0

Have the filter parser allow exist tests only on labels.

8bd6047

Also reduce the case where there's no namespace function.

Specify hard-wired fields to index alphabetically.

83b0a96

Remove unused variable.

5a2a386

Parser: 'metadata.labels[FIELD]' is valid

4881cb9

Pull in new gvk fields from main (and keep in alpha order).

c5947cf

Fixed a couple of drops done during the last rebase.

ddae63f

Add a reminder to keep the entries in alpha order.

b525e80

Update lasso ref

e4ae79f

Test TransformLabels

68953c3

Remove TransformLabels

0a91ad4

Remove unused/unneeded code.

1a2ec99

Describe diffs between our label-selector parser and upstream's.

4e606c5

Use the merged lasso 46333 work.

af2740b

Clean go.sum of old lasso entries.

db372f0

Drop unused field.

e7e3935

Tighten up the code.

339dc75

Specify which commit the label selector parser is based on.

8e1584e

Allow both single-quoted and double-quoted value matching, doc differ…

bb14dae

…ence.

More review-driven changes:

771a750

- Stricter processing of m.l.name keys: Require ending close-bracket for a start-bracket - Comment fix - Moving sql processing from lasso to steve: some changes missed in rebase

Drop support for double-quotes for string values.

a443683

For now on only single-quotes (or none where possible) are allowed.

Renaming and dropping an init block.

67a44aa

ericpromislow force-pushed the 46333-index-arbitrary-labels branch from 3f6a70a to 67a44aa Compare January 21, 2025 22:07

moio approved these changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Index arbitrary labels #317

Index arbitrary labels #317

ericpromislow commented Nov 7, 2024 •

edited

Loading

bigkevmcd left a comment

ericpromislow commented Jan 6, 2025

moio left a comment

moio Jan 16, 2025

richard-cox Jan 20, 2025

ericpromislow Jan 20, 2025

moio Jan 21, 2025

tomleb Jan 21, 2025

ericpromislow Jan 21, 2025

moio left a comment

tomleb left a comment

tomleb Jan 20, 2025

ericpromislow Jan 21, 2025

tomleb Jan 21, 2025

tomleb Jan 21, 2025

ericpromislow Jan 21, 2025

tomleb Jan 21, 2025

tomleb Jan 21, 2025

ericpromislow Jan 21, 2025

tomleb Jan 21, 2025

ericpromislow commented Jan 21, 2025

moio commented Jan 22, 2025

moio Jan 22, 2025

richard-cox commented Jan 22, 2025

	labelSelectorRegex = regexp.MustCompile(`^metadata.labels(?:\.\w[-a-zA-Z0-9_./]\|\[.])$`)
	labelSelectorRegex = regexp.MustCompile(`^metadata\.labels((\.\w[-a-zA-Z0-9_./])\|(\[.\]))$`)

Index arbitrary labels #317

Are you sure you want to change the base?

Index arbitrary labels #317

Conversation

ericpromislow commented Nov 7, 2024 • edited Loading

bigkevmcd left a comment

Choose a reason for hiding this comment

ericpromislow commented Jan 6, 2025

moio left a 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

moio left a comment

Choose a reason for hiding this comment

tomleb left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpromislow commented Jan 21, 2025

moio commented Jan 22, 2025

Choose a reason for hiding this comment

richard-cox commented Jan 22, 2025

ericpromislow commented Nov 7, 2024 •

edited

Loading