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: Added API calls for routing and resources management #8017

Merged
merged 10 commits into from Jan 18, 2018

Conversation

Projects
None yet
3 participants
@zombah
Contributor

zombah commented Jan 3, 2018

This PR includes API calls:
VRF listing and optional filter by hostname or vrfname
Get VRF by vrf id
Cbgp listing and optional filter by hostname
All VLANs listing
All IP Addresses listing
All IP Networks listing
Get IP Addresses by network id

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

zombah added some commits Jan 3, 2018

api: Added API call for VRF listing
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
api: Added API call to retrive VRF by ID
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
@laf

This comment has been minimized.

Member

laf commented Jan 4, 2018

Thanks for this. Why not add checks to see if a user has perms to see those devices so you don't have to restrict this to read only and above?

@laf laf added the API label Jan 4, 2018

api: Change list_vrf permissions
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
@zombah

This comment has been minimized.

Contributor

zombah commented Jan 5, 2018

Changed list_vrf to use device permissions. For get_vrf it wont work because this function has no per device capabilities.

@zombah zombah changed the title from api: Added API call for VRF listing to api: Added API calls for routing and resources management Jan 5, 2018

@zombah

This comment has been minimized.

Contributor

zombah commented Jan 5, 2018

I will add couple other new api calls i have to this PR

zombah added some commits Jan 5, 2018

api: Added new api calls for routing and resources mgmt
* This commit includes list_cbgp, list_vlans,
list_ip_addresses, list_ip_networks and
get_network_ip_addresses calls

Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
fix: Correct indentation issue in get_ip_addresses
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
@laf

An initial pass for comments done.

You've got a lot of is_read() checks going on when you could actually check if the user has permission for the device using a query as per other calls in the api. The idea being:

if is_admin() or is_read() == false then you need to expand the mysql query to check for devices_perms as well to see if the normal user has access to those devices (see other api calls for examples).

$sql_params = array();
$hostname = $_GET['hostname'] ?: '';
$device_id = ctype_digit($hostname) ? $hostname : getidbyname($hostname);
if (is_numeric($device_id)) {

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

We should error if this doesn't return true.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

Will fix in followup

if (!is_numeric($ip_addresses_count)) {
api_error(500, 'Error retrieving IP Addresses');
}
if ($ip_addresses_count == 0) {

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

This just means it might have no IPs. I wouldn't indicate the device doesn't exist here.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

Will fix in followup

if (!is_numeric($ip_addresses_count)) {
api_error(500, 'Error retrieving IP Addresses');
}
if ($ip_addresses_count == 0) {

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

Same goes for this.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

Will fix in followup

$sql = ' AND `vrfs`.`vrf_name`=?';
$sql_params = array($vrfname);
}
check_device_permission($device_id);

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

This needs to be moved into the is_numeric check above otherwise we don't have a device_id potentially. Should fall back to is_read() if if (is_numeric($device_id)) { is false.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

Will fix in followup.

@@ -1343,6 +1445,64 @@ function list_ipsec()
api_success($ipsec, 'ipsec');
}
function list_vlans()

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

Should also allow to pass hostname / device_id to limit the response.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

This will duplicate functionality of existing get_vlans function. I can add full listing into get_vlans, but i thought more simple functions better than fewer but with more nested conditions.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

Just realized that to have hostname listing is better in list functions, i will add it and in next pr will change get_vlans to more suitable form for get function, to get single vlan by its id.

This comment has been minimized.

@laf

laf Jan 6, 2018

Member

Thanks, forgot we had list_vlans but yes, better to just allow passing the hostname into that function and append an extra sql query.

}
function list_ip_addresses()

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

Should also allow to pass hostname / device_id to limit the response.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

Same as get_vlans above but with get_ip_addresses function.

This comment has been minimized.

@laf

laf Jan 6, 2018

Member

Then one function with the ability to pass hostname in would be good.

}
function list_ip_networks()

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

Should also allow to pass hostname / device_id to limit the response.

This comment has been minimized.

@zombah

zombah Jan 6, 2018

Contributor

Networks tables does not have any keys to other tables now, we can make triple join with ip_addresses table then with ports which have device_id, if this suitable i'll update or maybe you have any better idea how to get device_id with single join?

This comment has been minimized.

@laf

laf Jan 6, 2018

Member

Leave it for now as is_read() then.

zombah added some commits Jan 8, 2018

api: Fix per device permissions and messages
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
Fix identation issues
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
$bgp_counters[] = $bgp_counter;
}
$total_bgp_counters = count($bgp_counters);
if (!is_numeric($total_bgp_counters)) {

This comment has been minimized.

@laf

laf Jan 8, 2018

Member

This check seems pointless to me.

$ipv4 = dbFetchRows("SELECT * FROM `ipv4_addresses` WHERE `ipv4_network_id` = ?", array($network_id));
$ipv6 = dbFetchRows("SELECT * FROM `ipv6_addresses` WHERE `ipv6_network_id` = ?", array($network_id));
$ip_addresses_count = count(array_merge($ipv4, $ipv6));
if (!is_numeric($ip_addresses_count)) {

This comment has been minimized.

@laf

laf Jan 8, 2018

Member

Again, I think you can probably get rid of these.

$sql_params[] = $device_id;
}
if (!is_admin() && !is_read()) {
$sql .= " AND `bgpPeers_cbgp`.`device_id` IN (SELECT device_id FROM devices_perms WHERE user_id = ?)";

This comment has been minimized.

@laf

laf Jan 8, 2018

Member

Can you convert this sub query to a JOIN like the other code has for user lookup so things are consistent. Same goes for any other sub queries you do.

}
$vrfs[] = $vrf;
}
$total_vrfs = count($vrfs);

