Per discussion in #916, the new API hcsshim.ConvertToBaseLayer might want to be exposed via pkgs instead. Although perhaps that is true of the entire layer.go API, which is already just thin bounces into internal/ anyway.

Another API thought. Since this is currently being added to layer.go where almost all the APIs take info DriverInfo, layerId string rather than path string, should ConvertToBaseLayer be the same?

I didn't do this initially because the two current exceptions to this API structure are the functions the new API wraps, ProcessBaseLayer and ProcessUtilityVMImage.

I will post a comment here if we need to make any changes and then we can merge this. Thanks for your patience.

@ambarve It looks like that @TBBle is very patient, he's waiting for half a year already. Maybe it's time to finally admit that legal issue is not in fact an issue because layer exporting does not redistribute anything to anyone and move on with this PR that currently blocks BuildKit implementation for Windows containers?

First of all, apologies for the delay in responses on this thread. We had reservations about the legal aspects of this PR for two reasons. First the restrictions around creating (& publishing) empty scratch layers that are not bootable container layers. Most probably this won't be an issue but we are working with our internal teams to get a confirmation on it. I will try to provide an update here ASAP (hopefully within a week). Second, for the base layer export we still need to make sure that the exported base layer is correctly marked as a foreign layer (Non distributable layer) so that it won't be pushed to a repository. @TBBle can you please confirm if the base layers that are exported will be marked as foreign layers? Can you also please rebase this PR? Otherwise, this PR looks good and ready to be merged as soon as we have answers for both of these questions.

There's nothing in what I have here that would mark an exported base layer as foreign, because the intended use-case is not for exporting layers that were imported from elsewhere, but for exporting layers built on 'scratch', which would not be marked foreign by default.

In fact, while you can re-export an imported base layer, you cannot export an imported non-base layer, i.e. if I pull a Windows Server image of any kind, I get the base layer plus a layer for the updates, but due to the on-disk storage, I can only export that first base layer. (This is not something I plan to solve, as it's intrinsic to HCS at the moment, and so tooling here can't really do anything about it. This only affects the HCS-driven layer export process, a generic diff of course can still re-export imported layers without Windows-specific metadata.)

hcsshim layer-handling has no concept of "foreign layer" at this time, that would have to be handled by (or in concert with) a caller like containerd; I'm not sure if containerd carries the 'foreign layer' concept down into the layer storage (Snapshotter) system, since that's an attribute of the layer reference in the manifest, not intrinsic to the layer itself, as we recently discovered in a BuildKit discussion on the same topic: See moby/buildkit#2561 and moby/buildkit#2555.

I haven't checked containerd, so I don't know if the Content Store even maintains the content-type of an object, and how it would handle seeing the same object (by hash) with different content types in different contexts. That BuildKit PR touches upon stargz-based layers which actually have a similar problem.

As an idea, a newly-created empty layer could have a marker that indicates it's safe for push, but it would still be on higher-level tools to recognise this annotation (it could be exposed by hcsshim import API) and apply the appropriate foreign content type when creating a manifest that references the layer.

