fix(core): detect user/container and middleware label changes in job hash

cli/config.go

@@ -7,6 +7,7 @@ import (
 	"github.com/mcuadros/ofelia/core"
 	"github.com/mcuadros/ofelia/middlewares"
 
+	"github.com/gohugoio/hashstructure"
 	defaults "github.com/mcuadros/go-defaults"
 	gcfg "gopkg.in/gcfg.v1"
 )
@@ -266,6 +267,13 @@ type ExecJobConfig struct {
 	middlewares.MailConfig    `mapstructure:",squash"`
 }
 
+// Hash covers the job and its middleware settings, so a label change to
+// either of them (e.g. user or no-overlap) replaces the scheduled job.
+func (c *ExecJobConfig) Hash() uint64 {
+	hash, _ := hashstructure.Hash(c, nil)
+	return hash
+}
+
 func (c *ExecJobConfig) buildMiddlewares() {
 	c.ExecJob.Use(middlewares.NewOverlap(&c.OverlapConfig))
 	c.ExecJob.Use(middlewares.NewSlack(&c.SlackConfig))
@@ -290,6 +298,13 @@ type RunJobConfig struct {
 	middlewares.MailConfig    `mapstructure:",squash"`
 }
 
+// Hash covers the job and its middleware settings, so a label change to
+// either of them (e.g. user or no-overlap) replaces the scheduled job.
+func (c *RunJobConfig) Hash() uint64 {
+	hash, _ := hashstructure.Hash(c, nil)
+	return hash
+}
+
 func (c *RunJobConfig) buildMiddlewares() {
 	c.RunJob.Use(middlewares.NewOverlap(&c.OverlapConfig))
 	c.RunJob.Use(middlewares.NewSlack(&c.SlackConfig))
@@ -306,13 +321,27 @@ type LocalJobConfig struct {
 	middlewares.MailConfig    `mapstructure:",squash"`
 }
 
+// Hash covers the job and its middleware settings, so a label change to
+// either of them (e.g. user or no-overlap) replaces the scheduled job.
+func (c *LocalJobConfig) Hash() uint64 {
+	hash, _ := hashstructure.Hash(c, nil)
+	return hash
+}
+
 func (c *LocalJobConfig) buildMiddlewares() {
 	c.LocalJob.Use(middlewares.NewOverlap(&c.OverlapConfig))
 	c.LocalJob.Use(middlewares.NewSlack(&c.SlackConfig))
 	c.LocalJob.Use(middlewares.NewSave(&c.SaveConfig))
 	c.LocalJob.Use(middlewares.NewMail(&c.MailConfig))
 }
 
+// Hash covers the job and its middleware settings, so a label change to
+// either of them (e.g. user or no-overlap) replaces the scheduled job.
+func (c *RunServiceConfig) Hash() uint64 {
+	hash, _ := hashstructure.Hash(c, nil)
+	return hash
+}
+
 func (c *RunServiceConfig) buildMiddlewares() {
 	c.RunServiceJob.Use(middlewares.NewOverlap(&c.OverlapConfig))
 	c.RunServiceJob.Use(middlewares.NewSlack(&c.SlackConfig))

cli/docker_handler_test.go

@@ -363,7 +363,7 @@ func (s *TestDockerSuit) TestConsumeEventsTriggersLabelUpdate(c *check.C) {
 			{
 				Names: []string{"/myapp"},
 				Labels: map[string]string{
-					requiredLabel:                                  "true",
+					requiredLabel: "true",
 					labelPrefix + "." + jobExec + ".job1.schedule": "* * * * *",
 					labelPrefix + "." + jobExec + ".job1.command":  "echo hello",
 				},
@@ -417,7 +417,7 @@ func (s *TestDockerSuit) TestConsumeEventsDebounces(c *check.C) {
 			{
 				Names: []string{"/myapp"},
 				Labels: map[string]string{
-					requiredLabel:                                  "true",
+					requiredLabel: "true",
 					labelPrefix + "." + jobExec + ".job1.schedule": "* * * * *",
 					labelPrefix + "." + jobExec + ".job1.command":  "echo hello",
 				},
@@ -612,3 +612,61 @@ func withDockerEnv(envs map[string]string) func() {
 		}
 	}
 }
+
+func (s *TestDockerSuit) TestDockerLabelsUpdateDetectsJobChanges(c *check.C) {
+	execPrefix := labelPrefix + "." + jobExec + ".wpcron."
+	runPrefix := labelPrefix + "." + jobRun + ".backup."
+	labels := func(extra map[string]string) map[string]string {
+		l := map[string]string{
+			requiredLabel:              "true",
+			serviceLabel:               "true",
+			execPrefix + "schedule":    "@every 10m",
+			execPrefix + "command":     "wp cron event run --due-now",
+			runPrefix + "schedule":     "@hourly",
+			runPrefix + "image":        "alpine",
+			runPrefix + "command":      "true",
+			execPrefix + "environment": `["A=1","B=2","C=3"]`,
+			runPrefix + "volume":       `["/a:/a","/b:/b","/c:/c"]`,
+		}
+		for k, v := range extra {
+			l[k] = v
+		}
+		return l
+	}
+	update := func(conf *Config, extra map[string]string) {
+		conf.dockerLabelsUpdate(map[string]map[string]string{"site-php-1": labels(extra)})
+	}
+
+	mock := &mockCLIDockerClient{
+		containers: []container.Summary{
+			{Names: []string{"/site-php-1"}, Labels: labels(map[string]string{execPrefix + "user": "www-data"})},
+		},
+	}
+	mockLogger := &TestLogger{}
+	conf := NewConfig(mockLogger)
+	conf.sh = core.NewScheduler(mockLogger)
+	conf.dockerHandler = newTestDockerHandler(mock, nil)
+	c.Assert(conf.InitializeApp(), check.IsNil)
+
+	execJob, runJob := conf.ExecJobs["wpcron"], conf.RunJobs["backup"]
+	c.Assert(execJob.User, check.Equals, "www-data")
+	c.Assert(execJob.Hash(), check.Not(check.Equals), uint64(0))
+	c.Assert(runJob.Hash(), check.Not(check.Equals), uint64(0))
+
+	// Unchanged labels must keep the scheduled jobs untouched
+	for i := 0; i < 20; i++ {
+		update(conf, map[string]string{execPrefix + "user": "www-data"})
+		c.Assert(conf.ExecJobs["wpcron"], check.Equals, execJob)
+		c.Assert(conf.RunJobs["backup"], check.Equals, runJob)
+	}
+
+	// Removing the user label must switch the job to the container's user
+	update(conf, nil)
+	c.Assert(conf.ExecJobs["wpcron"].User, check.Equals, "")
+	c.Assert(conf.RunJobs["backup"], check.Equals, runJob)
+
+	// Middleware labels are part of the job config as well
+	update(conf, map[string]string{execPrefix + "no-overlap": "true", runPrefix + "user": "nobody"})
+	c.Assert(conf.ExecJobs["wpcron"].NoOverlap, check.Equals, true)
+	c.Assert(conf.RunJobs["backup"].User, check.Equals, "nobody")
+}

core/execjob.go

@@ -98,3 +98,9 @@ func (j *ExecJob) inspectExec(ctx *Context) (client.ExecInspectResult, error) {
 
 	return i, nil
 }
+
+// Hash overrides the promoted BareJob.Hash, which only covers the BareJob
+// fields, so changes to Container, User, TTY, etc. are detected too.
+func (j *ExecJob) Hash() uint64 {
+	return hashJob(j)
+}

core/job.go

@@ -54,6 +54,10 @@ func (j *BareJob) NotifyStop() {
 // Returns a hash of all the job attributes. Used to detect changes
 // unexported struct fields are ignored - https://pkg.go.dev/github.com/gohugoio/hashstructure#Hash
 func (j *BareJob) Hash() uint64 {
+	return hashJob(j)
+}
+
+func hashJob(j any) uint64 {
 	hash, _ := hashstructure.Hash(j, nil)
 	return hash
 }

core/jobhash_test.go

@@ -0,0 +1,85 @@
+package core
+
+import (
+	. "gopkg.in/check.v1"
+)
+
+type SuiteJobHash struct{}
+
+var _ = Suite(&SuiteJobHash{})
+
+func newHashExecJob(container, user string) *ExecJob {
+	return &ExecJob{
+		BareJob:   BareJob{Name: "wpcron", Schedule: "@every 10m", Command: "wp cron event run --due-now"},
+		Container: container,
+		User:      user,
+	}
+}
+
+func (s *SuiteJobHash) TestExecJobHashDetectsFieldChanges(c *C) {
+	base := newHashExecJob("site-php-1", "www-data")
+
+	user := newHashExecJob("site-php-1", "")
+	c.Assert(user.Hash(), Not(Equals), base.Hash())
+
+	container := newHashExecJob("other-php-1", "www-data")
+	c.Assert(container.Hash(), Not(Equals), base.Hash())
+
+	tty := newHashExecJob("site-php-1", "www-data")
+	tty.TTY = true
+	c.Assert(tty.Hash(), Not(Equals), base.Hash())
+
+	env := newHashExecJob("site-php-1", "www-data")
+	env.Environment = []string{"FOO=bar"}
+	c.Assert(env.Hash(), Not(Equals), base.Hash())
+}
+
+func (s *SuiteJobHash) TestExecJobHashIgnoresClientAndRuntimeState(c *C) {
+	a := newHashExecJob("site-php-1", "1000:1000")
+	b := newHashExecJob("site-php-1", "1000:1000")
+	b.Client = &mockDockerClient{}
+	b.cronID = 42
+	b.execID = "abc"
+	b.history = append(b.history, &Execution{})
+	c.Assert(b.Hash(), Equals, a.Hash())
+}
+
+func (s *SuiteJobHash) TestRunJobHashDetectsFieldChanges(c *C) {
+	newJob := func() *RunJob {
+		return &RunJob{BareJob: BareJob{Name: "r", Schedule: "@hourly", Command: "true"}, Image: "alpine", User: "nobody"}
+	}
+	base := newJob()
+
+	user := newJob()
+	user.User = "1000"
+	c.Assert(user.Hash(), Not(Equals), base.Hash())
+
+	image := newJob()
+	image.Image = "busybox"
+	c.Assert(image.Hash(), Not(Equals), base.Hash())
+
+	withClient := newJob()
+	withClient.Client = &mockDockerClient{}
+	c.Assert(withClient.Hash(), Equals, base.Hash())
+}
+
+func (s *SuiteJobHash) TestRunServiceJobHashDetectsFieldChanges(c *C) {
+	newJob := func() *RunServiceJob {
+		return &RunServiceJob{BareJob: BareJob{Name: "s", Schedule: "@hourly", Command: "true"}, Image: "alpine"}
+	}
+	base := newJob()
+
+	user := newJob()
+	user.User = "1000"
+	c.Assert(user.Hash(), Not(Equals), base.Hash())
+
+	withClient := newJob()
+	withClient.Client = &mockDockerClient{}
+	c.Assert(withClient.Hash(), Equals, base.Hash())
+}
+
+func (s *SuiteJobHash) TestLocalJobHashDetectsFieldChanges(c *C) {
+	base := &LocalJob{BareJob: BareJob{Name: "l", Schedule: "@hourly", Command: "true"}}
+	dir := &LocalJob{BareJob: BareJob{Name: "l", Schedule: "@hourly", Command: "true"}, Dir: "/tmp"}
+	c.Assert(dir.Hash(), Not(Equals), base.Hash())
+}

core/localjob.go

@@ -44,3 +44,9 @@ func (j *LocalJob) buildCommand(ctx *Context) (*exec.Cmd, error) {
 		Dir: j.Dir,
 	}, nil
 }
+
+// Hash overrides the promoted BareJob.Hash, which only covers the BareJob
+// fields, so changes to Dir and Environment are detected too.
+func (j *LocalJob) Hash() uint64 {
+	return hashJob(j)
+}

core/runjob.go

@@ -16,7 +16,7 @@ import (
 
 type RunJob struct {
 	BareJob `mapstructure:",squash"`
-	Client  DockerClient `json:"-"`
+	Client  DockerClient `json:"-" hash:"-"`
 	User    string       `default:""`
 
 	TTY bool `default:"false"`
@@ -264,3 +264,9 @@ func (j *RunJob) deleteContainer(ctx *Context) error {
 	_, err := j.Client.ContainerRemove(ctx.Context(), j.containerID, client.ContainerRemoveOptions{})
 	return err
 }
+
+// Hash overrides the promoted BareJob.Hash, which only covers the BareJob
+// fields, so changes to Image, User, Volume, etc. are detected too.
+func (j *RunJob) Hash() uint64 {
+	return hashJob(j)
+}

core/runservice.go

@@ -14,7 +14,7 @@ import (
 
 type RunServiceJob struct {
 	BareJob `mapstructure:",squash"`
-	Client  DockerClient `json:"-"`
+	Client  DockerClient `json:"-" hash:"-"`
 	User    string       `default:""`
 	TTY     bool         `default:"false"`
 	// do not use bool values with "default:true" because if
@@ -195,3 +195,9 @@ func (j *RunServiceJob) deleteService(ctx *Context, svcID string) error {
 
 	return err
 }
+
+// Hash overrides the promoted BareJob.Hash, which only covers the BareJob
+// fields, so changes to Image, User, Network, etc. are detected too.
+func (j *RunServiceJob) Hash() uint64 {
+	return hashJob(j)
+}