add way to set imagestreamtag labels

[deads2k]

Adds a field congruent to TagReference.Annotations that allows setting an imagestreamtag.label to provide a selection mechanism for openshift/origin#16580 that is consistent with object selection in kubernetes as a whole. A two stage selection (ns, then istag) provides a way to prevent rogue actors from preventing pruning and keeps overall cost of selection low.

@openshift/api-review @bparees

[bparees]

Thanks, but based on the customer feedback int he RFE the flag approach is going to be more useful to them. Among other things, the flag approach doesn't require them to ensure their new imagestreams created by builds contain appropriate labels to be excluded (they just have to be named properly).

We can consider adding this mechanism as well in the future, but for now we're going to deliver what's already been implemented and not invest further resources in this feature.

[bparees]

(that's not to say i'm averse to adding a labels field to ISTs if we want to proceed w/ doing that anyway, i think we've seen other times that having such a field would have been useful. so if you want to proceed w/ merging this i guess i'm ok with it, just saying we are not going to immediately leverage it for anything)

[deads2k]

Thanks, but based on the customer feedback int he RFE the flag approach is going to be more useful to them. Among other things, the flag approach doesn't require them to ensure their new imagestreams created by builds contain appropriate labels to be excluded (they just have to be named properly).

The standard/only selection mechanism for resources inside of the kube API and CLI is label selection. Rather than introduce a new technique, I think we should pursue making that standard technique available instead of introducing a new way to select objects that isn't necessary and leaves an inconsistent API both at the REST level and at the CLI level.

[deads2k]

We can consider adding this mechanism as well in the future, but for now we're going to deliver what's already been implemented and not invest further resources in this feature.

I don't think that "I spent some time making this" is sufficient reason to create a new and inconsistent API. I would say that rather than creating something new, you should leverage existing technique used throughout both products. The amount of time required to do either approach is fairly minimal.

[bparees]

"we spent the time we had budgeted on this already, you can either have what we did, or nothing" is sufficient reason to deliver what we did already so the customer can have a solution instead of no solution.

Not to mention I disagree that what we did is problematic.

[smarterclayton]

We use regular expression selection in other places in the CLI. Regexes are perfectly reasonable for some use cases, especially those where a user is processing a large list of things to remove / update.

Giving image stream tags labels is a bigger decision that needs to deal with whether the image stream has labels as well, and how users would expect to label both image streams and image stream tags. I don't think we should rush to add them right now, since we've balked the last few times we've discussed it.

[smarterclayton]

The standard/only selection mechanism for resources inside of the kube API and CLI is label selection.

That's not really true. Labels are our preferred selection mechanism. They are not the only, and our tools are intended to be used by many different consumers. Forcing people to use labels because they are the supported way is being unnecessarily rigid.

In general I agree that labelling an image stream tag is a cleaner way to implement this request. I don't think regexes on prune is divergent from any number of other unix commands that have the same use cases, just uglier. I would generally prefer to move towards label selection in the CLI for prune, but I'm not clear why the regex feature isn't also an acceptable outcome.

[deads2k]

We use regular expression selection in other places in the CLI. Regexes are perfectly reasonable for some use cases, especially those where a user is processing a large list of things to remove / update.

I can't seem to find these. Where are we taking them as args for CLI commands selecting resources? I found a reference in the proxy server for paths, but no where else outside of validation.

That's not really true. Labels are our preferred selection mechanism. They are not the only, and our tools are intended to be used by many different consumers. Forcing people to use labels because they are the supported way is being unnecessarily rigid.

There's a larger exploration of labels over regex in comments 9 and 14: https://bugzilla.redhat.com/show_bug.cgi?id=1410018 . Do you have a reason why in this case we want to move in the direction of regex because it provides a more consistent/superior experience selecting resources from a kube API? Using a field selector on an annotation would be just as easy implementation-wise, would be more consistent than regexes, and doesn't require an API change if you just wanted to wait on that piece.

