-
Notifications
You must be signed in to change notification settings - Fork 142
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
If one gpg check is working, upgrade should continue #3426
Conversation
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
This pull request does not have a backport label. Could you fix it @pierrehilbert? 🙏
NOTE: |
🌐 Coverage report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not fixing the issue but we can push this in and have at least one strategy with verifiers to reduce number of failures
internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go
Show resolved
Hide resolved
internal/pkg/agent/application/upgrade/artifact/download/verifier.go
Outdated
Show resolved
Hide resolved
return "", errors.New(err, "failed verification of agent binary") | ||
} | ||
|
||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really the standard way to do this, if the operation succeeded err
should be nil
.
Now that I've seen this approach, I think a better solution would be to have the composed verifier log each verifier that failed but return a nil
error if at least one of them succeeded.
If you wanted to keep doing it this way, you would want to return the details of what happened as a separate structure so it can be logged at this level but I think it will be much simpler to handle the logging directly in this block of the composed verifier
Something like the following should handle most of this, it already builds up a joined error of each failure and only returns it if all of the verifiers fail. We just need to add a log statement and a Name
method to the verifier interface so you can log which one failed.
--- a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go
+++ b/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go
@@ -5,7 +5,7 @@
package composed
import (
- "github.com/hashicorp/go-multierror"
+ "errors"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
@@ -43,11 +43,10 @@ func (e *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bo
return nil
}
- err = multierror.Append(err, e)
+ err = errors.Join(err, e)
if errors.As(e, &checksumMismatchErr) || errors.As(err, &invalidSignatureErr) {
- // Stop verification chain on checksum/signature errors.
- break
+ log.Warningw("Verifier failed!", "verifier", v.Name(), "error", err)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would still want the logger here to log the case where the all fail, which should be logged at the Error
level.
You should be able to pass the logger you have here into the function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to ensure that I get it correctly: you prefer me to log directly in the composed/verifier.go
instead of step_download.go
to avoid returning something in the error
output if at the end the upgrade was successful (from a conceptual perspective, success = no error)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need success = "no error".
The trouble you are running into is that you need to log twice, once inside the composed/verifier.go
file for non-fatal errors that won't be returned (since final success = no error) and once when the verifier is called from the upgrade process for the fatal error where all verifiers failed.
…e are now logging failing verifiers
(cherry picked from commit 7be7f62)
What does this PR do?
This PR removes the break in the verify loop to continue in case one verify is successful.
Why is it important?
This change will allow upgrade to continue if remote gpg key is unreachable.
Checklist
./changelog/fragments
using the changelog toolAuthor's Checklist
How to test this PR locally
Related issues
Closes #3368
Use cases
Screenshots
Logs
Questions to ask yourself