Pre-layer copy hook #218

erikh · 2017-01-19T13:23:00Z

This is a proof-of-concept and still needs tests and other completeness changes
(like for things that are not docker). Please let me know if you'd like to
accept the feature so I know to continue work towards getting this fleshed out
in the other image systems, tests, etc.

erikh/box has a layer editor inside it that allows it to filter out layers as
necessary. This is made possible currently with some hand-rolled image code.
I'd love to move to containers/image but it has a very coarse copy
implementation.

This adds a simple hook that provides the source layer information to a hook in
the copier which returns bool on whether to proceed to continue to copy
(contrast with filepath.Walk).

Here is some example code that demonstrates how it can be used: in this case,
the golang image is pulled from the host docker, the first layer is clipped
off the top and pushed to a new image named erikh/test. The other layer
relationships have been removed and naturally the image is much smaller.

This has proven useful for building minimal images of containers in the past.

package main

import (
	"fmt"

	"github.com/containers/image/copy"
	"github.com/containers/image/docker/daemon"
	"github.com/containers/image/docker/reference"
	"github.com/containers/image/types"
)

func main() {
	ref, err := daemon.ParseReference("docker.io/library/golang:latest")
	if err != nil {
		panic(err)
	}

	img, err := ref.NewImage(nil)
	if err != nil {
		panic(err)
	}
	defer img.Close()

	tgtRef, _ := reference.ParseNamed("docker.io/erikh/test:latest")
	tgt, err := daemon.NewReference("", tgtRef)
	if err != nil {
		panic(err)
	}

	b, _, err := img.Manifest()
	if err != nil {
		panic(err)
	}
	fmt.Println(string(b))

	var i int
	err = copy.Image(nil, tgt, ref, &copy.Options{
		RemoveSignatures: true,
		LayerCopyHook: func(srcLayer types.BlobInfo) bool {
			i++
			return i < 2 // only the first layer
		},
	})

	if err != nil {
		panic(err)
	}
}

runcom · 2017-01-19T13:45:06Z

I've not yet done a full code review but high level concept sounds really good to me. Maybe we can use the callback to also skip layers we're not interested in (or the other way around). I like it. @mtrmac WDYT? I'm not totally sure about the implications this code could have to the way we work with signatures.

erikh · 2017-01-19T14:07:00Z

https://erikh.github.io/box/user-guide/functions/#skip is a practical application of this function and does exactly what you're describing.

…

On Thu, Jan 19, 2017 at 5:45 AM, Antonio Murdaca ***@***.***> wrote: I've not yet done a full code review but high level concept sounds really good to me. Maybe we can use the callback to also *skip* layers we're not interested in (or the other way around). I like it. @mtrmac <https://github.com/mtrmac> WDYT? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6yJm39Pn6rdYfFk8ZB9MdQnzy7rWks5rT2jigaJpZM4LoGoZ> .

mtrmac

This is very similar to #159 , except that it uses a function instead of a prescribed set, and that it tries to update the manifest as well, whereas #159 just punts on that.

At a high level, there’s only so far copy.Image can go as a generic image editing system, without it becoming a completely unmanageable mess (and it is fairly close to that already, and it will get worse with things like #105 ). I don’t really know how flexible we can or want to make it…

For example, a plausible alternative could be that we would instead make some part of imageCopier public (the top-level copylayers, which would in the future handle parallelization? Almost certainly the non-trivial copyLayer should be made available.), and leave it to the callers to coordinate getting the necessary information for types.Image.UpdatedImage.

OTOH it does make sense for UpdatedImage to support adding/removing layers, and that’s, probably, the most difficult part of this.

Overall, I really don’t know right now.

mtrmac · 2017-01-19T18:13:40Z

copy/copy.go

+		// Please keep this policy check BEFORE reading any other information about the image.
+		if allowed, err := policyContext.IsRunningImageAllowed(unparsedImage); !allowed || err != nil { // Be paranoid and fail if either return value indicates so.
+			return errors.Wrap(err, "Source image rejected")
+		}


I’d rather force the caller to provide a policy. It is… not too difficult… to do:

signature.Policy{Default: []signature.PolicyRequirement{signature.NewPRInsecureAcceptAnything()}}

and passing nil here by mistake could be disastrous.

mtrmac · 2017-01-19T18:15:42Z

copy/copy.go

 	}

