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

Bug: Fixed Incorrect device match in Graylog #10501

Merged
merged 3 commits into from Aug 9, 2019

Conversation

@rsys-dev
Copy link
Contributor

commented Aug 7, 2019

Incorrect device match fixed. Because getAddresses(Device $device) also returned 127.0.0.1 and ::1, always matched the first device with a loopback address. Introduced in #10458

Filtered 127.0.0.1 and ::1 in return statement of getAddresses(Device $device)

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Incorrect device match fixed. Because getAddresses(Device $device) al…
…so returned 127.0.0.1 and ::1,

always matched the first device with a loopback address. Introduced in
#10458

@PipoCanaja PipoCanaja added the Bug 🐞 label Aug 7, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Hi @rsys-dev
Makes sense reading it but I have absolutely no way to test it. We'll wait for another admin to review it.

@PipoCanaja PipoCanaja changed the title Graylog: Incorrect device match fixed Bug: Fixed Incorrect device match in Graylog Aug 7, 2019

@@ -145,7 +145,11 @@ public function getAddresses(Device $device)
->merge($device->ipv6->pluck('ipv6_address'));
}
return $addresses->filter()->unique();
return $addresses->filter(

This comment has been minimized.

Copy link
@murrant

murrant Aug 8, 2019

Member

You should only filter 127.0.0.1 from the addresses tables, not when they are hostname or ip. So basically, move this filter up a few lines. Also, try not to use double negatives it makes things hard to understand: return $address == '127.0.0.1' || $address == '0000:0000:0000:0000:0000:0000:0000:0001'; (ipv6_address is not stored compressed)

This comment has been minimized.

Copy link
@rsys-dev

rsys-dev Aug 8, 2019

Author Contributor

You should only filter 127.0.0.1 from the addresses tables, not when they are hostname or ip. So basically, move this filter up a few lines.

Changed IPv6 to uncompressed form and moved the filter up to line 143.

Also, try not to use double negatives it makes things hard to understand: return $address == '127.0.0.1' || $address == '0000:0000:0000:0000:0000:0000:0000:0001';

Not needed anymore but this does not the same as return $address != '127.0.0.1' && $address != '::1'; or did I miss something? Your code will return only 127.0.0.1 and ::1, my code will return everything except 127.0.0.1 and ::1, or am I wrong?

rsys-dev added 2 commits Aug 8, 2019
Changed code as suggested by murrant
Expanded ::1 because IPv6 Adresses are not stored compressed.
@murrant
murrant approved these changes Aug 9, 2019
Copy link
Member

left a comment

Thanks

@murrant murrant merged commit a79837a into librenms:master Aug 9, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

@rsys-dev rsys-dev deleted the rsys-dev:graylog-bugfix branch Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.