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

Use add_kubernetes_metadata IP & port matching in Packetbeat #5707

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Nov 24, 2017

The code was there for Metricbeat already, this PR moves it to libbeat
and adapts Packetbeat to use it, with good defaults.

@@ -0,0 +1,23 @@
package add_kubernetes_metadata

Choose a reason for hiding this comment

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

don't use an underscore in package name

@@ -0,0 +1,23 @@
package add_kubernetes_metadata

Choose a reason for hiding this comment

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

don't use an underscore in package name

@exekias exekias force-pushed the packetbeat-kubernetes branch from c4453e4 to b041925 Compare November 24, 2017 17:12
The code was there for Metricbeat already, this PR moves it to libbeat
and adapts Packetbeat to include it
@exekias exekias force-pushed the packetbeat-kubernetes branch from b041925 to a990016 Compare November 24, 2017 17:59
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.

LGTM. Some minor comment. For the code comment, I'm aware it's just copy / paste.

hostPorts := h.GetIndexes(pod)
var metadata []MetadataIndex

if pod.Status.PodIP == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

If a pod does not have an IP address, it means no metadata is availabe? Should this return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We wouldn't be dealing with this error anyway, it could make sense to log this, but I think it's just defensive code

@@ -122,6 +122,8 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di

*Packetbeat*

- Configure good defaults for `add_kubernetes_metadata`. {pull}5707[5707]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add 2 change entries. One to "applies to all beats" that the IpProcessor is now available for all beats (also filebeat for example) and the other one as above that IpProcessor is set as default for PB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! I added a new entry

@exekias
Copy link
Contributor Author

exekias commented Nov 27, 2017

Just opened #5721 as a follow-up, with more changes needed after doing some real life usage with Packetbeat and APM

@ruflin ruflin merged commit 3cb7fba into elastic:master Nov 28, 2017
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.

3 participants