The code to do this drops right in here: https://github.com/openshift/origin/pull/16580/files#diff-8da618e3de8631ccdac1130b5e632829R362 . It's just a check of fields and labels instead of a regex evaluation.

[bparees]

Any solution that involves metadata on the imagestreamtag has problems i haven't heard addressed yet:

1. it requires the creators of the imagestreams to put that metadata in place. If they forget, their images are subject to pruning, potentially inadvertently
2. it allows the creator of an imagestream to get themselves excluded from pruning, which an admin may not want them to be able to do
3. it requires you go back and add that metadata to all existing imagestreams that you don't want pruned before you can run pruning

Allowing a cluster admin to simply pass a set of patterns to skip over avoids these problems. Regardless of its merits re: api consistency, it offers a workflow some admins may prefer.

[deads2k]

it requires the creators of the imagestreams to put that metadata in place. If they forget, their images are subject to pruning, potentially inadvertently

A namespace level label is easy to support: image.openshift.io/prune-protection=protect-namespace

it allows the creator of an imagestream to get themselves excluded from pruning, which an admin may not want them to be able to do

Not when protected as described in my original comment: openshift/origin#16580 (comment).

The use a regex here appears to make the command hard to use. In addition, if I'm reading the code correctly, the caller either has to specify specific namespaces in his regex to avoid accidentally matching image streams owned by other people that should be pruned.

Given those two things, why not just make it official and simply making some namespaces immune to pruning for a command. That could be done via label selection of namespaces (users can't label namespaces), it wouldn't require an additional flag, and it wouldn't be vulnerable to less than obvious matching problems.

it requires you go back and add that metadata to all existing imagestreams that you don't want pruned before you can run pruning

You were either going to add it to a metadata file or add it locally to the data you were protecting. From the bugzilla where I laid out the options here https://bugzilla.redhat.com/show_bug.cgi?id=1410018#c9, item 5:

Doing this would allow someone to...

1. explicitly protect their images in a persistent way that didn't rely particular job configuration
2. keep the data about protection closely associated with the data they're protecting
3. allow inspection and confirmation of protection using standard tools oc get namespaces -l image.openshift.io/prune=false instead of trying to parse dry-run output.
4. enable explicit organization or protected images along the existing ACL boundaries (per project)
5. allow repeatable cluster creation using templates that will automatically protect images in a particular spot

[bparees]

A namespace level label is easy to support: image.openshift.io/prune-protection=protect-namespace

(only) namespace level immunity was already rejected in the RFE as insufficiently finegrained.

Not when protected as described in my original comment: openshift/origin#16580 (comment).

because namespace level immunity is insufficiently granular as discussed in the RFE. Which means you're back to having to rely on imagestream tag annotations, which anyone can set.

At best you could define certain namespaces as "eligible" for being excluded from pruning, and then certain images within that namespace could be annotated as immune, but that is still not as fine grained as the admin being able to simply say "i'm not going to prune X", and it still puts power in the hands of (some) users that an admin may not want to grant.

With your suggestion, how do I, as an admin guarantee that:

1. foo/bar:tag is not pruned
2. foo/rab:tag is pruned

regardless of what actions the user who has admin permission inside "foo" takes in terms of editing annotations on bar and rab?

In addition, if I'm reading the code correctly, the caller either has to specify specific namespaces in his regex to avoid accidentally matching image streams owned by other people that should be pruned.

I still don't see that as burdensome, particularly in the context of the customer's use case.

You were either going to add it to a metadata file or add it locally to the data you were protecting.

yes and the single file is easier in some use cases than finding/updating all the imagestreams across all the projects in the cluster (especially if those imagestreams were created from templates i do not control and may be updated/recreated and lose the annotation). I can also carry it to a new cluster where people did not annotate their imagestreams.

[deads2k]

testing

/assign bparees

[deads2k]

/approve