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.

Sign up for GitHub

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

Jump to bottom

Fix: Agent failed to upgrade from 8.4.2 to 8.5.0 BC1 for MAC 12 agent using agent binary. #1392

Merged
merged 2 commits into from
Oct 3, 2022
Merged

Fix: Agent failed to upgrade from 8.4.2 to 8.5.0 BC1 for MAC 12 agent using agent binary. #1392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cb6321
Revert "Revert: Bundle elastic-agent.app for MacOS… #714 (#1387)"
aleksmaus Sep 30, 2022
07944c3
Fix upgrade for Mac OS
aleksmaus Oct 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev-tools/packaging/files/darwin/PkgInfo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
APPL????
233 changes: 133 additions & 100 deletions dev-tools/packaging/packages.yml
View file
Open in desktop

Large diffs are not rendered by default.

20 changes: 20 additions & 0 deletions dev-tools/packaging/templates/darwin/Info.plist.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>CFBundleExecutable</key>
<string>elastic-agent</string>
<key>CFBundleIdentifier</key>
<string>co.elastic.elastic-agent</string>
<key>CFBundleInfoDictionaryVersion</key>
<string>6.0</string>
<key>CFBundleName</key>
<string>elastic-agent</string>
<key>CFBundlePackageType</key>
<string>APPL</string>
<key>CFBundleShortVersionString</key>
<string>{{ beat_version }}</string>
<key>CFBundleVersion</key>
<string>{{ beat_version }}</string>
</dict>
</plist>
11 changes: 11 additions & 0 deletions dev-tools/packaging/templates/darwin/elastic-agent.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/sh
# Fix up the symlink and exit

set -e

symlink="/Library/Elastic/Agent/elastic-agent"

if test -L "$symlink"; then
ln -sfn "data/elastic-agent-{{ commit_short }}/elastic-agent.app/Contents/MacOS/elastic-agent" "$symlink"
fi

23 changes: 20 additions & 3 deletions internal/pkg/agent/application/info/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,27 @@ import (
"fmt"
"os"
"path/filepath"
"runtime"
"strings"

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

const (
darwin = "darwin"
)

// RunningInstalled returns true when executing Agent is the installed Agent.
//
// This verifies the running executable path based on hard-coded paths
// for each platform type.
func RunningInstalled() bool {
expected := filepath.Join(paths.InstallPath, paths.BinaryName)
expectedPaths := []string{filepath.Join(paths.InstallPath, paths.BinaryName)}
if runtime.GOOS == darwin {
// For the symlink on darwin the execPath is /usr/local/bin/elastic-agent
expectedPaths = append(expectedPaths, paths.ShellWrapperPath)
}
execPath, _ := os.Executable()
execPath, _ = filepath.Abs(execPath)
execName := filepath.Base(execPath)
Expand All @@ -28,13 +37,21 @@ func RunningInstalled() bool {
// executable path is being reported as being down inside of data path
// move up to directories to perform the comparison
execDir = filepath.Dir(filepath.Dir(execDir))
if runtime.GOOS == darwin {
execDir = filepath.Dir(filepath.Dir(filepath.Dir(execDir)))
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here documenting that you are stripping off the elastic-agent.app/Contents/MacOS paths? Either that or make a small helper function for this, I see the same logic in paths/common.go.

Those who haven't memorized the application bundle structure on Mac might not recognize what this is doing.

}
execPath = filepath.Join(execDir, execName)
}
return paths.ArePathsEqual(expected, execPath)
for _, expected := range expectedPaths {
if paths.ArePathsEqual(expected, execPath) {
return true
}
}
return false
}

// IsInsideData returns true when the exePath is inside of the current Agents data path.
func IsInsideData(exePath string) bool {
expectedPath := filepath.Join("data", fmt.Sprintf("elastic-agent-%s", release.ShortCommit()))
expectedPath := paths.BinaryDir(filepath.Join("data", fmt.Sprintf("elastic-agent-%s", release.ShortCommit())))
return strings.HasSuffix(exePath, expectedPath)
}
48 changes: 48 additions & 0 deletions internal/pkg/agent/application/info/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package info

import (
"fmt"
"path/filepath"
"testing"

"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
"github.com/elastic/elastic-agent/internal/pkg/release"
"github.com/google/go-cmp/cmp"
)

func TestIsInsideData(t *testing.T) {
validExePath := paths.BinaryDir(filepath.Join("data", fmt.Sprintf("elastic-agent-%s", release.ShortCommit())))
Copy link
Member

Choose a reason for hiding this comment

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

Should you explicitly include a case that contains the MacOS bundle path?


tests := []struct {
name string
exePath string
res bool
}{
{
name: "empty",
},
{
name: "invalid",
exePath: "data/elastic-agent",
},
{
name: "valid",
exePath: validExePath,
res: true,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
res := IsInsideData(tc.exePath)
diff := cmp.Diff(tc.res, res)
if diff != "" {
t.Error(diff)
}
})
}
}
31 changes: 28 additions & 3 deletions internal/pkg/agent/application/paths/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"os"
"path/filepath"
"runtime"
"strings"
"sync"

