Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "service context" support for exec and health checks #246

Merged
merged 12 commits into from
Jul 11, 2023
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,13 @@ checks:
# directly, not interpreted by a shell.
command: <commmand>

# (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.
context: <service-name>

# (Optional) A list of key/value pairs defining environment
# variables that should be set when running the command.
environment:
Expand Down
16 changes: 12 additions & 4 deletions client/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Context 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.
Expand Down Expand Up @@ -75,6 +81,7 @@ type ExecOptions struct {

type execPayload struct {
Command []string `json:"command"`
Context string `json:"context,omitempty"`
Environment map[string]string `json:"environment,omitempty"`
WorkingDir string `json:"working-dir,omitempty"`
Timeout string `json:"timeout,omitempty"`
Expand Down Expand Up @@ -123,6 +130,7 @@ func (client *Client) Exec(opts *ExecOptions) (*ExecProcess, error) {
}
payload := execPayload{
Command: opts.Command,
Context: opts.Context,
Environment: opts.Environment,
WorkingDir: opts.WorkingDir,
Timeout: timeoutStr,
Expand Down
3 changes: 3 additions & 0 deletions internals/cli/cmd_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand All @@ -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)",
Expand Down Expand Up @@ -144,6 +146,7 @@ func (cmd *cmdExec) Execute(args []string) error {

opts := &client.ExecOptions{
Command: command,
Context: cmd.Context,
Environment: env,
WorkingDir: cmd.WorkingDir,
Timeout: cmd.Timeout,
Expand Down
25 changes: 22 additions & 3 deletions internals/daemon/api_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@ 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"`
Context string `json:"context"`
Environment map[string]string `json:"environment"`
WorkingDir string `json:"working-dir"`
Timeout string `json:"timeout"`
Expand Down Expand Up @@ -67,8 +69,25 @@ func v1PostExec(c *Command, req *http.Request, _ *userState) Response {
return statusBadRequest("%v", err)
}

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.Context, 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)
}
Expand All @@ -79,8 +98,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,
Expand Down
49 changes: 49 additions & 0 deletions internals/daemon/api_exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand Down Expand Up @@ -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"},
Context: "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"},
Context: "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)
Expand Down
29 changes: 21 additions & 8 deletions internals/overlord/checkstate/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not about this PR, but looking at this logic makes me think there's a bug here that needs to be addressed separately. The cancel here is a request. Right above we're calling cancel() as if this would be a synchronous operation, but it's not. The goroutine may take its time to run. This may cause very awkward outcomes, such as an old check running after a new check. We need to use tomb or something similar to make the stopping of checkers become synchronous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! @flotter Actually fixed this exact bug a couple of weeks ago. It was causing intermittent issues in CI, but of course as you point out it could be a problem in production too.

@flotter Note that in future (perhaps for your servstate fix :-) we should probably use tombs for this rather than wait groups, to be consistent with other parts of Pebble. Pebble already uses tombs extensively elsewhere, so it won't be a new dependency. I should have mentioned that in the code review.

action: m.callFailureHandlers,
Expand All @@ -83,7 +83,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{
Expand All @@ -100,15 +100,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.Context, overrides)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very clean implementation. Thank you.

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:
Expand Down
73 changes: 70 additions & 3 deletions internals/overlord/checkstate/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,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")
Expand All @@ -304,7 +304,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")
Expand All @@ -323,7 +323,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")
Expand All @@ -334,3 +334,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",
Context: "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",
Context: "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")
}
Loading