Implement Alphabetical order policy #58

relu · 2020-11-24T21:47:27Z

This implementation allows one to set a Prefix to filter a list of tags and/or use Reverse alphabetical order through the respective policy type attributes.

Relates to #31

bigkevmcd · 2020-11-25T05:50:59Z

controllers/imagepolicy_controller.go

-			err = r.Status().Update(ctx, &pol)
-		}
-		return ctrl.Result{}, err
+		latest, err = r.calculateLatestImageSemver(&policy, repo.Status.CanonicalImageName)


I wonder if we couldn't break these out into implementations of a Policy interface, with a simple map translation here, which would make it easier to test the policies "standalone" ?

What does "map translation" mean here -- dispatch via a map of policy name -> implementation?

I think that makes sense. I'm going to try to come up with something.

@squaremo yes

I've moved the logic to an internal policy package and defined there the policy types and a factory to resolve the type based on the ImagePolicyChoice object. I think this is a lot better than the switch and allows us to extend further if needed without involving any controller logic changes.

squaremo

Just some comments, while this is a draft ..

squaremo · 2020-11-25T09:39:53Z

api/v1alpha1/imagepolicy_types.go

+	// If true the order would be: C B A (A would be chosen)
+	// If false the order would be: A B C (C would be chosen)
+	// +optional
+	Reversed *bool `json:"reverse,omitempty"`


An alternative, more familiar to some people, is using order: ASC|DESC (as in "ascending" or "descending"). The intuitive default would be ascending, which is the same as reverse: false, the default here and the most useful for e.g., calendar-dated tags.

My thinking was based more on the meaning of the adjective, which implies the default ascending order according to the alphabet, so reversed came to mind as the right modifier word. I would be inclined to change it if you think order makes more sense.

squaremo · 2020-11-25T09:45:06Z

api/v1alpha1/imagepolicy_types.go

+type AlphabeticalPolicy struct {
+	// Prefix enables filtering the list of tags by given prefix.
+	// +optional
+	Prefix string `json:"prefix,omitempty"`


Is this a pattern or a literal? Does the matching prefix get removed before ordering? (Worth putting these considerations in the docs!)

Right now it's just a literal, using strings.HasPrefix. We can discuss the possibility to use a pattern, though I wasn't able to think of any use case for that, however, some people might find support for that useful. What do you think?

As a side note, zooming out a bit, I think it might make sense to provide the ability to filter out tags at the level of the imagerepository resource as well, in order to prevent polluting the tags database with any unused entries.

I think you need to clarify the intent of the prefix, i.e. is it matching, or excluding.

To clarify, imagine you are reading the YAML for a policy, you shouldn't really need to go and read the CRD or Go source to work out what it does

I would think users might want to limit the tags the policy would apply to only to a subset of tags e.g rel-* or nightly-*. The intention was to use Prefix for matching this subset, excluding other irrelevant tags.

Would renaming this attribute to MatchTagPrefix make more sense?
What about supporting multiple values?
Or maybe adding support for regular expressions?

Made some changes, however, some of the questions I mentioned above are still valid.

squaremo · 2020-11-25T09:47:29Z

controllers/imagepolicy_controller.go

-			err = r.Status().Update(ctx, &pol)
-		}
-		return ctrl.Result{}, err
+		latest, err = r.calculateLatestImageSemver(&policy, repo.Status.CanonicalImageName)


What does "map translation" mean here -- dispatch via a map of policy name -> implementation?

squaremo

I had some minor nits to pick, so I've review-commented. We can also discuss the questions left to resolve, in the main comment thread.

api/v1alpha1/imagepolicy_types.go

squaremo · 2020-12-01T12:56:32Z

api/v1alpha1/imagepolicy_types.go

+	// literal prefix. If not provided, all the tags from the repository will be
+	// ordered and compared.
+	// +optional
+	MatchTagPrefix string `json:"matchTagPrefix,omitempty"`


To carry on from your suggestion in the previous conversation:

it might make sense to provide the ability to filter out tags at the level of the imagerepository resource

That does make some sense, yes. For some purposes I can see it being useful to filter in the policy -- e.g., if you want nightly-* builds. For other purposes, you might want to filter in the repository, e.g., if there's specific images that are unusable, or upset the scanning, that you want to filter out. I've posted #62 for this latter use case.

Would renaming this attribute to MatchTagPrefix make more sense?
What about supporting multiple values?
Or maybe adding support for regular expressions?