Expand All @@ -21,6 +22,8 @@ const (
// AgentLockFileName is the name of the overall Elastic Agent file lock.
AgentLockFileName = "agent.lock"
tempSubdir = "tmp"

darwin = "darwin"
)

var (
Expand Down Expand Up @@ -68,7 +71,9 @@ func SetTop(path string) {
func TempDir() string {
tmpDir := filepath.Join(Data(), tempSubdir)
tmpCreator.Do(func() {
// create tempdir as it probably don't exists
// Create tempdir as it probably don't exists.
// The error was not checked here before and the linter is not happy about it.
// Changing this now would lead to the wide change scope that intended at the moment, so just making the linter happy for now.
_ = os.MkdirAll(tmpDir, 0750)
})
return tmpDir
Expand Down Expand Up @@ -172,10 +177,15 @@ func SetInstall(path string) {
// initialTop returns the initial top-level path for the binary
//
// When nested in top-level/data/elastic-agent-${hash}/ the result is top-level/.
// The agent fexecutable for MacOS is wrappend in the bundle, so the path to the binary is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The agent fexecutable for MacOS is wrappend in the bundle, so the path to the binary is
// The agent executable for MacOS is wrapped in the bundle, so the path to the binary is

// top-level/data/elastic-agent-${hash}/elastic-agent.app/Contents/MacOS
func initialTop() string {
exePath := retrieveExecutablePath()
if insideData(exePath) {
return filepath.Dir(filepath.Dir(exePath))
exePath = filepath.Dir(filepath.Dir(exePath))
if runtime.GOOS == darwin {
exePath = filepath.Dir(filepath.Dir(filepath.Dir(exePath)))
}
}
return exePath
}
Expand All @@ -195,6 +205,21 @@ func retrieveExecutablePath() string {

// insideData returns true when the exePath is inside of the current Agents data path.
func insideData(exePath string) bool {
expectedPath := filepath.Join("data", fmt.Sprintf("elastic-agent-%s", release.ShortCommit()))
expectedPath := BinaryDir(filepath.Join("data", fmt.Sprintf("elastic-agent-%s", release.ShortCommit())))
return strings.HasSuffix(exePath, expectedPath)
}

// BinaryDir returns the application binary directory
// For macOS it appends the path inside of the app bundle
// For other platforms it returns the same dir
func BinaryDir(baseDir string) string {
if runtime.GOOS == darwin {
baseDir = filepath.Join(baseDir, "elastic-agent.app", "Contents", "MacOS")
}
return baseDir
}

// BinaryPath returns the application binary path that is concatenation of the directory and the agentName
func BinaryPath(baseDir, agentName string) string {
return filepath.Join(BinaryDir(baseDir), agentName)
}
4 changes: 1 addition & 3 deletions internal/pkg/agent/application/upgrade/service_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"fmt"
"os"
"os/exec"
"path/filepath"
"regexp"
"strconv"
"strings"
Expand Down Expand Up @@ -115,8 +114,7 @@ func (p *darwinPidProvider) piderFromCmd(ctx context.Context, name string, args
}

func invokeCmd(topPath string) *exec.Cmd {
homeExePath := filepath.Join(topPath, agentName)

homeExePath := paths.BinaryPath(topPath, agentName)
cmd := exec.Command(homeExePath, watcherSubcommand,
"--path.config", paths.Config(),
"--path.home", paths.Top(),
Expand Down
3 changes: 2 additions & 1 deletion internal/pkg/agent/application/upgrade/step_relink.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ func ChangeSymlink(ctx context.Context, log *logger.Logger, targetHash string) e
hashedDir := fmt.Sprintf("%s-%s", agentName, targetHash)

symlinkPath := filepath.Join(paths.Top(), agentName)
newPath := filepath.Join(paths.Top(), "data", hashedDir, agentName)

newPath := paths.BinaryPath(filepath.Join(paths.Top(), "data", hashedDir), agentName)
Copy link
Member

Choose a reason for hiding this comment

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

If we forget paths.BinaryPath here (or elsewhere) to wrap this is there a test that fails? If not, is there a unit test we could write?

I am concerned the need for this won't be obvious to the next engineer to touch this code, and it could be forgotten or removed in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I see the same thing in service_darwin.go, so this seems to be necessary in lots of places now but I am not sure if anything enforces the need for it.

func invokeCmd(topPath string) *exec.Cmd {
	homeExePath := paths.BinaryPath(topPath, agentName)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah currently the logic is scattered in different places, so we call these wrapper func in few different places. Relying on e2e tests to catch any issues more or less.
It's the same issue with the code before this change actually, nothing is enforcing the right path to be used.
That’s how I found all of the places, changing in on place and finding the next places where it breaks while running e2e upgrades.
I'll address the comments when backporting to main branch.


// handle windows suffixes
if runtime.GOOS == "windows" {
Expand Down
47 changes: 38 additions & 9 deletions internal/pkg/agent/install/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"runtime"

"github.com/otiai10/copy"

Expand All @@ -17,6 +18,10 @@ import (
"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
)

const (
darwin = "darwin"
)

// Install installs Elastic Agent persistently on the system including creating and starting its service.
func Install(cfgFile string) error {
dir, err := findDirectory()
Expand Down Expand Up @@ -53,15 +58,36 @@ func Install(cfgFile string) error {

// place shell wrapper, if present on platform
if paths.ShellWrapperPath != "" {
err = os.MkdirAll(filepath.Dir(paths.ShellWrapperPath), 0755)
if err == nil {
err = ioutil.WriteFile(paths.ShellWrapperPath, []byte(paths.ShellWrapper), 0755) // nolint:gosec,G306 // this is fine.
}
if err != nil {
return errors.New(
err,
fmt.Sprintf("failed to write shell wrapper (%s)", paths.ShellWrapperPath),
errors.M("destination", paths.ShellWrapperPath))
// Install symlink for darwin instead
Copy link
Member

Choose a reason for hiding this comment

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

Can we explain why we need to do this? Let's document the reason we need this symlink in the code or in the templates/darwin/elastic-agent.tmpl script itself for future engineers to understand why this was necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before this change the agent used the shell script wrapper to start the agent, this posed an issue with FDA cause the root process of the launchd service was sh instead of the elastic-agent, so in order to grant FDA the FDA needed to be granted to /bin/sh. Here we replace that wrapper shell script with symlink, so the root process of the service is the elastic-agent. Will add more comments here for main branch backports.

if runtime.GOOS == darwin {
// Check if previous shell wrapper or symlink exists and remove it so it can be overwritten
if _, err := os.Lstat(paths.ShellWrapperPath); err == nil {
if err := os.Remove(paths.ShellWrapperPath); err != nil {
return errors.New(
err,
fmt.Sprintf("failed to remove (%s)", paths.ShellWrapperPath),
errors.M("destination", paths.ShellWrapperPath))
}
}
err = os.Symlink("/Library/Elastic/Agent/elastic-agent", paths.ShellWrapperPath)
Copy link
Contributor

@michalpristas michalpristas Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

replace with:
filepath.Join(paths.InstallPath, paths.BinaryName)

Copy link
Member Author

Choose a reason for hiding this comment

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

will leave as is for now, even though the change is trivial, because:

  1. previously this path was hardcode in the shell wrapper script as is
	// ShellWrapper is the wrapper that is installed.
	ShellWrapper = `#!/bin/sh
exec /Library/Elastic/Agent/elastic-agent $@
`
  1. this is a platform specific for Darwin so path separators and the path are fixed

can change (and would have to retest) when merging to the main branch, not for the urgent fix

if err != nil {
return errors.New(
err,
fmt.Sprintf("failed to create elastic-agent symlink (%s)", paths.ShellWrapperPath),
errors.M("destination", paths.ShellWrapperPath))
}
} else {
err = os.MkdirAll(filepath.Dir(paths.ShellWrapperPath), 0755)
if err == nil {
//nolint: gosec // this is intended to be an executable shell script, not chaning the permissions for the linter
err = ioutil.WriteFile(paths.ShellWrapperPath, []byte(paths.ShellWrapper), 0755)
}
if err != nil {
return errors.New(
err,
fmt.Sprintf("failed to write shell wrapper (%s)", paths.ShellWrapperPath),
errors.M("destination", paths.ShellWrapperPath))
}
}
}

Expand Down Expand Up @@ -151,6 +177,9 @@ func findDirectory() (string, error) {
// executable path is being reported as being down inside of data path
// move up to directories to perform the copy
sourceDir = filepath.Dir(filepath.Dir(sourceDir))
if runtime.GOOS == darwin {
sourceDir = filepath.Dir(filepath.Dir(filepath.Dir(sourceDir)))
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 check we're inside *.app dir?

Copy link
Member Author

Choose a reason for hiding this comment

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

why? the agent binary will always be inside of the app bundle with this change, it's not going to be installed outside of the app bundle.

Copy link
Contributor

@michalpristas michalpristas Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yeah right if it's inside data it cannot be root executable.
I was just trying to prevent some unintentional mixup of executables execution as this is taken from os.Executable path. being defensive on the change so it won't surprise us

}
}
err = verifyDirectory(sourceDir)
if err != nil {
Expand Down