From 1045e6ef2d30abd68e05f9e5728886bf6b30de18 Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Tue, 6 Oct 2020 20:13:08 +0200 Subject: [PATCH] [Ingest Manager] Improved verify experience (#21573) (#21588) [Ingest Manager] Improved verify experience (#21573) --- .../pkg/agent/application/emitter.go | 13 ++++++++---- .../application/upgrade/step_download.go | 2 +- .../pkg/agent/operation/common_test.go | 2 +- .../pkg/agent/operation/operation_verify.go | 2 +- .../artifact/download/composed/verifier.go | 7 ++++--- .../pkg/artifact/download/fs/verifier.go | 17 +++++++++------- .../pkg/artifact/download/fs/verifier_test.go | 6 +++--- .../artifact/download/http/elastic_test.go | 2 +- .../pkg/artifact/download/http/verifier.go | 20 ++++++++++--------- .../pkg/artifact/download/verifier.go | 2 +- .../pkg/core/plugin/process/app.go | 2 +- .../pkg/core/plugin/process/start.go | 2 +- 12 files changed, 44 insertions(+), 33 deletions(-) diff --git a/x-pack/elastic-agent/pkg/agent/application/emitter.go b/x-pack/elastic-agent/pkg/agent/application/emitter.go index fc103366826d..3bd06043c304 100644 --- a/x-pack/elastic-agent/pkg/agent/application/emitter.go +++ b/x-pack/elastic-agent/pkg/agent/application/emitter.go @@ -44,10 +44,11 @@ type emitterController struct { reloadables []reloadable // state - lock sync.RWMutex - config *config.Config - ast *transpiler.AST - vars []*transpiler.Vars + lock sync.RWMutex + updateLock sync.Mutex + config *config.Config + ast *transpiler.AST + vars []*transpiler.Vars } func (e *emitterController) Update(c *config.Config) error { @@ -93,6 +94,10 @@ func (e *emitterController) Set(vars []*transpiler.Vars) { } func (e *emitterController) update() error { + // locking whole update because it can be called concurrently via Set and Update method + e.updateLock.Lock() + defer e.updateLock.Unlock() + e.lock.RLock() cfg := e.config rawAst := e.ast diff --git a/x-pack/elastic-agent/pkg/agent/application/upgrade/step_download.go b/x-pack/elastic-agent/pkg/agent/application/upgrade/step_download.go index cf3a36567241..3aea96da0ab0 100644 --- a/x-pack/elastic-agent/pkg/agent/application/upgrade/step_download.go +++ b/x-pack/elastic-agent/pkg/agent/application/upgrade/step_download.go @@ -38,7 +38,7 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI stri return "", errors.New(err, "failed upgrade of agent binary") } - matches, err := verifier.Verify(agentName, version, agentArtifactName) + matches, err := verifier.Verify(agentName, version, agentArtifactName, true) if err != nil { return "", errors.New(err, "failed verification of agent binary") } diff --git a/x-pack/elastic-agent/pkg/agent/operation/common_test.go b/x-pack/elastic-agent/pkg/agent/operation/common_test.go index 6e9b042fe929..e9d40bece876 100644 --- a/x-pack/elastic-agent/pkg/agent/operation/common_test.go +++ b/x-pack/elastic-agent/pkg/agent/operation/common_test.go @@ -143,7 +143,7 @@ var _ download.Downloader = &DummyDownloader{} type DummyVerifier struct{} -func (*DummyVerifier) Verify(p, v, _ string) (bool, error) { +func (*DummyVerifier) Verify(p, v, _ string, _ bool) (bool, error) { return true, nil } diff --git a/x-pack/elastic-agent/pkg/agent/operation/operation_verify.go b/x-pack/elastic-agent/pkg/agent/operation/operation_verify.go index 97cf906cace3..7626d339e578 100644 --- a/x-pack/elastic-agent/pkg/agent/operation/operation_verify.go +++ b/x-pack/elastic-agent/pkg/agent/operation/operation_verify.go @@ -66,7 +66,7 @@ func (o *operationVerify) Run(_ context.Context, application Application) (err e } }() - isVerified, err := o.verifier.Verify(o.program.BinaryName(), o.program.Version(), o.program.ArtifactName()) + isVerified, err := o.verifier.Verify(o.program.BinaryName(), o.program.Version(), o.program.ArtifactName(), true) if err != nil { return errors.New(err, fmt.Sprintf("operation '%s' failed to verify %s.%s", o.Name(), o.program.BinaryName(), o.program.Version()), diff --git a/x-pack/elastic-agent/pkg/artifact/download/composed/verifier.go b/x-pack/elastic-agent/pkg/artifact/download/composed/verifier.go index 9d6c4477733c..663484130f46 100644 --- a/x-pack/elastic-agent/pkg/artifact/download/composed/verifier.go +++ b/x-pack/elastic-agent/pkg/artifact/download/composed/verifier.go @@ -29,11 +29,12 @@ func NewVerifier(verifiers ...download.Verifier) *Verifier { } // Verify checks the package from configured source. -func (e *Verifier) Verify(programName, version, artifactName string) (bool, error) { +func (e *Verifier) Verify(programName, version, artifactName string, removeOnFailure bool) (bool, error) { var err error - for _, v := range e.vv { - b, e := v.Verify(programName, version, artifactName) + for i, v := range e.vv { + isLast := (i + 1) == len(e.vv) + b, e := v.Verify(programName, version, artifactName, isLast && removeOnFailure) if e == nil { return b, nil } diff --git a/x-pack/elastic-agent/pkg/artifact/download/fs/verifier.go b/x-pack/elastic-agent/pkg/artifact/download/fs/verifier.go index 09462ef3f234..56652d4f69c7 100644 --- a/x-pack/elastic-agent/pkg/artifact/download/fs/verifier.go +++ b/x-pack/elastic-agent/pkg/artifact/download/fs/verifier.go @@ -51,20 +51,23 @@ func NewVerifier(config *artifact.Config, allowEmptyPgp bool, pgp []byte) (*Veri // Verify checks downloaded package on preconfigured // location agains a key stored on elastic.co website. -func (v *Verifier) Verify(programName, version, artifactName string) (bool, error) { +func (v *Verifier) Verify(programName, version, artifactName string, removeOnFailure bool) (isMatch bool, err error) { filename, err := artifact.GetArtifactName(programName, version, v.config.OS(), v.config.Arch()) if err != nil { return false, errors.New(err, "retrieving package name") } fullPath := filepath.Join(v.config.TargetDirectory, filename) + defer func() { + if removeOnFailure && (!isMatch || err != nil) { + // remove bits so they can be redownloaded + os.Remove(fullPath) + os.Remove(fullPath + ".sha512") + os.Remove(fullPath + ".asc") + } + }() - isMatch, err := v.verifyHash(filename, fullPath) - if !isMatch || err != nil { - // remove bits so they can be redownloaded - os.Remove(fullPath) - os.Remove(fullPath + ".sha512") - os.Remove(fullPath + ".asc") + if isMatch, err := v.verifyHash(filename, fullPath); !isMatch || err != nil { return isMatch, err } diff --git a/x-pack/elastic-agent/pkg/artifact/download/fs/verifier_test.go b/x-pack/elastic-agent/pkg/artifact/download/fs/verifier_test.go index 975d9ecb14d4..a79f35bdd6f5 100644 --- a/x-pack/elastic-agent/pkg/artifact/download/fs/verifier_test.go +++ b/x-pack/elastic-agent/pkg/artifact/download/fs/verifier_test.go @@ -65,7 +65,7 @@ func TestFetchVerify(t *testing.T) { // first download verify should fail: // download skipped, as invalid package is prepared upfront // verify fails and cleans download - matches, err := verifier.Verify(programName, version, artifactName) + matches, err := verifier.Verify(programName, version, artifactName, true) assert.NoError(t, err) assert.Equal(t, false, matches) @@ -88,7 +88,7 @@ func TestFetchVerify(t *testing.T) { _, err = os.Stat(hashTargetFilePath) assert.NoError(t, err) - matches, err = verifier.Verify(programName, version, artifactName) + matches, err = verifier.Verify(programName, version, artifactName, true) assert.NoError(t, err) assert.Equal(t, true, matches) } @@ -162,7 +162,7 @@ func TestVerify(t *testing.T) { t.Fatal(err) } - isOk, err := testVerifier.Verify(beatName, version, artifactName) + isOk, err := testVerifier.Verify(beatName, version, artifactName, true) if err != nil { t.Fatal(err) } diff --git a/x-pack/elastic-agent/pkg/artifact/download/http/elastic_test.go b/x-pack/elastic-agent/pkg/artifact/download/http/elastic_test.go index fec1d991c880..0ff4dfd2b934 100644 --- a/x-pack/elastic-agent/pkg/artifact/download/http/elastic_test.go +++ b/x-pack/elastic-agent/pkg/artifact/download/http/elastic_test.go @@ -110,7 +110,7 @@ func TestVerify(t *testing.T) { t.Fatal(err) } - isOk, err := testVerifier.Verify(beatName, version, artifactName) + isOk, err := testVerifier.Verify(beatName, version, artifactName, false) if err != nil { t.Fatal(err) } diff --git a/x-pack/elastic-agent/pkg/artifact/download/http/verifier.go b/x-pack/elastic-agent/pkg/artifact/download/http/verifier.go index c0dfef9b30e2..b5b5e628b4a0 100644 --- a/x-pack/elastic-agent/pkg/artifact/download/http/verifier.go +++ b/x-pack/elastic-agent/pkg/artifact/download/http/verifier.go @@ -59,9 +59,7 @@ func NewVerifier(config *artifact.Config, allowEmptyPgp bool, pgp []byte) (*Veri // Verify checks downloaded package on preconfigured // location agains a key stored on elastic.co website. -func (v *Verifier) Verify(programName, version, artifactName string) (bool, error) { - // TODO: think about verifying asc for prepacked beats - +func (v *Verifier) Verify(programName, version, artifactName string, removeOnFailure bool) (isMatch bool, err error) { filename, err := artifact.GetArtifactName(programName, version, v.config.OS(), v.config.Arch()) if err != nil { return false, errors.New(err, "retrieving package name") @@ -72,12 +70,16 @@ func (v *Verifier) Verify(programName, version, artifactName string) (bool, erro return false, errors.New(err, "retrieving package path") } - isMatch, err := v.verifyHash(filename, fullPath) - if !isMatch || err != nil { - // remove bits so they can be redownloaded - os.Remove(fullPath) - os.Remove(fullPath + ".sha512") - os.Remove(fullPath + ".asc") + defer func() { + if removeOnFailure && (!isMatch || err != nil) { + // remove bits so they can be redownloaded + os.Remove(fullPath) + os.Remove(fullPath + ".sha512") + os.Remove(fullPath + ".asc") + } + }() + + if isMatch, err := v.verifyHash(filename, fullPath); !isMatch || err != nil { return isMatch, err } diff --git a/x-pack/elastic-agent/pkg/artifact/download/verifier.go b/x-pack/elastic-agent/pkg/artifact/download/verifier.go index 491979514ead..d7b2bc7a4151 100644 --- a/x-pack/elastic-agent/pkg/artifact/download/verifier.go +++ b/x-pack/elastic-agent/pkg/artifact/download/verifier.go @@ -6,5 +6,5 @@ package download // Verifier is an interface verifying GPG key of a downloaded artifact type Verifier interface { - Verify(programName, version, artifactName string) (bool, error) + Verify(programName, version, artifactName string, removeOnFailure bool) (bool, error) } diff --git a/x-pack/elastic-agent/pkg/core/plugin/process/app.go b/x-pack/elastic-agent/pkg/core/plugin/process/app.go index 5f9cf6efb254..8732af24f682 100644 --- a/x-pack/elastic-agent/pkg/core/plugin/process/app.go +++ b/x-pack/elastic-agent/pkg/core/plugin/process/app.go @@ -117,7 +117,7 @@ func (a *Application) Name() string { // Started returns true if the application is started. func (a *Application) Started() bool { - return a.state.Status != state.Stopped + return a.state.Status != state.Stopped && a.state.Status != state.Crashed && a.state.Status != state.Failed } // Stop stops the current application. diff --git a/x-pack/elastic-agent/pkg/core/plugin/process/start.go b/x-pack/elastic-agent/pkg/core/plugin/process/start.go index c5f1ffb38426..bc19910e53c9 100644 --- a/x-pack/elastic-agent/pkg/core/plugin/process/start.go +++ b/x-pack/elastic-agent/pkg/core/plugin/process/start.go @@ -39,7 +39,7 @@ func (a *Application) start(ctx context.Context, t app.Taggable, cfg map[string] }() // already started if not stopped or crashed - if a.state.Status != state.Stopped && a.state.Status != state.Crashed && a.state.Status != state.Failed { + if a.Started() { return nil }