[Ingest Manager] Improved verify experience (#21573) (#21588)

[Ingest Manager] Improved verify experience (#21573)
elastic · Oct 6, 2020 · 1045e6e · 1045e6e
1 parent 0e6ea45
commit 1045e6e
Show file tree

Hide file tree

Showing 12 changed files with 44 additions and 33 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 	}