From b7834664690dc7b11a630236872abf06c67e8beb Mon Sep 17 00:00:00 2001
From: Frederik Lotter <fred.lotter@canonical.com>
Date: Wed, 23 Oct 2024 12:28:04 +0200
Subject: [PATCH] feat(plan): add plan sub-directory support (#509)

Pebble currently only supports loading configuration layer files from
the root of $PEBBLE/layers.

Add support for adding sub-directory support at the root level (2 levels
in total only). [Spec
KO071.](https://docs.google.com/document/d/1-9GLCJTx9o_hSd0JGvDp7o5xNQjkJMvLam6o1Ba_bYM/edit?tab=t.0)

This scheme has an impact on the meaning of ```label``` and ```order```.
See the following examples to understand the new mapping:

- $PEBBLE/layers/001-foo.yaml
Order: 001-000 => 1000
Label: foo

- $PEBBLE/layers/005-bar.d/010-abc.yaml
Order: 005-010 => 5010
Label: bar/abc

- $PEBBLE/layers/005-bar.d/012-def.yaml
Order: 005-012 => 5012
Label: bar/def

- $PEBBLE/layers/006-baz.yaml
Order: 006-000 => 6000
Label: baz

Directory support allows other uses of Pebble using read-only file
systems more flexibility by now giving the option to bind-mount a
sub-directory to a writable disk location, for example.
---
 client/plan.go                               |  15 ++
 client/plan_test.go                          |  23 ++-
 docs/reference/cli-commands/add.md           |   2 +
 internals/cli/cmd_add.go                     |   3 +
 internals/cli/cmd_add_test.go                |   1 +
 internals/daemon/api_exec_test.go            |   4 +-
 internals/daemon/api_plan.go                 |   5 +-
 internals/overlord/planstate/manager.go      |  94 ++++++++--
 internals/overlord/planstate/manager_test.go | 128 ++++++++++---
 internals/overlord/planstate/package_test.go |   2 +-
 internals/overlord/servstate/manager_test.go |   2 +-
 internals/plan/extensions_test.go            |  18 +-
 internals/plan/plan.go                       | 182 ++++++++++++++++---
 internals/plan/plan_test.go                  | 152 ++++++++++++++++
 14 files changed, 543 insertions(+), 88 deletions(-)

diff --git a/client/plan.go b/client/plan.go
index 25b447f08..9c34f58e7 100644
--- a/client/plan.go
+++ b/client/plan.go
@@ -25,6 +25,11 @@ type AddLayerOptions struct {
 	// has the given label. False (the default) means append a new layer.
 	Combine bool
 
+	// Inner set to true means a new layer append may go into an existing
+	// subdirectory, even though it may not result in appending it
+	// to the end of the layers slice (it becomes an insert).
+	Inner bool
+
 	// Label is the label for the new layer if appending, and the label of the
 	// layer to combine with if Combine is true.
 	Label string
@@ -38,16 +43,26 @@ func (client *Client) AddLayer(opts *AddLayerOptions) error {
 	var payload = struct {
 		Action  string `json:"action"`
 		Combine bool   `json:"combine"`
+		Inner   bool   `json:"inner"`
 		Label   string `json:"label"`
 		Format  string `json:"format"`
 		Layer   string `json:"layer"`
 	}{
 		Action:  "add",
 		Combine: opts.Combine,
+		Inner:   opts.Inner,
 		Label:   opts.Label,
 		Format:  "yaml",
 		Layer:   string(opts.LayerData),
 	}
+
+	// Add label validation here once layer persistence is supported over
+	// the API. We cannot do this in the plan library because JUJU already
+	// has labels in production systems that violates the layers file
+	// naming convention (which includes the label). Since JUJU uses its
+	// own client, we can enforce the label naming convention on all other
+	// systems using the Pebble supplied client by validating it here.
+
 	var body bytes.Buffer
 	if err := json.NewEncoder(&body).Encode(&payload); err != nil {
 		return err
diff --git a/client/plan_test.go b/client/plan_test.go
index 385d6e202..38fd081a4 100644
--- a/client/plan_test.go
+++ b/client/plan_test.go
@@ -24,7 +24,22 @@ import (
 )
 
 func (cs *clientSuite) TestAddLayer(c *check.C) {
-	for _, combine := range []bool{false, true} {
+	for _, option := range []struct {
+		combine bool
+		inner   bool
+	}{{
+		combine: false,
+		inner:   false,
+	}, {
+		combine: true,
+		inner:   false,
+	}, {
+		combine: false,
+		inner:   true,
+	}, {
+		combine: true,
+		inner:   true,
+	}} {
 		cs.rsp = `{
 		"type": "sync",
 		"status-code": 200,
@@ -37,7 +52,8 @@ services:
         command: cmd
 `[1:]
 		err := cs.cli.AddLayer(&client.AddLayerOptions{
-			Combine:   combine,
+			Combine:   option.combine,
+			Inner:     option.inner,
 			Label:     "foo",
 			LayerData: []byte(layerYAML),
 		})
@@ -49,10 +65,11 @@ services:
 		c.Assert(json.NewDecoder(cs.req.Body).Decode(&body), check.IsNil)
 		c.Assert(body, check.DeepEquals, map[string]interface{}{
 			"action":  "add",
-			"combine": combine,
+			"combine": option.combine,
 			"label":   "foo",
 			"format":  "yaml",
 			"layer":   layerYAML,
+			"inner":   option.inner,
 		})
 	}
 }
diff --git a/docs/reference/cli-commands/add.md b/docs/reference/cli-commands/add.md
index 5ef741108..9e2299799 100644
--- a/docs/reference/cli-commands/add.md
+++ b/docs/reference/cli-commands/add.md
@@ -19,5 +19,7 @@ label (or append if the label is not found).
 [add command options]
       --combine         Combine the new layer with an existing layer that has
                         the given label (default is to append)
+      --inner           Allow appending a new layer inside an existing
+                        subdirectory
 ```
 <!-- END AUTOMATED OUTPUT -->
diff --git a/internals/cli/cmd_add.go b/internals/cli/cmd_add.go
index 1d8e841cd..247a4d6c8 100644
--- a/internals/cli/cmd_add.go
+++ b/internals/cli/cmd_add.go
@@ -35,6 +35,7 @@ type cmdAdd struct {
 	client *client.Client
 
 	Combine    bool `long:"combine"`
+	Inner      bool `long:"inner"`
 	Positional struct {
 		Label     string `positional-arg-name:"<label>" required:"1"`
 		LayerPath string `positional-arg-name:"<layer-path>" required:"1"`
@@ -48,6 +49,7 @@ func init() {
 		Description: cmdAddDescription,
 		ArgsHelp: map[string]string{
 			"--combine": "Combine the new layer with an existing layer that has the given label (default is to append)",
+			"--inner":   "Allow appending a new layer inside an existing subdirectory",
 		},
 		New: func(opts *CmdOptions) flags.Commander {
 			return &cmdAdd{client: opts.Client}
@@ -65,6 +67,7 @@ func (cmd *cmdAdd) Execute(args []string) error {
 	}
 	opts := client.AddLayerOptions{
 		Combine:   cmd.Combine,
+		Inner:     cmd.Inner,
 		Label:     cmd.Positional.Label,
 		LayerData: data,
 	}
diff --git a/internals/cli/cmd_add_test.go b/internals/cli/cmd_add_test.go
index a9990c581..c037be6fb 100644
--- a/internals/cli/cmd_add_test.go
+++ b/internals/cli/cmd_add_test.go
@@ -58,6 +58,7 @@ services:
 					"label":   "foo",
 					"format":  "yaml",
 					"layer":   layerYAML,
+					"inner":   false,
 				})
 				fmt.Fprint(w, `{
     "type": "sync",
diff --git a/internals/daemon/api_exec_test.go b/internals/daemon/api_exec_test.go
index 37694882b..b0ec05528 100644
--- a/internals/daemon/api_exec_test.go
+++ b/internals/daemon/api_exec_test.go
@@ -206,7 +206,7 @@ func (s *execSuite) TestContextNoOverrides(c *C) {
 			Environment: map[string]string{"FOO": "foo", "BAR": "bar"},
 			WorkingDir:  dir,
 		}},
-	})
+	}, false)
 	c.Assert(err, IsNil)
 
 	stdout, stderr, err := s.exec(c, "", &client.ExecOptions{
@@ -228,7 +228,7 @@ func (s *execSuite) TestContextOverrides(c *C) {
 			Environment: map[string]string{"FOO": "foo", "BAR": "bar"},
 			WorkingDir:  c.MkDir(),
 		}},
-	})
+	}, false)
 	c.Assert(err, IsNil)
 
 	overrideDir := c.MkDir()
