Skip to content

bootstrap: Resolve the release image to a digest early in install#780

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
smarterclayton:lock_release_image
Dec 4, 2018
Merged

bootstrap: Resolve the release image to a digest early in install#780
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
smarterclayton:lock_release_image

Conversation

@smarterclayton
Copy link
Contributor

After we pull the release image, the local store will contain the digest.
Read the digest and use that in place of the tag for CVO rendering, which
will ensure the CVO always starts with a deterministic payload.

This is similar in logic to openshift/cluster-version-operator#51 but can
jump through fewer hoops with podman.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 4, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2018
@smarterclayton
Copy link
Contributor Author

# crictl ps
CONTAINER ID        IMAGE                                                                                                                          CREATED              STATE               NAME                       ATTEMPT
68c75b6eb6953       registry.svc.ci.openshift.org/ci-op-xd3697pv/release@sha256:dbbe83aa9b4471c93d9e4d9c75f2b1137a1609799a734194047d0ce04db4b60f   About a minute ago   Running             cluster-version-operator   0

@smarterclayton
Copy link
Contributor Author

/assign @abhinavdahiya

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do a pull and then inspect here so that all call of <cvo+releaseimage> image <component> are from single image...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won’t repull on the ones above - once we’ve resolved once only an explicit pull would pull again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the current semantics at least guarantee they’re all correct

@smarterclayton
Copy link
Contributor Author

and @wking who reminded me to put it in here too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: WARNING / Warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@abhinavdahiya
Copy link
Contributor

/approve

@smarterclayton
Copy link
Contributor Author

updated with a better comment and slightly better message

Copy link
Member

@wking wking Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should rebase this to pick up #764. Then we can consolidate to a single podman inspect call in the "image has already been pulled" case with something like:

diff --git a/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template b/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template
index 86cec8a..05e04dc 100755
--- a/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template
+++ b/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template
@@ -3,24 +3,27 @@ set -e
 
 mkdir --parents /etc/kubernetes/{manifests,bootstrap-configs,bootstrap-manifests}
 
-if ! podman inspect {{.ReleaseImage}} &>/dev/null; then
-    echo "Pulling release image..."
-    podman pull {{.ReleaseImage}}
+# convert the release image pull spec to an "absolute" form if a digest is available
+RELEASE="$(podman inspect {{.ReleaseImage}} -f '{{"{{"}} index .RepoDigests 0 {{"}}"}}' || true)"
+if [ -z "${RELEASE}" ]; then
+	echo "Pulling release image..."
+	podman pull {{.ReleaseImage}}
+	RELEASE="$(podman inspect {{.ReleaseImage}} -f '{{"{{"}} index .RepoDigests 0 {{"}}"}}')"
 fi
 
-MACHINE_CONFIG_OPERATOR_IMAGE=$(podman run --rm {{.ReleaseImage}} image machine-config-operator)
-MACHINE_CONFIG_CONTROLLER_IMAGE=$(podman run --rm {{.ReleaseImage}} image machine-config-controller)
-MACHINE_CONFIG_SERVER_IMAGE=$(podman run --rm {{.ReleaseImage}} image machine-config-server)
-MACHINE_CONFIG_DAEMON_IMAGE=$(podman run --rm {{.ReleaseImage}} image machine-config-daemon)
+MACHINE_CONFIG_OPERATOR_IMAGE=$(podman run --rm "${RELEASE}" image machine-config-operator)
+MACHINE_CONFIG_CONTROLLER_IMAGE=$(podman run --rm "${RELEASE}" image machine-config-controller)
+MACHINE_CONFIG_SERVER_IMAGE=$(podman run --rm "${RELEASE}" image machine-config-server)
+MACHINE_CONFIG_DAEMON_IMAGE=$(podman run --rm "${RELEASE}" image machine-config-daemon)
 
-KUBE_APISERVER_OPERATOR_IMAGE=$(podman run --rm {{.ReleaseImage}} image cluster-kube-apiserver-operator)
-KUBE_CONTROLLER_MANAGER_OPERATOR_IMAGE=$(podman run --rm {{.ReleaseImage}} image cluster-kube-controller-manager-operator)
-KUBE_SCHEDULER_OPERATOR_IMAGE=$(podman run --rm {{.ReleaseImage}} image cluster-kube-scheduler-operator)
+KUBE_APISERVER_OPERATOR_IMAGE=$(podman run --rm "${RELEASE}" image cluster-kube-apiserver-operator)
+KUBE_CONTROLLER_MANAGER_OPERATOR_IMAGE=$(podman run --rm "${RELEASE}" image cluster-kube-controller-manager-operator)
+KUBE_SCHEDULER_OPERATOR_IMAGE=$(podman run --rm "${RELEASE}" image cluster-kube-scheduler-operator)
 
-OPENSHIFT_HYPERSHIFT_IMAGE=$(podman run --rm {{.ReleaseImage}} image hypershift)
-OPENSHIFT_HYPERKUBE_IMAGE=$(podman run --rm {{.ReleaseImage}} image hyperkube)
+OPENSHIFT_HYPERSHIFT_IMAGE=$(podman run --rm "${RELEASE}" image hypershift)
+OPENSHIFT_HYPERKUBE_IMAGE=$(podman run --rm "${RELEASE}" image hyperkube)
 
-CLUSTER_BOOTSTRAP_IMAGE=$(podman run --rm {{.ReleaseImage}} image cluster-bootstrap)
+CLUSTER_BOOTSTRAP_IMAGE=$(podman run --rm "${RELEASE}" image cluster-bootstrap)
 
 mkdir --parents ./{bootstrap-manifests,manifests}
 
@@ -31,10 +34,10 @@ then
 	# shellcheck disable=SC2154
 	podman run \
 		--volume "$PWD:/assets:z" \
-		"{{.ReleaseImage}}" \
+		"${RELEASE}" \
 		render \
 			--output-dir=/assets/cvo-bootstrap \
-			--release-image="{{.ReleaseImage}}"
+			--release-image="${RELEASE}"
 
 	cp cvo-bootstrap/bootstrap/* bootstrap-manifests/
 	cp cvo-bootstrap/manifests/* manifests/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this case a thing? How can you have an image that you could successfully pull but which doesn't have a digest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone provides a registry that isn't perfectly compatible with v2_2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone provides a registry that isn't perfectly compatible with v2_2.

Then we can't digest the image? How do references work in pre-v2_2? It's still a Merkle DAG, right? If we're relying on registries to tell us what digests are, we have big problems. And aren't all of the release-payload operator references based on digests?

After we pull the release image, the local store will contain the digest.
Read the digest and use that in place of the tag for CVO rendering, which
will ensure the CVO always starts with a deterministic payload.
@smarterclayton
Copy link
Contributor Author

rebased, much cleaner, thanks and PTAL

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 4, 2018
@abhinavdahiya
Copy link
Contributor

/retest

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants