From a807026f3b066efa00e81989900f38c2a151b73f Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Mon, 26 Jun 2023 23:21:25 +0100 Subject: [PATCH 01/12] Add support for service's to configure working directory (#245) This adds support for a "working-dir" option in the layered service configuration, for starting a service in a specified working directory (the default is still to start it in Pebble's working directory). Fixes #158 --- README.md | 7 ++- internals/overlord/servstate/handlers.go | 2 + internals/overlord/servstate/manager_test.go | 59 ++++++++++++++++++++ internals/plan/plan.go | 4 ++ internals/plan/plan_test.go | 17 ++++-- 5 files changed, 83 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index a0fe9e3ef..f0ab8e428 100644 --- a/README.md +++ b/README.md @@ -523,6 +523,10 @@ services: # group and group-id are specified, the group's GID must match group-id. group-id: + # (Optional) Working directory to run command in. By default, the + # command is run in the service manager's current directory. + working-dir: + # (Optional) Defines what happens when the service exits with a zero # exit code. Possible values are: "restart" (default) which restarts # the service after the backoff delay, "shutdown" which shuts down and @@ -653,7 +657,8 @@ checks: # match group-id. group-id: - # (Optional) Working directory to run command in. + # (Optional) Working directory to run command in. By default, the + # command is run in the service manager's current directory. working-dir: ``` diff --git a/internals/overlord/servstate/handlers.go b/internals/overlord/servstate/handlers.go index 1beb7d502..32348a253 100644 --- a/internals/overlord/servstate/handlers.go +++ b/internals/overlord/servstate/handlers.go @@ -347,6 +347,8 @@ func (s *serviceData) startInternal() error { environment[k] = v } + s.cmd.Dir = s.config.WorkingDir + // Start as another user if specified in plan. uid, gid, err := osutil.NormalizeUidGid(s.config.UserID, s.config.GroupID, s.config.User, s.config.Group) if err != nil { diff --git a/internals/overlord/servstate/manager_test.go b/internals/overlord/servstate/manager_test.go index 58a4d920f..629cf0298 100644 --- a/internals/overlord/servstate/manager_test.go +++ b/internals/overlord/servstate/manager_test.go @@ -1763,3 +1763,62 @@ type fakeLogManager struct{} func (f fakeLogManager) ServiceStarted(serviceName string, logs *servicelog.RingBuffer) { // no-op } + +func (s *S) TestNoWorkingDir(c *C) { + // Service command should run in current directory (package directory) + // if "working-dir" config option not set. + dir := c.MkDir() + err := os.Mkdir(filepath.Join(dir, "layers"), 0755) + c.Assert(err, IsNil) + manager, err := servstate.NewManager(s.st, s.runner, dir, nil, nil, fakeLogManager{}) + c.Assert(err, IsNil) + defer manager.Stop() + + outputPath := filepath.Join(dir, "output") + layer := parseLayer(c, 0, "layer", fmt.Sprintf(` +services: + nowrkdir: + override: replace + command: /bin/sh -c "pwd >%s; sleep %g" +`, outputPath, shortOkayDelay.Seconds()+0.01)) + err = manager.AppendLayer(layer) + c.Assert(err, IsNil) + + chg := s.startServices(c, []string{"nowrkdir"}, 1) + s.st.Lock() + c.Assert(chg.Err(), IsNil) + s.st.Unlock() + + output, err := ioutil.ReadFile(outputPath) + c.Assert(err, IsNil) + c.Check(string(output), Matches, ".*/overlord/servstate\n") +} + +func (s *S) TestWorkingDir(c *C) { + dir := c.MkDir() + err := os.Mkdir(filepath.Join(dir, "layers"), 0755) + c.Assert(err, IsNil) + manager, err := servstate.NewManager(s.st, s.runner, dir, nil, nil, fakeLogManager{}) + c.Assert(err, IsNil) + defer manager.Stop() + + outputPath := filepath.Join(dir, "output") + layer := parseLayer(c, 0, "layer", fmt.Sprintf(` +services: + wrkdir: + override: replace + command: /bin/sh -c "pwd >%s; sleep %g" + working-dir: %s +`, outputPath, shortOkayDelay.Seconds()+0.01, dir)) + err = manager.AppendLayer(layer) + c.Assert(err, IsNil) + + chg := s.startServices(c, []string{"wrkdir"}, 1) + s.st.Lock() + c.Assert(chg.Err(), IsNil) + s.st.Unlock() + + output, err := ioutil.ReadFile(outputPath) + c.Assert(err, IsNil) + c.Check(string(output), Equals, dir+"\n") +} diff --git a/internals/plan/plan.go b/internals/plan/plan.go index d142e184a..76fc1febe 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -79,6 +79,7 @@ type Service struct { User string `yaml:"user,omitempty"` GroupID *int `yaml:"group-id,omitempty"` Group string `yaml:"group,omitempty"` + WorkingDir string `yaml:"working-dir,omitempty"` // Auto-restart and backoff functionality OnSuccess ServiceAction `yaml:"on-success,omitempty"` @@ -154,6 +155,9 @@ func (s *Service) Merge(other *Service) { if other.Group != "" { s.Group = other.Group } + if other.WorkingDir != "" { + s.WorkingDir = other.WorkingDir + } s.After = append(s.After, other.After...) s.Before = append(s.Before, other.Before...) s.Requires = append(s.Requires, other.Requires...) diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index bc76513a5..2fe9f6e71 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -107,12 +107,14 @@ var planTests = []planTest{{ backoff-delay: 1s backoff-factor: 1.5 backoff-limit: 10s + working-dir: /workdir/srv1 srv2: override: replace startup: enabled command: cmd before: - srv3 + working-dir: /workdir/srv2 srv3: override: replace command: cmd @@ -131,6 +133,7 @@ var planTests = []planTest{{ - srv4 before: - srv5 + working-dir: /workdir/srv1/override srv2: override: replace startup: disabled @@ -173,16 +176,18 @@ var planTests = []planTest{{ "var0": "val0", "var2": "val2", }, + WorkingDir: "/workdir/srv1", BackoffDelay: plan.OptionalDuration{Value: time.Second, IsSet: true}, BackoffFactor: plan.OptionalFloat{Value: 1.5, IsSet: true}, BackoffLimit: plan.OptionalDuration{Value: 10 * time.Second, IsSet: true}, }, "srv2": { - Name: "srv2", - Override: "replace", - Command: "cmd", - Startup: plan.StartupEnabled, - Before: []string{"srv3"}, + Name: "srv2", + Override: "replace", + Command: "cmd", + WorkingDir: "/workdir/srv2", + Startup: plan.StartupEnabled, + Before: []string{"srv3"}, }, "srv3": { Name: "srv3", @@ -213,6 +218,7 @@ var planTests = []planTest{{ Environment: map[string]string{ "var3": "val3", }, + WorkingDir: "/workdir/srv1/override", }, "srv2": { Name: "srv2", @@ -269,6 +275,7 @@ var planTests = []planTest{{ "var2": "val2", "var3": "val3", }, + WorkingDir: "/workdir/srv1/override", BackoffDelay: plan.OptionalDuration{Value: time.Second, IsSet: true}, BackoffFactor: plan.OptionalFloat{Value: 1.5, IsSet: true}, BackoffLimit: plan.OptionalDuration{Value: 10 * time.Second, IsSet: true}, From 6892f25bd661a2ccdea661d0d9879dab0e7bc050 Mon Sep 17 00:00:00 2001 From: Frederik Lotter Date: Thu, 29 Jun 2023 01:58:10 +0200 Subject: [PATCH 02/12] checkstate: make tests more robust (#249) The checks manager uses contexts to cancel check runners. This happens during checkstate.PlanChanged() when the previous checkers are all terminated, and the new checker definition is applied. However, context cancellation is asynchronous (as highlighted in the context documentation), so the check runners will continue to complete even after the context cancellations returned. Due to this race condition, the runner can still launch a check even after the context cancellation is complete. This problem is reproduced here: https://gist.github.com/flotter/f6bf32d7cc33cb1d53a3ddc39fc3b64c The result of this is easily seen the following unit test failure: panic: internal error: reaper must be started goroutine 75 [running]: github.com/canonical/pebble/internals/reaper.StartCommand /home/flotter/pebble/internals/reaper/reaper.go:164 github.com/canonical/pebble/internals/overlord/checkstate.(*execChecker).check() /home/flotter/pebble/internals/overlord/checkstate/checkers.go:170 github.com/canonical/pebble/internals/overlord/checkstate.(*checkData).runCheck() /home/flotter/pebble/internals/overlord/checkstate/manager.go:199 github.com/canonical/pebble/internals/overlord/checkstate.(*checkData).loop() /home/flotter/pebble/internals/overlord/checkstate/manager.go:182 created by github.com/canonical/pebble/internals/overlord/checkstate.(*CheckManager).PlanChanged /home/flotter/pebble/internals/overlord/checkstate/manager.go:74 In this particular case, the problem is that test teardown stops the reaper process which is responsible for reaping check commands, while the final (edge case) run of the check is still completing. Following the same logic, this could also cause check overlap between terminating previous checks and starting new checks in checkstate.PlanChanged(). The following changes fix the problem: - Use a WaitGroup to represent the group of active checks in the check manager - Wait() on the group to complete before creating the next group of checks when PlanChanged is called. - Each check runner will use a deferred exit to decrement the group counter. * checkstate: make tests more robust The following changes are made: - Remove a check which depends on check output in TestTimeout as there is no way to remove the race between the 'echo FOO' completing before the timeout is triggered. This failure can easily be reproduced by running the test under heavy cpu load (i.e. stress --cpu 10 --io 4) manager_test.go:140: c.Assert(check.ErrorDetails, Equals, "FOO") ... obtained string = "" ... expected string = "FOO" - Make waitCheck() more conservative when waiting for a check runner iteration to complete. The worse case timeout can safely be much longer as this does not have a general impact on test duration. This failure can easily be reproduced by running the test under heavy cpu load (i.e. stress --cpu 10 --io 4) manager_test.go:134: check := waitCheck(c, mgr, "chk1", func(check *CheckInfo) bool { return check.Status != CheckStatusUp }) manager_test.go:283: c.Fatalf("timed out waiting for check %q", name) ... Error: timed out waiting for check "chk1" --- internals/overlord/checkstate/manager.go | 12 ++++++++- internals/overlord/checkstate/manager_test.go | 27 +++++++++++++------ 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/internals/overlord/checkstate/manager.go b/internals/overlord/checkstate/manager.go index abbde8e7c..6cba21caa 100644 --- a/internals/overlord/checkstate/manager.go +++ b/internals/overlord/checkstate/manager.go @@ -27,6 +27,7 @@ import ( // CheckManager starts and manages the health checks. type CheckManager struct { mutex sync.Mutex + wg sync.WaitGroup checks map[string]*checkData failureHandlers []FailureFunc } @@ -58,6 +59,12 @@ func (m *CheckManager) PlanChanged(p *plan.Plan) { for _, check := range m.checks { check.cancel() } + // Wait for all context cancellations to propagate and allow + // each goroutine to cleanly exit. + m.wg.Wait() + + // Set the size of the next wait group + m.wg.Add(len(p.Checks)) // Then configure and start new checks. checks := make(map[string]*checkData, len(p.Checks)) @@ -71,7 +78,10 @@ func (m *CheckManager) PlanChanged(p *plan.Plan) { action: m.callFailureHandlers, } checks[name] = check - go check.loop() + go func() { + defer m.wg.Done() + check.loop() + }() } m.checks = checks } diff --git a/internals/overlord/checkstate/manager_test.go b/internals/overlord/checkstate/manager_test.go index 62ee24abd..12b418b17 100644 --- a/internals/overlord/checkstate/manager_test.go +++ b/internals/overlord/checkstate/manager_test.go @@ -45,12 +45,14 @@ func (s *ManagerSuite) SetUpSuite(c *C) { setLoggerOnce.Do(func() { logger.SetLogger(logger.New(os.Stderr, "[test] ")) }) +} +func (s *ManagerSuite) SetUpTest(c *C) { err := reaper.Start() c.Assert(err, IsNil) } -func (s *ManagerSuite) TearDownSuite(c *C) { +func (s *ManagerSuite) TearDownTest(c *C) { err := reaper.Stop() c.Assert(err, IsNil) } @@ -137,7 +139,6 @@ func (s *ManagerSuite) TestTimeout(c *C) { c.Assert(check.Failures, Equals, 1) c.Assert(check.Threshold, Equals, 1) c.Assert(check.LastError, Equals, "exec check timed out") - c.Assert(check.ErrorDetails, Equals, "FOO") } func (s *ManagerSuite) TestCheckCanceled(c *C) { @@ -161,17 +162,15 @@ func (s *ManagerSuite) TestCheckCanceled(c *C) { }, }) - // Wait for command to start (output file grows in size) - prevSize := 0 + // Wait for command to start (output file is not zero in size) for i := 0; ; i++ { if i >= 100 { c.Fatalf("failed waiting for command to start") } b, _ := ioutil.ReadFile(tempFile) - if len(b) != prevSize { + if len(b) > 0 { break } - prevSize = len(b) time.Sleep(time.Millisecond) } @@ -185,7 +184,6 @@ func (s *ManagerSuite) TestCheckCanceled(c *C) { stopChecks(c, mgr) // Ensure command was terminated (output file didn't grow in size) - time.Sleep(50 * time.Millisecond) b1, err := ioutil.ReadFile(tempFile) c.Assert(err, IsNil) time.Sleep(20 * time.Millisecond) @@ -269,8 +267,20 @@ func (s *ManagerSuite) TestFailures(c *C) { c.Assert(failureName, Equals, "") } +// waitCheck is a time based approach to wait for a checker run to complete. +// The timeout value does not impact the general time it takes for tests to +// complete, but determines a worst case waiting period before giving up. +// The timeout value must take into account single core or very busy machines +// so it makes sense to pick a conservative number here as failing a test +// due to a busy test resource is more extensive than waiting a few more +// seconds. func waitCheck(c *C, mgr *CheckManager, name string, f func(check *CheckInfo) bool) *CheckInfo { - for i := 0; i < 100; i++ { + // Worst case waiting time for checker run(s) to complete. This + // period should be much longer (10x is good) than the longest + // check timeout value. + timeout := time.Second * 10 + + for start := time.Now(); time.Since(start) < timeout; { checks, err := mgr.Checks() c.Assert(err, IsNil) for _, check := range checks { @@ -280,6 +290,7 @@ func waitCheck(c *C, mgr *CheckManager, name string, f func(check *CheckInfo) bo } time.Sleep(time.Millisecond) } + c.Fatalf("timed out waiting for check %q", name) return nil } From 30f520b22a2cccf14f27c256365540dc6f7b8d3c Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Fri, 30 Jun 2023 04:44:43 +0100 Subject: [PATCH 03/12] Improve error messages with exec, especially if working dir missing (#236) Fixes #235 --- internals/daemon/api_exec.go | 15 ++++++++++++++- internals/daemon/api_exec_test.go | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/internals/daemon/api_exec.go b/internals/daemon/api_exec.go index 41c9d189a..cac58462e 100644 --- a/internals/daemon/api_exec.go +++ b/internals/daemon/api_exec.go @@ -64,7 +64,20 @@ func v1PostExec(c *Command, req *http.Request, _ *userState) Response { // Check up-front that the executable exists. _, err := exec.LookPath(payload.Command[0]) if err != nil { - return statusBadRequest("%v", err) + return statusBadRequest("cannot find executable %q", payload.Command[0]) + } + + // Also check that the working directory exists, to avoid a confusing + // error message later that implies the command doesn't exist: + // + // fork/exec /usr/local/bin/realcommand: no such file or directory + // + // Note that this check still doesn't check that the permissions are + // correct to use it as a working directory, but this is a good start. + if payload.WorkingDir != "" { + if !osutil.IsDir(payload.WorkingDir) { + return statusBadRequest("cannot find working directory %q", payload.WorkingDir) + } } // Convert User/UserID and Group/GroupID combinations into raw uid/gid. diff --git a/internals/daemon/api_exec_test.go b/internals/daemon/api_exec_test.go index bba0ea025..9ef9a6a79 100644 --- a/internals/daemon/api_exec_test.go +++ b/internals/daemon/api_exec_test.go @@ -336,7 +336,7 @@ func (s *execSuite) TestCommandNotFound(c *C) { c.Check(httpResp.StatusCode, Equals, http.StatusBadRequest) c.Check(execResp.StatusCode, Equals, http.StatusBadRequest) c.Check(execResp.Type, Equals, "error") - c.Check(execResp.Result["message"], Matches, ".*executable file not found.*") + c.Check(execResp.Result["message"], Matches, "cannot find executable .*") } func (s *execSuite) TestUserGroupError(c *C) { From 8e96c74a21cc104ac722bf4160d7db6e11ddeb60 Mon Sep 17 00:00:00 2001 From: Jordan Mitchell Barrett <90195985+barrettj12@users.noreply.github.com> Date: Tue, 4 Jul 2023 06:06:44 +0800 Subject: [PATCH 04/12] [JUJU-3777] Multiplex services to log targets using only a `log-target.services` field (#252) Add a `services` field to log targets, which replaces the `selection` field and the `log-targets` field of a service. --- README.md | 54 ++++++++ internals/plan/plan.go | 110 ++++++--------- internals/plan/plan_test.go | 259 +++++++++++++++++++++--------------- 3 files changed, 246 insertions(+), 177 deletions(-) diff --git a/README.md b/README.md index f0ab8e428..315e9d082 100644 --- a/README.md +++ b/README.md @@ -403,6 +403,60 @@ $ pebble run --verbose ... ``` + + ## Container usage Pebble works well as a local service manager, but if running Pebble in a separate container, you can use the exec and file management APIs to coordinate with the remote system over the shared unix socket. diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 76fc1febe..22d403c93 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -29,6 +29,7 @@ import ( "github.com/canonical/x-go/strutil/shlex" "gopkg.in/yaml.v3" + "github.com/canonical/pebble/internals/logger" "github.com/canonical/pebble/internals/osutil" ) @@ -89,9 +90,6 @@ type Service struct { BackoffFactor OptionalFloat `yaml:"backoff-factor,omitempty"` BackoffLimit OptionalDuration `yaml:"backoff-limit,omitempty"` KillDelay OptionalDuration `yaml:"kill-delay,omitempty"` - - // Log forwarding - LogTargets []string `yaml:"log-targets,omitempty"` } // Copy returns a deep copy of the service. @@ -120,7 +118,6 @@ func (s *Service) Copy() *Service { copied.OnCheckFailure[k] = v } } - copied.LogTargets = append([]string(nil), s.LogTargets...) return &copied } @@ -188,23 +185,6 @@ func (s *Service) Merge(other *Service) { if other.BackoffLimit.IsSet { s.BackoffLimit = other.BackoffLimit } - s.LogTargets = appendUnique(s.LogTargets, other.LogTargets...) -} - -// appendUnique appends into a the elements from b which are not yet present -// and returns the modified slice. -// TODO: move this function into canonical/x-go/strutil -func appendUnique(a []string, b ...string) []string { -Outer: - for _, bn := range b { - for _, an := range a { - if an == bn { - continue Outer - } - } - a = append(a, bn) - } - return a } // Equal returns true when the two services are equal in value. @@ -275,23 +255,22 @@ func CommandString(base, extra []string) string { } // LogsTo returns true if the logs from s should be forwarded to target t. -// This happens if: -// - t.Selection is "opt-out" or empty, and s.LogTargets is empty; or -// - t.Selection is not "disabled", and s.LogTargets contains t. func (s *Service) LogsTo(t *LogTarget) bool { - if t.Selection == DisabledSelection { - return false - } - if len(s.LogTargets) == 0 { - if t.Selection == UnsetSelection || t.Selection == OptOutSelection { + // Iterate backwards through t.Services until we find something matching + // s.Name. + for i := len(t.Services) - 1; i >= 0; i-- { + switch t.Services[i] { + case s.Name: return true - } - } - for _, targetName := range s.LogTargets { - if targetName == t.Name { + case ("-" + s.Name): + return false + case "all": return true + case "-all": + return false } } + // Nothing matching the service name, so it was not specified. return false } @@ -513,11 +492,11 @@ func (c *ExecCheck) Merge(other *ExecCheck) { // LogTarget specifies a remote server to forward logs to. type LogTarget struct { - Name string `yaml:"-"` - Type LogTargetType `yaml:"type"` - Location string `yaml:"location"` - Selection Selection `yaml:"selection,omitempty"` - Override Override `yaml:"override,omitempty"` + Name string `yaml:"-"` + Type LogTargetType `yaml:"type"` + Location string `yaml:"location"` + Services []string `yaml:"services"` + Override Override `yaml:"override,omitempty"` } // LogTargetType defines the protocol to use to forward logs. @@ -529,19 +508,10 @@ const ( UnsetLogTarget LogTargetType = "" ) -// Selection describes which services' logs will be forwarded to this target. -type Selection string - -const ( - OptOutSelection Selection = "opt-out" - OptInSelection Selection = "opt-in" - DisabledSelection Selection = "disabled" - UnsetSelection Selection = "" -) - // Copy returns a deep copy of the log target configuration. func (t *LogTarget) Copy() *LogTarget { copied := *t + copied.Services = append([]string(nil), t.Services...) return &copied } @@ -553,9 +523,7 @@ func (t *LogTarget) Merge(other *LogTarget) { if other.Location != "" { t.Location = other.Location } - if other.Selection != "" { - t.Selection = other.Selection - } + t.Services = append(t.Services, other.Services...) } // FormatError is the error returned when a layer has a format error, such as @@ -796,35 +764,28 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { } } - switch target.Selection { - case OptOutSelection, OptInSelection, DisabledSelection, UnsetSelection: - // valid, continue - default: + // Validate service names specified in log target + for _, serviceName := range target.Services { + serviceName = strings.TrimPrefix(serviceName, "-") + if serviceName == "all" { + continue + } + if _, ok := combined.Services[serviceName]; ok { + continue + } return nil, &FormatError{ - Message: fmt.Sprintf(`log target %q has invalid selection %q, must be %q, %q or %q`, - name, target.Selection, OptOutSelection, OptInSelection, DisabledSelection), + Message: fmt.Sprintf(`log target %q specifies unknown service %q`, + target.Name, serviceName), } } - if target.Location == "" && target.Selection != DisabledSelection { + if target.Location == "" { return nil, &FormatError{ Message: fmt.Sprintf(`plan must define "location" for log target %q`, name), } } } - // Validate service log targets - for serviceName, service := range combined.Services { - for _, targetName := range service.LogTargets { - _, ok := combined.LogTargets[targetName] - if !ok { - return nil, &FormatError{ - Message: fmt.Sprintf(`unknown log target %q for service %q`, targetName, serviceName), - } - } - } - } - // Ensure combined layers don't have cycles. err := combined.checkCycles() if err != nil { @@ -963,6 +924,15 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { Message: fmt.Sprintf("cannot use reserved service name %q", name), } } + // Deprecated service names + if name == "all" || name == "default" || name == "none" { + logger.Noticef("Using keyword %q as a service name is deprecated", name) + } + if strings.HasPrefix(name, "-") { + return nil, &FormatError{ + Message: fmt.Sprintf(`cannot use service name %q: starting with "-" not allowed`, name), + } + } if service == nil { return nil, &FormatError{ Message: fmt.Sprintf("service object cannot be null for service %q", name), diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 2fe9f6e71..1883ece4c 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -831,24 +831,21 @@ var planTests = []planTest{{ command: foo override: merge startup: enabled - log-targets: - - tgt1 svc2: command: bar override: merge startup: enabled - log-targets: - - tgt1 - - tgt2 log-targets: tgt1: type: loki location: http://10.1.77.196:3100/loki/api/v1/push + services: [all] override: merge tgt2: type: syslog location: udp://0.0.0.0:514 + services: [svc2] override: merge `}, result: &plan.Layer{ @@ -861,7 +858,6 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt1"}, }, "svc2": { Name: "svc2", @@ -871,7 +867,6 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt1", "tgt2"}, }, }, Checks: map[string]*plan.Check{}, @@ -880,12 +875,14 @@ var planTests = []planTest{{ Name: "tgt1", Type: plan.LokiTarget, Location: "http://10.1.77.196:3100/loki/api/v1/push", + Services: []string{"all"}, Override: plan.MergeOverride, }, "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, Location: "udp://0.0.0.0:514", + Services: []string{"svc2"}, Override: plan.MergeOverride, }, }, @@ -898,50 +895,50 @@ var planTests = []planTest{{ command: foo override: merge startup: enabled - log-targets: - - tgt1 svc2: command: bar override: merge startup: enabled - log-targets: - - tgt1 - - tgt2 log-targets: tgt1: type: loki location: http://10.1.77.196:3100/loki/api/v1/push + services: [all] override: merge tgt2: type: syslog location: udp://0.0.0.0:514 + services: [svc2] + override: merge + tgt3: + type: loki + location: http://10.1.77.206:3100/loki/api/v1/push + services: [all] override: merge `, ` services: svc1: command: foo override: merge - log-targets: - - tgt3 svc2: command: bar override: replace startup: enabled - log-targets: - - tgt3 log-targets: tgt1: + services: [-all, svc1] override: merge - selection: opt-in tgt2: type: syslog + location: udp://1.2.3.4:514 + services: [] override: replace - selection: disabled tgt3: - type: loki - location: http://10.1.77.206:3100/loki/api/v1/push + type: syslog + location: udp://0.0.0.0:514 + services: [-svc1] override: merge `}, layers: []*plan.Layer{{ @@ -949,18 +946,16 @@ var planTests = []planTest{{ Order: 0, Services: map[string]*plan.Service{ "svc1": { - Name: "svc1", - Command: "foo", - Override: plan.MergeOverride, - Startup: plan.StartupEnabled, - LogTargets: []string{"tgt1"}, + Name: "svc1", + Command: "foo", + Override: plan.MergeOverride, + Startup: plan.StartupEnabled, }, "svc2": { - Name: "svc2", - Command: "bar", - Override: plan.MergeOverride, - Startup: plan.StartupEnabled, - LogTargets: []string{"tgt1", "tgt2"}, + Name: "svc2", + Command: "bar", + Override: plan.MergeOverride, + Startup: plan.StartupEnabled, }, }, Checks: map[string]*plan.Check{}, @@ -969,12 +964,21 @@ var planTests = []planTest{{ Name: "tgt1", Type: plan.LokiTarget, Location: "http://10.1.77.196:3100/loki/api/v1/push", + Services: []string{"all"}, Override: plan.MergeOverride, }, "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, Location: "udp://0.0.0.0:514", + Services: []string{"svc2"}, + Override: plan.MergeOverride, + }, + "tgt3": { + Name: "tgt3", + Type: plan.LokiTarget, + Location: "http://10.1.77.206:3100/loki/api/v1/push", + Services: []string{"all"}, Override: plan.MergeOverride, }, }, @@ -983,36 +987,36 @@ var planTests = []planTest{{ Order: 1, Services: map[string]*plan.Service{ "svc1": { - Name: "svc1", - Command: "foo", - Override: plan.MergeOverride, - LogTargets: []string{"tgt3"}, + Name: "svc1", + Command: "foo", + Override: plan.MergeOverride, }, "svc2": { - Name: "svc2", - Command: "bar", - Override: plan.ReplaceOverride, - Startup: plan.StartupEnabled, - LogTargets: []string{"tgt3"}, + Name: "svc2", + Command: "bar", + Override: plan.ReplaceOverride, + Startup: plan.StartupEnabled, }, }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{ "tgt1": { - Name: "tgt1", - Override: plan.MergeOverride, - Selection: plan.OptInSelection, + Name: "tgt1", + Services: []string{"-all", "svc1"}, + Override: plan.MergeOverride, }, "tgt2": { - Name: "tgt2", - Type: plan.SyslogTarget, - Override: plan.ReplaceOverride, - Selection: plan.DisabledSelection, + Name: "tgt2", + Type: plan.SyslogTarget, + Location: "udp://1.2.3.4:514", + Services: []string{}, + Override: plan.ReplaceOverride, }, "tgt3": { Name: "tgt3", - Type: plan.LokiTarget, - Location: "http://10.1.77.206:3100/loki/api/v1/push", + Type: plan.SyslogTarget, + Location: "udp://0.0.0.0:514", + Services: []string{"-svc1"}, Override: plan.MergeOverride, }, }, @@ -1027,7 +1031,6 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt1", "tgt3"}, }, "svc2": { Name: "svc2", @@ -1037,28 +1040,28 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt3"}, }, }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{ "tgt1": { - Name: "tgt1", - Type: plan.LokiTarget, - Location: "http://10.1.77.196:3100/loki/api/v1/push", - Override: plan.MergeOverride, - Selection: plan.OptInSelection, + Name: "tgt1", + Type: plan.LokiTarget, + Location: "http://10.1.77.196:3100/loki/api/v1/push", + Services: []string{"all", "-all", "svc1"}, + Override: plan.MergeOverride, }, "tgt2": { - Name: "tgt2", - Type: plan.SyslogTarget, - Override: plan.ReplaceOverride, - Selection: plan.DisabledSelection, + Name: "tgt2", + Type: plan.SyslogTarget, + Location: "udp://1.2.3.4:514", + Override: plan.ReplaceOverride, }, "tgt3": { Name: "tgt3", - Type: plan.LokiTarget, - Location: "http://10.1.77.206:3100/loki/api/v1/push", + Type: plan.SyslogTarget, + Location: "udp://0.0.0.0:514", + Services: []string{"all", "-svc1"}, Override: plan.MergeOverride, }, }, @@ -1079,6 +1082,7 @@ var planTests = []planTest{{ log-targets: tgt1: type: loki + services: [all] override: merge `}}, { summary: "Unsupported log target type", @@ -1091,31 +1095,24 @@ var planTests = []planTest{{ override: merge `}, }, { - summary: "Invalid selection for log target", - error: `log target "tgt1" has invalid selection "foobar", must be "opt-out", "opt-in" or "disabled"`, + summary: "Log target specifies invalid service", + error: `log target "tgt1" specifies unknown service "nonexistent"`, input: []string{` log-targets: tgt1: type: loki location: http://10.1.77.196:3100/loki/api/v1/push + services: [nonexistent] override: merge - selection: foobar `}, }, { - summary: "Service specifies unknown log target", - error: `unknown log target "tgt2" for service "svc1"`, + summary: `Service name can't start with "-"`, + error: `cannot use service name "-svc1": starting with "-" not allowed`, input: []string{` services: - svc1: + -svc1: command: foo override: merge - log-targets: - - tgt2 - log-targets: - tgt1: - type: loki - location: http://10.1.77.196:3100/loki/api/v1/push - override: merge `}, }} @@ -1462,41 +1459,89 @@ func (s *S) TestParseCommand(c *C) { } } -func (s *S) TestSelectTargets(c *C) { - logTargets := []*plan.LogTarget{ - {Name: "unset", Selection: plan.UnsetSelection}, - {Name: "optout", Selection: plan.OptOutSelection}, - {Name: "optin", Selection: plan.OptInSelection}, - {Name: "disabled", Selection: plan.DisabledSelection}, - } - services := []*plan.Service{ - {Name: "svc1", LogTargets: nil}, - {Name: "svc2", LogTargets: []string{}}, - {Name: "svc3", LogTargets: []string{"unset"}}, - {Name: "svc4", LogTargets: []string{"optout"}}, - {Name: "svc5", LogTargets: []string{"optin"}}, - {Name: "svc6", LogTargets: []string{"disabled"}}, - {Name: "svc7", LogTargets: []string{"unset", "optin", "disabled"}}, - } +func (s *S) TestLogsTo(c *C) { + tests := []struct { + services []string + logsTo map[string]bool + }{{ + services: nil, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + }, + }, { + services: []string{}, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + }, + }, { + services: []string{"all"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": true, + }, + }, { + services: []string{"svc1"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": false, + }, + }, { + services: []string{"svc1", "svc2"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": true, + "svc3": false, + }, + }, { + services: []string{"all", "-svc2"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": false, + "svc3": true, + }, + }, { + services: []string{"svc1", "svc2", "-svc1", "all"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": true, + "svc3": true, + }, + }, { + services: []string{"svc1", "svc2", "-all"}, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + "svc3": false, + }, + }, { + services: []string{"all", "-all"}, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + "svc3": false, + }, + }, { + services: []string{"svc1", "svc2", "-all", "svc3", "svc1", "-svc3"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": false, + "svc3": false, + }, + }} - // Use pointers to bools so the test will fail if we forget to set a value - t, f := true, false - expected := map[string]map[string]*bool{ - "svc1": {"unset": &t, "optout": &t, "optin": &f, "disabled": &f}, - "svc2": {"unset": &t, "optout": &t, "optin": &f, "disabled": &f}, - "svc3": {"unset": &t, "optout": &f, "optin": &f, "disabled": &f}, - "svc4": {"unset": &f, "optout": &t, "optin": &f, "disabled": &f}, - "svc5": {"unset": &f, "optout": &f, "optin": &t, "disabled": &f}, - "svc6": {"unset": &f, "optout": &f, "optin": &f, "disabled": &f}, - "svc7": {"unset": &t, "optout": &f, "optin": &t, "disabled": &f}, - } + for _, test := range tests { + target := &plan.LogTarget{ + Services: test.services, + } - for _, service := range services { - for _, target := range logTargets { - exp := expected[service.Name][target.Name] - c.Assert(exp, NotNil, Commentf("no expected value defined for %s.LogsTo(%s)", service.Name, target.Name)) - c.Check(service.LogsTo(target), Equals, *exp, - Commentf("unexpected value for %s.LogsTo(%s)", service.Name, target.Name)) + for serviceName, shouldLogTo := range test.logsTo { + service := &plan.Service{ + Name: serviceName, + } + c.Check(service.LogsTo(target), Equals, shouldLogTo, + Commentf("matching service %q against 'services: %v'", serviceName, test.services)) } } } From 00bcd1f7dda570bcdf42600545ddc1707d24365e Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Tue, 11 Jul 2023 10:35:50 +0100 Subject: [PATCH 05/12] Add "service context" support for exec and health checks (#246) --- README.md | 7 + client/exec.go | 68 +++++----- internals/cli/cmd_exec.go | 33 ++--- internals/daemon/api_exec.go | 51 ++++--- internals/daemon/api_exec_test.go | 49 +++++++ internals/overlord/checkstate/manager.go | 29 ++-- internals/overlord/checkstate/manager_test.go | 73 +++++++++- internals/plan/plan.go | 125 ++++++++++++++---- internals/plan/plan_test.go | 86 ++++++++++++ 9 files changed, 426 insertions(+), 95 deletions(-) diff --git a/README.md b/README.md index 315e9d082..ab9a0cc27 100644 --- a/README.md +++ b/README.md @@ -688,6 +688,13 @@ checks: # directly, not interpreted by a shell. command: + # (Optional) Run the command in the context of this service. + # Specifically, inherit its environment variables, user/group + # settings, and working directory. The check's context (the + # settings below) will override the service's; the check's + # environment map will be merged on top of the service's. + service-context: + # (Optional) A list of key/value pairs defining environment # variables that should be set when running the command. environment: diff --git a/client/exec.go b/client/exec.go index 4b74242dd..88d405bd4 100644 --- a/client/exec.go +++ b/client/exec.go @@ -30,22 +30,28 @@ type ExecOptions struct { // Required: command and arguments (first element is the executable). Command []string + // Optional: run the command in the context of this service. Specifically, + // inherit its environment variables, user/group settings, and working + // and working directory. The other options in this struct will override + // the service context; Environment will be merged on top of the service's. + ServiceContext string + // Optional environment variables. Environment map[string]string // Optional working directory (default is $HOME or "/" if $HOME not set). WorkingDir string - // Optional timeout for the command execution, after which the process - // will be terminated. If zero, no timeout applies. - Timeout time.Duration - // Optional user ID and group ID for the process to run as. UserID *int User string GroupID *int Group string + // Optional timeout for the command execution, after which the process + // will be terminated. If zero, no timeout applies. + Timeout time.Duration + // True to ask the server to set up a pseudo-terminal (PTY) for stdout // (this also allows window resizing). The default is no PTY, and just // to use pipes for stdout/stderr. @@ -74,19 +80,20 @@ type ExecOptions struct { } type execPayload struct { - Command []string `json:"command"` - Environment map[string]string `json:"environment,omitempty"` - WorkingDir string `json:"working-dir,omitempty"` - Timeout string `json:"timeout,omitempty"` - UserID *int `json:"user-id,omitempty"` - User string `json:"user,omitempty"` - GroupID *int `json:"group-id,omitempty"` - Group string `json:"group,omitempty"` - Terminal bool `json:"terminal,omitempty"` - Interactive bool `json:"interactive,omitempty"` - SplitStderr bool `json:"split-stderr,omitempty"` - Width int `json:"width,omitempty"` - Height int `json:"height,omitempty"` + Command []string `json:"command"` + ServiceContext string `json:"service-context,omitempty"` + Environment map[string]string `json:"environment,omitempty"` + WorkingDir string `json:"working-dir,omitempty"` + Timeout string `json:"timeout,omitempty"` + UserID *int `json:"user-id,omitempty"` + User string `json:"user,omitempty"` + GroupID *int `json:"group-id,omitempty"` + Group string `json:"group,omitempty"` + Terminal bool `json:"terminal,omitempty"` + Interactive bool `json:"interactive,omitempty"` + SplitStderr bool `json:"split-stderr,omitempty"` + Width int `json:"width,omitempty"` + Height int `json:"height,omitempty"` } type execResult struct { @@ -122,19 +129,20 @@ func (client *Client) Exec(opts *ExecOptions) (*ExecProcess, error) { timeoutStr = opts.Timeout.String() } payload := execPayload{ - Command: opts.Command, - Environment: opts.Environment, - WorkingDir: opts.WorkingDir, - Timeout: timeoutStr, - UserID: opts.UserID, - User: opts.User, - GroupID: opts.GroupID, - Group: opts.Group, - Terminal: opts.Terminal, - Interactive: opts.Interactive, - SplitStderr: opts.Stderr != nil, - Width: opts.Width, - Height: opts.Height, + Command: opts.Command, + ServiceContext: opts.ServiceContext, + Environment: opts.Environment, + WorkingDir: opts.WorkingDir, + Timeout: timeoutStr, + UserID: opts.UserID, + User: opts.User, + GroupID: opts.GroupID, + Group: opts.Group, + Terminal: opts.Terminal, + Interactive: opts.Interactive, + SplitStderr: opts.Stderr != nil, + Width: opts.Width, + Height: opts.Height, } var body bytes.Buffer err := json.NewEncoder(&body).Encode(&payload) diff --git a/internals/cli/cmd_exec.go b/internals/cli/cmd_exec.go index 0298f2b29..1dae53005 100644 --- a/internals/cli/cmd_exec.go +++ b/internals/cli/cmd_exec.go @@ -39,6 +39,7 @@ type cmdExec struct { GroupID *int `long:"gid"` Group string `long:"group"` Timeout time.Duration `long:"timeout"` + Context string `long:"context"` Terminal bool `short:"t"` NoTerminal bool `short:"T"` Interactive bool `short:"i"` @@ -56,6 +57,7 @@ var execDescs = map[string]string{ "gid": "Group ID to run command as", "group": "Group name to run command as (group's GID must match gid if both present)", "timeout": "Timeout after which to terminate command", + "context": "Inherit the context of the named service (overridden by -w, --env, --uid/user, --gid/group)", "t": "Allocate remote pseudo-terminal and connect stdout to it (default if stdout is a TTY)", "T": "Disable remote pseudo-terminal allocation", "i": "Interactive mode: connect stdin to the pseudo-terminal (default if stdin and stdout are TTYs)", @@ -143,21 +145,22 @@ func (cmd *cmdExec) Execute(args []string) error { } opts := &client.ExecOptions{ - Command: command, - Environment: env, - WorkingDir: cmd.WorkingDir, - Timeout: cmd.Timeout, - UserID: cmd.UserID, - User: cmd.User, - GroupID: cmd.GroupID, - Group: cmd.Group, - Terminal: terminal, - Interactive: interactive, - Width: width, - Height: height, - Stdin: Stdin, - Stdout: Stdout, - Stderr: Stderr, + Command: command, + ServiceContext: cmd.Context, + Environment: env, + WorkingDir: cmd.WorkingDir, + Timeout: cmd.Timeout, + UserID: cmd.UserID, + User: cmd.User, + GroupID: cmd.GroupID, + Group: cmd.Group, + Terminal: terminal, + Interactive: interactive, + Width: width, + Height: height, + Stdin: Stdin, + Stdout: Stdout, + Stderr: Stderr, } // If stdout and stderr both refer to the same file or device (e.g., diff --git a/internals/daemon/api_exec.go b/internals/daemon/api_exec.go index cac58462e..eab7bd43d 100644 --- a/internals/daemon/api_exec.go +++ b/internals/daemon/api_exec.go @@ -24,22 +24,24 @@ import ( "github.com/canonical/pebble/internals/osutil" "github.com/canonical/pebble/internals/overlord/cmdstate" "github.com/canonical/pebble/internals/overlord/state" + "github.com/canonical/pebble/internals/plan" ) type execPayload struct { - Command []string `json:"command"` - Environment map[string]string `json:"environment"` - WorkingDir string `json:"working-dir"` - Timeout string `json:"timeout"` - UserID *int `json:"user-id"` - User string `json:"user"` - GroupID *int `json:"group-id"` - Group string `json:"group"` - Terminal bool `json:"terminal"` - Interactive bool `json:"interactive"` - SplitStderr bool `json:"split-stderr"` - Width int `json:"width"` - Height int `json:"height"` + Command []string `json:"command"` + ServiceContext string `json:"service-context"` + Environment map[string]string `json:"environment"` + WorkingDir string `json:"working-dir"` + Timeout string `json:"timeout"` + UserID *int `json:"user-id"` + User string `json:"user"` + GroupID *int `json:"group-id"` + Group string `json:"group"` + Terminal bool `json:"terminal"` + Interactive bool `json:"interactive"` + SplitStderr bool `json:"split-stderr"` + Width int `json:"width"` + Height int `json:"height"` } func v1PostExec(c *Command, req *http.Request, _ *userState) Response { @@ -80,8 +82,25 @@ func v1PostExec(c *Command, req *http.Request, _ *userState) Response { } } + p, err := c.d.overlord.ServiceManager().Plan() + if err != nil { + return statusBadRequest("%v", err) + } + overrides := plan.ContextOptions{ + Environment: payload.Environment, + UserID: payload.UserID, + User: payload.User, + GroupID: payload.GroupID, + Group: payload.Group, + WorkingDir: payload.WorkingDir, + } + merged, err := plan.MergeServiceContext(p, payload.ServiceContext, overrides) + if err != nil { + return statusBadRequest("%v", err) + } + // Convert User/UserID and Group/GroupID combinations into raw uid/gid. - uid, gid, err := osutil.NormalizeUidGid(payload.UserID, payload.GroupID, payload.User, payload.Group) + uid, gid, err := osutil.NormalizeUidGid(merged.UserID, merged.GroupID, merged.User, merged.Group) if err != nil { return statusBadRequest("%v", err) } @@ -92,8 +111,8 @@ func v1PostExec(c *Command, req *http.Request, _ *userState) Response { args := &cmdstate.ExecArgs{ Command: payload.Command, - Environment: payload.Environment, - WorkingDir: payload.WorkingDir, + Environment: merged.Environment, + WorkingDir: merged.WorkingDir, Timeout: timeout, UserID: uid, GroupID: gid, diff --git a/internals/daemon/api_exec_test.go b/internals/daemon/api_exec_test.go index 9ef9a6a79..db38ed728 100644 --- a/internals/daemon/api_exec_test.go +++ b/internals/daemon/api_exec_test.go @@ -31,6 +31,7 @@ import ( "github.com/canonical/pebble/client" "github.com/canonical/pebble/internals/logger" + "github.com/canonical/pebble/internals/plan" ) var _ = Suite(&execSuite{}) @@ -164,6 +165,54 @@ func (s *execSuite) TestTimeout(c *C) { c.Check(stderr, Equals, "") } +func (s *execSuite) TestContextNoOverrides(c *C) { + dir := c.MkDir() + err := s.daemon.overlord.ServiceManager().AppendLayer(&plan.Layer{ + Label: "layer1", + Services: map[string]*plan.Service{"svc1": { + Name: "svc1", + Override: "replace", + Command: "dummy", + Environment: map[string]string{"FOO": "foo", "BAR": "bar"}, + WorkingDir: dir, + }}, + }) + c.Assert(err, IsNil) + + stdout, stderr, err := s.exec(c, "", &client.ExecOptions{ + Command: []string{"/bin/sh", "-c", "echo FOO=$FOO BAR=$BAR; pwd"}, + ServiceContext: "svc1", + }) + c.Assert(err, IsNil) + c.Check(stdout, Equals, "FOO=foo BAR=bar\n"+dir+"\n") + c.Check(stderr, Equals, "") +} + +func (s *execSuite) TestContextOverrides(c *C) { + err := s.daemon.overlord.ServiceManager().AppendLayer(&plan.Layer{ + Label: "layer1", + Services: map[string]*plan.Service{"svc1": { + Name: "svc1", + Override: "replace", + Command: "dummy", + Environment: map[string]string{"FOO": "foo", "BAR": "bar"}, + WorkingDir: c.MkDir(), + }}, + }) + c.Assert(err, IsNil) + + overrideDir := c.MkDir() + stdout, stderr, err := s.exec(c, "", &client.ExecOptions{ + Command: []string{"/bin/sh", "-c", "echo FOO=$FOO BAR=$BAR; pwd"}, + ServiceContext: "svc1", + Environment: map[string]string{"FOO": "oof"}, + WorkingDir: overrideDir, + }) + c.Assert(err, IsNil) + c.Check(stdout, Equals, "FOO=oof BAR=bar\n"+overrideDir+"\n") + c.Check(stderr, Equals, "") +} + func (s *execSuite) TestCurrentUserGroup(c *C) { current, err := user.Current() c.Assert(err, IsNil) diff --git a/internals/overlord/checkstate/manager.go b/internals/overlord/checkstate/manager.go index 6cba21caa..3708559c7 100644 --- a/internals/overlord/checkstate/manager.go +++ b/internals/overlord/checkstate/manager.go @@ -72,7 +72,7 @@ func (m *CheckManager) PlanChanged(p *plan.Plan) { ctx, cancel := context.WithCancel(context.Background()) check := &checkData{ config: config, - checker: newChecker(config), + checker: newChecker(config, p), ctx: ctx, cancel: cancel, action: m.callFailureHandlers, @@ -93,7 +93,7 @@ func (m *CheckManager) callFailureHandlers(name string) { } // newChecker creates a new checker of the configured type. -func newChecker(config *plan.Check) checker { +func newChecker(config *plan.Check, p *plan.Plan) checker { switch { case config.HTTP != nil: return &httpChecker{ @@ -110,15 +110,28 @@ func newChecker(config *plan.Check) checker { } case config.Exec != nil: + overrides := plan.ContextOptions{ + Environment: config.Exec.Environment, + UserID: config.Exec.UserID, + User: config.Exec.User, + GroupID: config.Exec.GroupID, + Group: config.Exec.Group, + WorkingDir: config.Exec.WorkingDir, + } + merged, err := plan.MergeServiceContext(p, config.Exec.ServiceContext, overrides) + if err != nil { + // Context service name has already been checked when plan was loaded. + panic("internal error: " + err.Error()) + } return &execChecker{ name: config.Name, command: config.Exec.Command, - environment: config.Exec.Environment, - userID: config.Exec.UserID, - user: config.Exec.User, - groupID: config.Exec.GroupID, - group: config.Exec.Group, - workingDir: config.Exec.WorkingDir, + environment: merged.Environment, + userID: merged.UserID, + user: merged.User, + groupID: merged.GroupID, + group: merged.Group, + workingDir: merged.WorkingDir, } default: diff --git a/internals/overlord/checkstate/manager_test.go b/internals/overlord/checkstate/manager_test.go index 12b418b17..8e5cf5895 100644 --- a/internals/overlord/checkstate/manager_test.go +++ b/internals/overlord/checkstate/manager_test.go @@ -302,7 +302,7 @@ func (s *CheckersSuite) TestNewChecker(c *C) { URL: "https://example.com/foo", Headers: map[string]string{"k": "v"}, }, - }) + }, nil) http, ok := chk.(*httpChecker) c.Assert(ok, Equals, true) c.Check(http.name, Equals, "http") @@ -315,7 +315,7 @@ func (s *CheckersSuite) TestNewChecker(c *C) { Port: 80, Host: "localhost", }, - }) + }, nil) tcp, ok := chk.(*tcpChecker) c.Assert(ok, Equals, true) c.Check(tcp.name, Equals, "tcp") @@ -334,7 +334,7 @@ func (s *CheckersSuite) TestNewChecker(c *C) { Group: "group", WorkingDir: "/working/dir", }, - }) + }, nil) exec, ok := chk.(*execChecker) c.Assert(ok, Equals, true) c.Assert(exec.name, Equals, "exec") @@ -345,3 +345,70 @@ func (s *CheckersSuite) TestNewChecker(c *C) { c.Assert(exec.groupID, Equals, &groupID) c.Assert(exec.workingDir, Equals, "/working/dir") } + +func (s *CheckersSuite) TestExecContextNoOverride(c *C) { + svcUserID, svcGroupID := 10, 20 + chk := newChecker(&plan.Check{ + Name: "exec", + Exec: &plan.ExecCheck{ + Command: "sleep 1", + ServiceContext: "svc1", + }, + }, &plan.Plan{Services: map[string]*plan.Service{ + "svc1": { + Name: "svc1", + Environment: map[string]string{"k": "x", "a": "1"}, + UserID: &svcUserID, + User: "svcuser", + GroupID: &svcGroupID, + Group: "svcgroup", + WorkingDir: "/working/svc", + }, + }}) + exec, ok := chk.(*execChecker) + c.Assert(ok, Equals, true) + c.Check(exec.name, Equals, "exec") + c.Check(exec.command, Equals, "sleep 1") + c.Check(exec.environment, DeepEquals, map[string]string{"k": "x", "a": "1"}) + c.Check(exec.userID, DeepEquals, &svcUserID) + c.Check(exec.user, Equals, "svcuser") + c.Check(exec.groupID, DeepEquals, &svcGroupID) + c.Check(exec.workingDir, Equals, "/working/svc") +} + +func (s *CheckersSuite) TestExecContextOverride(c *C) { + userID, groupID := 100, 200 + svcUserID, svcGroupID := 10, 20 + chk := newChecker(&plan.Check{ + Name: "exec", + Exec: &plan.ExecCheck{ + Command: "sleep 1", + ServiceContext: "svc1", + Environment: map[string]string{"k": "v"}, + UserID: &userID, + User: "user", + GroupID: &groupID, + Group: "group", + WorkingDir: "/working/dir", + }, + }, &plan.Plan{Services: map[string]*plan.Service{ + "svc1": { + Name: "svc1", + Environment: map[string]string{"k": "x", "a": "1"}, + UserID: &svcUserID, + User: "svcuser", + GroupID: &svcGroupID, + Group: "svcgroup", + WorkingDir: "/working/svc", + }, + }}) + exec, ok := chk.(*execChecker) + c.Assert(ok, Equals, true) + c.Check(exec.name, Equals, "exec") + c.Check(exec.command, Equals, "sleep 1") + c.Check(exec.environment, DeepEquals, map[string]string{"k": "v", "a": "1"}) + c.Check(exec.userID, DeepEquals, &userID) + c.Check(exec.user, Equals, "user") + c.Check(exec.groupID, DeepEquals, &groupID) + c.Check(exec.workingDir, Equals, "/working/dir") +} diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 22d403c93..eccf42a90 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -105,12 +105,10 @@ func (s *Service) Copy() *Service { } } if s.UserID != nil { - userID := *s.UserID - copied.UserID = &userID + copied.UserID = copyIntPtr(s.UserID) } if s.GroupID != nil { - groupID := *s.GroupID - copied.GroupID = &groupID + copied.GroupID = copyIntPtr(s.GroupID) } if s.OnCheckFailure != nil { copied.OnCheckFailure = make(map[string]ServiceAction) @@ -139,15 +137,13 @@ func (s *Service) Merge(other *Service) { s.KillDelay = other.KillDelay } if other.UserID != nil { - userID := *other.UserID - s.UserID = &userID + s.UserID = copyIntPtr(other.UserID) } if other.User != "" { s.User = other.User } if other.GroupID != nil { - groupID := *other.GroupID - s.GroupID = &groupID + s.GroupID = copyIntPtr(other.GroupID) } if other.Group != "" { s.Group = other.Group @@ -431,13 +427,14 @@ func (c *TCPCheck) Merge(other *TCPCheck) { // ExecCheck holds the configuration for an exec health check. type ExecCheck struct { - Command string `yaml:"command,omitempty"` - Environment map[string]string `yaml:"environment,omitempty"` - UserID *int `yaml:"user-id,omitempty"` - User string `yaml:"user,omitempty"` - GroupID *int `yaml:"group-id,omitempty"` - Group string `yaml:"group,omitempty"` - WorkingDir string `yaml:"working-dir,omitempty"` + Command string `yaml:"command,omitempty"` + ServiceContext string `yaml:"service-context,omitempty"` + Environment map[string]string `yaml:"environment,omitempty"` + UserID *int `yaml:"user-id,omitempty"` + User string `yaml:"user,omitempty"` + GroupID *int `yaml:"group-id,omitempty"` + Group string `yaml:"group,omitempty"` + WorkingDir string `yaml:"working-dir,omitempty"` } // Copy returns a deep copy of the exec check configuration. @@ -450,12 +447,10 @@ func (c *ExecCheck) Copy() *ExecCheck { } } if c.UserID != nil { - userID := *c.UserID - copied.UserID = &userID + copied.UserID = copyIntPtr(c.UserID) } if c.GroupID != nil { - groupID := *c.GroupID - copied.GroupID = &groupID + copied.GroupID = copyIntPtr(c.GroupID) } return &copied } @@ -465,6 +460,9 @@ func (c *ExecCheck) Merge(other *ExecCheck) { if other.Command != "" { c.Command = other.Command } + if other.ServiceContext != "" { + c.ServiceContext = other.ServiceContext + } for k, v := range other.Environment { if c.Environment == nil { c.Environment = make(map[string]string) @@ -472,15 +470,13 @@ func (c *ExecCheck) Merge(other *ExecCheck) { c.Environment[k] = v } if other.UserID != nil { - userID := *other.UserID - c.UserID = &userID + c.UserID = copyIntPtr(other.UserID) } if other.User != "" { c.User = other.User } if other.GroupID != nil { - groupID := *other.GroupID - c.GroupID = &groupID + c.GroupID = copyIntPtr(other.GroupID) } if other.Group != "" { c.Group = other.Group @@ -733,6 +729,13 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { Message: fmt.Sprintf("plan check %q command invalid: %v", name, err), } } + _, contextExists := combined.Services[check.Exec.ServiceContext] + if check.Exec.ServiceContext != "" && !contextExists { + return nil, &FormatError{ + Message: fmt.Sprintf("plan check %q service context specifies non-existent service %q", + name, check.Exec.ServiceContext), + } + } _, _, err = osutil.NormalizeUidGid(check.Exec.UserID, check.Exec.GroupID, check.Exec.User, check.Exec.Group) if err != nil { return nil, &FormatError{ @@ -1075,3 +1078,79 @@ func ReadDir(dir string) (*Plan, error) { } return plan, err } + +// MergeServiceContext merges the overrides on top of the service context +// specified by serviceName, returning a new ContextOptions value. If +// serviceName is "" (context not specified), return overrides directly. +func MergeServiceContext(p *Plan, serviceName string, overrides ContextOptions) (ContextOptions, error) { + if serviceName == "" { + return overrides, nil + } + var service *Service + for _, s := range p.Services { + if s.Name == serviceName { + service = s + break + } + } + if service == nil { + return ContextOptions{}, fmt.Errorf("context service %q not found", serviceName) + } + + // Start with the config values from the context service. + merged := ContextOptions{ + Environment: make(map[string]string), + } + for k, v := range service.Environment { + merged.Environment[k] = v + } + if service.UserID != nil { + merged.UserID = copyIntPtr(service.UserID) + } + merged.User = service.User + if service.GroupID != nil { + merged.GroupID = copyIntPtr(service.GroupID) + } + merged.Group = service.Group + merged.WorkingDir = service.WorkingDir + + // Merge in fields from the overrides, if set. + for k, v := range overrides.Environment { + merged.Environment[k] = v + } + if overrides.UserID != nil { + merged.UserID = copyIntPtr(overrides.UserID) + } + if overrides.User != "" { + merged.User = overrides.User + } + if overrides.GroupID != nil { + merged.GroupID = copyIntPtr(overrides.GroupID) + } + if overrides.Group != "" { + merged.Group = overrides.Group + } + if overrides.WorkingDir != "" { + merged.WorkingDir = overrides.WorkingDir + } + + return merged, nil +} + +// ContextOptions holds service context config fields. +type ContextOptions struct { + Environment map[string]string + UserID *int + User string + GroupID *int + Group string + WorkingDir string +} + +func copyIntPtr(p *int) *int { + if p == nil { + return nil + } + copied := *p + return &copied +} diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 1883ece4c..c9f999d0d 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -823,6 +823,17 @@ var planTests = []planTest{{ exec: command: foo ' `}, +}, { + summary: `Invalid exec check service context`, + error: `plan check "chk1" service context specifies non-existent service "nosvc"`, + input: []string{` + checks: + chk1: + override: replace + exec: + command: foo + service-context: nosvc + `}, }, { summary: "Simple layer with log targets", input: []string{` @@ -1545,3 +1556,78 @@ func (s *S) TestLogsTo(c *C) { } } } + +func (s *S) TestMergeServiceContextNoContext(c *C) { + userID, groupID := 10, 20 + overrides := plan.ContextOptions{ + Environment: map[string]string{"x": "y"}, + UserID: &userID, + User: "usr", + GroupID: &groupID, + Group: "grp", + WorkingDir: "/working/dir", + } + merged, err := plan.MergeServiceContext(nil, "", overrides) + c.Assert(err, IsNil) + c.Check(merged, DeepEquals, overrides) +} + +func (s *S) TestMergeServiceContextBadService(c *C) { + _, err := plan.MergeServiceContext(&plan.Plan{}, "nosvc", plan.ContextOptions{}) + c.Assert(err, ErrorMatches, `context service "nosvc" not found`) +} + +func (s *S) TestMergeServiceContextNoOverrides(c *C) { + userID, groupID := 11, 22 + p := &plan.Plan{Services: map[string]*plan.Service{"svc1": { + Name: "svc1", + Environment: map[string]string{"x": "y"}, + UserID: &userID, + User: "svcuser", + GroupID: &groupID, + Group: "svcgroup", + WorkingDir: "/working/svc", + }}} + merged, err := plan.MergeServiceContext(p, "svc1", plan.ContextOptions{}) + c.Assert(err, IsNil) + c.Check(merged, DeepEquals, plan.ContextOptions{ + Environment: map[string]string{"x": "y"}, + UserID: &userID, + User: "svcuser", + GroupID: &groupID, + Group: "svcgroup", + WorkingDir: "/working/svc", + }) +} + +func (s *S) TestMergeServiceContextOverrides(c *C) { + svcUserID, svcGroupID := 10, 20 + p := &plan.Plan{Services: map[string]*plan.Service{"svc1": { + Name: "svc1", + Environment: map[string]string{"x": "y", "w": "z"}, + UserID: &svcUserID, + User: "svcuser", + GroupID: &svcGroupID, + Group: "svcgroup", + WorkingDir: "/working/svc", + }}} + userID, groupID := 11, 22 + overrides := plan.ContextOptions{ + Environment: map[string]string{"x": "a"}, + UserID: &userID, + User: "usr", + GroupID: &groupID, + Group: "grp", + WorkingDir: "/working/dir", + } + merged, err := plan.MergeServiceContext(p, "svc1", overrides) + c.Assert(err, IsNil) + c.Check(merged, DeepEquals, plan.ContextOptions{ + Environment: map[string]string{"x": "a", "w": "z"}, + UserID: &userID, + User: "usr", + GroupID: &groupID, + Group: "grp", + WorkingDir: "/working/dir", + }) +} From 71fea74ebe4e87716fd8d6ff7578298c1209428e Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Wed, 5 Jul 2023 14:52:38 +1200 Subject: [PATCH 06/12] Fix import order All Go files should have exactly three import groups: - stdlib - 3rd-party / non-project imports - project imports (from inside canonical/pebble) --- internals/cli/cli.go | 1 - internals/cli/cli_test.go | 1 - internals/cli/format_test.go | 3 --- internals/daemon/api_changes_test.go | 4 ++-- internals/daemon/api_files_test.go | 3 ++- internals/daemon/api_services_test.go | 4 ++-- internals/daemon/api_warnings_test.go | 4 ++-- internals/daemon/daemon.go | 3 +-- internals/daemon/daemon_test.go | 2 -- internals/osutil/bootid_test.go | 4 ++-- internals/osutil/env_test.go | 3 ++- internals/overlord/patch/patch_test.go | 4 ++-- internals/overlord/servstate/request.go | 4 ++-- internals/overlord/state/change_test.go | 4 ++-- internals/servicelog/ringbuffer_test.go | 3 ++- internals/systemd/systemd_test.go | 3 +-- 16 files changed, 22 insertions(+), 28 deletions(-) diff --git a/internals/cli/cli.go b/internals/cli/cli.go index 18a378510..cfdd4cb36 100644 --- a/internals/cli/cli.go +++ b/internals/cli/cli.go @@ -26,7 +26,6 @@ import ( "unicode/utf8" "github.com/canonical/go-flags" - "golang.org/x/crypto/ssh/terminal" "github.com/canonical/pebble/client" diff --git a/internals/cli/cli_test.go b/internals/cli/cli_test.go index 45943c925..16cf4f68a 100644 --- a/internals/cli/cli_test.go +++ b/internals/cli/cli_test.go @@ -11,7 +11,6 @@ import ( "testing" "golang.org/x/crypto/ssh/terminal" - . "gopkg.in/check.v1" "github.com/canonical/pebble/cmd" diff --git a/internals/cli/format_test.go b/internals/cli/format_test.go index 3b92d37a2..b6c6c5ea3 100644 --- a/internals/cli/format_test.go +++ b/internals/cli/format_test.go @@ -18,9 +18,6 @@ import ( "os" "runtime" - // "fmt" - // "net/http" - "gopkg.in/check.v1" "github.com/canonical/pebble/internals/cli" diff --git a/internals/daemon/api_changes_test.go b/internals/daemon/api_changes_test.go index 0f5f6228d..c06015de2 100644 --- a/internals/daemon/api_changes_test.go +++ b/internals/daemon/api_changes_test.go @@ -23,9 +23,9 @@ import ( "net/http/httptest" "time" - "github.com/canonical/pebble/internals/overlord/state" - "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/overlord/state" ) func setupChanges(st *state.State) []string { diff --git a/internals/daemon/api_files_test.go b/internals/daemon/api_files_test.go index ab41554da..3abbb7dfc 100644 --- a/internals/daemon/api_files_test.go +++ b/internals/daemon/api_files_test.go @@ -32,9 +32,10 @@ import ( "syscall" "time" + . "gopkg.in/check.v1" + "github.com/canonical/pebble/internals/osutil" "github.com/canonical/pebble/internals/osutil/sys" - . "gopkg.in/check.v1" ) var _ = Suite(&filesSuite{}) diff --git a/internals/daemon/api_services_test.go b/internals/daemon/api_services_test.go index e05017122..09ba46fcf 100644 --- a/internals/daemon/api_services_test.go +++ b/internals/daemon/api_services_test.go @@ -25,9 +25,9 @@ import ( "strings" "time" - "github.com/canonical/pebble/internals/overlord/state" - . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/overlord/state" ) var servicesLayer = ` diff --git a/internals/daemon/api_warnings_test.go b/internals/daemon/api_warnings_test.go index e8318f60b..6d97a60f3 100644 --- a/internals/daemon/api_warnings_test.go +++ b/internals/daemon/api_warnings_test.go @@ -21,9 +21,9 @@ import ( "net/url" "time" - "github.com/canonical/pebble/internals/overlord/state" - "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/overlord/state" ) func (s *apiSuite) testWarnings(c *check.C, all bool, body io.Reader) (calls string, result interface{}) { diff --git a/internals/daemon/daemon.go b/internals/daemon/daemon.go index b7405ee06..e85fdd02c 100644 --- a/internals/daemon/daemon.go +++ b/internals/daemon/daemon.go @@ -31,9 +31,8 @@ import ( "syscall" "time" - "gopkg.in/tomb.v2" - "github.com/gorilla/mux" + "gopkg.in/tomb.v2" "github.com/canonical/pebble/internals/logger" "github.com/canonical/pebble/internals/osutil" diff --git a/internals/daemon/daemon_test.go b/internals/daemon/daemon_test.go index c10d04d9e..aaa9fa113 100644 --- a/internals/daemon/daemon_test.go +++ b/internals/daemon/daemon_test.go @@ -30,9 +30,7 @@ import ( "time" "github.com/gorilla/mux" - "gopkg.in/check.v1" - // XXX Delete import above and make this file like the other ones. . "gopkg.in/check.v1" diff --git a/internals/osutil/bootid_test.go b/internals/osutil/bootid_test.go index 48f5b61bd..5d8f5fb44 100644 --- a/internals/osutil/bootid_test.go +++ b/internals/osutil/bootid_test.go @@ -20,9 +20,9 @@ package osutil_test import ( - "github.com/canonical/pebble/internals/osutil" - . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/osutil" ) type bootIdSuite struct{} diff --git a/internals/osutil/env_test.go b/internals/osutil/env_test.go index a5e05f4bf..5ad3a410d 100644 --- a/internals/osutil/env_test.go +++ b/internals/osutil/env_test.go @@ -15,8 +15,9 @@ package osutil_test import ( - "github.com/canonical/pebble/internals/osutil" . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/osutil" ) type envSuite struct{} diff --git a/internals/overlord/patch/patch_test.go b/internals/overlord/patch/patch_test.go index ea858ce5a..afe9fd8db 100644 --- a/internals/overlord/patch/patch_test.go +++ b/internals/overlord/patch/patch_test.go @@ -24,10 +24,10 @@ import ( "sort" "testing" + . "gopkg.in/check.v1" + "github.com/canonical/pebble/internals/overlord/patch" "github.com/canonical/pebble/internals/overlord/state" - - . "gopkg.in/check.v1" ) func Test(t *testing.T) { TestingT(t) } diff --git a/internals/overlord/servstate/request.go b/internals/overlord/servstate/request.go index 0d525a875..49c23f4f3 100644 --- a/internals/overlord/servstate/request.go +++ b/internals/overlord/servstate/request.go @@ -1,9 +1,9 @@ package servstate import ( - "github.com/canonical/pebble/internals/overlord/state" - "fmt" + + "github.com/canonical/pebble/internals/overlord/state" ) // ServiceRequest holds the details required to perform service tasks. diff --git a/internals/overlord/state/change_test.go b/internals/overlord/state/change_test.go index 52b77bfcd..339c94344 100644 --- a/internals/overlord/state/change_test.go +++ b/internals/overlord/state/change_test.go @@ -26,9 +26,9 @@ import ( "strings" "time" - "github.com/canonical/pebble/internals/overlord/state" - . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/overlord/state" ) type changeSuite struct{} diff --git a/internals/servicelog/ringbuffer_test.go b/internals/servicelog/ringbuffer_test.go index 08b27eb54..4af940c75 100644 --- a/internals/servicelog/ringbuffer_test.go +++ b/internals/servicelog/ringbuffer_test.go @@ -20,8 +20,9 @@ import ( "io" "testing" - "github.com/canonical/pebble/internals/servicelog" . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/servicelog" ) type ringBufferSuite struct{} diff --git a/internals/systemd/systemd_test.go b/internals/systemd/systemd_test.go index f32c363d7..45a211922 100644 --- a/internals/systemd/systemd_test.go +++ b/internals/systemd/systemd_test.go @@ -34,9 +34,8 @@ import ( "github.com/canonical/pebble/internals/osutil" "github.com/canonical/pebble/internals/osutil/squashfs" - "github.com/canonical/pebble/internals/testutil" - "github.com/canonical/pebble/internals/systemd" + "github.com/canonical/pebble/internals/testutil" ) type testreporter struct { From dc04f2e0df42444927d8bb21c0fe566879221537 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Wed, 5 Jul 2023 14:55:03 +1200 Subject: [PATCH 07/12] Fix daemon_test.go imports gocheck should be imported with `.`, as suggested in the comment. --- internals/daemon/daemon_test.go | 354 ++++++++++++++++---------------- 1 file changed, 176 insertions(+), 178 deletions(-) diff --git a/internals/daemon/daemon_test.go b/internals/daemon/daemon_test.go index aaa9fa113..1683b5463 100644 --- a/internals/daemon/daemon_test.go +++ b/internals/daemon/daemon_test.go @@ -30,8 +30,6 @@ import ( "time" "github.com/gorilla/mux" - "gopkg.in/check.v1" - // XXX Delete import above and make this file like the other ones. . "gopkg.in/check.v1" "github.com/canonical/pebble/internals/osutil" @@ -44,7 +42,7 @@ import ( ) // Hook up check.v1 into the "go test" runner -func Test(t *testing.T) { check.TestingT(t) } +func Test(t *testing.T) { TestingT(t) } type daemonSuite struct { pebbleDir string @@ -57,9 +55,9 @@ type daemonSuite struct { restoreBackends func() } -var _ = check.Suite(&daemonSuite{}) +var _ = Suite(&daemonSuite{}) -func (s *daemonSuite) SetUpTest(c *check.C) { +func (s *daemonSuite) SetUpTest(c *C) { s.pebbleDir = c.MkDir() s.statePath = filepath.Join(s.pebbleDir, ".pebble.state") systemdSdNotify = func(notif string) error { @@ -68,20 +66,20 @@ func (s *daemonSuite) SetUpTest(c *check.C) { } } -func (s *daemonSuite) TearDownTest(c *check.C) { +func (s *daemonSuite) TearDownTest(c *C) { systemdSdNotify = systemd.SdNotify s.notified = nil s.authorized = false s.err = nil } -func (s *daemonSuite) newDaemon(c *check.C) *Daemon { +func (s *daemonSuite) newDaemon(c *C) *Daemon { d, err := New(&Options{ Dir: s.pebbleDir, SocketPath: s.socketPath, HTTPAddress: s.httpAddress, }) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) d.addRoutes() return d } @@ -113,13 +111,13 @@ func (s *daemonSuite) TestExplicitPaths(c *C) { c.Assert(info.Mode(), Equals, os.ModeSocket|0666) } -func (s *daemonSuite) TestCommandMethodDispatch(c *check.C) { +func (s *daemonSuite) TestCommandMethodDispatch(c *C) { fakeUserAgent := "some-agent-talking-to-pebble/1.0" cmd := &Command{d: s.newDaemon(c)} handler := &fakeHandler{cmd: cmd} rf := func(innerCmd *Command, req *http.Request, user *userState) Response { - c.Assert(cmd, check.Equals, innerCmd) + c.Assert(cmd, Equals, innerCmd) return handler } cmd.GET = rf @@ -130,30 +128,30 @@ func (s *daemonSuite) TestCommandMethodDispatch(c *check.C) { for _, method := range []string{"GET", "POST", "PUT", "DELETE"} { req, err := http.NewRequest(method, "", nil) req.Header.Add("User-Agent", fakeUserAgent) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) rec := httptest.NewRecorder() cmd.ServeHTTP(rec, req) - c.Check(rec.Code, check.Equals, 401, check.Commentf(method)) + c.Check(rec.Code, Equals, 401, Commentf(method)) rec = httptest.NewRecorder() req.RemoteAddr = "pid=100;uid=0;socket=;" cmd.ServeHTTP(rec, req) - c.Check(handler.lastMethod, check.Equals, method) - c.Check(rec.Code, check.Equals, 200) + c.Check(handler.lastMethod, Equals, method) + c.Check(rec.Code, Equals, 200) } req, err := http.NewRequest("POTATO", "", nil) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) req.RemoteAddr = "pid=100;uid=0;socket=;" rec := httptest.NewRecorder() cmd.ServeHTTP(rec, req) - c.Check(rec.Code, check.Equals, 405) + c.Check(rec.Code, Equals, 405) } -func (s *daemonSuite) TestCommandRestartingState(c *check.C) { +func (s *daemonSuite) TestCommandRestartingState(c *C) { d := s.newDaemon(c) cmd := &Command{d: d} @@ -161,18 +159,18 @@ func (s *daemonSuite) TestCommandRestartingState(c *check.C) { return SyncResponse(nil) } req, err := http.NewRequest("GET", "", nil) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) req.RemoteAddr = "pid=100;uid=0;socket=;" rec := httptest.NewRecorder() cmd.ServeHTTP(rec, req) - c.Check(rec.Code, check.Equals, 200) + c.Check(rec.Code, Equals, 200) var rst struct { Maintenance *errorResult `json:"maintenance"` } err = json.Unmarshal(rec.Body.Bytes(), &rst) - c.Assert(err, check.IsNil) - c.Check(rst.Maintenance, check.IsNil) + c.Assert(err, IsNil) + c.Check(rst.Maintenance, IsNil) state := d.overlord.State() @@ -181,10 +179,10 @@ func (s *daemonSuite) TestCommandRestartingState(c *check.C) { state.Unlock() rec = httptest.NewRecorder() cmd.ServeHTTP(rec, req) - c.Check(rec.Code, check.Equals, 200) + c.Check(rec.Code, Equals, 200) err = json.Unmarshal(rec.Body.Bytes(), &rst) - c.Assert(err, check.IsNil) - c.Check(rst.Maintenance, check.DeepEquals, &errorResult{ + c.Assert(err, IsNil) + c.Check(rst.Maintenance, DeepEquals, &errorResult{ Kind: errorKindSystemRestart, Message: "system is restarting", }) @@ -194,16 +192,16 @@ func (s *daemonSuite) TestCommandRestartingState(c *check.C) { state.Unlock() rec = httptest.NewRecorder() cmd.ServeHTTP(rec, req) - c.Check(rec.Code, check.Equals, 200) + c.Check(rec.Code, Equals, 200) err = json.Unmarshal(rec.Body.Bytes(), &rst) - c.Assert(err, check.IsNil) - c.Check(rst.Maintenance, check.DeepEquals, &errorResult{ + c.Assert(err, IsNil) + c.Check(rst.Maintenance, DeepEquals, &errorResult{ Kind: errorKindDaemonRestart, Message: "daemon is restarting", }) } -func (s *daemonSuite) TestFillsWarnings(c *check.C) { +func (s *daemonSuite) TestFillsWarnings(c *C) { d := s.newDaemon(c) cmd := &Command{d: d} @@ -211,20 +209,20 @@ func (s *daemonSuite) TestFillsWarnings(c *check.C) { return SyncResponse(nil) } req, err := http.NewRequest("GET", "", nil) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) req.RemoteAddr = "pid=100;uid=0;socket=;" rec := httptest.NewRecorder() cmd.ServeHTTP(rec, req) - c.Check(rec.Code, check.Equals, 200) + c.Check(rec.Code, Equals, 200) var rst struct { WarningTimestamp *time.Time `json:"warning-timestamp,omitempty"` WarningCount int `json:"warning-count,omitempty"` } err = json.Unmarshal(rec.Body.Bytes(), &rst) - c.Assert(err, check.IsNil) - c.Check(rst.WarningCount, check.Equals, 0) - c.Check(rst.WarningTimestamp, check.IsNil) + c.Assert(err, IsNil) + c.Check(rst.WarningCount, Equals, 0) + c.Check(rst.WarningTimestamp, IsNil) st := d.overlord.State() st.Lock() @@ -233,14 +231,14 @@ func (s *daemonSuite) TestFillsWarnings(c *check.C) { rec = httptest.NewRecorder() cmd.ServeHTTP(rec, req) - c.Check(rec.Code, check.Equals, 200) + c.Check(rec.Code, Equals, 200) err = json.Unmarshal(rec.Body.Bytes(), &rst) - c.Assert(err, check.IsNil) - c.Check(rst.WarningCount, check.Equals, 1) - c.Check(rst.WarningTimestamp, check.NotNil) + c.Assert(err, IsNil) + c.Check(rst.WarningCount, Equals, 1) + c.Check(rst.WarningTimestamp, NotNil) } -func (s *daemonSuite) TestGuestAccess(c *check.C) { +func (s *daemonSuite) TestGuestAccess(c *C) { d := s.newDaemon(c) get := &http.Request{Method: "GET"} @@ -249,31 +247,31 @@ func (s *daemonSuite) TestGuestAccess(c *check.C) { del := &http.Request{Method: "DELETE"} cmd := &Command{d: d} - c.Check(cmd.canAccess(get, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(pst, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(del, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(pst, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(del, nil), Equals, accessUnauthorized) cmd = &Command{d: d, AdminOnly: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(pst, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(del, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(pst, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(del, nil), Equals, accessUnauthorized) cmd = &Command{d: d, UserOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(pst, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(del, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(pst, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(del, nil), Equals, accessUnauthorized) cmd = &Command{d: d, GuestOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(pst, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(del, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(pst, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(del, nil), Equals, accessUnauthorized) } -func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithUser(c *check.C) { +func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithUser(c *C) { d := s.newDaemon(c) remoteAddr := "pid=100;uid=1000;socket=" + d.untrustedSocketPath + ";" @@ -283,13 +281,13 @@ func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithUser(c *check.C) { del := &http.Request{Method: "DELETE", RemoteAddr: remoteAddr} cmd := &Command{d: d, UntrustedOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(pst, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(del, nil), check.Equals, accessOK) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessOK) + c.Check(cmd.canAccess(pst, nil), Equals, accessOK) + c.Check(cmd.canAccess(del, nil), Equals, accessOK) } -func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithRoot(c *check.C) { +func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithRoot(c *C) { d := s.newDaemon(c) remoteAddr := "pid=100;uid=0;socket=" + d.untrustedSocketPath + ";" @@ -299,43 +297,43 @@ func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithRoot(c *check.C) { del := &http.Request{Method: "DELETE", RemoteAddr: remoteAddr} cmd := &Command{d: d, UntrustedOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(pst, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(del, nil), check.Equals, accessOK) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessOK) + c.Check(cmd.canAccess(pst, nil), Equals, accessOK) + c.Check(cmd.canAccess(del, nil), Equals, accessOK) } -func (s *daemonSuite) TestUserAccess(c *check.C) { +func (s *daemonSuite) TestUserAccess(c *C) { d := s.newDaemon(c) get := &http.Request{Method: "GET", RemoteAddr: "pid=100;uid=42;socket=;"} put := &http.Request{Method: "PUT", RemoteAddr: "pid=100;uid=42;socket=;"} cmd := &Command{d: d} - c.Check(cmd.canAccess(get, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) cmd = &Command{d: d, AdminOnly: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) cmd = &Command{d: d, UserOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) cmd = &Command{d: d, GuestOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) // Since this request has a RemoteAddr, it must be coming from the pebble server // socket instead of the pebble one. In that case, UntrustedOK should have no // bearing on the default behavior, which is to deny access. cmd = &Command{d: d, UntrustedOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(put, nil), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, nil), Equals, accessUnauthorized) + c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized) } -func (s *daemonSuite) TestLoggedInUserAccess(c *check.C) { +func (s *daemonSuite) TestLoggedInUserAccess(c *C) { d := s.newDaemon(c) user := &userState{} @@ -343,27 +341,27 @@ func (s *daemonSuite) TestLoggedInUserAccess(c *check.C) { put := &http.Request{Method: "PUT", RemoteAddr: "pid=100;uid=42;socket=;"} cmd := &Command{d: d} - c.Check(cmd.canAccess(get, user), check.Equals, accessOK) - c.Check(cmd.canAccess(put, user), check.Equals, accessOK) + c.Check(cmd.canAccess(get, user), Equals, accessOK) + c.Check(cmd.canAccess(put, user), Equals, accessOK) cmd = &Command{d: d, AdminOnly: true} - c.Check(cmd.canAccess(get, user), check.Equals, accessUnauthorized) - c.Check(cmd.canAccess(put, user), check.Equals, accessUnauthorized) + c.Check(cmd.canAccess(get, user), Equals, accessUnauthorized) + c.Check(cmd.canAccess(put, user), Equals, accessUnauthorized) cmd = &Command{d: d, UserOK: true} - c.Check(cmd.canAccess(get, user), check.Equals, accessOK) - c.Check(cmd.canAccess(put, user), check.Equals, accessOK) + c.Check(cmd.canAccess(get, user), Equals, accessOK) + c.Check(cmd.canAccess(put, user), Equals, accessOK) cmd = &Command{d: d, GuestOK: true} - c.Check(cmd.canAccess(get, user), check.Equals, accessOK) - c.Check(cmd.canAccess(put, user), check.Equals, accessOK) + c.Check(cmd.canAccess(get, user), Equals, accessOK) + c.Check(cmd.canAccess(put, user), Equals, accessOK) cmd = &Command{d: d, UntrustedOK: true} - c.Check(cmd.canAccess(get, user), check.Equals, accessOK) - c.Check(cmd.canAccess(put, user), check.Equals, accessOK) + c.Check(cmd.canAccess(get, user), Equals, accessOK) + c.Check(cmd.canAccess(put, user), Equals, accessOK) } -func (s *daemonSuite) TestSuperAccess(c *check.C) { +func (s *daemonSuite) TestSuperAccess(c *C) { d := s.newDaemon(c) for _, uid := range []int{0, os.Getuid()} { @@ -372,28 +370,28 @@ func (s *daemonSuite) TestSuperAccess(c *check.C) { put := &http.Request{Method: "PUT", RemoteAddr: remoteAddr} cmd := &Command{d: d} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessOK) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessOK) cmd = &Command{d: d, AdminOnly: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessOK) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessOK) cmd = &Command{d: d, UserOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessOK) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessOK) cmd = &Command{d: d, GuestOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessOK) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessOK) cmd = &Command{d: d, UntrustedOK: true} - c.Check(cmd.canAccess(get, nil), check.Equals, accessOK) - c.Check(cmd.canAccess(put, nil), check.Equals, accessOK) + c.Check(cmd.canAccess(get, nil), Equals, accessOK) + c.Check(cmd.canAccess(put, nil), Equals, accessOK) } } -func (s *daemonSuite) TestAddRoutes(c *check.C) { +func (s *daemonSuite) TestAddRoutes(c *C) { d := s.newDaemon(c) expected := make([]string, len(api)) @@ -409,9 +407,9 @@ func (s *daemonSuite) TestAddRoutes(c *check.C) { c.Assert(d.router.Walk(func(route *mux.Route, router *mux.Router, ancestors []*mux.Route) error { got = append(got, route.GetName()) return nil - }), check.IsNil) + }), IsNil) - c.Check(got, check.DeepEquals, expected) // this'll stop being true if routes are added that aren't commands (e.g. for the favicon) + c.Check(got, DeepEquals, expected) // this'll stop being true if routes are added that aren't commands (e.g. for the favicon) } type witnessAcceptListener struct { @@ -443,13 +441,13 @@ func (l *witnessAcceptListener) Close() error { return l.closeErr } -func (s *daemonSuite) TestStartStop(c *check.C) { +func (s *daemonSuite) TestStartStop(c *C) { d := s.newDaemon(c) l1, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) l2, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) generalAccept := make(chan struct{}) d.generalListener = &witnessAcceptListener{Listener: l1, accept: generalAccept} @@ -483,14 +481,14 @@ func (s *daemonSuite) TestStartStop(c *check.C) { <-untrustedDone err = d.Stop(nil) - c.Check(err, check.IsNil) + c.Check(err, IsNil) } -func (s *daemonSuite) TestRestartWiring(c *check.C) { +func (s *daemonSuite) TestRestartWiring(c *C) { d := s.newDaemon(c) l, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) generalAccept := make(chan struct{}) d.generalListener = &witnessAcceptListener{Listener: l, accept: generalAccept} @@ -536,7 +534,7 @@ func (s *daemonSuite) TestRestartWiring(c *check.C) { } } -func (s *daemonSuite) TestGracefulStop(c *check.C) { +func (s *daemonSuite) TestGracefulStop(c *C) { d := s.newDaemon(c) responding := make(chan struct{}) @@ -553,10 +551,10 @@ func (s *daemonSuite) TestGracefulStop(c *check.C) { }) generalL, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) untrustedL, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) generalAccept := make(chan struct{}) generalClosed := make(chan struct{}) @@ -594,12 +592,12 @@ func (s *daemonSuite) TestGracefulStop(c *check.C) { go func() { res, err := http.Get(fmt.Sprintf("http://%s/endp", generalL.Addr())) - c.Assert(err, check.IsNil) - c.Check(res.StatusCode, check.Equals, 200) + c.Assert(err, IsNil) + c.Check(res.StatusCode, Equals, 200) body, err := ioutil.ReadAll(res.Body) res.Body.Close() - c.Assert(err, check.IsNil) - c.Check(string(body), check.Equals, "OKOK") + c.Assert(err, IsNil) + c.Check(string(body), Equals, "OKOK") close(alright) }() go func() { @@ -611,7 +609,7 @@ func (s *daemonSuite) TestGracefulStop(c *check.C) { <-responding err = d.Stop(nil) doRespond <- false - c.Check(err, check.IsNil) + c.Check(err, IsNil) select { case <-alright: @@ -620,11 +618,11 @@ func (s *daemonSuite) TestGracefulStop(c *check.C) { } } -func (s *daemonSuite) TestRestartSystemWiring(c *check.C) { +func (s *daemonSuite) TestRestartSystemWiring(c *C) { d := s.newDaemon(c) l, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) generalAccept := make(chan struct{}) d.generalListener = &witnessAcceptListener{Listener: l, accept: generalAccept} @@ -696,33 +694,33 @@ func (s *daemonSuite) TestRestartSystemWiring(c *check.C) { rs := d.restartSystem d.mu.Unlock() - c.Check(rs, check.Equals, true) + c.Check(rs, Equals, true) - c.Check(delays, check.HasLen, 1) - c.Check(delays[0], check.DeepEquals, rebootWaitTimeout) + c.Check(delays, HasLen, 1) + c.Check(delays[0], DeepEquals, rebootWaitTimeout) now := time.Now() err = d.Stop(nil) - c.Check(err, check.ErrorMatches, "expected reboot did not happen") + c.Check(err, ErrorMatches, "expected reboot did not happen") - c.Check(delays, check.HasLen, 2) - c.Check(delays[1], check.DeepEquals, 1*time.Minute) + c.Check(delays, HasLen, 2) + c.Check(delays[1], DeepEquals, 1*time.Minute) // we are not stopping, we wait for the reboot instead - c.Check(s.notified, check.DeepEquals, []string{"READY=1"}) + c.Check(s.notified, DeepEquals, []string{"READY=1"}) st.Lock() defer st.Unlock() var rebootAt time.Time err = st.Get("daemon-system-restart-at", &rebootAt) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) approxAt := now.Add(time.Minute) - c.Check(rebootAt.After(approxAt) || rebootAt.Equal(approxAt), check.Equals, true) + c.Check(rebootAt.After(approxAt) || rebootAt.Equal(approxAt), Equals, true) } -func (s *daemonSuite) TestRebootHelper(c *check.C) { +func (s *daemonSuite) TestRebootHelper(c *C) { cmd := testutil.FakeCommand(c, "shutdown", "", true) defer cmd.Restore() @@ -739,8 +737,8 @@ func (s *daemonSuite) TestRebootHelper(c *check.C) { for _, t := range tests { err := reboot(t.delay) - c.Assert(err, check.IsNil) - c.Check(cmd.Calls(), check.DeepEquals, [][]string{ + c.Assert(err, IsNil) + c.Check(cmd.Calls(), DeepEquals, [][]string{ {"shutdown", "-r", t.delayArg, "reboot scheduled to update the system"}, }) @@ -748,12 +746,12 @@ func (s *daemonSuite) TestRebootHelper(c *check.C) { } } -func makeDaemonListeners(c *check.C, d *Daemon) { +func makeDaemonListeners(c *C, d *Daemon) { generalL, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) untrustedL, err := net.Listen("tcp", "127.0.0.1:0") - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) generalAccept := make(chan struct{}) generalClosed := make(chan struct{}) @@ -766,7 +764,7 @@ func makeDaemonListeners(c *check.C, d *Daemon) { // This test tests that when a restart of the system is called // a sigterm (from e.g. systemd) is handled when it arrives before // stop is fully done. -func (s *daemonSuite) TestRestartShutdownWithSigtermInBetween(c *check.C) { +func (s *daemonSuite) TestRestartShutdownWithSigtermInBetween(c *C) { oldRebootNoticeWait := rebootNoticeWait defer func() { rebootNoticeWait = oldRebootNoticeWait @@ -790,12 +788,12 @@ func (s *daemonSuite) TestRestartShutdownWithSigtermInBetween(c *check.C) { ch <- syscall.SIGTERM // stop will check if we got a sigterm in between (which we did) err := d.Stop(ch) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) } // This test tests that when there is a shutdown we close the sigterm // handler so that systemd can kill pebble. -func (s *daemonSuite) TestRestartShutdown(c *check.C) { +func (s *daemonSuite) TestRestartShutdown(c *C) { oldRebootNoticeWait := rebootNoticeWait oldRebootWaitTimeout := rebootWaitTimeout defer func() { @@ -824,16 +822,16 @@ func (s *daemonSuite) TestRestartShutdown(c *check.C) { // ensure that the sigCh got closed as part of the stop _, chOpen := <-sigCh - c.Assert(chOpen, check.Equals, false) + c.Assert(chOpen, Equals, false) } -func (s *daemonSuite) TestRestartExpectedRebootIsMissing(c *check.C) { +func (s *daemonSuite) TestRestartExpectedRebootIsMissing(c *C) { curBootID, err := osutil.BootID() - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) fakeState := []byte(fmt.Sprintf(`{"data":{"patch-level":%d,"patch-sublevel":%d,"some":"data","system-restart-from-boot-id":%q,"daemon-system-restart-at":"%s"},"changes":null,"tasks":null,"last-change-id":0,"last-task-id":0,"last-lane-id":0}`, patch.Level, patch.Sublevel, curBootID, time.Now().UTC().Format(time.RFC3339))) err = ioutil.WriteFile(s.statePath, fakeState, 0600) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) oldRebootNoticeWait := rebootNoticeWait oldRebootRetryWaitTimeout := rebootRetryWaitTimeout @@ -848,19 +846,19 @@ func (s *daemonSuite) TestRestartExpectedRebootIsMissing(c *check.C) { defer cmd.Restore() d := s.newDaemon(c) - c.Check(d.overlord, check.IsNil) - c.Check(d.rebootIsMissing, check.Equals, true) + c.Check(d.overlord, IsNil) + c.Check(d.rebootIsMissing, Equals, true) var n int d.state.Lock() err = d.state.Get("daemon-system-restart-tentative", &n) d.state.Unlock() - c.Check(err, check.IsNil) - c.Check(n, check.Equals, 1) + c.Check(err, IsNil) + c.Check(n, Equals, 1) d.Start() - c.Check(s.notified, check.DeepEquals, []string{"READY=1"}) + c.Check(s.notified, DeepEquals, []string{"READY=1"}) select { case <-d.Dying(): @@ -873,57 +871,57 @@ func (s *daemonSuite) TestRestartExpectedRebootIsMissing(c *check.C) { d.Stop(sigCh) // an immediate shutdown was scheduled again - c.Check(cmd.Calls(), check.DeepEquals, [][]string{ + c.Check(cmd.Calls(), DeepEquals, [][]string{ {"shutdown", "-r", "+0", "reboot scheduled to update the system"}, }) } -func (s *daemonSuite) TestRestartExpectedRebootOK(c *check.C) { +func (s *daemonSuite) TestRestartExpectedRebootOK(c *C) { fakeState := []byte(fmt.Sprintf(`{"data":{"patch-level":%d,"patch-sublevel":%d,"some":"data","system-restart-from-boot-id":%q,"daemon-system-restart-at":"%s"},"changes":null,"tasks":null,"last-change-id":0,"last-task-id":0,"last-lane-id":0}`, patch.Level, patch.Sublevel, "boot-id-0", time.Now().UTC().Format(time.RFC3339))) err := ioutil.WriteFile(s.statePath, fakeState, 0600) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) cmd := testutil.FakeCommand(c, "shutdown", "", true) defer cmd.Restore() d := s.newDaemon(c) - c.Assert(d.overlord, check.NotNil) + c.Assert(d.overlord, NotNil) st := d.overlord.State() st.Lock() defer st.Unlock() var v interface{} // these were cleared - c.Check(st.Get("daemon-system-restart-at", &v), check.Equals, state.ErrNoState) - c.Check(st.Get("system-restart-from-boot-id", &v), check.Equals, state.ErrNoState) + c.Check(st.Get("daemon-system-restart-at", &v), Equals, state.ErrNoState) + c.Check(st.Get("system-restart-from-boot-id", &v), Equals, state.ErrNoState) } -func (s *daemonSuite) TestRestartExpectedRebootGiveUp(c *check.C) { +func (s *daemonSuite) TestRestartExpectedRebootGiveUp(c *C) { // we give up trying to restart the system after 3 retry tentatives curBootID, err := osutil.BootID() - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) fakeState := []byte(fmt.Sprintf(`{"data":{"patch-level":%d,"patch-sublevel":%d,"some":"data","system-restart-from-boot-id":%q,"daemon-system-restart-at":"%s","daemon-system-restart-tentative":3},"changes":null,"tasks":null,"last-change-id":0,"last-task-id":0,"last-lane-id":0}`, patch.Level, patch.Sublevel, curBootID, time.Now().UTC().Format(time.RFC3339))) err = ioutil.WriteFile(s.statePath, fakeState, 0600) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) cmd := testutil.FakeCommand(c, "shutdown", "", true) defer cmd.Restore() d := s.newDaemon(c) - c.Assert(d.overlord, check.NotNil) + c.Assert(d.overlord, NotNil) st := d.overlord.State() st.Lock() defer st.Unlock() var v interface{} // these were cleared - c.Check(st.Get("daemon-system-restart-at", &v), check.Equals, state.ErrNoState) - c.Check(st.Get("system-restart-from-boot-id", &v), check.Equals, state.ErrNoState) - c.Check(st.Get("daemon-system-restart-tentative", &v), check.Equals, state.ErrNoState) + c.Check(st.Get("daemon-system-restart-at", &v), Equals, state.ErrNoState) + c.Check(st.Get("system-restart-from-boot-id", &v), Equals, state.ErrNoState) + c.Check(st.Get("daemon-system-restart-tentative", &v), Equals, state.ErrNoState) } -func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) { +func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *C) { notifySocket := filepath.Join(c.MkDir(), "notify.socket") os.Setenv("NOTIFY_SOCKET", notifySocket) defer os.Setenv("NOTIFY_SOCKET", "") @@ -942,10 +940,10 @@ func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) { time.Sleep(5 * time.Millisecond) } - c.Assert(d.standbyOpinions.CanStandby(), check.Equals, false) + c.Assert(d.standbyOpinions.CanStandby(), Equals, false) f, _ := os.Create(notifySocket) f.Close() - c.Assert(d.standbyOpinions.CanStandby(), check.Equals, true) + c.Assert(d.standbyOpinions.CanStandby(), Equals, true) select { case <-d.Dying(): @@ -954,11 +952,11 @@ func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) { c.Errorf("daemon did not stop after 15s") } err := d.Stop(nil) - c.Check(err, check.Equals, ErrRestartSocket) - c.Check(d.restartSocket, check.Equals, true) + c.Check(err, Equals, ErrRestartSocket) + c.Check(d.restartSocket, Equals, true) } -func (s *daemonSuite) TestRestartIntoSocketModePendingChanges(c *check.C) { +func (s *daemonSuite) TestRestartIntoSocketModePendingChanges(c *C) { os.Setenv("NOTIFY_SOCKET", c.MkDir()) defer os.Setenv("NOTIFY_SOCKET", "") @@ -991,38 +989,38 @@ func (s *daemonSuite) TestRestartIntoSocketModePendingChanges(c *check.C) { chgStatus := chg.Status() st.Unlock() // ensure our change is valid and ready - c.Check(chgStatus, check.Equals, state.DoStatus) + c.Check(chgStatus, Equals, state.DoStatus) case <-time.After(5 * time.Second): c.Errorf("daemon did not stop after 5s") } // when the daemon got a pending change it just restarts err := d.Stop(nil) - c.Check(err, check.IsNil) - c.Check(d.restartSocket, check.Equals, false) + c.Check(err, IsNil) + c.Check(d.restartSocket, Equals, false) } -func (s *daemonSuite) TestConnTrackerCanShutdown(c *check.C) { +func (s *daemonSuite) TestConnTrackerCanShutdown(c *C) { ct := &connTracker{conns: make(map[net.Conn]struct{})} - c.Check(ct.CanStandby(), check.Equals, true) + c.Check(ct.CanStandby(), Equals, true) con := &net.IPConn{} ct.trackConn(con, http.StateActive) - c.Check(ct.CanStandby(), check.Equals, false) + c.Check(ct.CanStandby(), Equals, false) ct.trackConn(con, http.StateIdle) - c.Check(ct.CanStandby(), check.Equals, true) + c.Check(ct.CanStandby(), Equals, true) } -func doTestReq(c *check.C, cmd *Command, mth string) *httptest.ResponseRecorder { +func doTestReq(c *C, cmd *Command, mth string) *httptest.ResponseRecorder { req, err := http.NewRequest(mth, "", nil) - c.Assert(err, check.IsNil) + c.Assert(err, IsNil) req.RemoteAddr = "pid=100;uid=0;socket=;" rec := httptest.NewRecorder() cmd.ServeHTTP(rec, req) return rec } -func (s *daemonSuite) TestDegradedModeReply(c *check.C) { +func (s *daemonSuite) TestDegradedModeReply(c *C) { d := s.newDaemon(c) cmd := &Command{d: d} cmd.GET = func(*Command, *http.Request, *userState) Response { @@ -1037,22 +1035,22 @@ func (s *daemonSuite) TestDegradedModeReply(c *check.C) { // GET is ok even in degraded mode rec := doTestReq(c, cmd, "GET") - c.Check(rec.Code, check.Equals, 200) + c.Check(rec.Code, Equals, 200) // POST is not allowed rec = doTestReq(c, cmd, "POST") - c.Check(rec.Code, check.Equals, 500) + c.Check(rec.Code, Equals, 500) // verify we get the error var v struct{ Result errorResult } - c.Assert(json.NewDecoder(rec.Body).Decode(&v), check.IsNil) - c.Check(v.Result.Message, check.Equals, "foo error") + c.Assert(json.NewDecoder(rec.Body).Decode(&v), IsNil) + c.Check(v.Result.Message, Equals, "foo error") // clean degraded mode d.SetDegradedMode(nil) rec = doTestReq(c, cmd, "POST") - c.Check(rec.Code, check.Equals, 200) + c.Check(rec.Code, Equals, 200) } -func (s *daemonSuite) TestHTTPAPI(c *check.C) { +func (s *daemonSuite) TestHTTPAPI(c *C) { s.httpAddress = ":0" // Go will choose port (use listener.Addr() to find it) d := s.newDaemon(c) d.Init() From a375695db441bdefc71bbe85358cf3eff1e45c68 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Wed, 5 Jul 2023 15:17:28 +1200 Subject: [PATCH 08/12] Add docs about import order to HACKING.md --- HACKING.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/HACKING.md b/HACKING.md index 9ec335851..69f4cf72b 100644 --- a/HACKING.md +++ b/HACKING.md @@ -83,6 +83,39 @@ $ curl --unix-socket ~/pebble/.pebble.socket 'http://localhost/v1/services?names ``` +## Code style + +Pebble imports should be arranged in three groups: +- standard library imports +- third-party / non-Pebble imports +- Pebble imports (i.e. those prefixed with `github.com/canonical/pebble`) + +Imports should be sorted alphabetically within each group. + +We use the [`gopkg.in/check.v1`](https://pkg.go.dev/gopkg.in/check.v1) package for testing. Inside a test file, import this as follows: +```go +. "gopkg.in/check.v1" +``` +so that identifiers from that package will be added to the local namespace. + + +Here is an example of correctly arranged imports: + +```go +import ( + "fmt" + "net" + "os" + + "github.com/gorilla/mux" + . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/systemd" + "github.com/canonical/pebble/internals/testutil" +) +``` + + ## Running the tests Pebble has a suite of Go unit tests, which you can run using the regular `go test` command. To test all packages in the Pebble repository: From 731299e05fe84ccb369fe452cba04d80e7c1b29c Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Wed, 5 Jul 2023 14:58:58 +1200 Subject: [PATCH 09/12] Enforce import order using gci Add linting to the pre-merge checks to enforce Go import groups: - stdlib - 3rd-party / non-Pebble imports - Pebble imports --- .github/.golangci.yml | 14 ++++++++++++++ .github/workflows/lint.yml | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 .github/.golangci.yml create mode 100644 .github/workflows/lint.yml diff --git a/.github/.golangci.yml b/.github/.golangci.yml new file mode 100644 index 000000000..3f49df427 --- /dev/null +++ b/.github/.golangci.yml @@ -0,0 +1,14 @@ +linters: + disable-all: true + enable: + - gci +linters-settings: + gci: + sections: + - standard + - default + - Prefix(github.com/canonical/pebble) +issues: + # these values ensure that all issues will be surfaced + max-issues-per-linter: 0 + max-same-issues: 0 \ No newline at end of file diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 000000000..6122d1eaf --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,30 @@ +name: Lint +on: [push, pull_request] + +jobs: + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - uses: actions/setup-go@v4 + with: + go-mod-file: 'go.mod' + cache: false + + - name: golangci-lint + uses: golangci/golangci-lint-action@v3 + id: lint + with: + version: latest + args: '-c .github/.golangci.yml --out-format=colored-line-number' + skip-cache: true + + - name: Print error message + if: always() && steps.lint.outcome == 'failure' + run: | + echo ' + Linting failed. On your local machine, please run + golangci-lint run -c .github/.golangci.yml --fix + and check in the changes.' + exit 1 \ No newline at end of file From b073926a459e1da26aa9355e2327fa846d3c4857 Mon Sep 17 00:00:00 2001 From: Cristovao Cordeiro Date: Mon, 31 Jul 2023 17:38:49 +0200 Subject: [PATCH 10/12] ci: promote snap to candidate on every release (#255) --- .github/workflows/snap.yml | 61 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/.github/workflows/snap.yml b/.github/workflows/snap.yml index dc3cc9fed..78a6a5bdb 100644 --- a/.github/workflows/snap.yml +++ b/.github/workflows/snap.yml @@ -1,6 +1,13 @@ name: Pebble snap -on: [pull_request] +on: + pull_request: + branches: [master] + release: + types: [published] + +env: + SNAP_NAME: pebble jobs: build: @@ -27,6 +34,8 @@ jobs: test: runs-on: ubuntu-latest needs: [build] + outputs: + pebble-version: ${{ steps.install-pebble.outputs.version }} steps: - uses: actions/download-artifact@v3 @@ -34,12 +43,58 @@ jobs: name: ${{ needs.build.outputs.pebble-snap }} - name: Install the Pebble snap + id: install-pebble run: | + set -ex # Install the Pebble snap from the artifact built in the previous job sudo snap install --dangerous --classic ${{ needs.build.outputs.pebble-snap }} # Make sure Pebble is installed - pebble version + echo "version=$(pebble version --client)" >> "$GITHUB_OUTPUT" - name: Run smoke test - run: pebble enter exec echo Hello | grep Hello + run: pebble enter --create-dirs exec echo Hello | grep Hello + + promote: + if: ${{ github.event_name == 'release' }} + runs-on: ubuntu-latest + needs: [test] + strategy: + fail-fast: false + matrix: + arch: [amd64, arm64, ppc64el, armhf, s390x] + env: + TRACK: latest + DEFAULT_RISK: edge + TO_RISK: candidate + steps: + - name: Install Snapcraft + run: sudo snap install snapcraft --classic + + - name: Wait for ${{ needs.test.outputs.pebble-version }} to be released + env: + SNAPCRAFT_STORE_CREDENTIALS: ${{ secrets.SNAPCRAFT_STORE_CREDENTIALS }} + run: | + while ! `snapcraft status ${{ env.SNAP_NAME }} --track ${{ env.TRACK }} --arch ${{ matrix.arch }} \ + | grep "${{ env.DEFAULT_RISK }}" \ + | awk -F' ' '{print $2}' \ + | grep -Fxq "${{ needs.test.outputs.pebble-version }}"`; do + echo "[${{ matrix.arch }}] Waiting for ${{ needs.test.outputs.pebble-version }} \ + to be released to ${{ env.TRACK }}/${{ env.DEFAULT_RISK }}..." + sleep 10 + done + + # It would be easier to use `snapcraft promote`, but there's an error when trying + # to avoid the prompt with the "--yes" option: + # > 'latest/edge' is not a valid set value for --from-channel when using --yes. + - name: Promote ${{ needs.test.outputs.pebble-version }} (${{ matrix.arch }}) to ${{ env.TO_RISK }} + env: + SNAPCRAFT_STORE_CREDENTIALS: ${{ secrets.SNAPCRAFT_STORE_CREDENTIALS }} + run: | + revision="$(snapcraft status ${{ env.SNAP_NAME }} \ + --track ${{ env.TRACK }} --arch ${{ matrix.arch }} \ + | grep "${{ env.DEFAULT_RISK }}" | awk -F' ' '{print $3}')" + + snapcraft release ${{ env.SNAP_NAME }} \ + $revision \ + ${{ env.TRACK }}/${{ env.TO_RISK }} From 523aa06472b14010630a4f5b65551b1a879a3fe3 Mon Sep 17 00:00:00 2001 From: Mariyan Dimitrov Date: Tue, 1 Aug 2023 12:23:41 +0300 Subject: [PATCH 11/12] ci(secscan): Introduce security scanning (#196) Co-authored-by: Jon Seager --- .github/.trivyignore | 6 ++++++ .github/trivy.yaml | 4 ++++ .github/workflows/scanning.yml | 22 ++++++++++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 .github/.trivyignore create mode 100644 .github/trivy.yaml create mode 100644 .github/workflows/scanning.yml diff --git a/.github/.trivyignore b/.github/.trivyignore new file mode 100644 index 000000000..0d0839f3a --- /dev/null +++ b/.github/.trivyignore @@ -0,0 +1,6 @@ +# ignore known CVEs that are not backported before Go 1.17 + +CVE-2022-41721 +CVE-2022-41717 +CVE-2022-41723 +CVE-2022-32149 diff --git a/.github/trivy.yaml b/.github/trivy.yaml new file mode 100644 index 000000000..7337ce08c --- /dev/null +++ b/.github/trivy.yaml @@ -0,0 +1,4 @@ +timeout: 20m +scan: + offline-scan: true +ignore-file: .github/.trivyignore diff --git a/.github/workflows/scanning.yml b/.github/workflows/scanning.yml new file mode 100644 index 000000000..83a362333 --- /dev/null +++ b/.github/workflows/scanning.yml @@ -0,0 +1,22 @@ +name: Vulnerability scanning + +on: + push: + branches: [master] + pull_request: + branches: [master] + +jobs: + scan: + name: Scan for known vulnerabilities + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v2 + + - name: Run Github Trivy FS Action + uses: aquasecurity/trivy-action@master + with: + scan-type: 'fs' + scan-ref: '.' + trivy-config: .github/trivy.yaml From f3d036c7619864fcaf374a0124b17fa83ab0a477 Mon Sep 17 00:00:00 2001 From: Frederik Lotter Date: Tue, 1 Aug 2023 11:39:35 +0200 Subject: [PATCH 12/12] testutil: support empty string args in fakecommand (#244) --- internals/testutil/exec.go | 9 ++++----- internals/testutil/exec_test.go | 6 ++++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internals/testutil/exec.go b/internals/testutil/exec.go index 113f7cebc..86245b0e0 100644 --- a/internals/testutil/exec.go +++ b/internals/testutil/exec.go @@ -92,7 +92,7 @@ type FakeCmd struct { // faking commands that need "\n" in their args (like zenity) // we use the following convention: // - generate \0 to separate args -// - generate \0\0 to separate commands +// - generate \0\f\n\r magic sequence to separate commands var scriptTpl = `#!/bin/bash printf "%%s" "$(basename "$0")" >> %[1]q printf '\0' >> %[1]q @@ -102,7 +102,7 @@ for arg in "$@"; do printf '\0' >> %[1]q done -printf '\0' >> %[1]q +printf '\f\n\r' >> %[1]q %s ` @@ -176,12 +176,11 @@ func (cmd *FakeCmd) Calls() [][]string { if err != nil { panic(err) } - logContent := strings.TrimSuffix(string(raw), "\000") + logContent := strings.TrimSuffix(string(raw), "\000\f\n\r") allCalls := [][]string{} - calls := strings.Split(logContent, "\000\000") + calls := strings.Split(logContent, "\000\f\n\r") for _, call := range calls { - call = strings.TrimSuffix(call, "\000") allCalls = append(allCalls, strings.Split(call, "\000")) } return allCalls diff --git a/internals/testutil/exec_test.go b/internals/testutil/exec_test.go index 47c35a777..18bf08d0b 100644 --- a/internals/testutil/exec_test.go +++ b/internals/testutil/exec_test.go @@ -34,9 +34,15 @@ func (s *fakeCommandSuite) TestFakeCommand(c *check.C) { c.Assert(err, check.IsNil) err = exec.Command("cmd", "second-run", "--arg1", "arg2", "a %s").Run() c.Assert(err, check.IsNil) + err = exec.Command("cmd", "third-run", "--arg1", "arg2", "").Run() + c.Assert(err, check.IsNil) + err = exec.Command("cmd", "forth-run", "--arg1", "arg2", "", "a %s").Run() + c.Assert(err, check.IsNil) c.Assert(fake.Calls(), check.DeepEquals, [][]string{ {"cmd", "first-run", "--arg1", "arg2", "a space"}, {"cmd", "second-run", "--arg1", "arg2", "a %s"}, + {"cmd", "third-run", "--arg1", "arg2", ""}, + {"cmd", "forth-run", "--arg1", "arg2", "", "a %s"}, }) }