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

V2 command work dir #1061

Merged
merged 13 commits into from
Sep 7, 2022
5 changes: 5 additions & 0 deletions internal/pkg/agent/application/paths/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ func Data() string {
return filepath.Join(Top(), "data")
}

// Run returns the run directory for Agent
func Run() string {
return filepath.Join(Home(), "run")
}

// Components returns the component directory for Agent
func Components() string {
return filepath.Join(Home(), "components")
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/agent/application/periodic.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (p *periodic) work() error {
return nil
}

p.log.Info("No configuration change")
p.log.Debug("No configuration change")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return nil
}

Expand Down
5 changes: 3 additions & 2 deletions internal/pkg/agent/cmd/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
"github.com/elastic/elastic-agent/internal/pkg/agent/install"
"github.com/elastic/elastic-agent/internal/pkg/cli"
"github.com/elastic/elastic-agent/pkg/utils"
)

func newInstallCommandWithArgs(_ []string, streams *cli.IOStreams) *cobra.Command {
Expand Down Expand Up @@ -48,12 +49,12 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command) error {
return err
}

isAdmin, err := install.HasRoot()
isAdmin, err := utils.HasRoot()
if err != nil {
return fmt.Errorf("unable to perform install command while checking for administrator rights, %w", err)
}
if !isAdmin {
return fmt.Errorf("unable to perform install command, not executed with %s permissions", install.PermissionUser)
return fmt.Errorf("unable to perform install command, not executed with %s permissions", utils.PermissionUser)
}
status, reason := install.Status()
force, _ := cmd.Flags().GetBool("force")
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/agent/cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func run(override cfgOverrider, modifiers ...component.PlatformModifier) error {
}

if allowEmptyPgp, _ := release.PGP(); allowEmptyPgp {
logger.Info("Artifact has been built with security disabled. Elastic Agent will not verify signatures of the artifacts.")
logger.Info("Elastic Agent has been built with security disabled. Elastic Agent will not verify signatures of upgrade artifact.")
}

execPath, err := reexecPath()
Expand Down
5 changes: 3 additions & 2 deletions internal/pkg/agent/cmd/uninstall.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
"github.com/elastic/elastic-agent/internal/pkg/agent/install"
"github.com/elastic/elastic-agent/internal/pkg/cli"
"github.com/elastic/elastic-agent/pkg/utils"
)

func newUninstallCommandWithArgs(_ []string, streams *cli.IOStreams) *cobra.Command {
Expand All @@ -38,12 +39,12 @@ Unless -f is used this command will ask confirmation before performing removal.
}

