pkg/cli/admin/upgrade: Reject by-tag pullspecs #238
Conversation
openshift-ci-robot
added
the
size/XS
wking
force-pushed
the
oc-adm-upgrade-by-tag-guard
branch
from
8a2ac41 to
689e779
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
CC @smarterclayton, since you hit this when we were updating the CI build cluster. |
The cluster-version operator requires by-digest pullspecs [1,2] since verification landed in the CVO [3]. oc could translate by-tag pullspecs into by-digest pullspecs before updating the ClusterVersion object, but we might want it to perform some local validation first (e.g. checking that the tag matched the version name compiled into release-metadata). Until we have oc-side translation (which we may never have), add a client-side guard that explains the limitation so the user can find a by-digest pullspec themselves. That might be annoying, but it's nicer than happily passing the by-tag pullspec into ClusterVersion and waiting for them to notice the generic "the image may not be safe to use" failure message [4]. [1]: https://github.com/openshift/cluster-version-operator/blob/a45fa12c42047b24a70d8edaa17b85a7eaf6ad38/pkg/cvo/updatepayload.go#L87-L91 [2]: https://github.com/openshift/cluster-version-operator/blob/a45fa12c42047b24a70d8edaa17b85a7eaf6ad38/pkg/verify/verify.go#L218-L220 [3]: openshift/cluster-version-operator@55e3cb4#diff-15212450f32771cf972f0f81d63c78c0R212 [4]: https://github.com/openshift/cluster-version-operator/blob/a45fa12c42047b24a70d8edaa17b85a7eaf6ad38/pkg/payload/task.go#L188-L189
wking
force-pushed
the
oc-adm-upgrade-by-tag-guard
branch
from
689e779 to
d08a4e7
Compare
Dunno what that's about. Maybe a flake. /retest |
|
@smarterclayton might feel like this blanket block is too strong. #390 has a softer version. I'm fine with either, but would like to land something in this space ;). |
|
The softer #390 landed. I wish we could reject like this PR, but I guess we need to maintain backwards compat for crazy people who are used to updating to by-tag digests :p. /close |
|
@wking: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
2 participants
The cluster-version operator requires by-digest pullspecs since verification landed in the CVO.
occould translate by-tag pullspecs into by-digest pullspecs before updating the ClusterVersionobject, but we might want it to perform some local validation first (e.g. checking that the tag matched the version name compiled into
release-metadata). Until we haveoc-side translation (which we may never have), add a client-side guard that explains the limitation so the user can find a by-digest pullspec themselves. That might be annoying, but it's nicer than happily passing the by-tag pullspec into ClusterVersion and waiting for them to notice the genericthe image may not be safe to usefailure message.