-	canModifyManifest := len(sigs) == 0
+	canModifyManifest := len(sigs) == 0 || options.RemoveSignatures


Why is this necessary? Per

var sigs [][]byte if options != nil && options.RemoveSignatures { sigs = [][]byte{}

above, this seems redundant.

(Also, options can be nil.)

mtrmac · 2017-01-19T18:17:42Z

image/docker_schema2.go

 	copy := *m // NOTE: This is not a deep copy, it still shares slices etc.
+
 	if options.LayerInfos != nil {
-		if len(copy.LayersDescriptors) != len(options.LayerInfos) {


If we do add support for this, it should land for all manifest schemas simultaneously.

mtrmac · 2017-01-19T18:18:03Z

image/docker_schema2.go

+// Edits the layers in the manifest when called by replacing them with the
+// appropriate infos provided. This is metadata-only -- the actual layers still
+// have to get to the image.
+func (m *manifestSchema2) performEdits(infos []types.BlobInfo) error {


performEdits is too generic a name for this limited operation.

mtrmac · 2017-01-19T18:18:36Z

image/docker_schema2.go

+	m.LayersDescriptors = make([]descriptor, infolen)
+	for i, info := range infos {
+		imageConfig.History[i] = imageHistory{}
+		imageConfig.RootFS.DiffIDs[i] = info.Digest


This is incorrect if the layer is compressed. DiffID values must be the digests of the ~~compressed~~EDIT uncompressed layers.

(See also the UpdatedImageNeedsLayerDiffIDs hack. That decision mechanism would obviously not work with the “anything can be returned” hook, and it is also too late by that time; so “should we compute DiffIDs” would need a complete rethinking.)

mtrmac · 2017-01-19T18:19:27Z

image/docker_schema2.go

+		m.LayersDescriptors[i].Size = info.Size
+		m.LayersDescriptors[i].URLs = info.URLs
+	}
+	imageConfig.RootFS.BaseLayer = m.LayersDescriptors[0].Digest.String()


(Note to self: I didn’t review the config.json edits in detail here, … but this is inconsistent with what manifestSchema1.convertToManifestSchema2 does.)

mtrmac · 2017-01-19T18:22:18Z

copy/copy.go

 	ReportWriter     io.Writer
 	SourceCtx        *types.SystemContext
 	DestinationCtx   *types.SystemContext
+	LayerCopyHook    func(types.BlobInfo) bool


This should really have a more descriptive name; shouldLayerBeCopied or something shorter to that effect; “hook” does not describe the function at all.

… and does this really have to be a function? Could this be a set, as per #159 ?

Or, more generally, if we add “delete layer” here, pretty soon we will be adding an “add a layer at this index”. What would that interface look like, and can we define that instead right now? (Yeah, that would be making this PR more complex. Perhaps we should just merge this, or the “layer subset” non-hook variant, right now, with a // Warning: API likely to change, and not block on the higher-level discussions?)

we pass the layer id so really you could do anything you want, including keeping a table of images you want to keep, which is exactly what box does.

mtrmac · 2017-01-19T18:32:08Z

image/docker_schema2.go

+	}
+
+	// regurgitate the image configuration for recalculation of layers in the event the layer list has been edited.
+	imageConfig := &image{}


There is a fairly high risk that this would drop any fields of config.json added in the future.

erikh · 2017-01-19T19:36:12Z

The two security changes I made seemed necessary at the time but I will back them out.

erikh · 2017-01-19T19:41:20Z

Ok. I will not close this while you decide. I realize the code is not in the best shape; it's merely meant as a discussion-starter.

For the rest of it, I'm focused on it being doable; we accomplish this in box with a layer list we manage outside of the layer list in the image (and then repackage at the end) so there's no reason we couldn't do it that way too -- I just felt a function was a safer bet given how generic they can be. No objection to doing it the other way.

mtrmac · 2017-01-19T19:51:08Z

Ok. I will not close this while you decide. I realize the code is not in the best shape; it's merely meant as a discussion-starter.

Yeah; this strongly depends on how you want to use it in general.

Vaguely, the two major directions I see (but I may well be missing something)

Expose enough of copyLayers that a caller who exactly knows what it needs, and doesn’t care about general API availability (e.g. is restricted to schema2, only uncompressed layers, and the like) can use the copy/compress/verify-digest/future-parallel-copy functionality, and manually do the high-level image manipulation (copying/editing configs, manifests), using some but perhaps not all of the types.Image functions. Negative: Need to know much more about the image and the formats. Positive: almost infinite flexibility for editing the image.
Add an options.LayerEditor hook, which would receive ic.src.LayerInfos() and return an edited value, with some layers deleted, and some added (in which case, probably also supplying an io.Reader for the contents of that layer).:
```
LayerEditor func([]types.BlobInfo) []struct{info types.BlobInfo; sourceIfNewLayer io.Reader}`
```
Then, copy.Image would be on the hook for making these edits work. Negative: More work to support the various edits in containers/image (or a confusing API where some combinations work and some don’t); only those kinds of edits would be possible; Positive: Much easier to use for callers who want to do a single well-targeted edit to the layers only.

Does any of this make sense? Are there other options I have missed? @runcom ?

(And would the LayerSubset parameter instead of a hook from #159 work for you, or is there something about the hook which is more useful?)

mtrmac · 2017-01-19T19:53:02Z

(admittedly the LayerEditor name doesn’t mean anything and in general sucks.)

erikh · 2017-01-19T20:02:08Z

Shooting from the hip; the presence of the hook during the run should be enough to describe a capability to the appropriate image destination that it can reject if it discovers the hook and can't process it. Requires knowledge though, and will need to be maintained as destinations grow as a canonical part of the API. I don't think doing this just in schema2 is the right answer, if that's what you were suggesting. I need to mull over the rest. Will reply in a few hours.

…

On Thu, Jan 19, 2017 at 11:53 AM, Miloslav Trmač ***@***.***> wrote: (admittedly the LayerEditor name doesn’t mean anything and in general sucks.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6yzVr0eyM3ZR_sQNUawrm3wDmnZnks5rT78egaJpZM4LoGoZ> .

mtrmac · 2017-01-19T20:16:28Z

I don't think doing this just in schema2 is the right answer, if that's what you were suggesting.

To be fair, my initial comment on the docker_schema2.go file was probably too strong. It would definitely not be the first case where some types.Image.UpdatedImage functionality is only available for some schemas or schema combinations, and if nobody cared to implement this for $obscure_format, well, nobody would exactly be hurt. But it does make the interface confusing when we can’t easily explain to users what is and is not supposed to work.

erikh · 2017-01-19T20:19:32Z

agree entirely.

…

On Thu, Jan 19, 2017 at 12:16 PM, Miloslav Trmač ***@***.***> wrote: I don't think doing this just in schema2 is the right answer, if that's what you were suggesting. To be fair, my initial comment on the docker_schema2.go file was probably too strong. It would definitely not be the first case where some types.Image.UpdatedImage functionality is only available for some schemas or schema combinations, and if nobody cared to implement this for $obscure_format, well, nobody would exactly be *hurt*. But it does make the interface confusing when we can’t easily explain to users what is and is not supposed to work. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ64qY-Yf4Am5Ga8kz58WVeidfGu2Rks5rT8SdgaJpZM4LoGoZ> .

erikh · 2017-01-20T08:43:33Z

Hmm; maybe make the hook a part of the interface somehow, so it has to at least be considered when implementing a destination?

erikh · 2017-01-20T08:44:23Z

Also please note that I have elided the security changes, so we can focus on the meat. :)

mtrmac · 2017-01-20T08:46:57Z

Hmm; maybe make the hook a part of the interface somehow, so it has to at least be considered when implementing a destination?

As far as layers go, it seems to me that ManifestUpdateOptions.LayerInfos along with the data in .InformationOnly should allow any edits (additions, deletions, reorders). It would be up to copy.Image / copyLayers along with any hooks / edit parameters to supply the necessary info for the ManifestUpdateOptions. Am I missing anything?

erikh · 2017-01-20T08:48:29Z

Sure; I'm talking about the requirement for the hook to be implemented (or at least, considered when implementing a destination).

Unrelated, is there an irc or slack I can join to collaborate with you folks?

erikh · 2017-01-20T08:52:13Z

I'm sorry, I'm not being clear.

For a destination to work with copy, I think it should at least implement or at minimum, do something (perhaps by return an error) to indicate that it cannot do layer edits.

This could be done by satisfying an interface.

I hope that's clearer, sorry.

erikh · 2017-02-15T06:58:56Z

@mtrmac I'm going to try implementing option #2 in #218 (comment) tonight -- please let me know if you'd prefer I do it some other way.

I am already running with a fork with this PR in erikh/box so I could get moving, but I want to realign as soon as we can come to a conclusion of how to implement this.

erikh · 2017-02-15T06:59:13Z

... the layer editing hook, to be precise.

mtrmac · 2017-02-15T17:45:03Z

@mtrmac I'm going to try implementing option #2 in #218 (comment) tonight -- please let me know if you'd prefer I do it some other way.

No, I continue to think that an interface vaguely like that would be general and flexible enough to be useful long-term.

And it is quite acceptable to implement this kind of editing only for one of the image manifest formats in the initial PR, as long as an attempt to do it with the unsupported formats is clearly rejected.

erikh · 2017-02-15T19:42:46Z

Well ideally I'd like to set it up in a way that all (or at least, almost all) editing capabilities are added to image formats across the library; it wasn't my intent just to affect docker images this way. I need this for OCI and friends too.

…

On Wed, Feb 15, 2017 at 9:45 AM, Miloslav Trmač ***@***.***> wrote: @mtrmac <https://github.com/mtrmac> I'm going to try implementing option #2 <#2> in #218 <#218> (comment) tonight -- please let me know if you'd prefer I do it some other way. No, I continue to think that an interface vaguely like that would be general and flexible enough to be useful long-term. And it is quite acceptable to implement this kind of editing only for one of the image manifest formats in the initial PR, as long as an attempt to do it with the unsupported formats is clearly rejected. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ64ZcHPf0rfuFDY768WdZLMQeZ2r_ks5rczmggaJpZM4LoGoZ> .

erikh · 2017-02-19T00:00:02Z

I'm going to leave the new layer reader off for now. I really think this would be better done as a separate method on the src that accepts a reader for each layer.

The rest I'm working on now.

erikh · 2017-02-19T06:20:35Z

I've taken a stab at this (which I have overwritten the previous patch with)
but the crux of the problem nwo seems to be that the config is copied verbatim
(see ConfigBlob() in manifestSchema2 and copyConfig in copy.Image) and editing
it requires knowledge of the manifest format, which is hard to reap in a
portable way here.

Looking for some suggestions on how to resolve this.

mtrmac · 2017-02-21T18:23:40Z

copy/copy.go

+		if err := dest.PutSignatures(sigs); err != nil {
+			return errors.Wrap(err, "Error writing signatures")
+		}
+	*/


no idea what happened here. I'll investigate.

mtrmac · 2017-02-21T18:25:09Z

copy/copy.go

 	srcInfos := ic.src.LayerInfos()
+
+	if ic.layerEditor != nil {
+		srcInfos = ic.layerEditor(ic.src.LayerInfos())


Calling LayerInfos three times is a bit much… at the very least srcInfos = ic.layerEditor(srcInfos)

mtrmac · 2017-02-21T18:27:54Z

docker/daemon/daemon_dest.go

+	retval := types.BlobInfo{Digest: digester.Digest(), Size: inputInfo.Size}
+
+	d.blobs[inputInfo.Digest] = retval
+	return retval, nil


(WRT this, comment changes, empty lines, etc…) I guess, why not, but please, in the final version, one idea per commit. It should be clear why every change was introduced, and in big commits it is often difficult whether a small change is essential for the primary task of the commit or really a typo in an unrelated cleanup which was not supposed to change behavior.

… OTOH, in the extreme, having 10 one-line commits is just fine with me. Obviously correct, easy to review.

mtrmac · 2017-02-21T18:41:09Z

but the crux of the problem nwo seems to be that the config is copied verbatim
(see ConfigBlob() in manifestSchema2 and copyConfig in copy.Image) and editing
it requires knowledge of the manifest format, which is hard to reap in a
portable way here.

Yeah. Still doing that is really necessary.

A possible step towards making this possible could be to replace the existing ManifestUpdateOptions.LayerInfos with something like LayerInfosPreservingDiffIDs, with the existing functionality (editing the manifest only, not the config; we need to preserve this for all currently supported formats to be able compress blobs on upload), and a new LayerInfosReplacement (or a better name?), which requires a config update, and initially fail for all formats. The caller would presumably be allowed to set only one of the two. (Or perhaps ManifestUpdateOptions.LayerInfos just needs to be richer so that it can capture the necessary information in both of these cases.)

And then, we do need to figure out what the required config updates are. Right now, when the layers must come from the original src, the edits can only be a (possibly permuted) subset of the original layers; so we might be able to build the config updates by creating an equivalent (possibly permuted) subset of the original config entries. That seems fairly easy, or I may well be missing something essential about the config format.

The more general case, with arbitrary new layers being introduced, is noticeably harder; luckily we do have the DiffID computation code already, so it would “only” have to be enabled if necessary, but the hook might still want to provide the Created/Author/CreatedBy fields (from schema2 history). Or perhaps nobody cares about this metadata?

erikh · 2017-02-22T00:32:14Z

The problem is not editing the config, it's converting the config to the new layers; which means it has to be unmarshalled first and its properties changed and remarshalled, but to what format? At that level there's no indicator of what format to use, much less convert to.

mtrmac · 2017-02-22T19:39:31Z

At that level there's no indicator of what format to use, much less convert to.

I guess I don’t understand the problem; is there any ambiguity about config formats? We are already parsing / creating config.json files in manifestSchema[12].convertToManifestSchema[12]. It’s somewhat hairy but it seems perfectly doable.

Or are you perhaps saying that a schema2 manifest could refer to an OCI config, or an OCi manifest could refer to a schema1 config?

erikh · 2017-02-22T19:42:54Z

Yes but the abstraction doesn't support that, unless I'm missing something, by the time you're in ConfigBlob() you don't have access to the manifest format to unmarshal/marshal it. It's a straight byte read.

…

On Wed, Feb 22, 2017 at 11:39 AM, Miloslav Trmač ***@***.***> wrote: At that level there's no indicator of what format to use, much less convert to. I guess I don’t understand the problem; is there any ambiguity about config formats? We are already parsing / creating config.json files in manifestSchema[12].convertToManifestSchema[12]. It’s somewhat hairy but it seems perfectly doable. Or are you perhaps saying that a schema2 manifest could refer to an OCI config, or an OCi manifest could refer to a schema1 config? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ60477Un_63TTBNQ_Eah9CXV_nZZaks5rfI71gaJpZM4LoGoZ> .

erikh · 2017-02-22T19:44:22Z

https://github.com/containers/image/blob/master/image/docker_schema2.go#L83 for reference

…

On Wed, Feb 22, 2017 at 11:42 AM, Erik Hollensbe ***@***.***> wrote: Yes but the abstraction doesn't support that, unless I'm missing something, by the time you're in ConfigBlob() you don't have access to the manifest format to unmarshal/marshal it. It's a straight byte read. On Wed, Feb 22, 2017 at 11:39 AM, Miloslav Trmač ***@***.*** > wrote: > At that level there's no indicator of what format to use, much less > convert to. > > I guess I don’t understand the problem; is there any ambiguity about > config formats? We are already parsing / creating config.json files in > manifestSchema[12].convertToManifestSchema[12]. It’s somewhat hairy but > it seems perfectly doable. > > Or are you perhaps saying that a schema2 manifest could refer to an OCI > config, or an OCi manifest could refer to a schema1 config? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#218 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABJ60477Un_63TTBNQ_Eah9CXV_nZZaks5rfI71gaJpZM4LoGoZ> > . >

mtrmac · 2017-02-22T20:01:01Z

Yes but the abstraction doesn't support that, unless I'm missing something, by the time you're in ConfigBlob() you don't have access to the manifest format to unmarshal/marshal it.

What happens is that the copy code, after supplying all needed data, calls .UpdatedImage; that creates an in-memory-only variant of the original manifest (a memoryImage with a new manifest object); that new manifest object is initialized with an edited version of configBlob from the start. Follow e.g. https://github.com/containers/image/blob/master/image/docker_schema1.go#L171 → https://github.com/containers/image/blob/master/image/docker_schema1.go#L284 → https://github.com/containers/image/blob/master/image/docker_schema2.go#L56 .

(And of course the abstraction is not set in stone, image.genericManifest is a private type perfectly subject to change, and even the public types in types.go have been modified fairly frequently in the past. The existing structure does seem to be more or less suitable for this in principle, concentrating all the true complexity in .UpdatedImage and its helper methods, and leaving the rest as fairly simple parsers / getters. It can very likely be improved.)

erikh · 2017-02-22T20:15:44Z

Right, but if you read that code, it takes it right out of the original image. Or does the getblob in the code I linked from above take from the memory image?

…

On Wed, Feb 22, 2017 at 12:01 PM, Miloslav Trmač ***@***.***> wrote: Yes but the abstraction doesn't support that, unless I'm missing something, by the time you're in ConfigBlob() you don't have access to the manifest format to unmarshal/marshal it. What happens is that the copy code, after supplying all needed data, calls .UpdatedImage; that creates an in-memory-only variant of the original manifest (a memoryImage with a new manifest object); that new manifest object is initialized with an edited version of configBlob from the start. Follow e.g. https://github.com/containers/image/blob/master/image/ docker_schema1.go#L171 → https://github.com/containers/ image/blob/master/image/docker_schema1.go#L284 → https://github.com/containers/image/blob/master/image/ docker_schema2.go#L56 . (And of course the abstraction is not set in stone, image.genericManifest is a private type perfectly subject to change, and even the public types in types.go have been modified fairly frequently in the past. The existing structure does seem to be more or less suitable for this in principle, concentrating all the true complexity in .UpdatedImage and its helper methods, and leaving the rest as fairly simple parsers / getters. It can very likely be improved.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6-ePD3fL7js2CPCb86rltRqMDAo7ks5rfJP-gaJpZM4LoGoZ> .

erikh · 2017-02-22T20:15:58Z

I should admit that I have tested this and the manifest is not updated.

…

On Wed, Feb 22, 2017 at 12:15 PM, Erik Hollensbe ***@***.***> wrote: Right, but if you read that code, it takes it right out of the original image. Or does the getblob in the code I linked from above take from the memory image? On Wed, Feb 22, 2017 at 12:01 PM, Miloslav Trmač ***@***.*** > wrote: > Yes but the abstraction doesn't support that, unless I'm missing > something, by the time you're in ConfigBlob() you don't have access to the > manifest format to unmarshal/marshal it. > > What happens is that the copy code, after supplying all needed data, > calls .UpdatedImage; that creates an in-memory-only variant of the > original manifest (a memoryImage with a new manifest object); that new > manifest object is initialized with an edited version of configBlob from > the start. Follow e.g. https://github.com/containers/ > image/blob/master/image/docker_schema1.go#L171 → > https://github.com/containers/image/blob/master/image/docker > _schema1.go#L284 → https://github.com/containers/ > image/blob/master/image/docker_schema2.go#L56 . > > (And of course the abstraction is not set in stone, image.genericManifest > is a private type perfectly subject to change, and even the public types in > types.go have been modified fairly frequently in the past. The existing > structure does seem to be more or less suitable for this in principle, > concentrating all the true complexity in .UpdatedImage and its helper > methods, and leaving the rest as fairly simple parsers / getters. It can > very likely be improved.) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#218 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABJ6-ePD3fL7js2CPCb86rltRqMDAo7ks5rfJP-gaJpZM4LoGoZ> > . >

mtrmac · 2017-02-22T20:18:36Z

Right, but if you read that code, it takes it right out of the original image

What path exactly? In the conversion case I have demonstrated above, https://github.com/containers/image/blob/master/image/docker_schema2.go#L84 uses the m.configBlob != nil path, with m.configBlob prepopulated with a memory-only version which does not exist in the source (obviously, when the source is not a schema2 image and there is no separate config.json in the original at all).

erikh · 2017-02-22T20:24:11Z

I can check again, but I've printed that config right at that point. It's not updated and gets processed right before uploading to docker. The layers after editing do not save to docker because of this. This is literally the only thing keeping it from working. I'll amend the PR.

…

On Wed, Feb 22, 2017 at 12:18 PM, Miloslav Trmač ***@***.***> wrote: Right, but if you read that code, it takes it right out of the original image What path exactly? In the conversion case I have demonstrated above, https://github.com/containers/image/blob/master/image/ docker_schema2.go#L84 uses the m.configBlob != nil path, with m.configBlob prepopulated with a memory-only version which does not exist in the source (obviously, when the source is not a schema2 image and there is no separate config.json in the original at all). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ62ZleWWEGEulv5IHu1Y5N3z0Pc0Xks5rfJgdgaJpZM4LoGoZ> .

erikh · 2017-02-22T20:36:08Z

I've amended the PR with the print statement and a test.go which will exhibit the issue in the root directory of the repository. Can you validate that I am seeing what I am seeing?

…

On Wed, Feb 22, 2017 at 12:24 PM, Erik Hollensbe ***@***.***> wrote: I can check again, but I've printed that config right at that point. It's not updated and gets processed right before uploading to docker. The layers after editing do not save to docker because of this. This is literally the only thing keeping it from working. I'll amend the PR. On Wed, Feb 22, 2017 at 12:18 PM, Miloslav Trmač ***@***.*** > wrote: > Right, but if you read that code, it takes it right out of the original > image > > What path exactly? In the conversion case I have demonstrated above, > https://github.com/containers/image/blob/master/image/docker > _schema2.go#L84 uses the m.configBlob != nil path, with m.configBlob > prepopulated with a memory-only version which does not exist in the source > (obviously, when the source is not a schema2 image and there is no separate > config.json in the original at all). > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#218 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABJ62ZleWWEGEulv5IHu1Y5N3z0Pc0Xks5rfJgdgaJpZM4LoGoZ> > . >

mtrmac · 2017-02-22T20:42:22Z

That added fmt.Println is in the wrong place; again, in the in-memory case, m.configBlob is non-nil from the start and that is never reached.

If that’s not the case, I’m not immediately sure what is going on. Here’s what I have used to test that I am not going crazy:

diff --git a/vendor/github.com/containers/image/copy/copy.go b/vendor/github.com/containers/image/copy/copy.go
index d27e634..a0f3f8a 100644
--- a/vendor/github.com/containers/image/copy/copy.go
+++ b/vendor/github.com/containers/image/copy/copy.go
@@ -180,7 +180,7 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
        }
 
        pendingImage := src
-       if !reflect.DeepEqual(manifestUpdates, types.ManifestUpdateOptions{InformationOnly: manifestUpdates.InformationOnly}) {
+       if _ = reflect.DeepEqual; true { //} !reflect.DeepEqual(manifestUpdates, types.ManifestUpdateOptions{InformationOnly: manifestUpdates.InformationOnly}) {
                if !canModifyManifest {
                        return errors.Errorf("Internal error: copy needs an updated manifest but that was known to be forbidden")
                }
diff --git a/vendor/github.com/containers/image/image/docker_schema2.go b/vendor/github.com/containers/image/image/docker_schema2.go
index 76ceea7..1188cd9 100644
--- a/vendor/github.com/containers/image/image/docker_schema2.go
+++ b/vendor/github.com/containers/image/image/docker_schema2.go
@@ -151,6 +151,24 @@ func (m *manifestSchema2) UpdatedImageNeedsLayerDiffIDs(options types.ManifestUp
 // This does not change the state of the original Image object.
 func (m *manifestSchema2) UpdatedImage(options types.ManifestUpdateOptions) (types.Image, error) {
        copy := *m // NOTE: This is not a deep copy, it still shares slices etc.
+
+       if true {
+               logrus.Errorf("HERE")
+               blob, err := copy.ConfigBlob()
+               if err != nil {
+                       return nil, err
+               }
+               configJSON := bytes.Join([][]byte{[]byte("INVALIDPREFIX"), blob}, nil)
+               configDescriptor := descriptor{
+                       MediaType: "application/vnd.docker.container.image.v1+json",
+                       Size:      int64(len(configJSON)),
+                       Digest:    digest.FromBytes(configJSON),
+               }
+
+               m2 := manifestSchema2FromComponents(configDescriptor, nil, configJSON, copy.LayersDescriptors)
+               return memoryImageFromManifest(m2), nil
+       }
+
        if options.LayerInfos != nil {
                if len(copy.LayersDescriptors) != len(options.LayerInfos) {
                        return nil, errors.Errorf("Error preparing updated manifest: layer count changed from %d to %d", len(copy.LayersDescriptors), len(options.LayerInfos))

and ./skopeo --policy=default-policy.json copy docker://busybox:latest dir:t.

My best guess (but just a guess) is that the reflect.DeepEqual test (which the above stupid hack patches out) is preventing the run of UpdatedImage at all.

mtrmac · 2017-02-22T20:45:31Z

… actually, let’s step back: This PR, as far as manifest/config editing goes, only updates ManifestUpdateOptions.LayerInfos. That of course does not edit config.json simply because that code doesn’t exist. .LayerInfos has been created to update the manifest after blobs are compressed (changing their digests but not DiffIDs). Editing the config as necessary would be a new functionality which needs to be written. It isn’t already there just to enable.

erikh · 2017-02-22T20:48:18Z

run the code! It's reached, trust me.

…

On Wed, Feb 22, 2017 at 12:45 PM, Miloslav Trmač ***@***.***> wrote: … actually, let’s step back: This PR, as far as manifest/config editing goes, *only* updates ManifestUpdateOptions.LayerInfos. That of course does not edit config.json simply because that code doesn’t exist. .LayerInfos has been created to update the manifest after blobs are compressed (changing their digests but not DiffIDs). Editing the config as necessary would be a new functionality which needs to be written. It isn’t *already there* just to enable. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6zrdNnkg-Fy9UQkKvQHcyFlvl1l5ks5rfJ5rgaJpZM4LoGoZ> .

Signed-off-by: Erik Hollensbe <github@hollensbe.org>

Specifically, with the layer editing changes. The layers are variable and unvalidated now so this test will no longer pass. Signed-off-by: Erik Hollensbe <github@hollensbe.org>

rhatdan · 2018-01-29T13:08:43Z

@erikh Is this still something you want. Could you rebase the PR and @mtrmac PTAL/

erikh · 2018-01-29T17:49:43Z

it's ok, thanks and sorry to keep this hanging up things for so long

mtrmac reviewed Jan 19, 2017

View reviewed changes

mtrmac mentioned this pull request Jan 19, 2017

pulling with --storage ostree doesn't enforce signatures projectatomic/atomic#829

Open

erikh force-pushed the copy-hook branch from 77295b7 to 4ddf957 Compare January 20, 2017 08:44

erikh mentioned this pull request Feb 13, 2017

Port MakeImage (image editing facility) to containers/image box-builder/box#140

Merged

erikh force-pushed the copy-hook branch from 4ddf957 to 0be2c4d Compare February 15, 2017 07:05

erikh force-pushed the copy-hook branch 2 times, most recently from 798114b to 1a0fcb4 Compare February 19, 2017 06:17

mtrmac reviewed Feb 21, 2017

View reviewed changes

erikh force-pushed the copy-hook branch from 1a0fcb4 to 9cf5f8b Compare February 22, 2017 20:35

Erik Hollensbe added 4 commits February 25, 2017 02:45

manifest: remove ManifestList type

4228193

Signed-off-by: Erik Hollensbe <github@hollensbe.org>

temp

148c561

Signed-off-by: Erik Hollensbe <github@hollensbe.org>

test.go

d40555d

Signed-off-by: Erik Hollensbe <github@hollensbe.org>

image/docker_schema2_test.go: remove a test that is no longer valid

18386e4

Specifically, with the layer editing changes. The layers are variable and unvalidated now so this test will no longer pass. Signed-off-by: Erik Hollensbe <github@hollensbe.org>

erikh force-pushed the copy-hook branch from 9cf5f8b to 18386e4 Compare February 25, 2017 10:52

erikh closed this Jan 29, 2018

mtrmac mentioned this pull request Mar 7, 2018

Only copy a subpart of a source image to a directory containers/skopeo#481

Closed

Pre-layer copy hook #218

Pre-layer copy hook #218

Uh oh!

Conversation

erikh commented Jan 19, 2017

Uh oh!

runcom commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikh commented Jan 19, 2017 via email

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikh commented Jan 19, 2017

Uh oh!

erikh commented Jan 19, 2017

Uh oh!

mtrmac commented Jan 19, 2017

Uh oh!

mtrmac commented Jan 19, 2017

Uh oh!

erikh commented Jan 19, 2017 via email

Uh oh!

mtrmac commented Jan 19, 2017

Uh oh!

erikh commented Jan 19, 2017 via email

Uh oh!

erikh commented Jan 20, 2017

Uh oh!

erikh commented Jan 20, 2017

Uh oh!

mtrmac commented Jan 20, 2017

Uh oh!

erikh commented Jan 20, 2017

Uh oh!

erikh commented Jan 20, 2017

Uh oh!

erikh commented Feb 15, 2017

Uh oh!

erikh commented Feb 15, 2017

Uh oh!

mtrmac commented Feb 15, 2017

Uh oh!

erikh commented Feb 15, 2017 via email

Uh oh!

erikh commented Feb 19, 2017

Uh oh!

erikh commented Feb 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Jan 19, 2017 •

edited

Loading

mtrmac Jan 19, 2017 •

edited

Loading