Of course, all of this is only if one uses the hcsshim code and/or honours the resulting flags. There's no way to prevent someone exporting a layer into a manifest, editing the content type or ignoring some flag, and pushing it to a registry. After all, if one wanted to mark a layer as foreign that isn't currently (i.e. I built it myself, and don't want it repushed), it's the same process.

Technically, there's no requirement in OCI that foreign and non-foreign layers are actually interchangeable; so a marker could be included inside the tar stream to carry a "from-scratch" layer flag.

However, all discussion of flag files on-disk or in tar streams is complicated by the existence of both base and non-base layers without that flag, created by existing tools in the ecosystem.

Side-note: I don't think I'll have time to do the rebasing this week. My current repo is a mess as I haven't touched it in ages and last time I did, I was unsuccessfully trying to recreate a Windows Server 2019-seen issue in the containerd Snapshotter test suite. I also need to find a way to work out if TBBle@1803a21 is required for Windows Server 2022.

Hmm. Rebase built and passed, but I guess we still aren't running the actual tests on CI, and I don't have access to a Windows Server 2022 machine to verify if TBBle@1803a21 is needed.

I'm not even sure if the current tests would trigger that case, I'll try and put together a test-case that does trigger it. I've relied on the containerd snapshotter test suite to test it before now, but that work needs a bunch more rebasing (and I know it has conflicts) so it makes more sense to add the tests here.

Gaaah. So I finally think I found the reason I was having problems on RS5 in the containerd Snapshotter test suite, and it was me all along, leaking a reference to an opened file: https://github.com/microsoft/hcsshim/compare/0ce0a83127aa7ac90eae33b7b835781b70233333..3577daea1d5860867edce38a0129f8cd3cfafe45

Looks like somewhere after RS5 (and before 19042), this leaked reference stopped preventing deletion of the directory, which is why the error never reproduced for me locally. And any tests with the wclayer CLI binary closed the file when the owning process closed, so that never reproduced either. I'm actually surprised that the containerd test suite ever worked, so perhaps the leaked reference was only a problem for a short period of time, long enough to fail the integration tests I wrote here, and sometimes fail the containerd test suite, but short enough to not always fails the containerd test suite.

I also now have some integration tests for the base layer creation changes here which I plan to pull into this PR soon.

However, I suspect based on #901 (comment) that I'll need to split this PR into two, one for non-bootable base layer creation, where it sounds like the concern was minor:

First the restrictions around creating (& publishing) empty scratch layers that are not bootable container layers. Most probably this won't be an issue but we are working with our internal teams to get a confirmation on it. I will try to provide an update here ASAP (hopefully within a week).

and one for base layer export where the concern was legal. In this latter case, the worst-case is that there's an existing ecosystem-wide legal issue that my PR has surfaced:

Second, for the base layer export we still need to make sure that the exported base layer is correctly marked as a foreign layer (Non distributable layer) so that it won't be pushed to a repository.

My non-reassuring reply was focussed on this part.

For purposes of BuildKit functionality and (IIRC) the containerd snapshotter test suite, we don't actually need to export base layers, just be able to create them. And I reckon I could just punt 8778526 to bring us into that state, so this wouldn't be hard to do, if it's enough to get the rest of this PR merged.

I still need to write some import/export tests to validate if TBBle@1803a21 is needed on Server 2022 though, since that affects the base-layer creation code, not the base-layer export code.

Latest rebase pushes the changes I've broken out into #1374 to the front, so that (assuming that lands soon) this PR is reduced to just those things stuck behind the legal concerns.

Hey folks, jumping in here to provide an update and clear up some confusion from the Microsoft side. What held this up for so long were some reservations from our legal team on enabling an open-source tool to easily package and redistribute Windows binaries. With the secure supply chain executive order, and our general company policies on security, we are moving very cautiously on topics like these. With that said, I'm here to bring the good news that these issues have resolved, and this PR is good to move forward.

I know it's been incredibly frustrating for the community here. I want to emphasize that we are here to support the community and do the best we can to make things easy for you. This definitely took longer than it should have and we're changing how things are done on our end to ensure it doesn't happen again. If you have concerns or questions, always feel welcome to reach out to me. (Brandon Smith (MS) on K8s Slack)

@ambarve would you be able to move this forward from here?

Glad to hear that. I've clearly got some rebasing and updating to do, I'll try and make some time for that soon but I'm pretty snowed under at the moment. Maybe on the coming weekend I can flush the rebasing through, at least.

Due to #1375, I can't easily test this with the containerd Snapshotter test-suite as I did in the past. So I'll also have to pull-in and complete the tests I mentioned in April, and ensure everything's working on both LTSC 2019 and LTSC 2022 (although I'm confident it will be, since I squashed one bug and know of only one other likely stumbling point, for which I have a proposed fix). At least those platforms are available on GHA now.

@TBBle I'm trying to resolve #1375 with this #1463 if you want to take a look. Still going back and forth on that

Sorry, I haven't had a chance to look closely at #1463 yet. It shouldn't block this (unless an API I need changes...) as I think it's better to do tests in this repo anyway. I've rebased my existing work (it wasn't as bad as I feared, most of the big scary conflict warnings turned out to be delete/edit over test/vendor, and they're trivial to resolve.

I've pushed my WIP tests over to https://github.com/TBBle/hcsshim/tree/layer-manipulation-tests and will perhaps go over them tonight or tomorrow, and try and get them clean-enough (and passing-enough, and high-coverage-enough) to pull into this PR, in which case I'd be confident this code is ready-to-go.

I see we now have a containerd integration test in the CI suite, but my Go practice is a little rusty, and I'm not sure if it will pick up the checked-out hcsshim in its containerd build as I suspect it will prefer the vendored version. So once #1375 has been resolved in some way, if we force teach the containerd integration test to build against the checked-out hcsshim, then we get the containerd snapshotter test suite also running for us, once that side lands too in containerd/containerd#4419 or similar. (There's significant runway before that PR lands, which is another good reason to land this code here with its own tests.)

Sorry, I haven't had a chance to look closely at #1463 yet.

No worries at all. I found out I'm calling that FormatWritableLayerVhd function internally in a helper in the same package also which is rather annoying.. Hoping to have a clearer picture this coming week on what we wanna do there.

I see we now have a containerd integration test in the CI suite, but my Go practice is a little rusty, and I'm not sure if it will pick up the checked-out hcsshim in its containerd build as I suspect it will prefer the vendored version.

The containerd integration suite here should run against/build a shim from whatever is in the current PR. So containerd main + your branch for hcsshim.

The problem for the integration test is that the code I'm working on isn't in the shim, it's an in-tree snapshotter in containerd linking against hcsshim directly. If it was an external snapshotter instead, that'd be a different question (and also solves the manifesting blocker in-passing). Although there isn't currently support as I recall an external mounter in containerd, so we'd be a significant refactoring away from being able to decouple fully anyway, if that was a useful direction.

On option would be to programmatically add a replace rule to containerd's go.mod to use the local checkout of hcsshim for its in-tree work. That (but manually) is how my testing of the snapshotter worked in the containerd tree. That whole idea though is somewhat flawed, as unlike the runtime shim, there isn't a stable API/ABI boundary for this part, so such tests would fail when hcsshim evolves its API in breaking ways, ala 0.9 and (I assume) 0.10.

@TBBle Ahh gotcha gotcha, you meant the containerd suite. Yea it will still use whatever is in its go.mod for code it uses hcsshim as a library for. External snapshotter solving the manifesting issue is an interesting thought also.. That's actually one of the suggestions we heard internally, but I'm not sure it makes sense for the windows snapshotter to not be built in

I also had "take the snapshotter external" as an option in the first "hcsshim ABI broke hard" ticket; in a green-field development, knowing what I know now, I'd have built the Windows snapshotter as an external snapshotter in this repo, since it parallels the external runtime shim and better-isolates containerd from vagaries of HCS/computerstorage evolution. (And this is the direction the OS X containerd work was strongly encouraged by the containerd team, which is what put it on my radar at all; in that case, they also farmed off mounting by defining a CLI utility as the API boundary).

But given the half-decade-or-so weight of code for the Windows Snapshotter already in containerd, as well as having access to the snapshotter test suite once we get that working, it'd be a time-costly and burden-shifting change that'd need more justification and stakeholder buy-in than I'm currently aware of.

Carrying back results from the now-unblocked testing with my containerd PR that depends on this work, and it fails in what looks like #901 (comment) even on Windows Server 2019. So either something else has gone wrong, or there's been a platform update that pulled in whatever change introduced that in later Windows builds.

So for this PR, I'll need to finish my existing test suite work, and get it to the point where I can replicate that issue in this repo directly, and then rebase/reinstate/recreate TBBle@1803a21 into this PR as well, since it looks like it's all-versions necessary now. (A quick test suggests the cherry-pick will be easy, it didn't conflict anywhere except go.mod/go.sum and vendoring)

Note to self: The repro case seen in containerd is the import from tar of an exported tarball (i.e. wclayer export -> wclayer import; the latter hits internal/wclayer.ImportLayer via pkg/ociwclayer.ImportLayerFromTar) so it won't be hard to repro in the tests.

Later edit: I had a few minutes to do the rebase of the "hives" commit (on a new branch, https://github.com/TBBle/hcsshim/tree/base-layer-manipulation-with-hives, to keep things separate) and although the code itself rebased fine, I noticed a couple of issues:

• go mod vendor behaviour has changed, and the relevant data is no longer added to the vendor directory.
• When pulling hcsshim into containerd, it pulled and vendored the hive-manipulation library.

This suggests I should rework the go generate code to just fetch the blob from GitHub. Anyway, the important thing is to confirm that this leaves us in a good state for the containerd snapshotter, or reveals some other bitrot or API drift that's gone unnoticed while that PR was blocked by time or circumstances.

Converted to draft as I'm currently moving countries, and hence don't have time or computing power for at least the next month (and maybe longer... we'll see) to rebase and resolve merge conflicts, let-alone complete the work described in #901 (comment).

So for this PR, I'll need to finish my existing test suite work, and get it to the point where I can replicate that issue in this repo directly, and then rebase/reinstate/recreate TBBle@1803a21 into this PR as well, since it looks like it's all-versions necessary now. (A quick test suggests the cherry-pick will be easy, it didn't conflict anywhere except go.mod/go.sum and vendoring)

To generate the minimal hive, how about something like this:

package main

import (
	"fmt"
	"log"
	"os"
	"syscall"

	winio "github.com/Microsoft/go-winio"
	"golang.org/x/sys/windows/registry"
)

//go:generate go run golang.org/x/sys/windows/mkwinsyscall -output zsyscall_windows.go main.go
//sys   RegSaveKeyW(key syscall.Handle, file *uint16, access uint32) (regerrno error) = advapi32.RegSaveKeyW

func main() {
	privileges := []string{"SeBackupPrivilege"}
	if err := winio.EnableProcessPrivileges(privileges); err != nil {
		log.Fatal(err)
	}
	defer winio.DisableProcessPrivileges(privileges)

	k, existing, err := registry.CreateKey(syscall.HKEY_CURRENT_USER, "tempKey", registry.ALL_ACCESS)
	if err != nil {
		log.Fatal(err)
	}
	defer registry.DeleteKey(syscall.HKEY_CURRENT_USER, "tempKey")

	fmt.Println(k, existing)

	saveTo := "C:\\Users\\Administrator\\reg\\save.dat"
	if _, err := os.Stat(saveTo); err == nil {
		os.Remove(saveTo)
	}

	if err := RegSaveKeyW(syscall.Handle(k), syscall.StringToUTF16Ptr(saveTo), 0); err != nil {
		log.Fatal(err)
	}
}

This exports an empty key. That exported file should be usable as a valid hive. The tempKey becomes the root of the new hive. I think we can also just embed the empty hive as a base64 string in the code if there are no licensing issues.

Use the offline registry library functions instead https://learn.microsoft.com/windows/win32/devnotes/offline-registry-library-functions

Use the offline registry library functions instead https://learn.microsoft.com/windows/win32/devnotes/offline-registry-library-functions

That's a lot better! Thanks for the link!

@TBBle

Created a PR against your branch to generate empty hives using the offline registry library. I know you don't have time now, so have a look at your earliest convenience.

#1637, the successor for this PR, has merged.