This comment has been minimized.

@laf

laf Jan 8, 2018

Member

Same here, probably can remove this check.

$vrf = dbFetchRows("SELECT * FROM `vrfs` WHERE `vrf_id` IS NOT NULL AND `vrf_id` = ?", array($vrfId));
$vrf_count = count($vrf);
if (!is_numeric($vrf_count)) {

This comment has been minimized.

@laf

laf Jan 8, 2018

Member

Same again, drop this.

$vlans[] = $vlan;
}
$vlans_count = count($vlans);
if (!is_numeric($vlans_count)) {

This comment has been minimized.

@laf

laf Jan 8, 2018

Member

Same again + the other count checks you have.

zombah added some commits Jan 8, 2018

api: Remove excessive result counters
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
api: Add list_locations api call
* Get devices by location not very comfortable
without list all locations api call

Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
@laf

Mostly just small changes to the response text.

$locations = dbFetchRows("SELECT `locations`.* FROM `locations` WHERE `locations`.`location` IS NOT NULL");
$total_locations = count($locations);
if ($total_locations == 0) {
api_error(404, 'Locations does not exist');

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

Locations do not exist

@@ -822,11 +875,28 @@ function get_ip_addresses()
check_device_permission($device_id);
$ipv4 = dbFetchRows("SELECT `ipv4_addresses`.* FROM `ipv4_addresses` JOIN `ports` ON `ports`.`port_id`=`ipv4_addresses`.`port_id` WHERE `ports`.`device_id` = ? AND `deleted` = 0", array($device_id));
$ipv6 = dbFetchRows("SELECT `ipv6_addresses`.* FROM `ipv6_addresses` JOIN `ports` ON `ports`.`port_id`=`ipv6_addresses`.`port_id` WHERE `ports`.`device_id` = ? AND `deleted` = 0", array($device_id));
$ip_addresses_count = count(array_merge($ipv4, $ipv6));
if ($ip_addresses_count == 0) {
api_error(404, "Device $device_id does not have IP Addresses");

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

Device $device_id does not have any IP addresses

} elseif (isset($router['portid'])) {
$port_id = urldecode($router['portid']);
check_port_permission($port_id, null);
$ipv4 = dbFetchRows("SELECT * FROM `ipv4_addresses` WHERE `port_id` = ?", array($port_id));
$ipv6 = dbFetchRows("SELECT * FROM `ipv6_addresses` WHERE `port_id` = ?", array($port_id));
$ip_addresses_count = count(array_merge($ipv4, $ipv6));
if ($ip_addresses_count == 0) {
api_error(404, "Port $port_id does not have IP Addresses");

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

Port $port_id does not have any IP addresses

$ipv6 = dbFetchRows("SELECT * FROM `ipv6_addresses` WHERE `ipv6_network_id` = ?", array($network_id));
$ip_addresses_count = count(array_merge($ipv4, $ipv6));
if ($ip_addresses_count == 0) {
api_error(404, "IP Network $network_id does not exist or empty");

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

IP Network $network_id does not exist or is empty

}
$total_vrfs = count($vrfs);
if ($total_vrfs == 0) {
api_error(404, 'VRFs does not exist');

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

VRFs do not exist

}
$vlans_count = count($vlans);
if ($vlans_count == 0) {
api_error(404, 'VLANs does not exist');

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

VLANs do not exist

$ipv6_addresses = dbFetchRows("SELECT * FROM `ipv6_addresses`");
$ip_addresses_count = count(array_merge($ipv4_addresses, $ipv6_addresses));
if ($ip_addresses_count == 0) {
api_error(404, 'IP Addresses does not exist');

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

IP addresses do not exist

$ipv6_networks = dbFetchRows("SELECT * FROM `ipv6_networks`");
$ip_networks_count = count(array_merge($ipv4_networks, $ipv6_networks));
if ($ip_networks_count == 0) {
api_error(404, 'IP Networks does not exist');

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

IP networks do not exist

Spelling fix
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
@zombah

This comment has been minimized.

Contributor

zombah commented Jan 17, 2018

@laf Thank you for review! Where were my eyes... Submited fix

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 17, 2018

The inspection completed: 695 Issues, 25 Patches

@laf

laf approved these changes Jan 18, 2018

@laf laf merged commit 06c4676 into librenms:master Jan 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@laf

This comment has been minimized.

Member

laf commented Jan 18, 2018

Thanks @zombah :D

@zombah zombah deleted the zombah:api-vrfs branch Jan 18, 2018

@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.