Selectively replace the org when moving images #1047

carolynvs · 2020-05-15T21:51:50Z

What does this change

Don't use a string replace because when the same text is used in the org
and the image name and the registry, it can end up changing more text
than it should.

Don't replace the org when there isn't one present. Some registries
embed the org in the domain, e.g. getporter.azurecr.io, in which case
the path of the tag is the name of the bundle, and we shouldn't be
appending that to the images.

What issue does it fix

Closes #1040

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

Unit Tests
Documentation (working on that now)
Schema (porter.yaml) - Not Impacted

Don't use a string replace because when the same text is used in the org and the image name and the registry, it can end up changing more text than it should. Don't replace the org when there isn't one present. Some registries embed the org in the domain, e.g. getporter.azurecr.io, in which case the path of the tag is the name of the bundle, and we shouldn't be appending that to the images.

carolynvs · 2020-05-18T14:59:43Z

Should we have the same logic for renaming images between porter publish --archive and porter copy? This PR was fixing a bug in porter publish --archive and when I went to compare what porter copy does so that I could document them on the website (which is implemented in cnab-to-oci) I realized that they do different things... It seems confusing and honestly probably not what any user wants.

Here's what they do today:

Archive

porter publish --archive whalegap.tgz --tag example.com/org2/whalegap:v0.1.0.
- localhost:5000/org1/whalegap:v0.1.0 -> example.com/org2/whalegap:v0.1.0
- localhost:5000/org1/whalegap-installer -> example.com/org2/whalegap-installer
- nginx -> example.com/org2/nginx

Copy

porter copy --source localhost:5000/org1/whalegap:v0.1.0 --destination example.com/org2/whalegap:v0.1.0.
- localhost:5000/org1/whalegap-installer -> example.com/org2/whalegap/whalegap-installer
- nginx -> example.com/org2/whalegap/nginx

The key difference is that images are tagged under not only the org but the name of the bundle.

I wish I could find the original issue where we discussed how the image relocation works in cnab-to-oci and why it puts things where it does. Can anyone find it?

vdice · 2020-05-18T15:29:08Z

Interesting. porter publish --archive seems like the "right" approach to publishing/copying, from one org to another -- no extra "namespacing" on the resulting images. Is it porter copy that delegates to the cnab-to-oci library? Maybe we can engage discussion with the maintainers and/or at the next CNAB meeting to learn about the reasoning/design.

squillace · 2020-05-18T15:34:47Z

Interesting. porter publish --archive seems like the "right" approach to publishing/copying, from one org to another -- no extra "namespacing" on the resulting images. Is it porter copy that delegates to the cnab-to-oci library? Maybe we can engage discussion with the maintainers and/or at the next CNAB meeting to learn about the reasoning/design.

Yeah, the problem with the installer under the bundle name is that we are now imposing some "order" on the tagging without any plan for why this is necessary. If I do a porter publish now, my installer is on its own, not hierarchical with bundle. Why do I care? The bundle.json and porter do the tracking for me, and I can go look through them if necessary by tag specifically.

I'm with Vaughn: the porter publish --archive behavior seems logical. QUESTION: what happens when a nested installer is used with porter public --archive? Is the nesting recapitulated? That is, if a source bundle is org/bundle and a source installer is org/bundle/installer is that hierarchy preserved when porter publish --archive is called? It should be, but if it isn't, it is wrong to insert a bundle into a structure that did not have it previously because, as you say, it's "arbitrary" which means users will need to know it before they understand it.

carolynvs · 2020-05-20T16:09:12Z

if a source bundle is org/bundle and a source installer is org/bundle/installer is that hierarchy preserved when porter publish --archive is called?

I just tested your scenario with a custom invocation image that uses bundle/installer instead of bundle-installer

porter publish --archive --tag example.com/neworg/apache:v0.1.0

localhost:5000/myorg/apache/installer -> example.com/neworg/apache/installer

carolynvs · 2020-05-20T16:20:32Z