The name MatchTagPrefix is good. Your questions make me wonder about few things:

how to leave room for other ways of filtering later -- e.g., would it ever make sense to have prefix matching and regular expressions, and if not, how to prevent those both being supplied;

whether this is useful for all selection policies -- maybe you tag your images dev-<semver>;

if you might need to remove the prefix before considering the remainder for sorting, as in the dev-<semver> example.

(posting this in the main comment thread)

squaremo · 2020-12-01T12:58:25Z

controllers/imagepolicy_controller.go

@@ -103,12 +102,12 @@ func (r *ImagePolicyReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error)
 		return ctrl.Result{}, nil
 	}

-	policy := pol.Spec.Policy
+	resolver, err := policy.Policy(pol.Spec.Policy)


policy policy policy :-) What about policy.SelectorFromSpec(pol.Spec.Policy)

Oh man, I'm bad at naming things 🤦‍♂️

squaremo · 2020-12-01T13:02:19Z

internal/policy/factory.go

+func Policy(choice imagev1.ImagePolicyChoice) (Interface, error) {
+	var p Interface
+	var err error
+	switch {


Yep this works well 👍

squaremo · 2020-12-01T13:10:41Z

internal/policy/policy.go

+
+package policy
+
+type Interface interface {


I'd name this something that describes what it does. (The interface in go's sort package defines the single requirement for sort, which doesn't typically need to be named outside of its own package -- so it can be forgiven for being named sort.Interface.)

Fine, then I'll name it Policer (I had to check if that word exists and was pleasantly surprised it does).

squaremo · 2020-12-01T13:15:28Z

Open questions:

how to leave room for other ways of filtering later -- e.g., would it ever make sense to have prefix matching and regular expressions, and if not, how to prevent those both being supplied;
whether this is useful for all selection policies -- maybe you tag your images dev-;
if you might need to remove the prefix before considering the remainder for sorting, as in the dev- example.

relu · 2020-12-01T16:10:17Z

how to leave room for other ways of filtering later -- e.g., would it ever make sense to have prefix matching and regular expressions, and if not, how to prevent those both being supplied;

I would argue for policy-scoped filtering rather than per policy type implementation, for me this makes more sense because of my previous experience working with ECR Lifecycle Policies that also enable applying policies based on a prefix-based list of strings.

whether this is useful for all selection policies -- maybe you tag your images dev-;

I think this can be useful even for semver-like tags that maybe have an incompatible prefix e.g. ver1.2.3 instead of the supported format v1.2.3. This would enable folks to make use of semver policying w/o having to change the format of their tags, or in the situation that would not even be possible due to the repository being outside of their control.

if you might need to remove the prefix before considering the remainder for sorting, as in the dev- example.

An inclusive/exclusive comparison mode toggle would be useful considering the case highlighted above.

relu · 2020-12-02T12:15:17Z

An extra thought on the policy level prefix matching feature. I think it would be best if we implement this after we integrate a "real" database into the controller so that we can use its capabilities to filter out the tags dataset.

I'm inclined to remove the prefix matching implementation done in the scope of the Alphabetical ordering policy model type.

relu · 2020-12-02T22:58:46Z

api/v1alpha1/imagepolicy_types.go

@@ -34,6 +34,11 @@ type ImagePolicySpec struct {
 	// selecting the most recent image


Here's what I had in mind re: #62 (comment)

squaremo · 2020-12-03T14:38:48Z

I'm inclined to remove the prefix matching implementation done in the scope of the Alphabetical ordering policy model type.

As in, not include the prefix matching here, and just have alphabetical ordering? If so I think that's a good idea, until we resolve those discussions above.

relu · 2020-12-04T17:29:56Z

As in, not include the prefix matching here, and just have alphabetical ordering? If so I think that's a good idea, until we resolve those discussions above.

You can check the currently proposed design, I've moved the prefix filtering into the policy spec itself rather than at the level of the policy type implementation.

squaremo · 2020-12-09T14:11:57Z

api/v1alpha1/imagepolicy_types.go

+}
+
+// TagPrefixMatcher enables filtering tags based on a set of defined rules
+type TagPrefixMatcher struct {


I like how this is expressed. At risk of exploding the scope, is it ever useful to match by suffix? Or using a glob? (I would count regexp out, per xkcd).

Considering there are images that have suffixes such as -alpine, -slim, -rc etc. it might be a valid use case. I'm not sure what's the best approach around dealing with both of these use cases, I kind of like more the idea of globbing patterns vs prefix/suffix matching, would probably make sense.

Using something like this maybe?

The only downside moving to glob patterns would probably be that we wouldn't be able to support the current pre-sort "trimming" functionality.

I'm trying to figure out, as a user, what would bring in the most value at the moment.

Hmm yeah glob libraries don't tend to implement captures.

Meta-suggestion: shall we move this discussion to an issue or new PR, and reduce this PR to just the alphabetical ordering and related refactoring? That would still be pretty helpful, e.g., for calendar-versioned images.

I agree, will move out the prefix filtering to a separate branch and open a PR once this one gets merged.

@relu i see your branch for this, but do not see an open PR. Dont want to step on any toes, but can we open this PR? Our currently workflow prevents us from moving to the Helm Controller until we have this functionality, but the helm operator is breaking due to the deprecation of the old stable repo, so we are stuck currently: https://github.com/fluxcd/image-reflector-controller/tree/tag-prefix-matching

This implementation allows one to set a `MatchTagPrefix` to filter a list of tags and/or use `Order` to set the ordering rule by which tags are evaluated. Signed-off-by: Aurel Canciu <[email protected]>

squaremo

Neat! Thanks @relu 🍍 I really like seeing the tests bolstered too.

squaremo · 2020-12-10T12:12:01Z

controllers/imagepolicy_controller.go

@@ -174,7 +175,7 @@ func (r *ImagePolicyReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error)
 	}
 	r.event(pol, events.EventSeverityInfo, msg)

-	return ctrl.Result{}, nil
+	return ctrl.Result{}, err


Not sure of the intent of this change, though it doesn't change the meaning of the code as it stands.

Likely a relic from previous changes made that are no longer part of this PR. Should probably be changed back to nil.

relu mentioned this pull request Nov 24, 2020

Lexicographic sort policy #31

Closed

bigkevmcd reviewed Nov 25, 2020

View reviewed changes

squaremo reviewed Nov 25, 2020

View reviewed changes

relu force-pushed the alphabetical-order-policy branch 2 times, most recently from 3969f81 to 6d32f76 Compare November 27, 2020 20:12

relu marked this pull request as ready for review November 27, 2020 20:13

squaremo reviewed Dec 1, 2020

View reviewed changes

relu force-pushed the alphabetical-order-policy branch 2 times, most recently from e9089b8 to b42c6b6 Compare December 1, 2020 16:27

relu mentioned this pull request Dec 2, 2020

Filter out images at ImageRepository level #62

Open

relu force-pushed the alphabetical-order-policy branch from b42c6b6 to 9aa909d Compare December 2, 2020 22:53

relu commented Dec 2, 2020

View reviewed changes

relu force-pushed the alphabetical-order-policy branch from 9aa909d to aae7e20 Compare December 4, 2020 17:29

squaremo reviewed Dec 9, 2020

View reviewed changes

relu force-pushed the alphabetical-order-policy branch from aae7e20 to 3dd2d2d Compare December 9, 2020 15:13

Implement Alphabetical order policy

9be3bc2

This implementation allows one to set a `MatchTagPrefix` to filter a list of tags and/or use `Order` to set the ordering rule by which tags are evaluated. Signed-off-by: Aurel Canciu <[email protected]>

relu force-pushed the alphabetical-order-policy branch from 3dd2d2d to 9be3bc2 Compare December 9, 2020 19:02

relu requested a review from squaremo December 9, 2020 19:05

squaremo approved these changes Dec 10, 2020

View reviewed changes

relu merged commit 60e1fb8 into main Dec 10, 2020

relu deleted the alphabetical-order-policy branch December 10, 2020 13:00

squaremo mentioned this pull request Jan 11, 2021

Regular expression filtering #73

Closed

		@@ -34,6 +34,11 @@ type ImagePolicySpec struct {
		// selecting the most recent image

Implement Alphabetical order policy #58

Implement Alphabetical order policy #58

Conversation

relu commented Nov 24, 2020

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

squaremo 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

bigkevmcd Nov 25, 2020 • edited Loading

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

squaremo 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

squaremo commented Dec 1, 2020

relu commented Dec 1, 2020

relu commented Dec 2, 2020

Choose a reason for hiding this comment

squaremo commented Dec 3, 2020

relu commented Dec 4, 2020

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

squaremo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigkevmcd Nov 25, 2020 •

edited

Loading

squaremo left a comment •

edited

Loading