Drop image signature annotations #19037

miminar · 2018-03-20T17:55:56Z

Because they were never meant to be allowed.

Includes and superseds #19030 (kudos to @deads2k)
Closes #19011

Resolves: rhbz#1557607

/assign @bparees @deads2k
/cc @mfojtik

miminar · 2018-03-20T17:56:32Z

@openshift/api-review

miminar · 2018-03-20T17:56:51Z

/retest

bparees · 2018-03-20T18:18:16Z

pkg/image/registry/image/strategy.go

+	// validation; resolves rhbz#1557607
+	for i := range newImage.Signatures {
+		newImage.Signatures[i].Annotations = map[string]string{}
+	}


should only need this on prepare for update right?

on a create i think we want to reject the creation w/ the validation failure reason, rather than silently drop the user's metadata.

I agree. that is consistent with existing behavior

What if somebody wants to import backed up images?

deads2k · 2018-03-20T18:23:03Z

pkg/image/controller/signature/container_image_downloader.go

@@ -62,9 +62,6 @@ func (s *containerImageSignatureDownloader) DownloadImageSignatures(image *image
 		// signature.
 		sig.Name = imageapi.JoinImageStreamImage(image.Name, fmt.Sprintf("%x", sha256.Sum256(blob)))
 		sig.Content = blob
-		sig.Annotations = map[string]string{
-			SignatureManagedAnnotation: "true",


@mfojtik does the controller do something with this annotation later?

No, there's no other reference to the annotation apart those referenced here.

no and I don't remember adding this

deads2k · 2018-03-20T18:24:57Z

pkg/image/registry/image/strategy.go

+	// bug in image update logic allowed to set arbitrary annotations that would otherwise be rejected by
+	// validation; resolves rhbz#1557607
+	for i := range newImage.Signatures {
+		newImage.Signatures[i].Annotations = map[string]string{}


I would only drop the old controller annotation.

yeah i guess that makes sense. for any other annotation they'll run into the validation rule.

bparees · 2018-03-20T20:42:04Z

pkg/image/registry/image/strategy.go

@@ -49,6 +52,8 @@ func (s imageStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Obje
 		newImage.DockerImageManifest = ""
 		newImage.DockerImageConfig = ""
 	}
+
+	removeManagedSignatureAnnotation(newImage)


since it's now only going to drop the one annotation, i think i'm ok w/ doing this on create, for the (unlikely) backup/restore use case. @deads2k ?

since it's now only going to drop the one annotation, i think i'm ok w/ doing this on create, for the (unlikely) backup/restore use case. @deads2k ?

Ok.

deads2k · 2018-03-21T12:59:20Z

pkg/image/registry/image/strategy.go

@@ -16,6 +16,9 @@ import (
 	"github.com/openshift/origin/pkg/image/util"
 )

+// ManagedSignatureAnnotation used to be set by image signature import controller as a signature annotation.
+const ManagedSignatureAnnotation = "image.openshift.io/managed-signature"


nit: private, right?

good catch, fixed

deads2k · 2018-03-21T13:00:13Z

I think this is our best route forward.

@smarterclayton @liggitt any alternative ideas? We'll have to pick this fairly far back.

bparees · 2018-03-21T13:44:39Z

We'll have to pick this fairly far back.

back to 3.7, no further right?

bparees · 2018-03-21T19:48:25Z

We'll have to pick this fairly far back.
back to 3.7, no further right?

I see now, the validation check itself (the swap args) needs to be backported further to ensure users don't update image objects w/ invalid content.

deads2k · 2018-03-21T22:51:46Z

Add more commits by pushing to the naughty-image-signature-annotation branch on miminar/origin.

@mfojtik and that's what we think of your annotation! :)

bparees · 2018-03-21T22:54:48Z

/lgtm

openshift-bot · 2018-03-21T23:29:51Z

/retest