Is it porter copy that delegates to the cnab-to-oci library? Maybe we can engage discussion with the maintainers and/or at the next CNAB meeting to learn about the reasoning/design.

Yes, porter copy uses cnab-to-oci. I can't seem to find where a decision was made to namespace under the bundle name. That logic happens from this code:

https://github.com/cnabio/cnab-to-oci/blob/c9599a06b976f4cd5f505092292776caa924b549/remotes/fixup.go#L202

@radu-matei Do you have a link to why cnab-to-oci behaves this way? Where images referenced by a bundle, when copied to another registry are put under newregistry/ORG/BUNDLENAME/IMAGENAME, instead of newregistry/ORG/IMAGENAME?

radu-matei · 2020-05-26T18:30:28Z

cnab-to-oci [uses an OCI index])(https://github.com/cnabio/cnab-to-oci/#selection-of-oci-index) to represent a bundle in the registry - specifically, the bundle descriptor (bundle.json), the invocation image, and component images are part of the said index.

However, an index can only reference objects that are part of the same repository - this will eventually enable registries to treat all artifacts in the index as a whole (from vulnerability scanning to garbage collection).
(There is an ongoing discussion in the OCI community about allowing manifests to reference foreign objects, and it could enable this scenario, but the discussion is not advanced enough to continue that discussion).

So technically, because cnab-to-oci publishes an index, it cannot publish the images referenced in the bundle under newregistry/ORG/IMAGENAME.
(There is an open question as to whether it should tag the individual images from newregistry/ORG/BUNDLENAME/IMAGENAME though.)

Ultimately, cnab-to-oci ensures your bundle and referenced artifacts are available in the same repository - so if a user has access to the bundle, they also have access to pulling the images.

carolynvs · 2020-05-28T15:01:00Z

In the cnab meeting yesterday we decided to fix just the bug here, and document what each command is doing under the hood, and what resulting repositories are created.

* Document the intermediate artifacts created by porter publish --archive creates. * Clarify the documentation around how images are named when published.

carolynvs · 2020-05-28T16:16:20Z

I've updated https://deploy-preview-1047--porter.netlify.app/distribute-bundles/#image-references-after-publishing to clarify how images are renamed during publish normally and the intermediate artifacts generated during a porter publish --archive.

radu-matei · 2020-05-28T17:14:02Z

docs/content/distribute-bundles.md

@@ -90,7 +95,18 @@ porter publish -a mybunz1.1.tgz --tag getporter/megabundle:1.1.0

 ## Image References After Publishing

-When a bundle is published, the images that it will use are copied into the location of the published bundle. This simplifies access control and management of artifacts in the repository. Consider the following `porter.yaml` snippet:
+When a bundle is published, all images [referenced][image-map] by the bundle are
+published **inside** bundle repository. Bundles should be written to use the


I don't really follow what is the action a bundle author should take from this.
Is this part not covered by relocation maps?

There is another PR where I've been working on building up docs for how to actually use the relocation map properly in your bundle. For example just populating the images section isn't enough, you also then have to use helm charts that understand image digests (not tags which they all use) and then actually use the relocation map to get the final location of your images. This is something that porter helps with.

That PR is being held up by this bug at the moment. So I can explain more and then link to that once this is merged. I'll make a note of it on that PR?

radu-matei · 2020-05-28T17:14:16Z

docs/content/distribute-bundles.md

+  * REGISTRY/ORG/BUNDLE@**REFERENCED_IMAGE_1_DIGEST**
+  * REGISTRY/ORG/BUNDLE@**REFERENCED_IMAGE_2_DIGEST**
+
+NOTE: Digest refers to the the repository digest (not the image id).


Maybe link this section of the OCI specification?

radu-matei · 2020-05-28T17:17:21Z

docs/content/distribute-bundles.md

+process and they are not used by the final bundle.
+
+Using the example above, the following repositories are created and should be
+ignored:


While accurate for scenarios where users are only interested in the bundle alone, as we saw in the airgapped environments meeting, having the images in their own repositories could be a valid use case for a subset of users.

So rather than a side effect, we could potentially see this as a feature.

I'll change to "can be ignored" because for the purpose of the bundle, they are not used and we need to make that clear but yeah they could use it if they wanted to.

radu-matei

I have a few very small comments, but overall, the document does a great job explaining the current behaviour of the two commands.

Thanks!

vdice

Only optional grammatical suggestions. LGTM. Great clarification in added docs.

docs/content/distribute-bundles.md

Co-authored-by: Vaughn Dice <vadice@microsoft.com>

carolynvs force-pushed the fix-rename-img branch from 8ade604 to 50b0ad7 Compare May 28, 2020 16:12

Doc artifacts publish --archive creates

f571926

* Document the intermediate artifacts created by porter publish --archive creates. * Clarify the documentation around how images are named when published.

carolynvs force-pushed the fix-rename-img branch from 50b0ad7 to f571926 Compare May 28, 2020 16:16

carolynvs marked this pull request as ready for review May 28, 2020 16:16

carolynvs requested review from jeremyrickard and vdice as code owners May 28, 2020 16:16

carolynvs added this to In Progress in Porter and Mixins [OLD] via automation May 28, 2020

carolynvs requested a review from radu-matei May 28, 2020 16:17

radu-matei reviewed May 28, 2020

View reviewed changes

radu-matei approved these changes May 28, 2020

View reviewed changes

carolynvs mentioned this pull request May 28, 2020

Add airgap example #1042

Merged

4 tasks

Incorporate feedback

061ec5f

vdice approved these changes May 28, 2020

View reviewed changes

docs/content/distribute-bundles.md Outdated Show resolved Hide resolved

docs/content/distribute-bundles.md Outdated Show resolved Hide resolved

Apply suggestions from code review

e594bf3

Co-authored-by: Vaughn Dice <vadice@microsoft.com>

carolynvs merged commit 372ca70 into getporter:master May 29, 2020

Porter and Mixins [OLD] automation moved this from In Progress to Done May 29, 2020

carolynvs deleted the fix-rename-img branch May 29, 2020 14:00

vdice mentioned this pull request Jun 3, 2020

Porter archive restore not taking repository from tag - always pointing to docker.io #1055

Closed

vdice removed this from Done in Porter and Mixins [OLD] Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selectively replace the org when moving images #1047

Selectively replace the org when moving images #1047

carolynvs commented May 15, 2020 •

edited

carolynvs commented May 18, 2020 •

edited

vdice commented May 18, 2020

squillace commented May 18, 2020 •

edited

carolynvs commented May 20, 2020

carolynvs commented May 20, 2020

radu-matei commented May 26, 2020

carolynvs commented May 28, 2020

carolynvs commented May 28, 2020

radu-matei May 28, 2020

carolynvs May 28, 2020

radu-matei May 28, 2020

radu-matei May 28, 2020

carolynvs May 28, 2020

radu-matei left a comment

vdice left a comment

Selectively replace the org when moving images #1047

Selectively replace the org when moving images #1047

Conversation

carolynvs commented May 15, 2020 • edited

What does this change

What issue does it fix

Notes for the reviewer

Checklist

carolynvs commented May 18, 2020 • edited

vdice commented May 18, 2020

squillace commented May 18, 2020 • edited

carolynvs commented May 20, 2020

carolynvs commented May 20, 2020

radu-matei commented May 26, 2020

carolynvs commented May 28, 2020

carolynvs commented May 28, 2020

radu-matei May 28, 2020

Choose a reason for hiding this comment

carolynvs May 28, 2020

Choose a reason for hiding this comment

radu-matei May 28, 2020

Choose a reason for hiding this comment

radu-matei May 28, 2020

Choose a reason for hiding this comment

carolynvs May 28, 2020

Choose a reason for hiding this comment

radu-matei left a comment

Choose a reason for hiding this comment

vdice left a comment

Choose a reason for hiding this comment

carolynvs commented May 15, 2020 •

edited

carolynvs commented May 18, 2020 •

edited

squillace commented May 18, 2020 •

edited