From 1406e186beaf312827a3f9e2ea41ace86daaefcb Mon Sep 17 00:00:00 2001 From: Lucas Manuel Rodriguez Date: Wed, 1 Jun 2022 14:47:04 -0300 Subject: [PATCH] Orbit: Add early update checks before starting sub-systems (#5885) * Make orbit do a early check of updates * Support orbit dev-mode * Add test for NewRunner and Runner.UpdateAction * Remove unnecessary parallel test --- ee/fleetctl/updates_test.go | 4 +- .../issue-5875-orbit-start-up-update-check | 1 + orbit/cmd/orbit/orbit.go | 54 +++++++++++-------- orbit/cmd/orbit/shell.go | 2 +- orbit/pkg/packaging/packaging.go | 2 +- orbit/pkg/update/runner.go | 35 +++++++----- orbit/pkg/update/runner_test.go | 52 ++++++++++++++++++ orbit/pkg/update/update.go | 4 +- orbit/pkg/update/update_test.go | 12 +---- 9 files changed, 114 insertions(+), 52 deletions(-) create mode 100644 orbit/changes/issue-5875-orbit-start-up-update-check create mode 100644 orbit/pkg/update/runner_test.go diff --git a/ee/fleetctl/updates_test.go b/ee/fleetctl/updates_test.go index f0fd9fc2d9..59fa9dfe66 100644 --- a/ee/fleetctl/updates_test.go +++ b/ee/fleetctl/updates_test.go @@ -217,7 +217,7 @@ func TestUpdatesIntegration(t *testing.T) { // Initialize an update client localStore, err := filestore.New(filepath.Join(tmpDir, "tuf-metadata.json")) require.NoError(t, err) - updater, err := update.New(update.Options{ + updater, err := update.NewUpdater(update.Options{ RootDirectory: tmpDir, ServerURL: server.URL, RootKeys: roots, @@ -354,7 +354,7 @@ func TestUpdatesIntegration(t *testing.T) { tmpDir = t.TempDir() localStore, err = filestore.New(filepath.Join(tmpDir, "tuf-metadata.json")) require.NoError(t, err) - updater, err = update.New(update.Options{RootDirectory: tmpDir, ServerURL: server.URL, RootKeys: roots, LocalStore: localStore}) + updater, err = update.NewUpdater(update.Options{RootDirectory: tmpDir, ServerURL: server.URL, RootKeys: roots, LocalStore: localStore}) require.NoError(t, err) require.NoError(t, updater.UpdateMetadata()) } diff --git a/orbit/changes/issue-5875-orbit-start-up-update-check b/orbit/changes/issue-5875-orbit-start-up-update-check new file mode 100644 index 0000000000..7c50ec30da --- /dev/null +++ b/orbit/changes/issue-5875-orbit-start-up-update-check @@ -0,0 +1 @@ +* Added an early check for updates to Orbit (before sub-systems are started). diff --git a/orbit/cmd/orbit/orbit.go b/orbit/cmd/orbit/orbit.go index 40fdbea532..9f6de64cd8 100644 --- a/orbit/cmd/orbit/orbit.go +++ b/orbit/cmd/orbit/orbit.go @@ -250,21 +250,50 @@ func main() { opt.InsecureTransport = c.Bool("insecure") var ( - updater *update.Updater osquerydPath string desktopPath string + g run.Group ) // NOTE: When running in dev-mode, even if `disable-updates` is set, // it fetches osqueryd once as part of initialization. if !c.Bool("disable-updates") || c.Bool("dev-mode") { - updater, err = update.New(opt) + updater, err := update.NewUpdater(opt) if err != nil { return fmt.Errorf("create updater: %w", err) } if err := updater.UpdateMetadata(); err != nil { - log.Info().Err(err).Msg("update metadata. using saved metadata.") + log.Info().Err(err).Msg("update metadata. using saved metadata") } + + targets := []string{"orbit", "osqueryd"} + if c.Bool("fleet-desktop") { + targets = append(targets, "desktop") + } + if c.Bool("dev-mode") { + targets = targets[1:] // exclude orbit itself on dev-mode. + } + updateRunner, err := update.NewRunner(updater, update.RunnerOptions{ + CheckInterval: c.Duration("update-interval"), + Targets: targets, + }) + if err != nil { + return err + } + + // Perform early check for updates before starting any sub-system. + // This is to prevent bugs in other sub-systems to mess up with + // the download of available updates. + didUpdate, err := updateRunner.UpdateAction() + if err != nil { + log.Info().Err(err).Msg("early update check failed") + } + if didUpdate && !c.Bool("dev-mode") { + log.Info().Msg("exiting due to successful early update") + return nil + } + g.Add(updateRunner.Execute, updateRunner.Interrupt) + osquerydLocalTarget, err := updater.Get("osqueryd") if err != nil { return fmt.Errorf("get osqueryd target: %w", err) @@ -283,7 +312,7 @@ func main() { } } else { log.Info().Msg("running with auto updates disabled") - updater = update.NewDisabled(opt) + updater := update.NewDisabled(opt) osquerydPath, err = updater.ExecutableLocalPath("osqueryd") if err != nil { log.Fatal().Err(err).Msg("locate osqueryd") @@ -321,23 +350,6 @@ func main() { return fmt.Errorf("cleanup old files: %w", err) } - var g run.Group - - if !c.Bool("disable-updates") { - targets := []string{"orbit", "osqueryd"} - if c.Bool("fleet-desktop") { - targets = append(targets, "desktop") - } - updateRunner, err := update.NewRunner(updater, update.RunnerOptions{ - CheckInterval: c.Duration("update-interval"), - Targets: targets, - }) - if err != nil { - return err - } - g.Add(updateRunner.Execute, updateRunner.Interrupt) - } - var options []osquery.Option options = append(options, osquery.WithDataPath(c.String("root-dir"))) options = append(options, osquery.WithLogPath(filepath.Join(c.String("root-dir"), "osquery_log"))) diff --git a/orbit/cmd/orbit/shell.go b/orbit/cmd/orbit/shell.go index 0d69c452a6..32c971f0d0 100644 --- a/orbit/cmd/orbit/shell.go +++ b/orbit/cmd/orbit/shell.go @@ -61,7 +61,7 @@ var shellCommand = &cli.Command{ opt.LocalStore = localStore opt.InsecureTransport = c.Bool("insecure") - updater, err := update.New(opt) + updater, err := update.NewUpdater(opt) if err != nil { return err } diff --git a/orbit/pkg/packaging/packaging.go b/orbit/pkg/packaging/packaging.go index dec8625d9b..2a24af13bf 100644 --- a/orbit/pkg/packaging/packaging.go +++ b/orbit/pkg/packaging/packaging.go @@ -107,7 +107,7 @@ func InitializeUpdates(updateOpt update.Options) (*UpdatesData, error) { } updateOpt.LocalStore = localStore - updater, err := update.New(updateOpt) + updater, err := update.NewUpdater(updateOpt) if err != nil { return nil, fmt.Errorf("failed to init updater: %w", err) } diff --git a/orbit/pkg/update/runner.go b/orbit/pkg/update/runner.go index 3d5d17e924..91d6d43a59 100644 --- a/orbit/pkg/update/runner.go +++ b/orbit/pkg/update/runner.go @@ -34,10 +34,10 @@ type Runner struct { // started with Execute. func NewRunner(updater *Updater, opt RunnerOptions) (*Runner, error) { if opt.CheckInterval <= 0 { - return nil, errors.New("Runner must be configured with interval greater than 0") + return nil, errors.New("runner must be configured with interval greater than 0") } if len(opt.Targets) == 0 { - return nil, errors.New("Runner must have nonempty subscriptions") + return nil, errors.New("runner must have nonempty subscriptions") } // Initialize the hashes of the local files for all tracked targets. @@ -48,18 +48,22 @@ func NewRunner(updater *Updater, opt RunnerOptions) (*Runner, error) { for _, target := range opt.Targets { meta, err := updater.Lookup(target) if err != nil { - return nil, fmt.Errorf("initialize update cache: %w", err) + return nil, fmt.Errorf("target %s lookup: %w", target, err) } localTarget, err := updater.localTarget(target) if err != nil { - return nil, fmt.Errorf("failed to get local path for %s: %w", target, err) + return nil, fmt.Errorf("get local path for %s: %w", target, err) } - _, localHash, err := fileHashes(meta, localTarget.Path) - if err != nil { + switch _, localHash, err := fileHashes(meta, localTarget.Path); { + case err == nil: + localHashes[target] = localHash + log.Info().Msgf("hash(%s)=%x", target, localHash) + case errors.Is(err, os.ErrNotExist): + // This is expected to happen if the target is not yet downloaded, + // or if the user manually changed the target channel. + default: return nil, fmt.Errorf("%s file hash: %w", target, err) } - localHashes[target] = localHash - log.Info().Msgf("hash(%s)=%x", target, localHash) } return &Runner{ @@ -84,10 +88,8 @@ func (r *Runner) Execute() error { select { case <-r.cancel: return nil - case <-ticker.C: - // On each tick, check for updates - didUpdate, err := r.updateAction() + didUpdate, err := r.UpdateAction() if err != nil { log.Info().Err(err).Msg("update failed") } @@ -99,14 +101,19 @@ func (r *Runner) Execute() error { } } -func (r *Runner) updateAction() (bool, error) { - var didUpdate bool +// UpdateAction checks for updates on all targets. +// Returns true if one of the targets has been updated. +// +// NOTE: If it returns (true, non-nil error) then it means some target/s +// were successfully upgraded and some failed to upgrade. +func (r *Runner) UpdateAction() (bool, error) { if err := r.updater.UpdateMetadata(); err != nil { // Consider this a non-fatal error since it will be common to be offline // or otherwise unable to retrieve the metadata. - return didUpdate, fmt.Errorf("update metadata: %w", err) + return false, fmt.Errorf("update metadata: %w", err) } + var didUpdate bool for _, target := range r.opt.Targets { meta, err := r.updater.Lookup(target) if err != nil { diff --git a/orbit/pkg/update/runner_test.go b/orbit/pkg/update/runner_test.go new file mode 100644 index 0000000000..3b6e93c260 --- /dev/null +++ b/orbit/pkg/update/runner_test.go @@ -0,0 +1,52 @@ +package update + +import ( + "testing" + "time" + + "github.com/fleetdm/fleet/v4/pkg/nettest" + "github.com/stretchr/testify/require" +) + +func TestNewRunner(t *testing.T) { + // TODO(lucas): Do not use our TUF remote repository + // but instead create local repository and serve with a httptest server. + // For that, we need to move and export some functionality currently in + // "ee/fleetctl/updates.go" (as it doesn't make sense to have such functionality + // there and import such eefleetctl package here). + nettest.Run(t) + + rootDir := t.TempDir() + updateOpts := DefaultOptions + updateOpts.RootDirectory = rootDir + + u, err := NewUpdater(updateOpts) + require.NoError(t, err) + + err = u.UpdateMetadata() + require.NoError(t, err) + + runnerOpts := RunnerOptions{ + CheckInterval: 1 * time.Second, + Targets: []string{"osqueryd"}, + } + // NewRunner should not fail if targets do not exist locally. + r, err := NewRunner(u, runnerOpts) + require.NoError(t, err) + execPath, err := u.ExecutableLocalPath("osqueryd") + require.NoFileExists(t, execPath) + + // r.UpdateAction should download osqueryd. + didUpdate, err := r.UpdateAction() + require.NoError(t, err) + require.True(t, didUpdate) + require.FileExists(t, execPath) + + // Create another Runner but with the target already existing. + r2, err := NewRunner(u, runnerOpts) + require.NoError(t, err) + + didUpdate, err = r2.UpdateAction() + require.NoError(t, err) + require.False(t, didUpdate) +} diff --git a/orbit/pkg/update/update.go b/orbit/pkg/update/update.go index 90d9990168..38aa5f34c0 100644 --- a/orbit/pkg/update/update.go +++ b/orbit/pkg/update/update.go @@ -86,9 +86,9 @@ type TargetInfo struct { CustomCheckExec func(execPath string) error } -// New creates a new updater given the provided options. All the necessary +// NewUpdater creates a new updater given the provided options. All the necessary // directories are initialized. -func New(opt Options) (*Updater, error) { +func NewUpdater(opt Options) (*Updater, error) { if opt.LocalStore == nil { return nil, errors.New("opt.LocalStore must be non-nil") } diff --git a/orbit/pkg/update/update_test.go b/orbit/pkg/update/update_test.go index 9f6c6233a4..8249b6d31a 100644 --- a/orbit/pkg/update/update_test.go +++ b/orbit/pkg/update/update_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/fleetdm/fleet/v4/orbit/pkg/constant" - "github.com/jinzhu/copier" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -32,8 +31,6 @@ func assertDir(t *testing.T, path string) { } func TestMakeRepoPath(t *testing.T) { - t.Parallel() - testCases := []struct { name string version string @@ -50,15 +47,8 @@ func TestMakeRepoPath(t *testing.T) { } for _, tt := range testCases { - tt := tt t.Run(tt.expected, func(t *testing.T) { - t.Parallel() - - var opt Options - // Must deep copy DefaultOptions, otherwise there is a race condition when modifying the - // opt.Targets map in parallel tests below. - err := copier.CopyWithOption(&opt, DefaultOptions, copier.Option{DeepCopy: true}) - require.NoError(t, err) + opt := DefaultOptions osqueryd := opt.Targets[tt.name] osqueryd.Platform = tt.platform