Skip to content

Commit

Permalink
pkg/cli/admin/upgrade: Client-side by-tag guard
Browse files Browse the repository at this point in the history
Adding a client-side guard like the server-side guard [1] from
openshift/cluster-version-operator@55e3cb450f (verifier: Add public
key verification of release image digests, 2019-04-19,
openshift/cluster-version-operator#170).  This isn't that big a deal,
because the client-side guard exists, but it does save users the
effort of pushing a by-tag pullspec into ClusterVersion and then
having to circle back around to notice that the cluster-version
operator is complaining about the verification failure.

The warning on --force (instead of a hard failure) is because Clayton
considers blocking this flow completely to be a breaking API change,
and also considers that failing those folks early is more invasive
than letting them continue to slip through here and have some subset
of them potentially blow up later if:

1. You ask to update to a by-tag pullspec.
2. Cluster updates.
3. Someone clobbers the tag you used in the registry to point it at a
   different release.
4. Cluster continues on, blissfully unaware.
5. CVO gets rescheduled for whatever reason.
6. Cluster pulls a fresh registry image for the new pod, but it's
   by-tag, so you get the new content.
7. CVO thinks it's still in reconciling mode, because the pullspec
   hasn't changed.
8. World explodes as the new manifests get applied in a parallel,
   randomized order.

Or maybe the new CVO defaults to starting in install mode or some
such.  But still, random release rollouts trigged by CVO pod restarts
are more exitement than I'd wish on anyone, even folks who use
--force.

It would also be acceptable to have oc attempt to resolve the by-tag
pullspec into a by-digest pullspec, but there's no guarantee that the
host running 'oc' has access to the same registry that the in-cluster
CRI-O will be pulling from, so getting consistent client-side
resolution would be tricky.

[1]: https://github.com/openshift/cluster-version-operator/blame/89cb270523675e27a2f54918431170946636f5d5/pkg/verify/verify.go#L203-L204
  • Loading branch information
wking committed May 5, 2020
1 parent 29e9a33 commit 43cb3c8
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions pkg/cli/admin/upgrade/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ func (o *Options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string
if len(ref.ID) == 0 && len(ref.Tag) == 0 {
return fmt.Errorf("--to-image must be a valid image pull spec: no tag or digest specified")
}
if len(ref.Tag) > 0 {
if o.Force {
fmt.Fprintln(o.ErrOut, "warning: Using by-tag pull specs is dangerous, and while we still allow it in combination with --force for backward compatibility, it would be much safer to pass a by-digest pull spec instead")
} else {
return fmt.Errorf("--to-image must be a by-digest pull spec, unless --force is also set, because release images that are not accessed via digest cannot be verified by the cluster. Even when --force is set, using tags is not recommended, although we continue to allow it for backwards compatibility")
}
}
}

cfg, err := f.ToRESTConfig()
Expand Down

0 comments on commit 43cb3c8

Please sign in to comment.