config: detect duplicate & internal job names at parse time (#908)

Before this PR, config parsing would accept duplicate job names. `zrepl
daemon` would later fail to start with a panic. But tools like `zrepl
configcheck` would pass.

This PR adds a check to ensure job names are unique.

Similarly, internal job names were not being rejected by config parsing
Move that check to parse-time as well.

Last, drive-by change: remove `internal/config/config_include_test.go`
introduced in #856 . These aren't necessary because there is already a
wildcard test for all valid configs.
This PR adds the complimentary "invalid config" wildcard test.

internal/client/status/viewmodel/render.go

@@ -11,7 +11,7 @@ import (
 	yaml "github.com/zrepl/yaml-config"
 
 	"github.com/zrepl/zrepl/internal/client/status/viewmodel/stringbuilder"
-	"github.com/zrepl/zrepl/internal/daemon"
+	"github.com/zrepl/zrepl/internal/config"
 	"github.com/zrepl/zrepl/internal/daemon/job"
 	"github.com/zrepl/zrepl/internal/daemon/pruner"
 	"github.com/zrepl/zrepl/internal/daemon/snapper"
@@ -85,7 +85,7 @@ func (m *M) Update(p Params) {
 	// filter out internal jobs
 	var jobsList []*Job
 	for _, j := range m.jobsList {
-		if daemon.IsInternalJobName(j.name) {
+		if config.IsInternalJobName(j.name) {
 			continue
 		}
 		jobsList = append(jobsList, j)

internal/config/config.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	pathpkg "path"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -693,9 +694,32 @@ func ParseConfig(path string) (rootConfig *Config, err error) {
 		return nil, err
 	}
 
+	if err = validateJobNames(rootConfig); err != nil {
+		return nil, err
+	}
+
 	return rootConfig, err
 }
 
+func IsInternalJobName(s string) bool {
+	return strings.HasPrefix(s, "_")
+}
+
+func validateJobNames(config *Config) error {
+	seen := make(map[string]struct{})
+	for _, job := range config.Jobs {
+		name := job.Name()
+		if IsInternalJobName(name) {
+			return errors.Errorf("job name %q is reserved for internal use (starts with _)", name)
+		}
+		if _, ok := seen[name]; ok {
+			return errors.Errorf("duplicate job name %q", name)
+		}
+		seen[name] = struct{}{}
+	}
+	return nil
+}
+
 func expandConfigInclude(configPath string, config *Config) (err error) {
 	var includeConfigPaths []string
 	for _, path := range config.Include {

internal/config/config_include_test.go

@@ -1,44 +0,0 @@
-package config
-
-import (
-	"testing"
-
-	"github.com/kr/pretty"
-	"github.com/stretchr/testify/require"
-)
-
-func TestIncludeSingle(t *testing.T) {
-	path := "./samples/include.yml"
-
-	t.Run(path, func(t *testing.T) {
-		config, err := ParseConfig(path)
-		if err != nil {
-			t.Errorf("error parsing %s:\n%+v", path, err)
-		}
-
-		require.NotNil(t, config)
-		require.NotNil(t, config.Global)
-		require.NotEmpty(t, config.Jobs)
-
-		t.Logf("file: %s", path)
-		t.Log(pretty.Sprint(config))
-	})
-}
-
-func TestIncludeDirectory(t *testing.T) {
-	path := "./samples/include_directory.yml"
-
-	t.Run(path, func(t *testing.T) {
-		config, err := ParseConfig(path)
-		if err != nil {
-			t.Errorf("error parsing %s:\n%+v", path, err)
-		}
-
-		require.NotNil(t, config)
-		require.NotNil(t, config.Global)
-		require.NotEmpty(t, config.Jobs)
-
-		t.Logf("file: %s", path)
-		t.Log(pretty.Sprint(config))
-	})
-}

internal/config/config_test.go

@@ -45,6 +45,20 @@ func TestSampleConfigsAreParsedWithoutErrors(t *testing.T) {
 
 }
 
+func TestInvalidSampleConfigsFailToParse(t *testing.T) {
+	paths, err := filepath.Glob("./samples/invalid/*/zrepl.yml")
+	require.NoError(t, err, "glob failed")
+	require.NotEmpty(t, paths, "no invalid sample configs found")
+
+	for _, p := range paths {
+		t.Run(p, func(t *testing.T) {
+			_, err := ParseConfig(p)
+			require.Error(t, err, "expected config %s to fail parsing", p)
+			t.Logf("config %s failed as expected: %v", p, err)
+		})
+	}
+}
+
 // template must be a template/text template with a single '{{ . }}' as placeholder for val
 //
 //nolint:deadcode,unused

internal/config/samples/invalid/duplicate_job_names/zrepl.yml

@@ -0,0 +1,24 @@
+jobs:
+  - type: snap
+    name: "my_job"
+    filesystems: {
+      "<": true,
+    }
+    snapshotting:
+      type: manual
+    pruning:
+      keep:
+        - type: last_n
+          count: 10
+
+  - type: snap
+    name: "my_job"
+    filesystems: {
+      "<": true,
+    }
+    snapshotting:
+      type: manual
+    pruning:
+      keep:
+        - type: last_n
+          count: 5

internal/config/samples/invalid/duplicate_job_names_with_include/included.yml

@@ -0,0 +1,12 @@
+jobs:
+  - type: snap
+    name: "my_job"
+    filesystems: {
+      "<": true,
+    }
+    snapshotting:
+      type: manual
+    pruning:
+      keep:
+        - type: last_n
+          count: 5

internal/config/samples/invalid/duplicate_job_names_with_include/zrepl.yml

@@ -0,0 +1,15 @@
+jobs:
+  - type: snap
+    name: "my_job"
+    filesystems: {
+      "<": true,
+    }
+    snapshotting:
+      type: manual
+    pruning:
+      keep:
+        - type: last_n
+          count: 10
+
+include:
+  - ./included.yml

internal/config/samples/invalid/internal_job_name/zrepl.yml

@@ -0,0 +1,12 @@
+jobs:
+  - type: snap
+    name: "_internal_job"
+    filesystems: {
+      "<": true,
+    }
+    snapshotting:
+      type: manual
+    pruning:
+      keep:
+        - type: last_n
+          count: 10

internal/daemon/daemon.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
-	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -64,7 +63,7 @@ func Run(ctx context.Context, conf *config.Config) error {
 	})
 
 	for _, job := range confJobs {
-		if IsInternalJobName(job.Name()) {
+		if config.IsInternalJobName(job.Name()) {
 			panic(fmt.Sprintf("internal job name used for config job '%s'", job.Name())) //FIXME
 		}
 	}
@@ -211,10 +210,6 @@ const (
 	jobNameControl    = "_control"
 )
 
-func IsInternalJobName(s string) bool {
-	return strings.HasPrefix(s, "_")
-}
-
 func (s *jobs) start(ctx context.Context, j job.Job, internal bool) {
 	s.m.Lock()
 	defer s.m.Unlock()
@@ -222,10 +217,12 @@ func (s *jobs) start(ctx context.Context, j job.Job, internal bool) {
 	ctx = logging.WithInjectedField(ctx, logging.JobField, j.Name())
 
 	jobName := j.Name()
-	if !internal && IsInternalJobName(jobName) {
+
+	// package `config` enforces these with clean errors, these are just assertions
+	if !internal && config.IsInternalJobName(jobName) {
 		panic(fmt.Sprintf("internal job name used for non-internal job %s", jobName))
 	}
-	if internal && !IsInternalJobName(jobName) {
+	if internal && !config.IsInternalJobName(jobName) {
 		panic(fmt.Sprintf("internal job does not use internal job name %s", jobName))
 	}
 	if _, ok := s.jobs[jobName]; ok {