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

Add IPv6 support for network table #591

Merged
merged 41 commits into from Jul 4, 2019
Merged

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jun 10, 2019

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10


Add IPv6 support for the network table. This updates the database version to 5.

As we expect an interface to have a number of addresses in the IPv6 world, we introduce a new database table network_addresses with three columns:

  1. id corresponds to the id of the network table and, as such, to a unique hardware address.
  2. ip contains a given IP address corresponding to the hardware address identified by this particular id
  3. lastQuery: Unix timestamp of last query seen from this particular IP address.

Conflict management has been implemented to only update the lastQuery column if a given (id,ip) pair already exists in the database. This avoids multiple entries from the same data.

This is a backend implementation. This frontend needs to be modified as well in order to be able to source the new data. This is not a breaking change as the new version can be used with the current dashboard without any issues.

This template was created based on the work of udemy-dl.

@dschaper
Copy link
Member

Can we be sure that the ip utility is installed in every platform?

@DL6ER
Copy link
Member Author

DL6ER commented Jun 11, 2019

@dschaper Yes, we install it as dependency here or here and use it in the installer here, here, here and in a few other places.

networktable.c Outdated Show resolved Hide resolved
networktable.c Outdated Show resolved Hide resolved
networktable.c Outdated Show resolved Hide resolved
…erence to the network(id) field using a foreign key contraint. SQLite foreign key constraints are used to enforce "exists" relationships between the two tables. Attempting to insert a row into the network_addresses table that does not correspond to any row in the network table will fail, as will attempting to delete a row from the network table when there exist dependent rows in the network_addresses table.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
networktable.c Outdated Show resolved Hide resolved
…t make queries to FTL at the moment.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…last seen in the ARP/NDP cache)

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
networktable.c Outdated Show resolved Hide resolved
Signed-off-by: DL6ER <dl6er@dl6er.de>
…S must be set to ON for each database connection.

Signed-off-by: DL6ER <dl6er@dl6er.de>
…ce uniqueness on a pair of columns.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…his index when adding the entry to the network_addresses table.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@AzureMarker
Copy link
Contributor

Now that we are storing IPs in the network_addresses table, can we remove the ip column from the network table?

@DL6ER
Copy link
Member Author

DL6ER commented Jun 16, 2019

Now that we are storing IPs in the network_addresses table, can we remove the ip column from the network table?

Removing a column is only possible by creating a new table, copying all but the ip column to it, removing the old table, and renaming the new table to network. This might take some time (thinking about those users with large deployments like in schools) and the user should better not interrupt this upgrading process.
However, canceling the process will likely only trigger need for support but shouldn't cause harm as the copying step will take the majority of time and is non-destructive.

I also agree that we can create an entry in network_addresses for each device during the upgrade process to have only one source of truth for the addresses.

networktable.c Outdated Show resolved Hide resolved
Signed-off-by: DL6ER <dl6er@dl6er.de>
…an value on error.

Signed-off-by: DL6ER <dl6er@dl6er.de>
networktable.c Outdated Show resolved Hide resolved
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
database.c Outdated Show resolved Hide resolved
…nnections through a compile time flag.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER force-pushed the new/networktable_ip_neigh branch from 08717e7 to adbdf3f Compare June 25, 2019 06:43
networktable.c Outdated Show resolved Hide resolved
@AzureMarker
Copy link
Contributor

I got this error when it tried to upgrade the database:

[2019-06-27 22:50:41.131 14380] SQLite3 message: abort at 24 in [DROP TABLE network;]: FOREIGN KEY constraint failed (787)
[2019-06-27 22:50:41.131 14380] dbquery(DROP TABLE network;) - SQL error (19): FOREIGN KEY constraint failed
[2019-06-27 22:50:41.131 14380] check_database(19): Disabling database connection due to error
[2019-06-27 22:50:41.131 14380] create_network_addresses_table(): "DROP TABLE network;" failed!
[2019-06-27 22:50:41.131 14380] Network-addresses table not initialized, database not available

@AzureMarker
Copy link
Contributor

It looks like since we enable foreign keys by default, we need to disable them as part of step one:

  1. If foreign key constraints are enabled, disable them using PRAGMA foreign_keys=OFF.

https://www.sqlite.org/lang_altertable.html#otheralter

…es necessary as we enabled foreign key enforcement by default for all database connections.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER requested a review from AzureMarker July 1, 2019 17:47
networktable.c Outdated Show resolved Hide resolved
@DL6ER DL6ER requested a review from AzureMarker July 3, 2019 09:46
AzureMarker
AzureMarker previously approved these changes Jul 4, 2019
Copy link
Contributor

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

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

Approved pending merge conflict resolution.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER merged commit 892da71 into development Jul 4, 2019
@DL6ER DL6ER deleted the new/networktable_ip_neigh branch July 4, 2019 09:43
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/apple-devices-not-in-network-overview/22177/6

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/network-table-seeing-clients-as-hostname-of-router-rather-than-devices/31597/7

@DL6ER DL6ER mentioned this pull request May 6, 2020
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.

None yet

4 participants