diff --git a/internals/daemon/api_plan.go b/internals/daemon/api_plan.go
index 3555b6c11..1ad5e4fec 100644
--- a/internals/daemon/api_plan.go
+++ b/internals/daemon/api_plan.go
@@ -43,6 +43,7 @@ func v1PostLayers(c *Command, r *http.Request, _ *UserState) Response {
 	var payload struct {
 		Action  string `json:"action"`
 		Combine bool   `json:"combine"`
+		Inner   bool   `json:"inner"`
 		Label   string `json:"label"`
 		Format  string `json:"format"`
 		Layer   string `json:"layer"`
@@ -68,9 +69,9 @@ func v1PostLayers(c *Command, r *http.Request, _ *UserState) Response {
 
 	planMgr := overlordPlanManager(c.d.overlord)
 	if payload.Combine {
-		err = planMgr.CombineLayer(layer)
+		err = planMgr.CombineLayer(layer, payload.Inner)
 	} else {
-		err = planMgr.AppendLayer(layer)
+		err = planMgr.AppendLayer(layer, payload.Inner)
 	}
 	if err != nil {
 		if _, ok := err.(*planstate.LabelExists); ok {
diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go
index a4fb44140..568325628 100644
--- a/internals/overlord/planstate/manager.go
+++ b/internals/overlord/planstate/manager.go
@@ -16,6 +16,8 @@ package planstate
 
 import (
 	"fmt"
+	"slices"
+	"strings"
 	"sync"
 
 	"github.com/canonical/pebble/internals/plan"
@@ -100,8 +102,10 @@ func (m *PlanManager) Plan() *plan.Plan {
 
 // AppendLayer takes a Layer, appends it to the plan's layers and updates the
 // layer.Order field to the new order. If a layer with layer.Label already
-// exists, return an error of type *LabelExists.
-func (m *PlanManager) AppendLayer(layer *plan.Layer) error {
+// exists, return an error of type *LabelExists. Inner must be set to true
+// if the append operation may be demoted to an insert due to the layer
+// configuration being located in a sub-directory.
+func (m *PlanManager) AppendLayer(layer *plan.Layer, inner bool) error {
 	var newPlan *plan.Plan
 	defer func() { m.callChangeListeners(newPlan) }()
 
@@ -113,14 +117,17 @@ func (m *PlanManager) AppendLayer(layer *plan.Layer) error {
 		return &LabelExists{Label: layer.Label}
 	}
 
-	newPlan, err := m.appendLayer(layer)
+	newPlan, err := m.appendLayer(layer, inner)
 	return err
 }
 
 // CombineLayer takes a Layer, combines it to an existing layer that has the
 // same label. If no existing layer has the label, append a new one. In either
-// case, update the layer.Order field to the new order.
-func (m *PlanManager) CombineLayer(layer *plan.Layer) error {
+// case, update the layer.Order field to the new order. Inner must be set to
+// true if a combine operation gets demoted to an append operation (due to the
+// layer not yet existing), and if the configuration layer is located in a
+// sub-directory (see AppendLayer).
+func (m *PlanManager) CombineLayer(layer *plan.Layer, inner bool) error {
 	var newPlan *plan.Plan
 	defer func() { m.callChangeListeners(newPlan) }()
 
@@ -131,7 +138,7 @@ func (m *PlanManager) CombineLayer(layer *plan.Layer) error {
 	if index < 0 {
 		// No layer found with this label, append new one.
 		var err error
-		newPlan, err = m.appendLayer(layer)
+		newPlan, err = m.appendLayer(layer, inner)
 		return err
 	}
 
@@ -155,19 +162,78 @@ func (m *PlanManager) CombineLayer(layer *plan.Layer) error {
 	return nil
 }
 
-func (m *PlanManager) appendLayer(layer *plan.Layer) (*plan.Plan, error) {
-	newOrder := 1
-	if len(m.plan.Layers) > 0 {
-		last := m.plan.Layers[len(m.plan.Layers)-1]
-		newOrder = last.Order + 1
+// appendLayer appends (or inserts) a new layer configuration
+// into the layers slice of the current plan. One important
+// task of this method is to determine the new order of the layer.
+//
+//	| File (inside layersDir)    | Order           | Label   |
+//	| -------------------------- | --------------- | ------- |
+//	| 001-foo.yaml               | 001-000 => 1000 | foo     |
+//	| 002-bar.d/001-aaa.yaml     | 002-001 => 2001 | bar/aaa |
+//	| 002-bar.d/002-bbb.yaml     | 002-002 => 2002 | bar/bbb |
+//	| 003-baz.yaml               | 003-000 => 3000 | baz     |
+//
+// The new incoming layer only supplies the label, which may include
+// a sub-directory prefix. Normally without a sub-directory prefix,
+// the new layer will always be appended, which means incrementing
+// the root level order (+ 1000). If a sub-directory already exists
+// in the layers slice, its order was already allocated, which means
+// we can at most insert the layer as the last entry in the
+// sub-directory. However, this insert is only allowed if explicitly
+// requested by the user (inner=true).
+func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan, error) {
+	// The starting index and order assumes no existing layers.
+	newIndex := 0
+	newOrder := 1000
+
+	// If we have existing layers, things get a little bit more complex.
+	layersCount := len(m.plan.Layers)
+	if layersCount > 0 {
+		// We know at this point the complete label does not yet exist.
+		// However, let's see if the first part of the label is a
+		// sub-directory that exists and for which we already allocated
+		// an order?
+		newSubLabel, _, hasSub := strings.Cut(newLayer.Label, "/")
+		for i := layersCount - 1; i >= 0; i-- {
+			layer := m.plan.Layers[i]
+			layerSubLabel, _, _ := strings.Cut(layer.Label, "/")
+			// If we have a sub-directory match we know it already exists.
+			// Since we searched backwards, we know the order should be the
+			// next integer value.
+			if layerSubLabel == newSubLabel {
+				newOrder = layer.Order + 1
+				newIndex = i + 1
+				break
+			}
+		}
+
+		// If we did not match a sub-directory, this is simply an append.
+		// However, we need to know if this a inside a sub-directory or not
+		// as it has an impact on how we allocate the order.
+		if newIndex == 0 {
+			newIndex = layersCount
+			newOrder = ((m.plan.Layers[layersCount-1].Order / 1000) + 1) * 1000
+			if hasSub {
+				// The first file in the sub-directory gets an order of "001".
+				newOrder += 1
+			}
+		}
+	}
+
+	// If the append operation requires an insert because the layer is added
+	// inside an already existing sub-directory, with higher order items already
+	// allocated beyond it, we allow it only if the request specifically
+	// authorised it (inner=true).
+	if newIndex != layersCount && !inner {
+		return nil, fmt.Errorf("cannot insert sub-directory layer without 'inner' attribute set")
 	}
 
-	newLayers := append(m.plan.Layers, layer)
+	newLayers := slices.Insert(m.plan.Layers, newIndex, newLayer)
 	newPlan, err := m.updatePlanLayers(newLayers)
 	if err != nil {
 		return nil, err
 	}
-	layer.Order = newOrder
+	newLayer.Order = newOrder
 	return newPlan, nil
 }
 
@@ -243,6 +309,6 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error {
 		}
 	}
 
-	newPlan, err := m.appendLayer(newLayer)
+	newPlan, err := m.appendLayer(newLayer, false)
 	return err
 }
diff --git a/internals/overlord/planstate/manager_test.go b/internals/overlord/planstate/manager_test.go
index 300e04113..54d1879f4 100644
--- a/internals/overlord/planstate/manager_test.go
+++ b/internals/overlord/planstate/manager_test.go
@@ -116,9 +116,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 1)
+	c.Assert(layer.Order, Equals, 1000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -142,7 +142,7 @@ test-field:
         override: foobar
         a: something else
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err.(*planstate.LabelExists).Label, Equals, "label1")
 	c.Assert(ps.planYAML(c), Equals, `
 services:
@@ -167,9 +167,9 @@ test-field:
         override: replace
         a: else
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 2)
+	c.Assert(layer.Order, Equals, 2000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -193,9 +193,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 3)
+	c.Assert(layer.Order, Equals, 3000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -233,9 +233,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 1)
+	c.Assert(layer.Order, Equals, 1000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -259,9 +259,9 @@ test-field:
         override: replace
         a: else
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 2)
+	c.Assert(layer.Order, Equals, 2000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -291,9 +291,9 @@ test-field:
         override: replace
         a: else
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 1)
+	c.Assert(layer.Order, Equals, 1000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -323,9 +323,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 2)
+	c.Assert(layer.Order, Equals, 2000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -361,9 +361,9 @@ test-field:
         override: replace
         a: nothing
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 3)
+	c.Assert(layer.Order, Equals, 3000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -383,7 +383,7 @@ test-field:
 	ps.planLayersHasLen(c, 3)
 
 	// Make sure that layer validation is happening.
-	layer, err = plan.ParseLayer(0, "label4", []byte(`
+	_, err = plan.ParseLayer(0, "label4", []byte(`
 checks:
     bad-check:
         override: replace
@@ -394,7 +394,7 @@ checks:
 	c.Check(err, ErrorMatches, `(?s).*plan check.*must be "alive" or "ready".*`)
 
 	// Make sure that layer validation is happening for extensions.
-	layer, err = plan.ParseLayer(0, "label4", []byte(`
+	_, err = plan.ParseLayer(0, "label4", []byte(`
 test-field:
     my1:
         override: replace
@@ -421,7 +421,8 @@ services:
         override: replace
         command: foo
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, IsNil)
 
 	// Set arguments to services.
 	serviceArgs := map[string][]string{
@@ -455,8 +456,8 @@ func (ps *planSuite) TestChangeListenerAndLocking(c *C) {
 		// so we should be able to acquire it.
 		planLock := manager.PlanLock()
 		planLock.Lock()
+		calls++ // calls incremented here to satisfy staticcheck.
 		planLock.Unlock()
-		calls++
 	})
 
 	// Run operations in goroutine so we can time out the test if it fails.
@@ -477,10 +478,10 @@ services:
         override: replace
         command: /bin/sh
 `)
-		err = manager.AppendLayer(layer1)
+		err = manager.AppendLayer(layer1, false)
 		c.Assert(err, IsNil)
 
-		err = manager.CombineLayer(layer1)
+		err = manager.CombineLayer(layer1, false)
 		c.Assert(err, IsNil)
 
 		layer2 := ps.parseLayer(c, 0, "label2", `
@@ -489,7 +490,7 @@ services:
         override: replace
         command: /bin/sh
 `)
-		err = manager.CombineLayer(layer2)
+		err = manager.CombineLayer(layer2, false)
 		c.Assert(err, IsNil)
 
 		err = manager.SetServiceArgs(map[string][]string{
@@ -508,3 +509,82 @@ services:
 
 	c.Assert(calls, Equals, 5)
 }
+
+func (ps *planSuite) TestAppendLayersWithoutInner(c *C) {
+	plan.RegisterSectionExtension(testField, testExtension{})
+	defer plan.UnregisterSectionExtension(testField)
+	var err error
+	ps.planMgr, err = planstate.NewManager(ps.layersDir)
+	c.Assert(err, IsNil)
+
+	layer := ps.parseLayer(c, 0, "foo/bar", "")
+	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, IsNil)
+	layer = ps.parseLayer(c, 0, "baz", "")
+	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, IsNil)
+	layer = ps.parseLayer(c, 0, "foo/baz", "")
+	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, ErrorMatches, ".*cannot insert sub-directory.*")
+}
+
+func (ps *planSuite) TestAppendLayersWithInner(c *C) {
+	plan.RegisterSectionExtension(testField, testExtension{})
+	defer plan.UnregisterSectionExtension(testField)
+	var err error
+	ps.planMgr, err = planstate.NewManager(ps.layersDir)
+	c.Assert(err, IsNil)
+
+	appendLabels := []string{
+		"foo",
+		"baz/aaa",
+		"something",
+		"baz/bbb",
+		"else",
+		"baz/ccc",
+		"zzz/zzz",
+		"final",
+	}
+
+	for _, label := range appendLabels {
+		layer := ps.parseLayer(c, 0, label, "")
+		err = ps.planMgr.AppendLayer(layer, true)
+		c.Assert(err, IsNil)
+	}
+
+	layersResult := []struct {
+		label string
+		order int
+	}{{
+		label: "foo",
+		order: 1000,
+	}, {
+		label: "baz/aaa",
+		order: 2001,
+	}, {
+		label: "baz/bbb",
+		order: 2002,
+	}, {
+		label: "baz/ccc",
+		order: 2003,
+	}, {
+		label: "something",
+		order: 3000,
+	}, {
+		label: "else",
+		order: 4000,
+	}, {
+		label: "zzz/zzz",
+		order: 5001,
+	}, {
+		label: "final",
+		order: 6000,
+	}}
+
+	plan := ps.planMgr.Plan()
+	// Check the layers order and each layer's order is correct.
+	for i, layer := range layersResult {
+		c.Assert(plan.Layers[i].Order, Equals, layer.order)
+		c.Assert(plan.Layers[i].Label, Equals, layer.label)
+	}
+}
diff --git a/internals/overlord/planstate/package_test.go b/internals/overlord/planstate/package_test.go
index 4e79d7e6f..a8c242728 100644
--- a/internals/overlord/planstate/package_test.go
+++ b/internals/overlord/planstate/package_test.go
@@ -147,7 +147,7 @@ type testSection struct {
 func (ts *testSection) Validate() error {
 	// Fictitious test requirement: fields must start with t
 	prefix := "t"
-	for field, _ := range ts.Entries {
+	for field := range ts.Entries {
 		if !strings.HasPrefix(field, prefix) {
 			return fmt.Errorf("%q entry names must start with %q", testField, prefix)
 		}
diff --git a/internals/overlord/servstate/manager_test.go b/internals/overlord/servstate/manager_test.go
index 3a8c9d30d..f2774374c 100644
--- a/internals/overlord/servstate/manager_test.go
+++ b/internals/overlord/servstate/manager_test.go
@@ -1759,7 +1759,7 @@ func (s *S) planChanged(c *C) {
 
 func (s *S) planAddLayer(c *C, layerYAML string) {
 	cnt := len(s.plan.Layers)
-	layer, err := plan.ParseLayer(cnt, fmt.Sprintf("testPlanLayer%v", cnt), []byte(layerYAML))
+	layer, err := plan.ParseLayer(cnt, fmt.Sprintf("test-plan-layer-%v", cnt), []byte(layerYAML))
 	c.Assert(err, IsNil)
 	// Resolve {{.NotifyDoneCheck}}
 	s.insertDoneChecks(c, layer)
diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go
index 0be9d0518..ff193b1c9 100644
--- a/internals/plan/extensions_test.go
+++ b/internals/plan/extensions_test.go
@@ -16,7 +16,6 @@ package plan_test
 
 import (
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"slices"
@@ -402,8 +401,7 @@ nexttest:
 			return n.field == xField
 		}) {
 			// Verify "x-field" data.
-			var x *xSection
-			x = p.Sections[xField].(*xSection)
+			x := p.Sections[xField].(*xSection)
 			c.Assert(err, IsNil)
 			c.Assert(x.Entries, DeepEquals, testData.result.x.Entries)
 		}
@@ -412,8 +410,7 @@ nexttest:
 			return n.field == yField
 		}) {
 			// Verify "y-field" data.
-			var y *ySection
-			y = p.Sections[yField].(*ySection)
+			y := p.Sections[yField].(*ySection)
 			c.Assert(err, IsNil)
 			c.Assert(y.Entries, DeepEquals, testData.result.y.Entries)
 		}
@@ -474,6 +471,7 @@ func (s *S) TestSectionOrderExt(c *C) {
 		Sections:   combined.Sections,
 	}
 	data, err := yaml.Marshal(plan)
+	c.Assert(err, IsNil)
 	c.Assert(string(data), Equals, string(reindent(`
 		services:
 			srv1:
@@ -511,7 +509,7 @@ func (s *S) writeLayerFiles(c *C, layersDir string, inputs []*inputLayer) {
 	c.Assert(err, IsNil)
 
 	for _, input := range inputs {
-		err := ioutil.WriteFile(filepath.Join(layersDir, fmt.Sprintf("%03d-%s.yaml", input.order, input.label)), reindent(input.yaml), 0644)
+		err := os.WriteFile(filepath.Join(layersDir, fmt.Sprintf("%03d-%s.yaml", input.order, input.label)), reindent(input.yaml), 0644)
 		c.Assert(err, IsNil)
 	}
 }
@@ -548,17 +546,15 @@ func (x xExtension) CombineSections(sections ...plan.Section) (plan.Section, err
 }
 
 func (x xExtension) ValidatePlan(p *plan.Plan) error {
-	var xs *xSection
-	xs = p.Sections[xField].(*xSection)
+	xs := p.Sections[xField].(*xSection)
 	if xs != nil {
-		var ys *ySection
-		ys = p.Sections[yField].(*ySection)
+		ys := p.Sections[yField].(*ySection)
 
 		// Test dependency: Make sure every Y field in X refer to an existing Y entry.
 		for xEntryField, xEntryValue := range xs.Entries {
 			for _, yReference := range xEntryValue.Y {
 				found := false
-				for yEntryField, _ := range ys.Entries {
+				for yEntryField := range ys.Entries {
 					if yReference == yEntryField {
 						found = true
 						break
diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index d75146b7f..3d65675fe 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -154,6 +154,16 @@ func (p *Plan) MarshalYAML() (interface{}, error) {
 	return plan, nil
 }
 
+// Layer represents an unmarshalled YAML layer configuration file. Layer files
+// are maintained as part of the Plan, ordered by their respective order
+// number. Each layer configuration also has a unique label, used for locating
+// and updating a specific layer configuration.
+//
+// Pebble supports a two-level layer configuration directory structure. In the
+// root layers directory, both layer files and layer sub-directories are
+// allowed. Within a sub-directory, only layer files are allowed.
+//
+// Please see ReadLayersDir for more details.
 type Layer struct {
 	Order       int                   `yaml:"-"`
 	Label       string                `yaml:"-"`
@@ -823,7 +833,7 @@ func (layer *Layer) Validate() error {
 	for name, service := range layer.Services {
 		if name == "" {
 			return &FormatError{
-				Message: fmt.Sprintf("cannot use empty string as service name"),
+				Message: "cannot use empty string as service name",
 			}
 		}
 		if name == "pebble" {
@@ -880,7 +890,7 @@ func (layer *Layer) Validate() error {
 	for name, check := range layer.Checks {
 		if name == "" {
 			return &FormatError{
-				Message: fmt.Sprintf("cannot use empty string as check name"),
+				Message: "cannot use empty string as check name",
 			}
 		}
 		if check == nil {
@@ -890,7 +900,7 @@ func (layer *Layer) Validate() error {
 		}
 		if name == "" {
 			return &FormatError{
-				Message: fmt.Sprintf("cannot use empty string as log target name"),
+				Message: "cannot use empty string as log target name",
 			}
 		}
 		if check.Level != UnsetLevel && check.Level != AliveLevel && check.Level != ReadyLevel {
@@ -1259,7 +1269,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) {
 	// will at least have an empty node. This means we can consistently
 	// let the extension allocate and decode the yaml node for all sections,
 	// and in the case where it is zero, we get an empty backing type instance.
-	for field, _ := range sectionExtensions {
+	for field := range sectionExtensions {
 		sections[field] = yaml.Node{}
 	}
 	err := yaml.Unmarshal(data, &sections)
@@ -1349,44 +1359,155 @@ func validServiceAction(action ServiceAction, additionalValid ...ServiceAction)
 	}
 }
 
-var fnameExp = regexp.MustCompile("^([0-9]{3})-([a-z](?:-?[a-z0-9]){2,}).yaml$")
+// ReadLayersDir loads the YAML layer files from the first two directory
+// levels starting at layersDir in the order as specified by the order
+// directory and file order prefixes. The directory and file suffixes
+// are dropped in the returned labels.
+//
+//	| File (inside layersDir)    | Order           | Label   |
+//	| -------------------------- | --------------- | ------- |
+//	| 001-foo.yaml               | 001-000 => 1000 | foo     |
+//	| 002-bar.d/001-aaa.yaml     | 002-001 => 2001 | bar/aaa |
+//	| 002-bar.d/002-bbb.yaml     | 002-002 => 2002 | bar/bbb |
+//	| 003-baz.yaml               | 003-000 => 3000 | baz     |
+func ReadLayersDir(layersDir string) ([]*Layer, error) {
+	var layers []*Layer
+
+	// Read the first-level directory
+	l1Entries, err := configLayerEntries(layersDir, true)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, l1Entry := range l1Entries {
+		l1Path := filepath.Join(layersDir, l1Entry.name)
+
+		// Let's check if the path (including a symlink) is a directory.
+		info, err := os.Stat(l1Path)
+		if err != nil {
+			return nil, err
+		}
+
+		if info.IsDir() {
+			// Read the second-level directory
+			l2Entries, err := configLayerEntries(l1Path, false)
+			if err != nil {
+				return nil, err
+			}
+
+			// Add the config files from the second level
+			for _, l2Entry := range l2Entries {
+				layer, err := loadConfigLayer(layersDir, l1Entry, l2Entry)
+				if err != nil {
+					return nil, err
+				}
+				layers = append(layers, layer)
+			}
+		} else {
+			// Add the config files from the first level
+			layer, err := loadConfigLayer(layersDir, l1Entry, nil)
+			if err != nil {
+				return nil, err
+			}
+			layers = append(layers, layer)
+		}
+	}
+
+	return layers, nil
+}
 
-func ReadLayersDir(dirname string) ([]*Layer, error) {
-	finfos, err := os.ReadDir(dirname)
+// loadConfigLayer loads a layer configuration file and returns a Layer
+// on success. The layer configuration is typically in the Pebble layers
+// root directory, in which case l2Entry must be nil. If the file is
+// inside a sub-directory, l1Entry must supply information on the directory,
+// while l2Entry information on the file name itself.
+func loadConfigLayer(layersDir string, l1Entry *configEntry, l2Entry *configEntry) (*Layer, error) {
+	// Resolve the order and label, which may include additional
+	// information from an optional sub-directory prefix.
+	label := l1Entry.label
+	order := 1000 * l1Entry.order
+	path := filepath.Join(layersDir, l1Entry.name)
+	if l2Entry != nil {
+		// Config layer is inside a sub-directory.
+		label = label + "/" + l2Entry.label
+		order = order + l2Entry.order
+		path = filepath.Join(path, l2Entry.name)
+	}
+
+	data, err := os.ReadFile(path)
 	if err != nil {
 		// Errors from package os generally include the path.
+		return nil, fmt.Errorf("cannot read layer file: %v", err)
+	}
+
+	layer, err := ParseLayer(order, label, data)
+	if err != nil {
+		return nil, err
+	}
+
+	return layer, nil
+}
+
+// configEntryRegexp matches either a valid config layer YAML file name or a
+// valid config layer directory. Match[1] is the 3-digit order and match[2]
+// is the label.
+var configEntryRegexp = regexp.MustCompile(`^([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})(.yaml|.d)$`)
+
+type configEntry struct {
+	name  string
+	order int
+	label string
+}
+
+// configLayerEntries reads a directory containing config layer files or
+// sub-directories and validates that the naming is valid. If dirOK is
+// set to false it will not permit sub-directories within the supplied
+// configDir path. The returned string slice is ordered alphanumerically
+// (so names are automatically ordered by their 'order').
+func configLayerEntries(configDir string, dirOK bool) (configs []*configEntry, err error) {
+	entries, err := os.ReadDir(configDir)
+	if err != nil {
 		return nil, fmt.Errorf("cannot read layers directory: %v", err)
 	}
 
 	orders := make(map[int]string)
 	labels := make(map[string]int)
-
-	// Documentation says ReadDir result is already sorted by name.
-	// This is fundamental here so if reading changes make sure the
-	// sorting is preserved.
-	var layers []*Layer
-	for _, finfo := range finfos {
-		if finfo.IsDir() || !strings.HasSuffix(finfo.Name(), ".yaml") {
+	for _, entry := range entries {
+		// Let's not fail to start the system up if some unrelated file ended
+		// up in the layers directory by accident.
+		if !strings.HasSuffix(entry.Name(), ".yaml") && !strings.HasSuffix(entry.Name(), ".d") {
 			continue
 		}
-		// TODO Consider enforcing permissions and ownership here to
-		//      avoid mistakes that could lead to hacks.
-		match := fnameExp.FindStringSubmatch(finfo.Name())
+
+		// Let's check if the path (including a symlink) is a directory.
+		info, err := os.Stat(filepath.Join(configDir, entry.Name()))
+		if err != nil {
+			return nil, err
+		}
+
+		// Let's ensure the file or sub-directory name is valid.
+		match := configEntryRegexp.FindStringSubmatch(entry.Name())
 		if match == nil {
-			return nil, fmt.Errorf("invalid layer filename: %q (must look like \"123-some-label.yaml\")", finfo.Name())
+			if info.IsDir() {
+				return nil, fmt.Errorf("invalid layer sub-directory name: %q (must look like \"123-some-label.d\")", entry.Name())
+			} else {
+				return nil, fmt.Errorf("invalid layer filename: %q (must look like \"123-some-label.yaml\")", entry.Name())
+			}
 		}
 
-		data, err := os.ReadFile(filepath.Join(dirname, finfo.Name()))
-		if err != nil {
-			// Errors from package os generally include the path.
-			return nil, fmt.Errorf("cannot read layer file: %v", err)
+		// Only the root layers directory support sub-directories.
+		if info.IsDir() && !dirOK {
+			return nil, fmt.Errorf("cannot have a layers sub-directory at this level")
 		}
+
+		// Extract the order and label from the match.
 		label := match[2]
 		order, err := strconv.Atoi(match[1])
 		if err != nil {
 			panic(fmt.Sprintf("internal error: filename regexp is wrong: %v", err))
 		}
 
+		// Let's make sure no duplicate orders or labels appear.
 		oldLabel, dupOrder := orders[order]
 		oldOrder, dupLabel := labels[label]
 		if dupOrder {
@@ -1395,19 +1516,20 @@ func ReadLayersDir(dirname string) ([]*Layer, error) {
 			oldLabel = label
 		}
 		if dupOrder || dupLabel {
-			return nil, fmt.Errorf("invalid layer filename: %q not unique (have \"%03d-%s.yaml\" already)", finfo.Name(), oldOrder, oldLabel)
+			return nil, fmt.Errorf("invalid layer filename: %q not unique (have \"%03d-%s.yaml\" already)", entry.Name(), oldOrder, oldLabel)
 		}
-
 		orders[order] = label
 		labels[label] = order
 
-		layer, err := ParseLayer(order, label, data)
-		if err != nil {
-			return nil, err
-		}
-		layers = append(layers, layer)
+		// All is good for this entry.
+		configs = append(configs, &configEntry{
+			name:  entry.Name(),
+			order: order,
+			label: label,
+		})
 	}
-	return layers, nil
+
+	return configs, nil
 }
 
 // ReadDir reads the configuration layers from layersDir,
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index 99b617a8b..000e0c30e 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -20,6 +20,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"slices"
 	"strings"
 	"time"
 	"unicode"
@@ -2138,6 +2139,7 @@ func (s *S) TestSectionOrder(c *C) {
 		LogTargets: combined.LogTargets,
 	}
 	data, err := yaml.Marshal(plan)
+	c.Assert(err, IsNil)
 	c.Assert(string(data), Equals, string(reindent(`
 	services:
 		srv1:
@@ -2156,3 +2158,153 @@ func (s *S) TestSectionOrder(c *C) {
 			services: []
 			override: replace`)))
 }
+
+// createLayerPath combines the base path with a configuration layer
+// filename (which may include a single sub-directory), and creates
+// the missing directories and an empty configuration file.
+func createLayerPath(c *C, base string, name string) {
+	path := filepath.Join(base, name)
+	err := os.MkdirAll(filepath.Dir(path), 0777)
+	c.Assert(err, IsNil)
+	file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0644)
+	c.Assert(err, IsNil)
+	// Let's mix in the layer name into the command so we can
+	// verify the correct layer has the correct file content.
+	_, err = file.Write(reindent(fmt.Sprintf(`
+	services:
+		srv:
+			override: replace
+			command: %s`, name)))
+	c.Assert(err, IsNil)
+	err = file.Close()
+	c.Assert(err, IsNil)
+}
+
+var readDirTests = []struct {
+	summary    string
+	layerNames []string
+	orders     []int
+	labels     []string
+	error      string
+}{{
+	summary: "Invalid filename #1",
+	layerNames: []string{
+		"001foo.yaml",
+	},
+	error: ".*invalid layer filename.*",
+}, {
+	summary: "Invalid filename #2",
+	layerNames: []string{
+		"001-foo.d/001foo.yaml",
+	},
+	error: ".*invalid layer filename.*",
+}, {
+	summary: "Invalid sub-directory",
+	layerNames: []string{
+		"001foo.d/001-bar.yaml",
+	},
+	error: ".*invalid layer sub.*",
+}, {
+	summary: "Not unique order #1",
+	layerNames: []string{
+		"001-foo.yaml",
+		"001-bar.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Not unique order #2",
+	layerNames: []string{
+		"002-dir.d/001-foo.yaml",
+		"002-dir.d/001-bar.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Not unique label #1",
+	layerNames: []string{
+		"001-foo.yaml",
+		"002-foo.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Not unique label #2",
+	layerNames: []string{
+		"002-dir.d/001-foo.yaml",
+		"002-dir.d/002-foo.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Valid load",
+	layerNames: []string{
+		"001-aaa.yaml",
+		"010-bbb.yaml",
+		"002-dir.d/100-foo.yaml",
+		"002-dir.d/090-bar.yaml",
+		"008-ccc.yaml",
+		"900-overlay.d/002-something.yaml",
+		"900-overlay.d/001-else.yaml",
+		"003-plans.d/999-final.yaml",
+		"009-baz.d/009-baz.yaml",
+	},
+	orders: []int{
+		1000,
+		2090,
+		2100,
+		3999,
+		8000,
+		9009,
+		10000,
+		900001,
+		900002,
+	},
+	labels: []string{
+		"aaa",
+		"dir/bar",
+		"dir/foo",
+		"plans/final",
+		"ccc",
+		"baz/baz",
+		"bbb",
+		"overlay/else",
+		"overlay/something",
+	},
+}}
+
+func (s *S) TestReadLayersDir(c *C) {
+	for _, test := range readDirTests {
+		c.Logf("Running ReadLayersDir: %s", test.summary)
+
+		tempDir := c.MkDir()
+
+		for _, layerName := range test.layerNames {
+			createLayerPath(c, tempDir, layerName)
+		}
+
+		layers, err := plan.ReadLayersDir(tempDir)
+		if test.error != "" || err != nil {
+			c.Assert(err, ErrorMatches, test.error)
+		}
+
+		c.Assert(len(layers), Equals, len(test.labels))
+		c.Assert(len(layers), Equals, len(test.orders))
+
+		for i, layer := range layers {
+			c.Assert(layer.Order, Equals, test.orders[i])
+			c.Assert(layer.Label, Equals, test.labels[i])
+
+			// Let's make sure each file contains the expected
+			// command. This will confirm the content is loaded
+			// from the correct file.
+			ordered := make([]string, 0, len(test.layerNames))
+			ordered = append(ordered, test.layerNames...)
+			slices.Sort(ordered)
+
+			c.Assert(layer.Services, DeepEquals, map[string]*plan.Service{
+				"srv": {
+					Name:     "srv",
+					Override: "replace",
+					Command:  ordered[i],
+				},
+			})
+		}
+	}
+}