Cannot update ImageStreamTags created via Push

[bparees]

When an image is push to the internal registry, an imagestreamtag is created in the form:

kind: ImageStreamTag
lookupPolicy:
  local: false
metadata:
  creationTimestamp: 2018-02-07T22:06:42Z
  name: foobar:latest
  namespace: p1
  resourceVersion: "2545"
  selfLink: /apis/image.openshift.io/v1/namespaces/p1/imagestreamtags/foobar%3Alatest
  uid: 2dd34eef-0c53-11e8-9652-507b9d27b5d9
tag: null

Now, normally when an IST is created or updated, the associated imagestream is also updated to add the IST to the imagestream.spec.tags list.

However when the IST is created via push, it is created via an imagestreammapping which means that the imagestream associated w/ the IST is not updated. So the imagestream has a nil spec.tag list.

If you then attempt to update the ImageStreamTag manually (e.g. oc edit, to add an annotation to it), the IST update logic attempts to add the IST to the imagestream spec list, except that the IST we're adding is a TagReference with no information(see tag: null above), including notably no ReferencePolicyType value.

https://github.com/openshift/origin/blob/master/pkg/image/registry/imagestreamtag/rest.go#L262-L280

This results in us attempting to run the ImageStream validation logic on the imagestream and failing here:
https://github.com/openshift/origin/blob/master/pkg/image/registry/imagestream/strategy.go#L543
https://github.com/openshift/origin/blob/master/pkg/image/apis/image/validation/validation.go#L281-L299
https://github.com/openshift/origin/blob/master/pkg/image/apis/image/validation/validation.go#L201-L204
https://github.com/openshift/origin/blob/master/pkg/image/apis/image/validation/validation.go#L265-L269

Now, the imagestreamtag itself does not have a ReferencePolicyType because it doesn't even have a TagReference (note tag: null above). (If it did have a TagReference it would have had the ReferencePolicy defaulted because we have a defaulter for this).

But we seemingly manufacture a TagReference out of thin air here (with, again an empty ReferencePolicyType):
https://github.com/openshift/origin/blob/master/pkg/image/registry/imagestreamtag/rest.go#L265

Which again bypasses defaulting behavior.

So we create a TagReference that doesn't have a ReferencePolicyType, we add that TagReference to the ImageStream.spec.tags, and then try to run it through validation and fail.

Here are my questions:

1. why are we trying to add ISTs to ISes when editing an IST? Shouldn't it be reasonable to assume the IST is already listed in the IS if it's supposed to be?

2. why aren't "pushed" ISTs represented in IS.spec.tags upon creation? it's not like i can't import an image from the internal registry. It seems silly to treat them specially.

3. What's the right fix there? I see three options:
   a) stop adding tag refs to imagestreams during an IST update, period (see above, i don't know why we do this)
   b) variant on (a) - don't add the tag to the IST if the IST's tagreference field is null.
   c) add the ReferencePolicy to the "manufactured" TagRef so the IS update succeeds (essentially re-implement defaulting). Probably wrong because it's going to result in the tagref being added to the IS if someone updates the IST, and that doesn't seem to be the current expected behavior.

@mfojtik @smarterclayton @legionus @dmage @miminar @deads2k

[smarterclayton]

IST update is broken and need to be fixed. I have a separate issue that attempts to address it. It's going to be a new API version because the current model isn't backward compatible. Pushed images aren't represented because specs are requests to tag, status are pushes. It doesn't have to do with where, it has to do with a spec tag being an intent. An IST update today is supposed to mean "update spec tag" if tag ref is non nil. I think it may be acceptable to not tag the IST if the tag reference is nil, but i'm not positive that's backwards compatible.

…

