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

Kubernetes: node autodiscover, add InternalIP #15930

Merged

Conversation

odacremolbap
Copy link
Contributor

complementary to #14738 (comment)

Autodiscover for nodes should use not only the external IP but also the internal one.

@odacremolbap odacremolbap added containers Related to containers use case Team:Platforms Label for the Integrations - Platforms team review labels Jan 29, 2020
@odacremolbap odacremolbap requested a review from a team January 29, 2020 16:08
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

At first glance it looks a bit complicated and I was thinking for a while how you can make this straighforward...
What do you think about splitting responsibilities here into half, firstly - find external and internal IP, secondly - take a decision about IP address. You may even don't need a comment :)

}

// getAddress returns an IP address from a kubernetes node.
//
// It will use external IP if existing, falling back to Internal IP if not,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: either InternalIP or internal IP

}

// getAddress returns an IP address from a kubernetes node.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Kubernetes

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

LGTM

@exekias
Copy link
Contributor

exekias commented Jan 30, 2020

With this change we have an order of preference for the address we return. We should probably document it, WDYT?

Would it make sense to also expose both internal and external IPs in other fields of the event?

@odacremolbap
Copy link
Contributor Author

@sayden thanks for the 👍

@mtojek ok, I'll push your suggested changes

@exekias I will add a line on how the host field gets populated.

@odacremolbap odacremolbap merged commit 6273589 into elastic:master Jan 31, 2020
@odacremolbap odacremolbap deleted the task/use-any-node-ip-kubernetes branch January 31, 2020 11:55
odacremolbap pushed a commit to odacremolbap/beats that referenced this pull request Jan 31, 2020
* use any node ip

(cherry picked from commit 6273589)
odacremolbap pushed a commit that referenced this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case review Team:Platforms Label for the Integrations - Platforms team v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants