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 the given service,
# that is, 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
8 changes: 8 additions & 0 deletions client/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ type ExecOptions struct {
// Required: command and arguments (first element is the executable).
Command []string

// Optional service name to run the command in the context of this service,
benhoyt marked this conversation as resolved.
Show resolved Hide resolved
// that is, inherit its environment variables, user/group settings,
// 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

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": "Run the command in the context of this service (overridden by -w, --env, --user, --uid, --group, --gid)",
benhoyt marked this conversation as resolved.
Show resolved Hide resolved
"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