Implement policy-level tag regex filtering #75

relu · 2021-01-11T15:24:43Z

Tag regex filtering allows the user to filter tags based on a regular
expression pattern and enables tag version extraction through capture
group replacement reference.

Fixes #73

Example use cases (for documentation):

filterTags:
  pattern: '^v'

filterTags:
  pattern: '^nightly-.*'

filterTags:
  pattern: '^dev-v(.*)'
  extract: '$1'

filterTags:
  pattern: '^dev-(?P<version>[0-9]+)-alpha.*'
  extract: '$version'

filterTags:
  pattern: '^rel-(\d+)$'
  extract: '$1'

etc.

relu · 2021-01-11T15:29:05Z

internal/policy/filter.go

@@ -0,0 +1,70 @@
+/*


Not super happy about how this was laid out, couldn't think of a nicer way to implement it at the moment though.

api/v1alpha1/imagepolicy_types.go

stefanprodan · 2021-01-11T15:43:04Z

@relu can this be used for automating Minio releases? https://hub.docker.com/r/minio/minio/tags?page=1&ordering=last_updated note that users will want the latest multi-arch image (doesn't end in arm/amd/etc) and only the stable releases that start with RELEASE.

relu · 2021-01-11T17:06:09Z

@stefanprodan yes, since it uses the RFC3339 format it should be lexicographically sortable, using the alphabetical ordering policy it would go something like this:

filterTags:
  pattern: '^RELEASE\.(?P<timestamp>.*)Z$'
  extract: '$timestamp'
policy:
  alphabetical:
    order: asc

I think it can even be simplified to this:

filterTags:
  pattern: '^RELEASE.*'
policy:
  alphabetical:
    order: asc

squaremo · 2021-01-11T16:39:59Z

internal/policy/filter.go

@@ -0,0 +1,70 @@
+/*
+Copyright 2020 The Flux authors


Elsewhere I have used 2020, 2021 for files that are either new, or modified this year. 2020 is OK, since the important bit is the year of first publication -- adding 2021 is just more information for readers.

Yes, I didn't bother with this because I already had this laying around since last year. I think 2021 is better for this particular file because it was pretty much rewritten entirely today.

squaremo · 2021-01-11T17:02:01Z

internal/policy/filter.go

+func NewRegexFilter(pattern string, replace string) (*RegexFilter, error) {
+	m, err := regexp.Compile(pattern)
+	if err != nil {
+		return nil, fmt.Errorf("invalid regular expression pattern '%s': %s", pattern, err.Error())


Suggested change

return nil, fmt.Errorf("invalid regular expression pattern '%s': %s", pattern, err.Error())

return nil, fmt.Errorf("invalid regular expression pattern '%s': %w", pattern, err)

squaremo · 2021-01-11T17:04:34Z

internal/policy/filter.go

+		if f.Regexp.MatchString(item) {
+			tag := item
+			if f.Replace != "" {
+				tag = f.Regexp.ReplaceAllString(item, f.Replace)


I think you can do this without repeating work, using Find*Index and Expand.

Not sure I follow here, what do you refer to as repeated work? I would assume it'll look kind of like this, right?

if submatches := f.Regexp.FindStringSubmatchIndex(item); len(submatches) > 0 { tag := item if f.Replace != "" { result := []byte{} result = f.Regexp.ExpandString(result, f.Replace, item, submatches) tag = string(result) } f.filtered[tag] = item }

The above works just fine but I'm not sure I see the benefit 😕

Both MatchString and ReplaceAllString "run" the regular expression -- it's just that the former disregards submatches, and the latter also does substitution. On the other hand, FindStringSubmatchIndex runs the regular expression to get the submatches (and whether it matches at all), but ExpandString does only the substitution work.

It is unlikely to make a vast difference to any individual match, but all else being equal, it's worth avoiding extra calculation.

Got it, thanks for the explanation, I just looked into more detail in the implementation of ReplaceAllString vs ExpandString and I did notice now what you mean. I will refactor.

squaremo · 2021-01-11T17:14:36Z

controllers/imagepolicy_controller.go

+				}
+				filter.Apply(tags)
+			}
+			latest, err = policer.Latest(tags, filter)


Running the flow of control the other way would be clearer here. This way around, you supply a filter to the Policer, and it has to know how to deal with it. But if you filter here, and give the Policer the extracted names, it can determine which one it wants without having to know whether they are filtered or not. Then you can map back to the original again here (if there was a filter in the first place).

One issue, whichever way around you do it, is what to do if the extracted strings are duplicated? E.g., if you have (dev|stage)-(.*), and extract $2, you will get a duplicate from the tags {"dev-v1.0", "stage-v1.0"}. At present the last one encountered wins (since RegexpFilter keeps track of them in a map). But this isn't stable -- you can get different results, without anything actually changing.

Thanks for the feedback, I think your suggestion makes a lot of sense and would simplify things significantly in the policies Latest implementation.

As for the second comment, I thought about this as well when writing the tests, I wasn't able to think of a real-world scenario where this would become an issue but I think it does make sense to specify this in the documentation since it's indeed an edge case some might stumble upon. I don't know if we can do anything about this conflict aside from warning users about it.

it does make sense to specify this in the documentation since it's indeed an edge case some might stumble upon

I agree with this, and

warning users about it

with this. If it happens, it could be pretty mysterious, so we could log a message with a reference to docs or decent search term.

(we can discuss these alternatives a bit more in #77)

nomeelnoj · 2021-01-12T04:17:41Z

How would this work if you were adding a build ID to the end? For example, you have two images:

dev-v1.2.0-37
dev-v1.2.0-38

And your policy is defined as:

filterTags:
  pattern: '^dev-v(.*)-[0-9]+'
  extract: '$1'
policy:
  semver:
    range: 1.2.x

Would the dev-v1.2.0-38 image get deployed because it is newer than the previous? or would the reflector controller ignore this image because it already has one that matches the filter?

relu · 2021-01-12T09:06:32Z

@nomeelnoj you will want to include the pre-release part (what you mentioned as being the build id) to the capture group.

filterTags:
  pattern: '^dev-v(.*)'
  extract: '$1'
policy:
  semver:
    range: 1.2.x-0

Notice that I've added a -0 to your range, this is to instruct semver evaluation to consider the pre-release part of the version as well.

nomeelnoj · 2021-01-12T20:10:57Z

@relu thanks! very excited for this to be merged, as I believe this is the final blocker from getting us over to v2 (v1 is less than functional because the helm operator is now having issues with the old stable repo being deprecated). How long after this is merged will a release be cut?

stefanprodan

LGTM

Thanks @relu 🥇

squaremo

A very welcome addition to the API, thank you @relu.

squaremo · 2021-01-13T12:44:31Z

I'll take a look at those test failures ...

Tag regex filtering allows the user to filter tags based on a regular expression pattern and enables tag version extraction through capture group replacement reference. Fixes #73 Signed-off-by: Aurel Canciu <[email protected]>

relu commented Jan 11, 2021

View reviewed changes

api/v1alpha1/imagepolicy_types.go Show resolved Hide resolved

relu force-pushed the tag-prefix-matching branch from 3dca47f to bc7f16e Compare January 11, 2021 17:12

squaremo reviewed Jan 11, 2021

View reviewed changes

relu force-pushed the tag-prefix-matching branch from bc7f16e to 5a0089e Compare January 11, 2021 19:11

stefanprodan approved these changes Jan 13, 2021

View reviewed changes

relu force-pushed the tag-prefix-matching branch from fd8a131 to 1434254 Compare January 13, 2021 11:58

squaremo approved these changes Jan 13, 2021

View reviewed changes

relu force-pushed the tag-prefix-matching branch 2 times, most recently from 25a1ccd to 81624b5 Compare January 13, 2021 13:34

Implement policy-level tag regex filtering

cbcad12

Tag regex filtering allows the user to filter tags based on a regular expression pattern and enables tag version extraction through capture group replacement reference. Fixes #73 Signed-off-by: Aurel Canciu <[email protected]>

relu force-pushed the tag-prefix-matching branch from 81624b5 to cbcad12 Compare January 13, 2021 14:42

hiddeco merged commit d6db893 into main Jan 13, 2021

hiddeco deleted the tag-prefix-matching branch January 13, 2021 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement policy-level tag regex filtering #75

Implement policy-level tag regex filtering #75

relu commented Jan 11, 2021

relu Jan 11, 2021

stefanprodan commented Jan 11, 2021

relu commented Jan 11, 2021

squaremo Jan 11, 2021

relu Jan 11, 2021

squaremo Jan 11, 2021

squaremo Jan 11, 2021

relu Jan 11, 2021

squaremo Jan 13, 2021

relu Jan 13, 2021

squaremo Jan 11, 2021

squaremo Jan 11, 2021

relu Jan 11, 2021

squaremo Jan 13, 2021

squaremo Jan 13, 2021

nomeelnoj commented Jan 12, 2021

relu commented Jan 12, 2021

nomeelnoj commented Jan 12, 2021

stefanprodan left a comment

squaremo left a comment

squaremo commented Jan 13, 2021

	return nil, fmt.Errorf("invalid regular expression pattern '%s': %s", pattern, err.Error())
	return nil, fmt.Errorf("invalid regular expression pattern '%s': %w", pattern, err)

Implement policy-level tag regex filtering #75

Implement policy-level tag regex filtering #75

Conversation

relu commented Jan 11, 2021

Choose a reason for hiding this comment

stefanprodan commented Jan 11, 2021

relu commented Jan 11, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nomeelnoj commented Jan 12, 2021

relu commented Jan 12, 2021

nomeelnoj commented Jan 12, 2021

stefanprodan left a comment

Choose a reason for hiding this comment

squaremo left a comment

Choose a reason for hiding this comment

squaremo commented Jan 13, 2021