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.

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

Make docker input check if container strings are empty #7960

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

vjsamuel
Copy link
Contributor

We have seen certain docker inputs being spun up with the config:

paths: [/var/lib/docker/containers/*.log]

This is because of empty container IDs. This PR makes sure that we do a check of empty strings to avoid this as there might be serious repercussions if that path had a valid file.

@vjsamuel vjsamuel force-pushed the check_empty_cid_docker_input branch from 5790b47 to 4546b84 Compare August 13, 2018 21:16
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

// Docker input should make sure that no callers should ever pass empty strings as container IDs
// Hence we explicitly make sure that we catch such things and print stack traces in the event of
// an invocation so that it can be fixed.
ids := make([]string, 0)
Copy link
Member

Choose a reason for hiding this comment

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

ids = append(ids, containerID)
} else {
logp.Err("Docker container ID can't be empty for Docker input config")
logp.Debug("docker", "stack trace for empty docker container ID is %v", string(debug.Stack()))
Copy link
Member

Choose a reason for hiding this comment

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

fyi: The structured logger has support for logging a stack trace. No obligation to change anything - just fyi.

Normally you would create a logger instance and use it for the life of your struct/object, but this code hasn't been update to do this and it's still using the global logger. But the form would be

log := logp.NewLogger("docker") 
log.Error("Docker container ID can't be empty for Docker input config")
log.Debugw("Empty Docker container container ID was received", logp.Stack("stacktrace"))

or to use the structured logger in an ad-hoc manner

logp.L().Named("docker").Debugw("Empty Docker container container ID was received", logp.Stack("stacktrace"))

if containerID != "" {
ids = append(ids, containerID)
} else {
logp.Err("Docker container ID can't be empty for Docker input config")
Copy link
Contributor

Choose a reason for hiding this comment

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

In what cases is the container id empty? I wonder if we should log it on the error level, meaning it's something the user can fix and then the error will disappear or it's something that just happens from time to time. In this case we better log it on the info level.

Do we really need 2 log entries or could we have just one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the 2 log entries are just to have something on record for via err and dig further in case of a debug scenario.

we havent really figured out what the scenario is for an empty container. hence the reasoning for adding stack trace to figure out what that place could be.

@vjsamuel vjsamuel force-pushed the check_empty_cid_docker_input branch from 4546b84 to 8980d9e Compare August 14, 2018 16:22
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Code LGTM. See fmt comment.

"github.com/elastic/beats/filebeat/input/log"
"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/common/cfgwarn"
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need make fmt to clean up the identation.

@@ -37,6 +40,10 @@ func init() {
}
}

var (
logger = logp.NewLogger("docker")
Copy link
Member

Choose a reason for hiding this comment

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

Move this into NewInput. See the documentation of NewLogger for the reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense why i didnt see that log message lol :)

@vjsamuel vjsamuel force-pushed the check_empty_cid_docker_input branch from 8980d9e to 130b0cf Compare August 15, 2018 16:58
@vjsamuel vjsamuel force-pushed the check_empty_cid_docker_input branch from 130b0cf to b4b8c06 Compare August 15, 2018 17:21
@ruflin ruflin merged commit 2f65060 into elastic:master Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants