Update elastic-agent-client for elastic-agent in 7.x#39800
Merged
michel-laterman merged 7 commits intoelastic:7.17from Jun 20, 2024
Merged
Update elastic-agent-client for elastic-agent in 7.x#39800michel-laterman merged 7 commits intoelastic:7.17from
michel-laterman merged 7 commits intoelastic:7.17from
Conversation
elastic#39586) Update elastic-agent-client to a tagged release (v7.8.1), and rename control proto package to cproto so it does not conflict with elastic-agent-client import
Contributor
Author
|
With debug enabled the enrollement output (from container mode) is:
The error above is emitted from: but the ast has been conveted into a map with a call to ast.Map
EDIT 2 |
Contributor
Author
|
I've added more logging around the container and enroll_cmd entrypoints as well as When the enroll executes we can see (for both images) that the config it has in memory indicates fleet is enabled. However in However for a working 7.17 image it loads from a file: |
michel-laterman
commented
Jun 17, 2024
| backExp.Wait() | ||
| err = storeAgentInfo(s, reader) | ||
| if err != filelock.ErrAppAlreadyRunning { | ||
| if !stderror.Is(err, filelock.ErrAppAlreadyRunning) { |
Contributor
Author
There was a problem hiding this comment.
I originally forgot to invert this check which was causing the elastic-agent container bootstrap to fail.
It now works
Contributor
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
pchila
approved these changes
Jun 20, 2024
Member
pchila
left a comment
There was a problem hiding this comment.
No major blockers, added a few suggestions about handling errors instead of adding nolint directives even in defer statements and changing alias for/removing our errors package
| func (c *enrollCmd) stopAgent() { | ||
| if c.agentProc != nil { | ||
| c.agentProc.StopWait() | ||
| c.agentProc.StopWait() //nolint:errcheck // no error check here |
Member
There was a problem hiding this comment.
Could we check the error and log it at least instead of adding the nolint directive ?
| import ( | ||
| "bytes" | ||
| "context" | ||
| stderror "errors" |
Member
There was a problem hiding this comment.
[Naming] stderror gave me os.Stderr vibes when I was reading the code... can we use a different alias like goerrors or stdliberrors maybe ?
Even better: we could get rid of the "github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/agent/errors" import and only use standard library errors package
| return err | ||
| } | ||
| defer fileLock.Unlock() | ||
| defer fileLock.Unlock() //nolint:errcheck // defered call |
Member
There was a problem hiding this comment.
Maybe we can handle the error here, for example with a closure:
Suggested change
| defer fileLock.Unlock() //nolint:errcheck // defered call | |
| defer func(fileLock *filelock.AppLocker) { | |
| unlockErr := fileLock.Unlock() | |
| if unlockErr != nil { | |
| // log error here or join it with the internal error returned | |
| } | |
| }(fileLock) | |
| } | ||
|
|
||
| res, err := client.Get("http://" + endpoint + "/") | ||
| req, err := http.NewRequestWithContext(ctx, "GET", "http://"+endpoint+"/", nil) |
Member
There was a problem hiding this comment.
We can use http module constants here
Suggested change
| req, err := http.NewRequestWithContext(ctx, "GET", "http://"+endpoint+"/", nil) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://"+endpoint+"/", nil) |
| if ok := certPool.AppendCertsFromPEM(s.ca.Crt()); !ok { | ||
| return errors.New("failed to append root CA", errors.TypeSecurity) | ||
| } | ||
| //nolint:gosec // G402: TLS MinVersion too low. |
Member
There was a problem hiding this comment.
Not sure if it breaks backward compatibility but it would be nice if we stopped accepting insecure TLS versions... Although this also silences the linter.
@cmacknz maybe we want to address this in a separate issue?
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed commit message
Cherry-pick #39586 which was the PR to update elastic-agent-libs and rename the control proto internally to avoid a namespace collision. The PR was reverted because the agent did not start when deployed in cloud.
Added
mage cloud:*targets to see if the issue was due to the agent running in under a container, or if it was how our containers are provisioned and found it's due to the container mode start.Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.How to test this PR locally
Build a local image with
mage cloud:imageStart a 7.17.22-SNAPSHOT cloud deployment
Create a policy with only the fleet-server integration
Start a container with
Related issues
Fixes elastic/fleet-server#3592