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

API mac search #12964

Merged
merged 21 commits into from
Aug 10, 2021
Merged

API mac search #12964

merged 21 commits into from
Aug 10, 2021

Conversation

mpikzink
Copy link
Contributor

@mpikzink mpikzink commented Jun 18, 2021

A useful API search if you are looking for a client that is not included in LNMS. This can be used to locate the connected switchport based on the client MAC.

Please give a short description what your pull request is for

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.

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.

function search_by_mac(Illuminate\Http\Request $request)
{
$search = $request->route('search');
$portlist = PortsFdb::select('port_id')
Copy link
Member

@Jellyfrog Jellyfrog Jun 19, 2021

Choose a reason for hiding this comment

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

I think this should return all ports matching the Mac Instead?
Something like

Port::whereHas('fdbEntries', function($q) use ($search) {
   $q->where('mac_address', $search);
})->get();

Copy link
Contributor Author

@mpikzink mpikzink Jun 19, 2021

Choose a reason for hiding this comment

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

I just want to get the Downlink.
e.g. :

PortsFdb::whereIn('port_id',PortsFdb::where('mac_address', $search)->select('port_id'))
->groupBy('port_id')
->orderByRaw('count(port_id)')
->select('port_id')
->first()
->port;

Copy link
Member

Choose a reason for hiding this comment

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

@mpikzink your code has two queries, but the inside on isn't actually executed. Also, it looks like you want to select the port, so perhaps you should start with Port:: for the query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Murrat Yes I need a port. But I am looking for a port that "only" has this MAC in the FDB. Traditionally as SQL query I can only achieve this via a subquery so that I can count the remaining MAC addresses on the port in the FDB.

Copy link
Member

Choose a reason for hiding this comment

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

Remember that a MAC can be on many ports (switch downlinks).

Subquery is supported, but that is not the correct syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that it can be on many ports but I think if I search for a client the client will be on the port with the fewest count of other MACs. I have this (first commit) option in our setup since a year and it works very well for our client search which we use multiple times per day. I will have a look for the right syntax for laravel.

BTW: Since my first commit I think my chosen route is also not the best place and I need to describe in the doc as well.

@Jellyfrog
Copy link
Member

I think for this endpoint we except all ports that match to be returned.
Add a filter option if you want more granular results.
Also, there should already be some function to normalize the Mac address, so users can search with different formats

@mpikzink mpikzink changed the title Api mac search WIP Api mac search Jun 21, 2021
@mpikzink mpikzink changed the title WIP Api mac search API mac search Jun 27, 2021
@PipoCanaja PipoCanaja added the API label Jul 4, 2021
LibreNMS/Util/Rewrite.php Outdated Show resolved Hide resolved
@mpikzink mpikzink requested a review from murrant August 3, 2021 03:59
@Jellyfrog Jellyfrog closed this Aug 6, 2021
@Jellyfrog Jellyfrog reopened this Aug 6, 2021
Copy link
Member

@murrant murrant left a comment

Choose a reason for hiding this comment

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

Yay, unit tests :)

@murrant murrant merged commit 93209a0 into librenms:master Aug 10, 2021
@librenms-bot
Copy link

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

https://community.librenms.org/t/21-8-0-changelog/16685/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants