Fix Pushing Bundles that have a relocationMap #1815

anjayajodha · 2021-11-05T19:40:45Z

What does this change

This allows users to successfully copy bundles that use source images in a repository they don't have access to.

What issue does it fix

Closes #1814

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

Unit Tests
Documentation
Schema (porter.yaml)

If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Signed-off-by: Anjay Ajodha <anajod@microsoft.com>

carolynvs · 2021-11-08T15:55:22Z

@vdice I was reviewing this PR and wondering if you had any extra context on why when Porter publishes from an archive, that we don't use a relocation mapping file and preserve the original bundle.

See this very wonderfully long function comment that you wrote for context:

porter/pkg/porter/publish.go

Lines 172 to 186 in 784d61c

    
           // publishFromArchive (re-)publishes a bundle, provided by the archive file, using the provided tag. 
        
           // 
        
           // After the bundle is extracted from the archive, we iterate through all of the images (invocation 
        
           // and application) listed in the bundle, grab their digests by parsing the extracted 
        
           // OCI Layout, rename each based on the registry/org values derived from the provided tag 
        
           // and then push each updated image with the original digests 
        
           // 
        
           // Finally, we generate a new bundle from the old, with all image names and digests updated, based 
        
           // on the newly copied images, and then push this new bundle using the provided tag. 
        
           // (Currently we use the docker/cnab-to-oci library for this logic.) 
        
           // 
        
           // In the generation of a new bundle, we therefore don't preserve content digests and can't maintain 
        
           // signature verification throughout the process.  Once we wish to preserve content digest and such verification, 
        
           // this approach will need to be refactored, via preserving the original bundle and employing 
        
           // a relocation mapping approach to associate the bundle's (old) images with the newly copied images.

This PR is correcting a bug in Porter where when we copy a bundle from one registry to another, we aren't preserving the relocation map. I was checking all the code that I thought would use that function and wasn't clear on why we are essentially "re-publishing" the bundle from the archive instead of preserving the original.

carolynvs

Before we can merge this, it will require a test that verifies the new copy behavior. Since there's been a bunch of churn in our integration testing and it's not terribly straightforward to add this type of test, I will push a commit to your PR adding in an integration test to validate this scenario (copying without access to the original images).

carolynvs · 2021-11-08T16:02:33Z

pkg/cnab/cnab-to-oci/registry.go

@@ -114,6 +114,36 @@ func (r *Registry) PushBundle(bun bundle.Bundle, tag string, insecureRegistry bo
 	return &rm, nil
 }

+// Pushes a bundle while using a relocationMap to correctly locate copied images. Identical to above, except for remotes.WithRelocationMap(reloMap)
+func (r *Registry) PushBundleWithRelocationMap(bun bundle.Bundle, tag string, reloMap relocation.ImageRelocationMap, insecureRegistry bool) (*relocation.ImageRelocationMap, error) {


There are three cases where we call the original function PushBundle:

porter publish, this doesn't have a relocation map.

porter publish --archive I have pulled in @vdice to get more context on this one.

porter copy, which is what you are fixing.

Porter does not have an API compatibility guarantee at the code level, so we are free to edit the signature of the original function. Would you please update the original function PushBundle with the additional parameter reloMap relocation.ImageRelocationMap. In case #1 above, since we don't have one, we can pass in an empty relocation map.

vdice · 2021-11-08T17:02:47Z

@vdice I was reviewing this PR and wondering if you had any extra context on why when Porter publishes from an archive, that we don't use a relocation mapping file and preserve the original bundle.

Thanks for linking that comment! Phew, written 2 years ago :) If only it had supplied the reasoning on why we (apparently) weren't ready to harness the relocation image mapping at the time. Alas, I can't recall the reasoning myself...

Indeed, it seems like the linked functionality (publish from archive) should be refactored to use the relocation mapping file, when present.

carolynvs · 2021-11-08T17:28:19Z

