diff --git a/core/mount/manager/manager.go b/core/mount/manager/manager.go index 97e67b4d22..bf826d7b4a 100644 --- a/core/mount/manager/manager.go +++ b/core/mount/manager/manager.go @@ -408,9 +408,17 @@ func (mm *mountManager) Activate(ctx context.Context, name string, mounts []moun mounted = append(mounted, active) } - // If first system mount is converted, fill in the format - if mountConv != nil { - for _, tr := range mountConv[firstSystemMount] { + // If the first system mount has transforms, apply the ones the caller + // has not claimed. A claim can only be honored as a suffix of the + // chain: applyCount always covers at least every transform up to and + // including the last unclaimed one, since each depends on the last's + // output; see planActivation. + // + // firstSystemMount can reach len(mounts) when every mount is handled + // inside the manager (for example, every mount claimed via Temporary); + // there is then no system mount left to transform. + if mountConv != nil && firstSystemMount < len(mounts) { + for _, tr := range mountConv[firstSystemMount][:plan.applyCount[firstSystemMount]] { newM, err := tr.Transform(ctx, mounts[firstSystemMount], mounted) if err != nil { return mount.ActivationInfo{}, err diff --git a/core/mount/manager/plan.go b/core/mount/manager/plan.go index ae71712129..498cc0ae30 100644 --- a/core/mount/manager/plan.go +++ b/core/mount/manager/plan.go @@ -42,6 +42,18 @@ type activationPlan struct { // prefix. transforms [][]mount.Transformer + // applyCount[i] is how many of transforms[i], from the start, the + // manager must apply itself: through the last transform in the chain the + // caller did not claim. Meaningful only for mounts[firstSystemMount], + // the one mount returned to the caller with some of its own transforms + // possibly still unapplied; every earlier mount's transforms are always + // applied in full; they are internal to the manager and never exposed + // to the caller. A transform, once applied, resolves the input the next + // one in the chain depends on, so a run of claimed transforms can only + // be honored as a suffix: transforms strictly after the last unclaimed + // one in the chain. + applyCount []int + // handlers[i] is the Handler to use for mounts[i], or nil to perform a // plain system mount. handlers []mount.Handler @@ -51,6 +63,16 @@ type activationPlan struct { // must apply and whether the manager or the caller performs the mount // itself, honoring the mount types and transforms config.AllowMountTypes and // config.AllowTransforms claim. +// +// A claimed transform is honored only for mounts[firstSystemMount] in the +// result, and only as a suffix of its transform chain: transforms apply +// outside-in, so an inner transform's input depends on an outer, unclaimed +// transform's output, and the manager must still apply that outer transform +// even though the caller performs the suffix itself. For example, in +// "format/mkdir/overlay", claiming "mkdir" causes the manager to apply +// "format" and hand the caller "mkdir/overlay" to finish; claiming "format" +// alone does nothing, since "mkdir" cannot run without it having already +// run. func planActivation(ctx context.Context, mounts []mount.Mount, config mount.ActivateOptions, transforms map[string]mount.Transformer, handlers map[string]mount.Handler) activationPlan { shouldTransform := func(p string, t string) bool { return !slices.Contains(config.AllowTransforms, p) && !slices.Contains(config.AllowMountTypes, t) @@ -73,7 +95,10 @@ func planActivation(ctx context.Context, mounts []mount.Mount, config mount.Acti break } - if shouldTransform(transformType, mounts[i].Type) { //nolint:gosec // G602: i is bounded by range mounts + // mustApply reports whether the manager itself must apply this + // transform, as opposed to the caller having claimed it. + mustApply := shouldTransform(transformType, mounts[i].Type) //nolint:gosec // G602: i is bounded by range mounts + if mustApply { // At least everything before this must be mounted // by the mount manager plan.firstSystemMount = i @@ -85,11 +110,21 @@ func planActivation(ctx context.Context, mounts []mount.Mount, config mount.Acti if plan.transforms == nil { plan.transforms = make([][]mount.Transformer, len(mounts)) } + if plan.applyCount == nil { + plan.applyCount = make([]int, len(mounts)) + } plan.transforms[i] = append(plan.transforms[i], typeTransformer{ Transformer: tr, mountType: mt, }) + if mustApply { + // Every transform up to and including this one must be + // applied by the manager: this one because it is unclaimed, + // and every one before it because this one's input depends + // on their output. + plan.applyCount[i] = len(plan.transforms[i]) + } mountType = mt } diff --git a/core/mount/manager/plan_test.go b/core/mount/manager/plan_test.go index 097df2867f..aa9bf1e22b 100644 --- a/core/mount/manager/plan_test.go +++ b/core/mount/manager/plan_test.go @@ -65,30 +65,47 @@ func TestPlanActivationTransformChain(t *testing.T) { name string opts []mount.ActivateOpt expectFirstSystem int + expectApplyCount int }{ { - name: "nothing claimed requires the manager", + // Nothing claimed: the manager must apply the whole chain, the + // same as it always has. + name: "nothing claimed applies the whole chain", expectFirstSystem: 0, + expectApplyCount: 2, }, { - name: "a claimed transform still requires the manager for now", + // Claiming the innermost transform lets the manager stop after + // the one before it: mkdir is left for the caller. + name: "innermost transform claimed is honored", opts: []mount.ActivateOpt{mount.WithAllowTransform("mkdir")}, expectFirstSystem: 0, + expectApplyCount: 1, }, { - name: "every transform claimed no longer requires the manager", + // Claiming the outermost transform alone cannot be honored: + // mkdir's input depends on format having already run. + name: "outermost transform claimed alone is not honored", + opts: []mount.ActivateOpt{mount.WithAllowTransform("format")}, + expectFirstSystem: 0, + expectApplyCount: 2, + }, + { + name: "both transforms claimed applies neither", opts: []mount.ActivateOpt{ mount.WithAllowTransform("format"), mount.WithAllowTransform("mkdir"), }, expectFirstSystem: -1, + expectApplyCount: 0, }, { // Claiming the full literal mount type is equivalent to // claiming every transform in its chain. - name: "full mount type claimed no longer requires the manager", + name: "full mount type claimed applies neither", opts: []mount.ActivateOpt{mount.WithAllowMountType("format/mkdir/overlay")}, expectFirstSystem: -1, + expectApplyCount: 0, }, } { t.Run(tc.name, func(t *testing.T) { @@ -99,11 +116,33 @@ func TestPlanActivationTransformChain(t *testing.T) { plan := planActivation(context.Background(), mounts, config, testTransforms, nil) assert.Equal(t, tc.expectFirstSystem, plan.firstSystemMount) - require.Len(t, plan.transforms[0], 2) + if tc.expectFirstSystem >= 0 { + require.NotNil(t, plan.applyCount) + assert.Equal(t, tc.expectApplyCount, plan.applyCount[0]) + require.Len(t, plan.transforms[0], 2) + } }) } } +// TestPlanActivationClaimedGapInMiddle covers a three transform chain where +// an unclaimed transform sits between two claimed ones: the middle one still +// forces the manager to apply everything up to and including it, regardless +// of the outer claim, and only the true suffix after it is left claimed. +func TestPlanActivationClaimedGapInMiddle(t *testing.T) { + mounts := []mount.Mount{{Type: "format/mkfs/mkdir/overlay"}} + config := mount.ActivateOptions{} + mount.WithAllowTransform("format")(&config) + mount.WithAllowTransform("mkdir")(&config) + // mkfs is left unclaimed. + + plan := planActivation(context.Background(), mounts, config, testTransforms, nil) + require.Equal(t, 0, plan.firstSystemMount) + require.Len(t, plan.transforms[0], 3) + // format(0) and mkfs(1) must be applied; mkdir(2) is left to the caller. + assert.Equal(t, 2, plan.applyCount[0]) +} + func TestPlanActivationUnknownTransform(t *testing.T) { mounts := []mount.Mount{{Type: "unknown/overlay"}} plan := planActivation(context.Background(), mounts, mount.ActivateOptions{}, testTransforms, nil) @@ -173,3 +212,19 @@ func TestPlanActivationTemporary(t *testing.T) { require.Len(t, plan.handlers, 1) assert.Equal(t, 1, plan.firstSystemMount) } + +func TestPlanActivationTemporaryIgnoresTransformClaims(t *testing.T) { + mounts := []mount.Mount{{Type: "format/mkdir/overlay"}} + config := mount.ActivateOptions{} + mount.WithTemporary(&config) + mount.WithAllowTransform("format")(&config) + mount.WithAllowTransform("mkdir")(&config) + + plan := planActivation(context.Background(), mounts, config, testTransforms, nil) + // Temporary forces this, the only mount, to be handled, advancing + // firstSystemMount past it. It does not change how much of a transform + // chain is required, which is governed solely by what is claimed: both + // transforms here are claimed, so applyCount stays at 0. + assert.Equal(t, 1, plan.firstSystemMount) + assert.Equal(t, 0, plan.applyCount[0]) +}