Skip to content

Commit 860a9be

Browse files
problameproblame
authored
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.
1 parent 4d6583e commit 860a9be

9 files changed

Lines changed: 108 additions & 54 deletions

File tree

‎internal/client/status/viewmodel/render.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import (
1111
yaml "github.com/zrepl/yaml-config"
1212

1313
"github.com/zrepl/zrepl/internal/client/status/viewmodel/stringbuilder"
14-
"github.com/zrepl/zrepl/internal/daemon"
14+
"github.com/zrepl/zrepl/internal/config"
1515
"github.com/zrepl/zrepl/internal/daemon/job"
1616
"github.com/zrepl/zrepl/internal/daemon/pruner"
1717
"github.com/zrepl/zrepl/internal/daemon/snapper"
@@ -85,7 +85,7 @@ func (m *M) Update(p Params) {
8585
// filter out internal jobs
8686
var jobsList []*Job
8787
for _, j := range m.jobsList {
88-
if daemon.IsInternalJobName(j.name) {
88+
if config.IsInternalJobName(j.name) {
8989
continue
9090
}
9191
jobsList = append(jobsList, j)

‎internal/config/config.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
pathpkg "path"
88
"path/filepath"
9+
"strings"
910
"time"
1011

1112
"github.com/pkg/errors"
@@ -693,9 +694,32 @@ func ParseConfig(path string) (rootConfig *Config, err error) {
693694
return nil, err
694695
}
695696

697+
if err = validateJobNames(rootConfig); err != nil {
698+
return nil, err
699+
}
700+
696701
return rootConfig, err
697702
}
698703

704+
func IsInternalJobName(s string) bool {
705+
return strings.HasPrefix(s, "_")
706+
}
707+
708+
func validateJobNames(config *Config) error {
709+
seen := make(map[string]struct{})
710+
for _, job := range config.Jobs {
711+
name := job.Name()
712+
if IsInternalJobName(name) {
713+
return errors.Errorf("job name %q is reserved for internal use (starts with _)", name)
714+
}
715+
if _, ok := seen[name]; ok {
716+
return errors.Errorf("duplicate job name %q", name)
717+
}
718+
seen[name] = struct{}{}
719+
}
720+
return nil
721+
}
722+
699723
func expandConfigInclude(configPath string, config *Config) (err error) {
700724
var includeConfigPaths []string
701725
for _, path := range config.Include {

‎internal/config/config_include_test.go‎

Lines changed: 0 additions & 44 deletions
This file was deleted.

‎internal/config/config_test.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,20 @@ func TestSampleConfigsAreParsedWithoutErrors(t *testing.T) {
4545

4646
}
4747

48+
func TestInvalidSampleConfigsFailToParse(t *testing.T) {
49+
paths, err := filepath.Glob("./samples/invalid/*/zrepl.yml")
50+
require.NoError(t, err, "glob failed")
51+
require.NotEmpty(t, paths, "no invalid sample configs found")
52+
53+
for _, p := range paths {
54+
t.Run(p, func(t *testing.T) {
55+
_, err := ParseConfig(p)
56+
require.Error(t, err, "expected config %s to fail parsing", p)
57+
t.Logf("config %s failed as expected: %v", p, err)
58+
})
59+
}
60+
}
61+
4862
// template must be a template/text template with a single '{{ . }}' as placeholder for val
4963
//
5064
//nolint:deadcode,unused

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
jobs:
2+
- type: snap
3+
name: "my_job"
4+
filesystems: {
5+
"<": true,
6+
}
7+
snapshotting:
8+
type: manual
9+
pruning:
10+
keep:
11+
- type: last_n
12+
count: 10
13+
14+
- type: snap
15+
name: "my_job"
16+
filesystems: {
17+
"<": true,
18+
}
19+
snapshotting:
20+
type: manual
21+
pruning:
22+
keep:
23+
- type: last_n
24+
count: 5

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
jobs:
2+
- type: snap
3+
name: "my_job"
4+
filesystems: {
5+
"<": true,
6+
}
7+
snapshotting:
8+
type: manual
9+
pruning:
10+
keep:
11+
- type: last_n
12+
count: 5

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
jobs:
2+
- type: snap
3+
name: "my_job"
4+
filesystems: {
5+
"<": true,
6+
}
7+
snapshotting:
8+
type: manual
9+
pruning:
10+
keep:
11+
- type: last_n
12+
count: 10
13+
14+
include:
15+
- ./included.yml

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
jobs:
2+
- type: snap
3+
name: "_internal_job"
4+
filesystems: {
5+
"<": true,
6+
}
7+
snapshotting:
8+
type: manual
9+
pruning:
10+
keep:
11+
- type: last_n
12+
count: 10

‎internal/daemon/daemon.go‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"fmt"
66
"os"
77
"os/signal"
8-
"strings"
98
"sync"
109
"syscall"
1110
"time"
@@ -64,7 +63,7 @@ func Run(ctx context.Context, conf *config.Config) error {
6463
})
6564

6665
for _, job := range confJobs {
67-
if IsInternalJobName(job.Name()) {
66+
if config.IsInternalJobName(job.Name()) {
6867
panic(fmt.Sprintf("internal job name used for config job '%s'", job.Name())) //FIXME
6968
}
7069
}
@@ -211,21 +210,19 @@ const (
211210
jobNameControl = "_control"
212211
)
213212

214-
func IsInternalJobName(s string) bool {
215-
return strings.HasPrefix(s, "_")
216-
}
217-
218213
func (s *jobs) start(ctx context.Context, j job.Job, internal bool) {
219214
s.m.Lock()
220215
defer s.m.Unlock()
221216

222217
ctx = logging.WithInjectedField(ctx, logging.JobField, j.Name())
223218

224219
jobName := j.Name()
225-
if !internal && IsInternalJobName(jobName) {
220+
221+
// package `config` enforces these with clean errors, these are just assertions
222+
if !internal && config.IsInternalJobName(jobName) {
226223
panic(fmt.Sprintf("internal job name used for non-internal job %s", jobName))
227224
}
228-
if internal && !IsInternalJobName(jobName) {
225+
if internal && !config.IsInternalJobName(jobName) {
229226
panic(fmt.Sprintf("internal job does not use internal job name %s", jobName))
230227
}
231228
if _, ok := s.jobs[jobName]; ok {

0 commit comments

Comments
 (0)