-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Pass context with timeout to FQDN lookup #37756
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
This pull request is now in conflicts. Could you fix it? 🙏
|
💔 Build Failed
Expand to view the summary
Build stats
Pipeline error
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
This pull request is now in conflicts. Could you fix it? 🙏
|
fqdnLookupCtx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) | ||
defer cancel() | ||
|
||
fqdn, err := h.FQDNWithContext(fqdnLookupCtx) |
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.
If this times out it falls back to the non-FQDN, does that ever get updated? Or is the Beat stuck with the non-FQDN.
If the first rule of network communication is "never make a request without a timeout", the second one is "never rely on a single network request to succeed".
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.
No, it doesn't get updated. I agree that it would be nice to keep retrying but I also think that's out of scope for this PR here; this one is just about implementing the first rule. I'll file a separate issue to implement the second one.
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.
💚 Build Succeeded
Expand to view the summary
Build stats
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
@@ -96,7 +97,7 @@ func New(cfg *config.C) (beat.Processor, error) { | |||
} | |||
|
|||
// create a unique ID for this instance of the processor | |||
cbIDStr := "" | |||
var cbIDStr string |
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 change is unrelated to this PR; it is included in this PR to make the linter happy. Without it, the linter was failing with this error:
assigned to cbIDStr, but reassigned without using the value (wastedassign)
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
|
💚 Build Succeeded
Expand to view the summary
Build stats
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
|
💔 Build Failed
Failed CI StepsHistory
cc @ycombinator |
💚 Build Succeeded
Expand to view the summary
Build stats
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
* Pass context with timeout to FQDN lookup * Add temporary gomod replace * Add CHANGELOG entry * Update dependency * Update method calls * Add nolint for rand deprecation warning * Make linter happy (cherry picked from commit e4b448f) # Conflicts: # NOTICE.txt # go.mod # go.sum
) * Pass context with timeout to FQDN lookup (#37756) * Pass context with timeout to FQDN lookup * Add temporary gomod replace * Add CHANGELOG entry * Update dependency * Update method calls * Add nolint for rand deprecation warning * Make linter happy (cherry picked from commit e4b448f) # Conflicts: # NOTICE.txt # go.mod # go.sum * Fix conflicts --------- Co-authored-by: Shaunak Kashyap <[email protected]>
* Pass context with timeout to FQDN lookup * Add temporary gomod replace * Add CHANGELOG entry * Update dependency * Update method calls * Add nolint for rand deprecation warning * Make linter happy
Proposed commit message
This PR improves the FQDN lookup function calls by passing a context with a 1 minute timeout, thus preventing the calls from blocking indefinitely due to DNS issues and such.
Depends on: elastic/go-sysinfo#199 and a new release of that library.