Remove labels and annotations validations check for imagesignature #19011
Conversation
Currently, labels and annotaions for ImageSignature are regarded as invalid in validation check. However, OpenShift puts `SignatureManagedAnnotation: "true"` in `DownloadImageSignatures()` by itself*1. This patch changes to remove the validations. *1 https://github.com/openshift/origin/blob/release-3.7/pkg/image/controller/signature/container_image_downloader.go#L65-L67
openshift-ci-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nak3 Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This address https://bugzilla.redhat.com/show_bug.cgi?id=1557607 I looked through #9181, but could not find the reason the validations were introduced. |
|
I don't think this is the right solution, i think the validation logic needs to ensure signatures aren't being added, while allowing the existing signatures to remain. But deferring to @miminar who implemented this in the first place, and @deads2k who merged it. @miminar @deads2k this seems like a significant issue for upgrade migration? /assign @miminar |
|
I see. Then, please feel free to close this PR and create your new one. (But as I mentioned many of user's image has the annotation in imagsignature already, so it would be appreciate a solution which saves existing users hopefully.) |
Yes, we have to allow users with image signatures to migrate without removing the signatures. |
| if len(signature.Labels) > 0 { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("metadata").Child("labels"), "signature labels cannot be set")) | ||
| } | ||
| if len(signature.Annotations) > 0 { |
There was a problem hiding this comment.
The decision to avoid these was primarily based on the idea that labels and annotations that cannot be "normally" modified are not terribly useful and break CLI usage. Nested annotations and labels caused problems long ago on imagestreamtags and my fix from way back when didn't even cover all cases (found that out recently).
I wanted to avoid the problem, but this bug #19030 effectively allowed this to be bypassed one time. Now we have a mess.
There was a problem hiding this comment.
right, well in the meantime the imagesignature controller was apparently written to expect that it would be updating/adding these annotations. So was it designed wrong, or do we need to permit this annotations to be added/updated?
There was a problem hiding this comment.
right, well in the meantime the imagesignature controller was apparently written to expect that it would be updating/adding these annotations. So was it designed wrong, or do we need to permit this annotations to be added/updated?
I would strip that out in the PrepareForUpdate unless @mfojtik has a strong reason for needing it.
There was a problem hiding this comment.
presumably we should also remove the logic from the controller that is currently adding this annotation.
|
Originally, the signature annotations weren't supposed to be set at all until we find a use for that. However, due to a bug fixed here, the annotations were actually allowed to propagate into image. Since we do set this annotation in a controller and the annotation is already there in etcd of a lot of users, we should whitelist it in order not to reject further image updates once #19030 is merged. |
…tion Automatic merge from submit-queue (batch tested with PRs 18905, 18968, 19016, 19037, 19056). Drop image signature annotations Because they were never meant to be allowed. Includes and superseds #19030 (kudos to @deads2k) Closes #19011 Resolves: [rhbz#1557607](https://bugzilla.redhat.com/show_bug.cgi?id=1557607) /assign @bparees @deads2k /cc @mfojtik
Currently, labels and annotaions for ImageSignature are regarded as
invalid in validation check. However, OpenShift puts
SignatureManagedAnnotation: "true"inDownloadImageSignatures()byitself*1.
This patch changes to remove the validations.
*1 https://github.com/openshift/origin/blob/release-3.7/pkg/image/controller/signature/container_image_downloader.go#L65-L67