Thanks @vdice! I'll create a follow-up issue to change publish from archive to preserve the original bundle and use a relocation map.

#1815

Validate that we are using the relocation map when copying the bundle. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs · 2021-11-09T15:55:43Z

Oops, sorry about that the new test I added failed when run on CI. I'll fix that right now.

I've cherry-picked some test improvements from main to help with the new porter copy test: * track the repo root and always use absolute paths to avoid problems caused by tests that change directories I've also added the last assert in the copy test that was accidentally not included in the prior commit. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

* Ensure that the relocation map is initialized (not null) before calling into cnab-to-oci since it assumes the map is not null Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

We refactored the existing function so this variation isn't needed anymore. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs · 2021-11-22T16:55:51Z

@anjayajodha Thanks for the patch! I have fixed the build and will merge once it passes. 👍

carolynvs · 2021-11-22T17:27:53Z

/azp run porter-integration

azure-pipelines · 2021-11-22T17:28:02Z

Azure Pipelines successfully started running 1 pipeline(s).

carolynvs

Thanks again for calling this out and fixing it! 💯

* Fix Pushing Bundles that have a relocationMap (getporter#1815) * chore: add avinashupadhya99 to Contributors.md * docs: remove accidental packr word from contribution guide * Improve error message when cnab-to-oci fixes up a bundle * docs: fix command in contrib tutorial * docs: update parameters doc with allowable types (getporter#1792) Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

anjayajodha requested review from carolynvs, jeremyrickard and vdice as code owners November 5, 2021 19:40

adding PushBundleWithRelocationMap function

5fcd996

Signed-off-by: Anjay Ajodha <anajod@microsoft.com>

anjayajodha force-pushed the fix-copy-with-relocationmap branch from fe62d24 to 5fcd996 Compare November 5, 2021 19:42

carolynvs self-assigned this Nov 8, 2021

carolynvs requested changes Nov 8, 2021

View reviewed changes

carolynvs mentioned this pull request Nov 8, 2021

Publish from archive should use a relocation map #1819

Closed

Add a smoke test for porter copy

c71e4d7

Validate that we are using the relocation map when copying the bundle. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs and others added 4 commits November 9, 2021 11:12

Added reloMap parameter into PushBundle

1e3f931

Initialize relocation map before pushing

ac110ce

* Ensure that the relocation map is initialized (not null) before calling into cnab-to-oci since it assumes the map is not null Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

Remove unused test helper

b36064a

We refactored the existing function so this variation isn't needed anymore. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs approved these changes Nov 22, 2021

View reviewed changes

carolynvs merged commit c1e1a04 into getporter:main Nov 22, 2021

carolynvs mentioned this pull request Nov 23, 2021

Sync v1 #1840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Pushing Bundles that have a relocationMap #1815

Fix Pushing Bundles that have a relocationMap #1815

anjayajodha commented Nov 5, 2021

carolynvs commented Nov 8, 2021

carolynvs left a comment

carolynvs Nov 8, 2021

vdice commented Nov 8, 2021

carolynvs commented Nov 8, 2021 •

edited

Loading

carolynvs commented Nov 9, 2021

carolynvs commented Nov 22, 2021

carolynvs commented Nov 22, 2021

azure-pipelines bot commented Nov 22, 2021

carolynvs left a comment

Fix Pushing Bundles that have a relocationMap #1815

Fix Pushing Bundles that have a relocationMap #1815

Conversation

anjayajodha commented Nov 5, 2021

What does this change

What issue does it fix

Notes for the reviewer

Checklist

carolynvs commented Nov 8, 2021

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs Nov 8, 2021

Choose a reason for hiding this comment

vdice commented Nov 8, 2021

carolynvs commented Nov 8, 2021 • edited Loading

carolynvs commented Nov 9, 2021

carolynvs commented Nov 22, 2021

carolynvs commented Nov 22, 2021

azure-pipelines bot commented Nov 22, 2021

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs commented Nov 8, 2021 •

edited

Loading