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

Vsphere nil pointers access in Metricbeat module #9784

Merged
merged 1 commit into from
Dec 28, 2018

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Dec 24, 2018

It seems that some fields within the vsphere module on Metricbeat are pointers where the nil check access is not being done. This can cause panics that are not easy to reproduce.

This PR checks every pointer that acces a pointer from the gvmomi library which is used to access vsphere data.

Initially, we only know about panics in the virtualmachine metricset but I have seen that the host metricset could potentially have the same problem

@sayden sayden requested a review from ruflin December 24, 2018 16:00
@sayden sayden requested a review from a team as a code owner December 24, 2018 16:02
@sayden
Copy link
Contributor Author

sayden commented Dec 24, 2018

jenkins, test this

@sayden
Copy link
Contributor Author

sayden commented Dec 24, 2018

Fail seems unrelated. Awaiting for review

@sayden sayden self-assigned this Dec 24, 2018
@ruflin ruflin added the needs_backport PR is waiting to be backported to other branches. label Dec 25, 2018
Copy link
Member

@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, thanks for the quick fix. Left two minor comments about logging.

I also added the needs_backport label as I think we should backport this to 6.5, 6.6, 6.x.

CI failure is not related.

metricbeat/module/vsphere/host/host.go Show resolved Hide resolved
metricbeat/module/vsphere/virtualmachine/virtualmachine.go Outdated Show resolved Hide resolved
@sayden sayden force-pushed the bugfix/mb/vsphere-nil-pointers branch from 1aebe7d to 4dfefcd Compare December 27, 2018 13:58
@ruflin ruflin added Team:Integrations Label for the Integrations team and removed Packetbeat labels Dec 28, 2018
Copy link
Member

@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.

Left a few minor comments. Mostly nit picks to make the code nicer. Let me know what you think.

},
}

if hs.Summary.Hardware != nil {
totalCPU := int64(hs.Summary.Hardware.CpuMhz) * int64(hs.Summary.Hardware.NumCpuCores)
cpu["total"] = common.MapStr{
Copy link
Member

Choose a reason for hiding this comment

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

What you can use as an alternative here is cpu.Put('total.mhz', totalCPU). It will have the same effect. Using this you could still generate the full event as you had previously on line 118 and then use:

event.Put('cpu.total.mhz', totalCPU)

The outcome of the code is exactly the same but it would make it shorter and hopefully more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entries have been updated. I quick look will give me some calm brain cycles 😄

metricbeat/module/vsphere/host/host.go Show resolved Hide resolved
CHANGELOG.asciidoc Outdated Show resolved Hide resolved
@sayden sayden force-pushed the bugfix/mb/vsphere-nil-pointers branch from 1808197 to 72dbb2d Compare December 28, 2018 11:08
@sayden sayden merged commit 7c31ced into elastic:master Dec 28, 2018
sayden added a commit to sayden/beats that referenced this pull request Dec 28, 2018
…elastic#9784)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit 7c31ced)
@sayden sayden added v6.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Dec 28, 2018
sayden added a commit to sayden/beats that referenced this pull request Dec 28, 2018
…elastic#9784)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit 7c31ced)
@sayden sayden added the v6.5.5 label Dec 28, 2018
sayden added a commit to sayden/beats that referenced this pull request Dec 28, 2018
…elastic#9784)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit 7c31ced)
@sayden sayden added the v6.6.0 label Dec 28, 2018
sayden added a commit that referenced this pull request Dec 31, 2018
…#9784) (#9822)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit 7c31ced)
sayden added a commit that referenced this pull request Dec 31, 2018
…#9784) (#9823)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit 7c31ced)
sayden added a commit that referenced this pull request Dec 31, 2018
…#9784) (#9824)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit 7c31ced)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…elastic#9784) (elastic#9824)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit a05b14e)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…elastic#9784) (elastic#9823)

In Vsphere module, some fields returned by the Vsphere library contain pointers to some values that could potentially be nil. With this PR, we do a nil-check prior every access to any of those fields to avoid their writing in case they are nil and protecting Metricbeat from panicking.

(cherry picked from commit a05b14e)
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