Fix warewulfd for new contextual overlay names

The recent PR #883 replaced the standard overlay naming convention with
static names for each of the runtime and wwinit/system contexts; but the
method by which it left the "legacy" naming convention in place led to a
bug where warewulfd was still attempting to serve overlays with the
previous naming convention, though this name wasn't being built.

This PR refactors contextual overlay image naming to be, I think, a more
typical golang pattern, and updates affected code to match.

This PR also included tests for all functions modified by this change.

Fixes #896

Signed-off-by: Jonathon Anderson <janderson@ciq.com>

internal/pkg/overlay/config.go

@@ -28,29 +28,32 @@ func OverlaySourceDir(overlayName string) string {
 	return overlaypath
 }
 
-/*
-Returns the overlay name of the image for a given node
-*/
-func OverlayImage(nodeName string, overlayName []string, img_context ...string) string {
+// OverlayImage returns the full path to an overlay image based on the
+// context and the overlays contained in it.
+//
+// If a context is provided, the image file name is based on that
+// context name, in the form __{CONTEXT}__.
+//
+// If the context is empty ("") the image file name is a concatenated
+// list of the contained overlays joined by "-".
+//
+// If the context is empty and no overlays are specified, the empty
+// string is returned.
+func OverlayImage(nodeName string, context string, overlayNames []string) string {
 	var name string
-	var context string
-
-	/* Check optional context argument. If missing, default to legacy. */
-	if len(img_context) == 0 {
-		context = "legacy"
+	if context != "" {
+		if len(overlayNames) > 0 {
+			wwlog.Warn("context(%v) and overlays(%v) specified: prioritizing context(%v)",
+				context, overlayNames, context)
+		}
+		name = "__" + strings.ToUpper(context) + "__.img"
+	} else if len(overlayNames) > 0 {
+		name = strings.Join(overlayNames, "-")+".img"
 	} else {
-		context = img_context[0]
+		wwlog.Warn("unable to generate overlay image path: no context or overlays specified")
+		return ""
 	}
 
 	conf := warewulfconf.Get()
-
-	switch context {
-	case "legacy":
-		name = strings.Join(overlayName, "-")+".img"
-	default:
-		wwlog.Warn("Context %s passed to OverlayImage(), using %s to build image name.", context, "__" + strings.ToUpper(context) + "__")
-		name = "__" + strings.ToUpper(context) + "__.img"
-	}
-
 	return path.Join(conf.Paths.WWProvisiondir, "overlays/", nodeName, name)
 }

internal/pkg/overlay/config_test.go

@@ -0,0 +1,38 @@
+package overlay
+
+import (
+	"testing"
+	warewulfconf "github.com/hpcng/warewulf/internal/pkg/config"
+)
+
+var overlayImageTests = []struct{
+	description string
+	node string
+	context string
+	overlays []string
+	image string
+}{
+	{"all empty", "", "", nil, ""},
+	{"empty with named context", "", "system", nil, "p/overlays/__SYSTEM__.img"},
+	{"empty with named overlay", "", "", []string{"o1"}, "p/overlays/o1.img"},
+	{"empty with two named overlays", "", "", []string{"o1", "o2"}, "p/overlays/o1-o2.img"},
+	{"empty node", "node1", "", nil, ""},
+	{"node system overlay", "node1", "system", nil, "p/overlays/node1/__SYSTEM__.img"},
+	{"node runtime overlay", "node1", "runtime", nil, "p/overlays/node1/__RUNTIME__.img"},
+	{"node single overlay", "node1", "", []string{"o1"}, "p/overlays/node1/o1.img"},
+	{"node two overlays", "node1", "", []string{"o1", "o2"}, "p/overlays/node1/o1-o2.img"},
+	{"node with context and overlays", "node1", "system", []string{"o1", "o2"}, "p/overlays/node1/__SYSTEM__.img"},
+}
+
+func Test_OverlayImage(t *testing.T) {
+	conf := warewulfconf.Get()
+	conf.Paths.WWProvisiondir = "p"
+	for _, tt := range overlayImageTests {
+		t.Run(tt.description, func(t *testing.T) {
+			out := OverlayImage(tt.node, tt.context, tt.overlays)
+			if  tt.image != out {
+				t.Errorf("got %q, want %q", out, tt.image)
+			}
+		})
+	}
+}

internal/pkg/overlay/overlay.go

@@ -47,13 +47,13 @@ func BuildAllOverlays(nodes []node.NodeInfo) error {
 
 		sysOverlays := n.SystemOverlay.GetSlice()
 		wwlog.Info("Building system overlays for %s: [%s]", n.Id.Get(), strings.Join(sysOverlays, ", "))
-		err := BuildOverlay(n, sysOverlays, "system")
+		err := BuildOverlay(n, "system", sysOverlays)
 		if err != nil {
 			return errors.Wrapf(err, "could not build system overlays %v for node %s", sysOverlays, n.Id.Get())
 		}
 		runOverlays := n.RuntimeOverlay.GetSlice()
 		wwlog.Info("Building runtime overlays for %s: [%s]", n.Id.Get(), strings.Join(runOverlays, ", "))
-		err = BuildOverlay(n, runOverlays, "runtime")
+		err = BuildOverlay(n, "runtime", runOverlays)
 		if err != nil {
 			return errors.Wrapf(err, "could not build runtime overlays %v for node %s", runOverlays, n.Id.Get())
 		}
@@ -67,11 +67,11 @@ func BuildAllOverlays(nodes []node.NodeInfo) error {
 func BuildSpecificOverlays(nodes []node.NodeInfo, overlayNames []string) error {
 	for _, n := range nodes {
 		wwlog.Info("Building overlay for %s: %v", n.Id.Get(), overlayNames)
-                for _, overlayName := range overlayNames {
-                        err := BuildOverlay(n, []string{overlayName})
-		        if err != nil {
-			      return errors.Wrapf(err, "could not build overlay %s for node %s", overlayName, n.Id.Get())
-                        }
+		for _, overlayName := range overlayNames {
+			err := BuildOverlay(n, "", []string{overlayName})
+			if err != nil {
+				return errors.Wrapf(err, "could not build overlay %s for node %s", overlayName, n.Id.Get())
+			}
 		}
 
 	}
@@ -140,18 +140,10 @@ func OverlayInit(overlayName string) error {
 /*
 Build the given overlays for a node and create a Image for them
 */
-func BuildOverlay(nodeInfo node.NodeInfo, overlayNames []string, img_context ...string) error {
-	var context string
-	/* Check optional context argument. If missing, default to legacy. */
-	if len(img_context) == 0 {
-		context = "legacy"
-	} else {
-		context = img_context[0]
-	}
-
+func BuildOverlay(nodeInfo node.NodeInfo, context string, overlayNames []string) error {
 	// create the dir where the overlay images will reside
 	name := fmt.Sprintf("overlay %s/%v", nodeInfo.Id.Get(), overlayNames)
-	overlayImage := OverlayImage(nodeInfo.Id.Get(), overlayNames, context)
+	overlayImage := OverlayImage(nodeInfo.Id.Get(), context, overlayNames)
 	overlayImageDir := path.Dir(overlayImage)
 
 	err := os.MkdirAll(overlayImageDir, 0755)

internal/pkg/overlay/overlay_test.go

@@ -0,0 +1,232 @@
+package overlay
+
+import (
+	"io"
+	"os"
+	"path"
+	"strings"
+	"sort"
+	"testing"
+	"github.com/stretchr/testify/assert"
+	"github.com/sassoftware/go-rpmutils/cpio"
+	warewulfconf "github.com/hpcng/warewulf/internal/pkg/config"
+	"github.com/hpcng/warewulf/internal/pkg/node"
+)
+
+var buildOverlayTests = []struct{
+	description string
+	nodeName string
+	context string
+	overlays []string
+	image string
+	contents []string
+}{
+	{"empty", "", "", nil, "", nil},
+	{"empty node", "node1", "", nil, "", nil},
+	{"empty context", "", "system", nil, "", nil},
+	{"empty overlay", "", "", []string{"o1"}, "o1.img", []string{"o1.txt"}},
+	{"single overlay", "node1", "", []string{"o1"}, "node1/o1.img", []string{"o1.txt"}},
+	{"multiple overlays", "node1", "", []string{"o1", "o2"}, "node1/o1-o2.img", []string{"o1.txt", "o2.txt"}},
+	{"empty system overlay", "node1", "system", nil, "", nil},
+	{"empty runtime overlay", "node1", "runtime", nil, "", nil},
+	{"single system overlay", "node1", "system", []string{"o1"}, "node1/__SYSTEM__.img", []string{"o1.txt"}},
+	{"single runtime overlay", "node1", "runtime", []string{"o1"}, "node1/__RUNTIME__.img", []string{"o1.txt"}},
+	{"two system overlays", "node1", "system", []string{"o1", "o2"}, "node1/__SYSTEM__.img", []string{"o1.txt", "o2.txt"}},
+	{"two runtime overlays", "node1", "runtime", []string{"o1", "o2"}, "node1/__RUNTIME__.img", []string{"o1.txt", "o2.txt"}},
+}
+
+func Test_BuildOverlay(t *testing.T) {
+	conf := warewulfconf.Get()
+	overlayDir, overlayDirErr := os.MkdirTemp(os.TempDir(), "ww-test-overlay-*")
+	assert.NoError(t, overlayDirErr)
+	defer os.RemoveAll(overlayDir)
+	conf.Paths.WWOverlaydir = overlayDir
+	assert.NoError(t, os.Mkdir(path.Join(overlayDir, "o1"), 0700))
+	{ _, err := os.Create(path.Join(overlayDir, "o1", "o1.txt")); assert.NoError(t, err) }
+	assert.NoError(t, os.Mkdir(path.Join(overlayDir, "o2"), 0700))
+	{ _, err := os.Create(path.Join(overlayDir, "o2", "o2.txt")); assert.NoError(t, err) }
+
+	for _, tt := range buildOverlayTests {
+		assert.True(t, (tt.image != "" && tt.contents != nil) || (tt.image == "" && tt.contents == nil),
+			"image and contents must eiher be populated or empty together")
+
+		nodeInfo := node.NodeInfo{}
+		nodeInfo.Id.Set(tt.nodeName)
+		t.Run(tt.description, func(t *testing.T) {
+			provisionDir, provisionDirErr := os.MkdirTemp(os.TempDir(), "ww-test-provision-*")
+			assert.NoError(t, provisionDirErr)
+			defer os.RemoveAll(provisionDir)
+			conf.Paths.WWProvisiondir = provisionDir
+
+			err := BuildOverlay(nodeInfo, tt.context, tt.overlays)
+			if len(tt.image) > 0 {
+				image := path.Join(provisionDir, "overlays", tt.image)
+				assert.FileExists(t, image)
+				assert.NoError(t, err)
+
+				sort.Strings(tt.contents)
+				files := cpioFiles(t, image)
+				sort.Strings(files)
+				assert.Equal(t, tt.contents, files)
+			} else {
+				assert.Error(t, err)
+				dirName := path.Join(provisionDir, "overlays", tt.nodeName)
+				isEmpty := dirIsEmpty(t, dirName)
+				assert.True(t, isEmpty, "%v should be empty, but isn't", dirName)
+			}
+		})
+	}
+}
+
+var buildAllOverlaysTests = []struct{
+	description string
+	nodes []string
+	systemOverlays []string
+	runtimeOverlays []string
+	succeed bool
+}{
+	{"no nodes", nil, nil, nil, true},
+	{"single empty node", []string{"node1"}, nil, nil, false},
+	{"two empty node", []string{"node1", "node2"}, nil, nil, false},
+	{"single node with system overlay", []string{"node1"},
+		[]string{"o1"}, nil, false},
+	{"two nodes with system overlays", []string{"node1", "node2"},
+		[]string{"o1", "o1,o2"}, nil, false},
+	{"single node with runtime overlay", []string{"node1"},
+		nil, []string{"o1"}, false},
+	{"two nodes with runtime overlays", []string{"node1", "node2"},
+		nil, []string{"o1", "o1,o2"}, false},
+	{"stingle node with full overlays", []string{"node1"},
+		[]string{"o1"}, []string{"o2"}, true},
+	{"two nodes with full overlays", []string{"node1", "node2"},
+		[]string{"o1", "o1,o2"}, []string{"o2", "o2"}, true},
+}
+
+
+func Test_BuildAllOverlays(t *testing.T) {
+	conf := warewulfconf.Get()
+	overlayDir, overlayDirErr := os.MkdirTemp(os.TempDir(), "ww-test-overlay-*")
+	assert.NoError(t, overlayDirErr)
+	defer os.RemoveAll(overlayDir)
+	conf.Paths.WWOverlaydir = overlayDir
+	assert.NoError(t, os.Mkdir(path.Join(overlayDir, "o1"), 0700))
+	assert.NoError(t, os.Mkdir(path.Join(overlayDir, "o2"), 0700))
+
+	for _, tt := range buildAllOverlaysTests {
+		t.Run(tt.description, func(t *testing.T) {
+			provisionDir, provisionDirErr := os.MkdirTemp(os.TempDir(), "ww-test-provision-*")
+			assert.NoError(t, provisionDirErr)
+			defer os.RemoveAll(provisionDir)
+			conf.Paths.WWProvisiondir = provisionDir
+
+			var nodes []node.NodeInfo
+			for i, nodeName := range tt.nodes {
+				nodeInfo := node.NodeInfo{}
+				nodeInfo.Id.Set(nodeName)
+				if tt.systemOverlays != nil {
+					nodeInfo.SystemOverlay.SetSlice(strings.Split(tt.systemOverlays[i], ","))
+				}
+				if tt.runtimeOverlays != nil {
+					nodeInfo.RuntimeOverlay.SetSlice(strings.Split(tt.runtimeOverlays[i], ","))
+				}
+				nodes = append(nodes, nodeInfo)
+			}
+			err := BuildAllOverlays(nodes)
+			if !tt.succeed {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				for _, nodeName := range tt.nodes {
+					assert.FileExists(t, path.Join(provisionDir, "overlays", nodeName, "__SYSTEM__.img"))
+					assert.FileExists(t, path.Join(provisionDir, "overlays", nodeName, "__RUNTIME__.img"))
+				}
+			}
+		})
+	}
+}
+
+var buildSpecificOverlaysTests = []struct{
+	description string
+	nodes []string
+	overlays string
+	images []string
+	succeed bool
+}{
+	{"no nodes", nil, "", nil, true},
+	{"single empty node", []string{"node1"}, "", nil, false},
+	{"two empty node", []string{"node1", "node2"}, "", nil, false},
+	{"single node with single overlay", []string{"node1"}, "o1",
+		[]string{"node1/o1.img"}, true},
+	{"two nodes with single overlay", []string{"node1", "node2"}, "o1",
+		[]string{"node1/o1.img", "node2/o1.img"}, true},
+	{"single node with multi overlay", []string{"node1"}, "o1,o2",
+		[]string{"node1/o1.img", "node1/o2.img"}, true},
+	{"two nodes with multi overlays", []string{"node1", "node2"}, "o1,o2",
+		[]string{"node1/o1.img", "node1/o2.img", "node2/o1.img", "node2/o2.img"}, true},
+}
+
+
+func Test_BuildSpecificOverlays(t *testing.T) {
+	conf := warewulfconf.Get()
+	overlayDir, overlayDirErr := os.MkdirTemp(os.TempDir(), "ww-test-overlay-*")
+	assert.NoError(t, overlayDirErr)
+	defer os.RemoveAll(overlayDir)
+	conf.Paths.WWOverlaydir = overlayDir
+	assert.NoError(t, os.Mkdir(path.Join(overlayDir, "o1"), 0700))
+	assert.NoError(t, os.Mkdir(path.Join(overlayDir, "o2"), 0700))
+
+	for _, tt := range buildSpecificOverlaysTests {
+		t.Run(tt.description, func(t *testing.T) {
+			provisionDir, provisionDirErr := os.MkdirTemp(os.TempDir(), "ww-test-provision-*")
+			assert.NoError(t, provisionDirErr)
+			defer os.RemoveAll(provisionDir)
+			conf.Paths.WWProvisiondir = provisionDir
+
+			var nodes []node.NodeInfo
+			for _, nodeName := range tt.nodes {
+				nodeInfo := node.NodeInfo{}
+				nodeInfo.Id.Set(nodeName)
+				nodes = append(nodes, nodeInfo)
+			}
+			err := BuildSpecificOverlays(nodes, strings.Split(tt.overlays, ","))
+			if !tt.succeed {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				for _, image := range tt.images {
+					assert.FileExists(t, path.Join(provisionDir, "overlays", image))
+				}
+			}
+		})
+	}
+}
+
+func dirIsEmpty(t *testing.T, name string) bool {
+	f, err := os.Open(name)
+	if err != nil {
+		t.Log(err)
+		return true
+	}
+	defer f.Close()
+
+	dirnames, err2 := f.Readdirnames(1)
+	if err2 == io.EOF {
+		t.Log(err2)
+		return true
+	}
+	t.Log(dirnames)
+	return false
+}
+
+func cpioFiles(t *testing.T, name string) (files []string) {
+	f, openErr := os.Open(name)
+	if openErr != nil { return }
+	defer f.Close()
+
+	reader := cpio.NewReader(f)
+	for {
+		header, err := reader.Next()
+		if err != nil { return }
+		files = append(files, header.Filename())
+	}
+}