func uninstallCmd(streams *cli.IOStreams, cmd *cobra.Command) error {
isAdmin, err := install.HasRoot()
isAdmin, err := utils.HasRoot()
if err != nil {
return fmt.Errorf("unable to perform command while checking for administrator rights, %w", err)
}
if !isAdmin {
return fmt.Errorf("unable to perform command, not executed with %s permissions", install.PermissionUser)
return fmt.Errorf("unable to perform command, not executed with %s permissions", utils.PermissionUser)
}
status, reason := install.Status()
if status == install.NotInstalled {
Expand Down
31 changes: 30 additions & 1 deletion pkg/component/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ package component

import (
"fmt"
"os"
"strings"

"github.com/elastic/elastic-agent/pkg/utils"

Copy link
Contributor

Choose a reason for hiding this comment

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

are these newlines coming from autoimport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes its nice, but also a pain. Fixed.

"github.com/elastic/elastic-agent-client/v7/pkg/client"
"github.com/elastic/elastic-agent-client/v7/pkg/proto"

Expand Down Expand Up @@ -87,6 +90,10 @@ func (r *RuntimeSpecs) ToComponents(policy map[string]interface{}) ([]Component,
}

// set the runtime variables that are available in the input specification runtime checks
hasRoot, err := utils.HasRoot()
if err != nil {
return nil, err
}
vars, err := transpiler.NewVars(map[string]interface{}{
"runtime": map[string]interface{}{
"platform": r.platform.String(),
Expand All @@ -96,6 +103,11 @@ func (r *RuntimeSpecs) ToComponents(policy map[string]interface{}) ([]Component,
"major": r.platform.Major,
"minor": r.platform.Minor,
},
"user": map[string]interface{}{
cmacknz marked this conversation as resolved.
Show resolved Hide resolved
"uid": os.Geteuid(),
"gid": os.Getgid(),
Copy link
Contributor

Choose a reason for hiding this comment

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

dont we want Getegid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we do. Changed it.

"root": hasRoot,
},
}, nil)
if err != nil {
return nil, err
Expand Down Expand Up @@ -260,12 +272,16 @@ func toIntermediate(policy map[string]interface{}) (map[string]outputI, error) {
}
idRaw, ok := input[idKey]
if !ok {
return nil, fmt.Errorf("invalid 'inputs.%d', 'id' missing", idx)
// no ID; fallback to type
idRaw = t
}
id, ok := idRaw.(string)
if !ok {
return nil, fmt.Errorf("invalid 'inputs.%d.id', expected a string not a %T", idx, idRaw)
}
if hasDuplicate(outputsMap, id) {
return nil, fmt.Errorf("invalid 'inputs.%d.id', has a duplicate id (id is required to be unique)", idx)
blakerouse marked this conversation as resolved.
Show resolved Hide resolved
}
outputName := "default"
if outputRaw, ok := input[useKey]; ok {
outputNameVal, ok := outputRaw.(string)
Expand Down Expand Up @@ -346,6 +362,19 @@ func validateRuntimeChecks(spec *InputSpec, store eql.VarStore) error {
return nil
}

func hasDuplicate(outputsMap map[string]outputI, id string) bool {
for _, o := range outputsMap {
for _, i := range o.inputs {
for _, j := range i {
if j.id == id {
return true
}
}
}
}
return false
}

func getLogLevel(val map[string]interface{}) (client.UnitLogLevel, error) {
const logLevelKey = "log_level"

Expand Down
7 changes: 5 additions & 2 deletions pkg/component/component_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func TestToComponents(t *testing.T) {
Err: "invalid 'inputs.0.type', expected a string not a int",
},
{
Name: "Invalid: inputs entry missing id",
Name: "Invalid: inputs entry duplicate because of missing id",
Platform: linuxAMD64Platform,
Policy: map[string]interface{}{
"outputs": map[string]interface{}{
Expand All @@ -184,9 +184,12 @@ func TestToComponents(t *testing.T) {
map[string]interface{}{
"type": "filestream",
},
map[string]interface{}{
"type": "filestream",
},
},
},
Err: "invalid 'inputs.0', 'id' missing",
Err: "invalid 'inputs.1.id', has a duplicate id (id is required to be unique)",
},
{
Name: "Invalid: inputs entry id not a string",
Expand Down
128 changes: 94 additions & 34 deletions pkg/component/runtime/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ import (
"fmt"
"os"
"os/exec"
"path/filepath"
"time"

"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
"github.com/elastic/elastic-agent/pkg/utils"

"github.com/elastic/elastic-agent-client/v7/pkg/client"
"github.com/elastic/elastic-agent/pkg/component"
"github.com/elastic/elastic-agent/pkg/core/process"
Expand All @@ -22,6 +26,11 @@ type actionMode int
const (
actionStart = actionMode(0)
actionStop = actionMode(1)

runDirMod = 0770

envAgentComponentID = "AGENT_COMPONENT_ID"
envAgentComponentInputType = "AGENT_COMPONENT_INPUT_TYPE"
)

type procState struct {
Expand Down Expand Up @@ -69,6 +78,7 @@ func NewCommandRuntime(comp component.Component) (ComponentRuntime, error) {
// ever be called again.
func (c *CommandRuntime) Run(ctx context.Context, comm Communicator) error {
checkinPeriod := c.current.Spec.Spec.Command.Timeouts.Checkin
restartPeriod := c.current.Spec.Spec.Command.Timeouts.Restart
Copy link
Contributor

Choose a reason for hiding this comment

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

can we run into nil pointer somewhere here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is checked in the initialization of the struct. The depth after that does not have any pointers.

if comp.Spec.Spec.Command == nil {
		return nil, errors.New("must have command defined in specification")
	}

c.forceCompState(client.UnitStateStarting, "Starting")
t := time.NewTicker(checkinPeriod)
defer t.Stop()
Expand All @@ -81,25 +91,22 @@ func (c *CommandRuntime) Run(ctx context.Context, comm Communicator) error {
switch as {
case actionStart:
if err := c.start(comm); err != nil {
c.forceCompState(client.UnitStateFailed, err.Error())
c.forceCompState(client.UnitStateFailed, fmt.Sprintf("Failed: %s", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add even more context?

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

besides, it's already an error, just Failed is redundant.
another thing, it'd be better to wrap the error using fmt.Errorf and %w.
If not wrapping, then, perhaps, %v would be better as it might print more information about the error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the context should already be coming from the c.start(). It adds extra data in those error messages.

}
t.Reset(checkinPeriod)
case actionStop:
if err := c.stop(ctx); err != nil {
c.forceCompState(client.UnitStateFailed, err.Error())
c.forceCompState(client.UnitStateFailed, fmt.Sprintf("Failed: %s", err))
blakerouse marked this conversation as resolved.
Show resolved Hide resolved
}
}
case ps := <-c.procCh:
// ignores old processes
if ps.proc == c.proc {
c.proc = nil
if c.handleProc(ps.state) {
// start again
if err := c.start(comm); err != nil {
c.forceCompState(client.UnitStateFailed, err.Error())
}
// start again after restart period
t.Reset(restartPeriod)
}
t.Reset(checkinPeriod)
}
case newComp := <-c.compCh:
sendExpected := c.state.syncExpected(&newComp)
Expand Down Expand Up @@ -140,30 +147,37 @@ func (c *CommandRuntime) Run(ctx context.Context, comm Communicator) error {
c.sendObserved()
}
case <-t.C:
if c.proc != nil && c.actionState == actionStart {
// running and should be running
now := time.Now().UTC()
if c.lastCheckin.IsZero() {
// never checked-in
c.missedCheckins++
} else if now.Sub(c.lastCheckin) > checkinPeriod {
// missed check-in during required period
c.missedCheckins++
} else if now.Sub(c.lastCheckin) <= checkinPeriod {
c.missedCheckins = 0
}
if c.missedCheckins == 0 {
c.compState(client.UnitStateHealthy)
} else if c.missedCheckins > 0 && c.missedCheckins < maxCheckinMisses {
c.compState(client.UnitStateDegraded)
} else if c.missedCheckins >= maxCheckinMisses {
// something is wrong; the command should be checking in
//
// at this point it is assumed the sub-process has locked up and will not respond to a nice
// termination signal, so we jump directly to killing the process
msg := fmt.Sprintf("Failed: pid '%d' missed %d check-ins and will be killed", c.proc.PID, maxCheckinMisses)
c.forceCompState(client.UnitStateFailed, msg)
_ = c.proc.Kill() // watcher will handle it from here
if c.actionState == actionStart {
if c.proc == nil {
// not running, but should be running
if err := c.start(comm); err != nil {
c.forceCompState(client.UnitStateFailed, fmt.Sprintf("Failed: %s", err))
Copy link
Member

Choose a reason for hiding this comment

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

Same as the others

}
} else {
Comment on lines +154 to +159
Copy link
Member

Choose a reason for hiding this comment

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

[Suggestion]
Use continue instead of the else branch. Usually it makes the code easier to read and understand

// running and should be running
now := time.Now().UTC()
if c.lastCheckin.IsZero() {
// never checked-in
c.missedCheckins++
} else if now.Sub(c.lastCheckin) > checkinPeriod {
// missed check-in during required period
c.missedCheckins++
Comment on lines +162 to +167
Copy link
Member

Choose a reason for hiding this comment

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

[Suggestion]
Use just one if statement as they have the same outcome. You can comment on each condition independently

Suggested change
if c.lastCheckin.IsZero() {
// never checked-in
c.missedCheckins++
} else if now.Sub(c.lastCheckin) > checkinPeriod {
// missed check-in during required period
c.missedCheckins++
if c.lastCheckin.IsZero() || // never checked-in
now.Sub(c.lastCheckin) > checkinPeriod { // missed check-in during required period
// missed check-in during required period
c.missedCheckins++

} else if now.Sub(c.lastCheckin) <= checkinPeriod {
c.missedCheckins = 0
}
if c.missedCheckins == 0 {
c.compState(client.UnitStateHealthy)
Copy link
Member

Choose a reason for hiding this comment

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

[Suggestion]
What is compState? compute state or component state? I'd suggest to use the full name

} else if c.missedCheckins > 0 && c.missedCheckins < maxCheckinMisses {
c.compState(client.UnitStateDegraded)
} else if c.missedCheckins >= maxCheckinMisses {
// something is wrong; the command should be checking in
//
// at this point it is assumed the sub-process has locked up and will not respond to a nice
// termination signal, so we jump directly to killing the process
msg := fmt.Sprintf("Failed: pid '%d' missed %d check-ins and will be killed", c.proc.PID, maxCheckinMisses)
Copy link
Member

Choose a reason for hiding this comment

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

[Suggestion]
Is there any information about the name of the component? if so, it'd be good to add it to the error message

c.forceCompState(client.UnitStateFailed, msg)
_ = c.proc.Kill() // watcher will handle it from here
Copy link
Member

Choose a reason for hiding this comment

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

if there a logger here? if so, I'd log the error

}
}
}
}
Expand Down Expand Up @@ -243,11 +257,26 @@ func (c *CommandRuntime) start(comm Communicator) error {
return nil
}
cmdSpec := c.current.Spec.Spec.Command
env := make([]string, 0, len(cmdSpec.Env))
env := make([]string, 0, len(cmdSpec.Env)+2)
cmacknz marked this conversation as resolved.
Show resolved Hide resolved
for _, e := range cmdSpec.Env {
env = append(env, fmt.Sprintf("%s=%s", e.Name, e.Value))
}
proc, err := process.Start(c.current.Spec.BinaryPath, os.Geteuid(), os.Getgid(), cmdSpec.Args, env, attachOutErr)
env = append(env, fmt.Sprintf("%s=%s", envAgentComponentID, c.current.ID))
env = append(env, fmt.Sprintf("%s=%s", envAgentComponentInputType, c.current.Spec.InputType))
uid, gid := os.Geteuid(), os.Getgid()
Copy link
Contributor

Choose a reason for hiding this comment

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

again: getegid vs getgid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

workDir, err := c.workDir(uid, gid)
cmacknz marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

more context please.

Copy link
Member

Choose a reason for hiding this comment

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

+1
please wrap the error with more context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

workDir already adds the context information to the error.

}
path, err := filepath.Abs(c.current.Spec.BinaryPath)
if err != nil {
return fmt.Errorf("failed to determine absolute path: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

+1

}
err = utils.HasStrictExecPerms(path, uid)
if err != nil {
return fmt.Errorf("execution of component prevented: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

[Supperstion]
unless HasStrictExecPerms will return a descriptive error, it'd be better to say why the execution was prevented.

}
proc, err := process.Start(path, uid, gid, cmdSpec.Args, env, attachOutErr, dirPath(workDir))
if err != nil {
return err
blakerouse marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -261,7 +290,14 @@ func (c *CommandRuntime) start(comm Communicator) error {

func (c *CommandRuntime) stop(ctx context.Context) error {
if c.proc == nil {
// already stopped
// already stopped ensure that state of the component is also stopped
blakerouse marked this conversation as resolved.
Show resolved Hide resolved
if c.state.State != client.UnitStateStopped {
if c.state.State == client.UnitStateFailed {
c.forceCompState(client.UnitStateStopped, "Stopped: never started successfully")
} else {
c.forceCompState(client.UnitStateStopped, "Stopped: already stopped")
}
}
return nil
}
cmdSpec := c.current.Spec.Spec.Command
Expand Down Expand Up @@ -314,8 +350,32 @@ func (c *CommandRuntime) handleProc(state *os.ProcessState) bool {
return false
}

func (c *CommandRuntime) workDir(uid int, gid int) (string, error) {
path := filepath.Join(paths.Run(), c.current.ID)
err := os.MkdirAll(path, runDirMod)
if err != nil {
return "", fmt.Errorf("failed to create path: %s, %w", path, err)
blakerouse marked this conversation as resolved.
Show resolved Hide resolved
}
err = os.Chown(path, uid, gid)
if err != nil {
return "", fmt.Errorf("failed to chown %s: %w", path, err)
}
err = os.Chmod(path, runDirMod)
if err != nil {
return "", fmt.Errorf("failed to chmod: %s, %w", path, err)
blakerouse marked this conversation as resolved.
Show resolved Hide resolved
}
return path, nil
}

func attachOutErr(cmd *exec.Cmd) error {
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
return nil
}

func dirPath(path string) process.Option {
return func(cmd *exec.Cmd) error {
cmd.Dir = path
return nil
}
}
Loading