Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imgpkg-003 Rename images when copying bundles #22

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

joaopapereira
Copy link
Member

A proposal based on the following issues:
carvel-dev/imgpkg#60
carvel-dev/kbld#79

Base automatically changed from master to develop March 9, 2021 22:36
@pivotaljohn pivotaljohn changed the title Rename images when copying bundles imgpkg-003 Rename images when copying bundles Mar 10, 2021
@joaopapereira joaopapereira marked this pull request as ready for review March 12, 2021 18:56
Add title for some Use Cases with this proposal
Add description of the Overrides section of the CopyWithRename structure
Reply to some open questions
Add one more reason for not having this implemented from the beginning
Add new matching capability
Add implementation split into phases
Add ECR Repository creation constraint
Change names of the strategies to be easier to understand
Add a new Use Case to ensure `imgpkg` can tag images when copies them
Add requirement for `kbld` to add annotations when creating ImagesLock
Add functions to `ytt` starlark library to allow check for tags and
retrieve the registry location for an image
Add more schema information about types and what fields are required
Answer one question
Update the name of the matchers to keep consistency with `kbld`
Add note about not allowing `overrides` and `mappingFunctionYttStar` to
be present in the same configuration
- Make more clear some restrictions
- Rewords parts of the proposal
- Correct some notation
strategy: MaintainOriginRepository
overrides:
- image: public-reg.io/exact-app@sha256:aaaaaaa
destinationRepo: myname/exact-app
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused here based on the above discussions. Will this override result in the image being found at destination.io/myname/exact-app? What happens when a user copies with the destination as destination.io/my-namespace because of the above behavior? Would the image be located at destination.io/my-namespace/myname/exact-app or destination.io/myname/exact-app?

It feels like we have a partial definition of what constitutes the repo portion of a url, which is resulting in us having three different meanings for the term based on where it is used:

  1. imageRepo matcher - should contain full url which has registry, namespace, and repo
  2. destinationRepo - shouldn't contain registry but is allowed to contain both namespace and repo
  3. Preserved repo when using MaintainOriginRepository - only recognizes last portion of url as repo (drops namespace and registry which are included in above defs)

My understanding is that repo is the last segment of the url. For example, based on the terminology section, I would expect index.docker.io/_/nginx@sha256:... to have:

  • registry: index.docker.io
  • namespace: _
  • repo: nginx
  • image: sha256...

This understanding may be incorrect (and potentially harder to implement because ggcr doesn't have the idea of namespace iirc) but if it is, I think it should be clarified and made consistent wherever the terms are used or else users will have no way of knowing how we've chosen to define repo in a particular location.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ewrenn8 I made some changes to the verbiage of the full proposal and tried to make it consistent. Please check it out and if it looks good, please resolve this issue

- Add Use Case for the replicated registry
- Add question about replicated registries
Change the way Destination field works
proposals/imgpkg/003-copy-bundles-with-rename/Readme.md Outdated Show resolved Hide resolved
```

When copying a Bundle to `destination.io/bundle` all OCI Images will be copied
to `destination.io/{Original Repository Name}`

Choose a reason for hiding this comment

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

Again, what's the command, and where's destination.io coming from?

Choose a reason for hiding this comment

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

Now I see the examples below, but not the imgpkg command, so not so obvious.

proposals/imgpkg/003-copy-bundles-with-rename/Readme.md Outdated Show resolved Hide resolved
proposals/imgpkg/003-copy-bundles-with-rename/Readme.md Outdated Show resolved Hide resolved

Function will remove all information from an OCI Image location and return the Image name.

Definition: `def imageName(image) -> string`

Choose a reason for hiding this comment

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

Interesting as above this is described as the Repository (although I find that name confusing and I like better image name).

@jorgemoralespou
Copy link

Is there a way to capture in the ImagesLock file (or in a resulting file) the result of the copy so that it can be tracked? Source image, destination image and strategy used on copy.?

- Add commands to show how to use the configuration file
- Better explain the exact mathc by tag
- Add Image that helps with terminology
- Convert strategy name `MaintainOriginRepository` to `RelativeToRegistryKeepNamespace`
Strategies:
- Copy all OCI Images to the new Registry and all paths are relative to Bundle
  Namespace and keeps Image Namespace
- Copy all OCI Images to the new Registry and all paths are relative to Bundle
  Namespace
`corp.gcr.io/projects/project-a/bank-app:v1` explodes into:

- **Registry** (`corp.gcr.io`): Server that will be used to store and retrieve OCI Images
- other examples: goharbor.io, gcr.io, index.docker.io
Copy link
Contributor

Choose a reason for hiding this comment

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

may be we should add foo.com/blah as another example of registry to indicate that it's not only hostname.

proposals/imgpkg/003-copy-bundles-with-rename/Readme.md Outdated Show resolved Hide resolved
proposals/imgpkg/003-copy-bundles-with-rename/Readme.md Outdated Show resolved Hide resolved

**Note:** `overrides` and `mappingFunctionYttStar` cannot be present at the same time in a configuration.

#### Copy Strategies
Copy link
Contributor

@cppforlife cppforlife Apr 22, 2021

Choose a reason for hiding this comment

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

i think it would be useful to have RelativeToBundleDashedFullImageName:

blah.com/some/ns/image-name would become registry.com/another-ns/some-ns-image-name

this would better help with cases where multiple unrelated images are called controller.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was mentioned that it could be useful to provide "portable" configuration (i.e. be able to use the same config for multiple imgpkg copy invocations, each targeting a different registry)...

Would we be able to detect when a copy fails because namespaces aren't supported? If so, we could get closer to that portability goal if RelativeToBundleKeepNamespace first attempts with the namespace, as is... and if it fails in a definitive way, could downshift to the dashed image name?

Choose a reason for hiding this comment

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

I think that the fact that there's a new strategy RelativeToBundleDashedFullImageName almost similar to an existing one RelativeToBundle to me denotes that multiple variations of strategies will come, and not just to one, but to many, so eventually there will be a explosion of strategies to cope with this subtleties. I would recommend not adding so many strategies but adding additional configuration to the strategies, e.g:

apiVersion: imgpkg.carvel.dev/v1alpha1
kind: CopyConfig
strategy: RelativeToBundle
keep: Namespace|Image|Repositiry
dashed: Namespace|Image|Repository

So that with some additional attributes you can get multiple different strategies.

- Correct the ImagesLock yaml
- Enumerate the strategies for better visibility
- Start using ytt modules, instead of create things like regexMatch.
  Since we will have access to these module we can take advantage of all
  of them
@joaopapereira joaopapereira added the proposal This issue contains a proposal that is being discussed with the community label May 6, 2021
@braunsonm
Copy link

Any update on this? Would still be great to see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required proposal This issue contains a proposal that is being discussed with the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants