Bug 1686589 - Cincinnati should return digests, not tags in the graph
Summary: Cincinnati should return digests, not tags in the graph
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: Cluster Version Operator
Version: 4.1.0
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: ---
: 4.3.0
Assignee: Stefan Junker
QA Contact: liujia
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-03-07 18:44 UTC by Clayton Coleman
Modified: 2019-11-08 16:26 UTC (History)
12 users (show)

Fixed In Version:
Doc Type: Enhancement
Doc Text:
Feature: Deterministic payload references Reason: Payload references aren't deterministic. Applying the same release of the same graph at different points in time might yield different results. Result: Payload references contain the SHA of the image the release was found at, if the container registry provides the manifestref.
Clone Of: 1685225
Environment:
Last Closed: 2019-11-08 16:26:05 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Github openshift cincinnati pull 158 'None' 'closed' 'graph-builder/registry: require the manifestref' 2019-12-04 07:41:19 UTC

Description Clayton Coleman 2019-03-07 18:44:38 UTC
In order to guarantee reproducibility, Cincinnati should return digests instead of tags:

{
  "nodes":[
    {
      "version":"4.0.0-5",
      "payload":"quay.io/openshift-release-dev/ocp-release:4.0.0-5",
      "metadata":{}
    },
...

Payload should be quay.io/openshift-release-dev/ocp-release@sha256:xxxxxxxx 

Prevents attackers from seizing control of our tags and redirecting to a bad release image, and also prevents lack of clarity about what version a cluster is going to use.

Comment 1 Stefan Junker 2019-03-08 22:17:52 UTC
In a recent version of Cincinnati a node in the graph looks as follows:

...
    {
      "version": "4.0.0-0.5",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.5",
      "metadata": {
        "com.openshift.upgrades.graph.release.manifestref": "sha256:6bfad9b32e8f4f94ba08424db4242da03b4dd14c7a65e216d69f5f13508882cd"
      }
    },
...

Is that fine or do you need the information in "payload"?

Comment 2 Stefan Junker 2019-03-20 20:09:20 UTC
The requested change is currently waiting for a review at https://github.com/openshift/cincinnati/pull/75.

Comment 3 Stefan Junker 2019-03-21 14:57:42 UTC
For a discussion in the review process I would like to get input from the reporter.

Is it acceptable that in environments where the container registry does not provide the manifestref to Cincinnati it will continue to return tag instead of digest? I have encountered Satellite to not provide the manifestref during integration experiments and have therefore made that codepath depend on the availability. So in most cases https://github.com/openshift/cincinnati/pull/75 will deliver the digest, but in some it might not.
The alternative to this would be to make this a requirement towards container registries and treat it as an error if it's failed to be provided.

Comment 4 Stefan Junker 2019-03-22 19:41:44 UTC
https://github.com/openshift/cincinnati/pull/75 has been merged and is running at staging.

The result from there contains:

...
    {
      "version": "4.0.0-5",
      "payload": "quay.io/openshift-release-dev/ocp-release@sha256:ed45d79377a8f36aa5db7be63c1278ae864031ec3d50b80fdd9902c6be4517ba",
      "metadata": {
        "com.openshift.upgrades.graph.release.manifestref": "sha256:ed45d79377a8f36aa5db7be63c1278ae864031ec3d50b80fdd9902c6be4517ba"
      }
...

Comment 5 Brenton Leanhardt 2019-03-28 14:00:24 UTC
Clayton, would you be able to answer Stefan's question from Comment #1?

Comment 6 Stefan Junker 2019-03-29 18:21:12 UTC
Just adding a note to Brenton's request.

The question in comment 1 is obsolete with regards to the api.openshift.com instance of Cincinnati, but it might still be valid for other environments. Therefore please consider comment 3 when answering.

Comment 7 Clayton Coleman 2019-04-02 19:47:50 UTC
Supporting SHAs should be a requirement of 4.x in general - I'd like to dig in why Satellite is not returning that.

We need the payload pointing to the SHA

Comment 8 Stefan Junker 2019-04-04 16:11:19 UTC
I requested feedback on this from the Satellite team there will be a meeting on this tomorrow. Specifically requesting info from Partha for this ticket, thanks!

Comment 9 Stefan Junker 2019-04-11 18:17:00 UTC
It looks like we're not going to have a quick fix for this in Satellite. In order to remain compatible with Satellite for test setups, and keeping in mind that this bug is already fixed for the api.openshift.com deployments, the two options are:

1. Introduce a flag to Cincinnati which controls if the digest is required from the registry, which would default to 'true' and would need to be set to 'false' for Satellite test setups
2. Move this ticket to 4.2 and wait it out

Choosing option (2)

Comment 10 Brenton Leanhardt 2019-08-29 18:01:12 UTC
I'm assuming this is not a blocker or it would have been resolved long ago.  Stefan, would you mind providing an update here?

Comment 11 Stefan Junker 2019-09-05 12:45:49 UTC
After digging for upstream changes and repeating the experiments with Satellite I can confirm that the digests are now reported to Cincinnati.
For those interested, this is the relevant upstream change: https://github.com/Katello/katello/pull/8070/files .

There are still a few issues to resolve to make Satellite fully compatible but they are unrelated to this bug.
The last step I see is to enforce getting the digest from the registry in Cincinnati, and treat the absence as a hard error.

Comment 12 Stefan Junker 2019-09-05 19:22:26 UTC
The PR which requires the manifest digest sent by the registry is online: https://github.com/openshift/cincinnati/pull/158

Comment 14 Stefan Junker 2019-11-08 16:26:05 UTC
This has by now landed [1] in Cincinnati production.

[1]: https://gitlab.cee.redhat.com/service/saas-cincinnati/merge_requests/8


Note You need to log in before you can comment on or make changes to this bug.