Skip to content
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.

Sign up for GitHub

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

Fix stopping of modules started by kubernetes autodiscover #10476

Merged
merged 11 commits into from
Feb 7, 2019
Merged

Fix stopping of modules started by kubernetes autodiscover #10476

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5a6ce23
Fix stop of kubernetes autodiscovered modules
jsoriano Feb 1, 2019
4e25ed8
Use an id for containers that don't depend on its state
jsoriano Feb 5, 2019
39c9898
Use a better id
jsoriano Feb 5, 2019
3cfff44
Revert uneeded change
jsoriano Feb 5, 2019
bf9317f
Fix changelog
jsoriano Feb 5, 2019
972ceeb
Fix
jsoriano Feb 5, 2019
1d79f50
Keep skipping containers on start if they don't exist, and extend com…
jsoriano Feb 6, 2019
a8a3305
Merge remote-tracking branch 'origin/master' into kubernetes-autodisc…
jsoriano Feb 6, 2019
38ba23c
Maintain container.id value as before, and fix test
jsoriano Feb 6, 2019
d0d575a
Make hound happy
jsoriano Feb 6, 2019
604d2bd
Add test cases for stop
jsoriano Feb 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Fix TLS certificate DoS vulnerability. {pull}10302[10302]
- Fix panic and file unlock in spool on atomic operation (arm, x86-32). File lock was not released when panic occurs, leading to the beat deadlocking on startup. {pull}10289[10289]
- Fix encoding of timestamps when using disk spool. {issue}10099[10099]
- Fix stopping of modules started by kubernetes autodiscover. {pull}10476[10476]

*Auditbeat*

Expand Down
18 changes: 7 additions & 11 deletions libbeat/autodiscover/providers/kubernetes/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package kubernetes

import (
"fmt"
"time"

"github.com/gofrs/uuid"
Expand Down Expand Up @@ -145,28 +146,23 @@ func (p *Provider) emitEvents(pod *kubernetes.Pod, flag string, containers []*ku
host := pod.Status.GetPodIP()

// Do not emit events without host (container is still being configured)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Do not emit events without host (container is still being configured)
// Do not emit events without host for containers which are not stopping (container is still being configured)

Extending the comment here as I had to read this line multiple times. The part that wondered me here is if should be flag == start or something like that to dismiss the start state but not all others. Not sure what the available flags are.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments extended.

Regarding flags, we only have start and stop at the moment, but I only wanted to change the behaviour of stop, and this is why explicitly check for that.

if host == "" {
if host == "" && flag != "stop" {
return
}

// Collect all container IDs and runtimes from status information.
containerIDs := map[string]string{}
// Collect all runtimes from status information.
runtimes := map[string]string{}
for _, c := range containerstatuses {
cid, runtime := kubernetes.ContainerIDWithRuntime(c)
containerIDs[c.GetName()] = cid
_, runtime := kubernetes.ContainerIDWithRuntime(c)
runtimes[c.GetName()] = runtime
}

// Emit container and port information
for _, c := range containers {
cid := containerIDs[c.GetName()]
// This must be an id that doesn't depend on the state of the container
// so it works also on `stop` if containers have been already stopped.
cid := fmt.Sprintf("%s.%s", pod.Metadata.GetUid(), c.GetName())

// If there is a container ID that is empty then ignore it. It either means that the container is still starting
// up or the container is shutting down.
if cid == "" {
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the case where the container is still starting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that just checking for the ID doesn't ensure that the container is ready, so it wouldn't matter so much. But the truth is that there is no need to change this here. I will keep the check for non-stop events.

Thanks for taking the time to review this!

}
cmeta := common.MapStr{
"id": cid,
"name": c.GetName(),
Expand Down