On Wed, Feb 7, 2018 at 5:31 PM, Ben Parees ***@***.***> wrote: When an image is push to the internal registry, an imagestreamtag is created in the form: kind: ImageStreamTag lookupPolicy: local: false metadata: creationTimestamp: 2018-02-07T22:06:42Z name: foobar:latest namespace: p1 resourceVersion: "2545" selfLink: /apis/image.openshift.io/v1/namespaces/p1/imagestreamtags/foobar%3Alatest uid: 2dd34eef-0c53-11e8-9652-507b9d27b5d9 tag: null Now, normally when an IST is created or updated, the associated imagestream is also updated to add the IST to the imagestream.spec.tags list. However when the IST is created via push, it is created via an imagestreammapping which means that the imagestream associated w/ the IST is *not* updated. So the imagestream has a nil spec.tag list. If you then attempt to update the ImageStreamTag manually (e.g. oc edit, to add an annotation to it), the IST update logic attempts to add the IST to the imagestream spec list, except that the IST we're adding is a TagReference with no information(see tag: null above), including notably no ReferencePolicyType value. https://github.com/openshift/origin/blob/master/pkg/image/ registry/imagestreamtag/rest.go#L262-L280 This results in us attempting to run the ImageStream validation logic on the imagestream and failing here: https://github.com/openshift/origin/blob/master/pkg/image/ registry/imagestream/strategy.go#L543 https://github.com/openshift/origin/blob/master/pkg/image/ apis/image/validation/validation.go#L281-L299 https://github.com/openshift/origin/blob/master/pkg/image/ apis/image/validation/validation.go#L201-L204 https://github.com/openshift/origin/blob/master/pkg/image/ apis/image/validation/validation.go#L265-L269 Now, the imagestreamtag itself does not have a ReferencePolicyType because it doesn't even have a TagReference (note tag: null above). (If it did have a TagReference it would have had the ReferencePolicy defaulted because we have a defaulter for this). But we seemingly manufacture a TagReference out of thin air here (with, again an empty ReferencePolicyType): https://github.com/openshift/origin/blob/master/pkg/image/ registry/imagestreamtag/rest.go#L265 Which again bypasses defaulting behavior. So we create a TagReference that doesn't have a ReferencePolicyType, we add that TagReference to the ImageStream.spec.tags, and then try to run it through validation and fail. Here are my questions: 1. why are we trying to add ISTs to ISes when editing an IST? Shouldn't it be reasonable to assume the IST is already listed in the IS if it's supposed to be? 2. why aren't "pushed" ISTs represented in IS.spec.tags upon creation? it's not like i can't import an image from the internal registry. It seems silly to treat them specially. 3. What's the right fix there? I see three options: a) stop adding tag refs to imagestreams during an IST update, period (see above, i don't know why we do this) b) variant on (a) - don't add the tag to the IST if the IST's tagreference field is null. c) add the ReferencePolicy to the "manufactured" TagRef so the IS update succeeds (essentially re-implement defaulting). Probably wrong because it's going to result in the tagref being added to the IS if someone updates the IST, and that doesn't seem to be the current expected behavior. @mfojtik <https://github.com/mfojtik> @smarterclayton <https://github.com/smarterclayton> @legionus <https://github.com/legionus> @dmage <https://github.com/dmage> @miminar <https://github.com/miminar> @deads2k <https://github.com/deads2k> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18519>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p8zrUdNTNvvNxBqsEBgQ7W5LUdOeks5tSiQogaJpZM4R9f52> .

[bparees]

I think it may be acceptable to not tag the IST if the tag reference is nil, but i'm not positive that's backwards compatible.

As it stands today, no one can update an IST if the tag ref is nil, so the only thing we'd be breaking is now those updates would succeed(while still not updating the IS) instead of failing entirely.

[mfojtik]

/cc deads2k

[bparees]

ok so (as i come to understand the image resource apis), adding an annotation to an imagestreamtag necessarily requires adding the tag to the imagestream.spec. So for a tag that was only pushed, you literally cannot add annotations to it, unless you're willing to accept that said tag gets added to imagestream.spec in the process.

I can make that happen (by defaulting the referencepolicy as we're updating the imagestream) but do we even want to enable this flow, or is it basically "working as designed" that you can't update an imagestreamtag that doesn't exist in the imagestream.spec?

[bparees]

An IST update today is supposed to mean "update spec tag" if tag ref is non nil.

So basically (I'm realizing) it's impossible to update an IST if there is no imagestream.spec.tag for it because there is nowhere to store the update content. The error may not be great(users get a weird validation error reported from the imagestream, when they were trying to update the imagestreamtag), but the behavior seemingly can't be fixed (the only way to add annotations to a pushed IST would be to manufacture a spec tag and add the annotations to it...and it's seemingly a design point that pushed tags don't have a spec tag).