Hi David,
Thanks for the changes. Everything looks like it's in working order,
except for one error handling bug in commit #4 (see my reply for commit
#4, which I will send shortly).
There are a bunch of issues/bugs that shouldn't affect proper operation,
but would be cleaned up in an ideal world (read: if the time it would
take to clean them up was worthwhile). Here is a summary of the bigger
items:
* These changes introduce a LOT of duplicated code.
* Most of the commits should be split into multiple separate
commits. This is especially true for commit #4.
* Type mismatches for database API (uint32_t when it should be
unsigned int, uint64_t when it should be unsigned long long,
uint8_t when it should be unsigned char, etc.).
* Potential alignment/padding/offset issues in fillPduIpPrefix()
* The code makes an assumption that you can always distinguish
address families via address length.
* Lots of verbiage could be eliminated via initializer lists.
I'll reply in-line to each patch pointing out specific issues, but most
of the issues are minor enough to not be worth fixing now. (This is
especially true given that we want to get a release out very soon.) We
can clean things up later (after these commits are merged and we've cut
a new release), though some of the issues may not ever be worth fixing.
I'll leave it to your judgment to decide which are worth fixing and
which are not.
A comment about my review style: When I ask a question like:
What about foo?
I'm mostly trying to convey:
Is this a bug or intentional? If intentional, someone reading this
code might wonder to themselves, 'What about foo?', so there should
be a supporting comment.
In other words, don't tell me the answer to the question, tell all of
the people that could potentially read the code.
Specific comments for commit #1 are below.
On 2015-02-11 15:59, David Mandelberg wrote:
> From: David Mandelberg <dmand...@bbn.com>
>
> This commit does two things to improve how prefixes are stored in the
> database:
> 1. For ROAs, switch from a single, very large cell per ROA to a
> separate table with one row per prefix. This removes an upper
> bound on the number of prefixes per ROA.
> 2. For all prefixes, switch from a textual format to a simple binary
> format.
>
> WARNING: This commit only updates the schema, it does not update any
> of the code that relies on the schema. As such, this commit breaks a
> lot of code. Subsequent commit(s) will update that code to work again.
>
> addresses [#6] and [#7]
> ---
> bin/rpki/upgrade.in | 105
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
> lib/rpki/scmmain.h | 52 +++++++++++++++++++++++---
> 2 files changed, 151 insertions(+), 6 deletions(-)
>
> diff --git a/bin/rpki/upgrade.in b/bin/rpki/upgrade.in
> index 1ecfb57..7334e6f 100644
> --- a/bin/rpki/upgrade.in
> +++ b/bin/rpki/upgrade.in
Not really related to this commit, but:
* Since there is no daemon other than the database itself, is
there a way to lock everything during the upgrade? (don't want
the update cron job to run during the middle of an upgrade)
* What happens if someone tries to downgrade?
> @@ -52,10 +52,109 @@ show_msgs () {
> }
>
>
> -upgrade_from_0_9 () {
> +upgrade_from_0_10 () {
> + echo "Clearing rpki-rtr data before altering schema."
It'd be nice if we had a 'log' function in shell_utils, then use that
instead of 'echo' for status messages.
> + rpki-rtr-clear -f || \
> + {
> + error "Could not clear rpki-rtr data."
> + return 1
> + }
Use 'fatal ...' instead of '{ error ...; return 1; }'.
It'd be nice if we had a 'try' function in shell_utils, then the above
could simply be 'try rpki-rtr-clear -f'.
> +
> + echo "Removing all ROAs from the database before altering schema."
> + find "`config_get RPKICacheDir`" -type f -name \*.roa \
Two things:
* use $(...) instead of `...`
* collect the output of 'config_get' in a separate line to check its
return value. for example:
RPKICacheDir=$(try config_get RPKICacheDir) || exit 1
find "${RPKICacheDir}" ...
> + -exec rcli -d "{}" ";" || \
Two things:
* style: instead of:
foo || \
{
stuff
}
I prefer:
foo || {
stuff
}
Or, if 'stuff' is a single line:
foo || stuff
Or, if the line would be too long:
foo \
|| stuff
* 'find' won't exit non-0 if rcli fails unless you use the '+'
variant of '-exec' rather than the ';' variant. If rcli can't
take multiple filename arguments, you can do something like this:
find ... -exec sh -c '
' - '{}' + \
|| add_msg "..."
> + {
> + add_msg "\
> +Potentially failed to remove all ROA files before upgrading the
> +database schema. This is probably not a problem."
> + }
> +
> + echo "Updating the database schema."
> + {
> + echo "ALTER TABLE rpki_roa DROP COLUMN ip_addrs;"
It's good to be in the habit of avoiding 'echo' even if the
argument(s) avoid all POSIX ambiguities ('printf' is more portable).
> +
> + echo \
> + "CREATE TABLE rpki_roa_prefix (" \
> + " roa_local_id INT UNSIGNED NOT NULL," \
> + " prefix VARBINARY(16) NOT NULL," \
> + " prefix_length TINYINT UNSIGNED NOT NULL," \
> + " prefix_max_length TINYINT UNSIGNED NOT NULL," \
> + " KEY (roa_local_id, prefix)," \
> + " FOREIGN KEY (roa_local_id) REFERENCES rpki_roa (local_id)" \
> + " ON DELETE CASCADE" \
> + " ON UPDATE CASCADE," \
> + " CHECK (length(prefix) = 4 OR length(prefix) = 16)," \
> + " CHECK (prefix_length <= prefix_max_length)," \
> + " CHECK (prefix_max_length <= length(prefix) * 8)" \
> + ");"
> +
> + # Note that this does not add SCM_CHECKS_PREFIX_MAXLEN because
> + # MySQL's ALTER TABLE statement doesn't provide the syntax to
> + # add a CHECK. MySQL ignores CHECKs completely though, so this
> + # doesn't really matter.
Even though the MySQL reference doesn't clearly show that it supports
'ALTER TABLE ... ADD CHECK ...', the reference does say, "The CHECK
clause is parsed but ignored by all storage engines." The syntax is
standard SQL and should be supported by MySQL even if the MySQL
documentation doesn't say that it is supported.
Perhaps it would be better to use a TRIGGER rather than a CHECK
constraint (or switch to PostgreSQL). Support for TRIGGER was added
in MySQL 5.5.
> + echo \
> + "ALTER TABLE rtr_full " \
> + " DROP PRIMARY KEY," \
> + " DROP COLUMN ip_addr," \
> + " ADD COLUMN (" \
> + " prefix VARBINARY(16) NOT NULL," \
> + " prefix_length TINYINT UNSIGNED NOT NULL," \
> + " prefix_max_length TINYINT UNSIGNED NOT NULL)," \
> + " ADD PRIMARY KEY (serial_num, asn, prefix, prefix_length,
> prefix_max_length)" \
> + ";"
Since all ROAs have been removed, why not just drop the table and
rebuild from scratch? That way you get the CHECK constraints (which
is good even if MySQL ignores the CHECK constraints).
> +
> + # See the above note about CHECKs.
> + echo \
> + "ALTER TABLE rtr_incremental " \
> + " DROP PRIMARY KEY," \
> + " DROP COLUMN ip_addr," \
> + " ADD COLUMN (" \
> + " prefix VARBINARY(16) NOT NULL," \
> + " prefix_length TINYINT UNSIGNED NOT NULL," \
> + " prefix_max_length TINYINT UNSIGNED NOT NULL)," \
> + " ADD PRIMARY KEY (serial_num, asn, prefix, prefix_length,
> prefix_max_length)" \
> + ";"
> + } | mysql_cmd || \
> + {
> + error "Could not update the database schema."
> + return 1
> + }
A few things:
* Rather than a bunch of 'echo' commands and double-quoted strings piped
to 'mysql', it'd be nicer to use a here-doc like this:
try mysql_cmd <<\EOF
ALTER TABLE rpki_roa DROP COLUMN ip_addrs;
CREATE TABLE rpki_roa_prefix (
...
);
-- Note that this does not add SCM_CHECKS_PREFIX_MAXLEN because
-- MySQL's ALTER TABLE statement doesn't provide the syntax to
-- add a CHECK. MySQL ignores CHECKs completely though, so this
-- doesn't really matter.
ALTER TABLE rtr_full
...
;
...
EOF
Note that the above also avoids the pipeline, which is always a
plus (it's hard to get exit statuses out of pipelines). It also
uses 'try', eliminating 4 lines of code.
* This duplicates the SQL code in scmmain.h. I think it would be
better to have a C utility that updates the tables using the
strings from scmmain.h, then run that utility from upgrade.in. If
you don't:
- Move the three new columns to a separate variable like you do
in scmmain.h.
- Move the CHECK constraints to a separate variable like you
do in scmmain.h.
> +
> + echo "Re-adding ROAs with new schema."
> + find "`config_get RPKICacheDir`" -type f -name \*.roa -print \
> + | rcli -l || \
> + {
> + error "Failed to re-add ROAs."
> + return 1
> + }
> +
> + echo "Re-initializing rpki-rtr data."
> + rpki-rtr-initialize || \
> + {
> + error "Could not re-initialize rpki-rtr."
> + return 1
> + }
> +
> + # Don't run rpki-rtr-update here, because it's possible that the
> + # user has tweaked their local cache in some way that they're not
> + # ready to push out to routers yet. In the nominal case, our not
> + # running rpki-rtr-update here isn't a big deal because it will
> + # be run reasonably soon by cron, and the routers should be
> + # configured with multiple cache servers anyway.
I think this script should run rpki-rtr-update, perhaps with an option
to not run it. (It seems odd and thus rare for someone to want to
tweak the local cache and upgrade RPSTIR but not want to update the
rpki-rtr db entries.)
> + add_msg "\
> +The rpki-rtr data was cleared and re-initialized, but no updates were
> +created. If you want to start sending data to routers again, run
> +%s-rpki-rtr-update." \
> + "@PACKAGE_NAME@"
> +
> return 0
> }
>
> +upgrade_from_0_9 () {
> + upgrade_from_0_10
> + return $?
> +}
> +
> upgrade_from_0_8 () {
> echo "Attempting to create @pkgvarlibdir@."
> mkdir -p "@pkgvarlibdir@" && chmod 700 "@pkgvarlibdir@" \
> @@ -136,6 +235,10 @@ case "$OLD_VERSION" in
> upgrade_from_0_9 || exit $?
> ;;
>
> + 0.10)
> + upgrade_from_0_10 || exit $?
> + ;;
> +
> "@PACKAGE_VERSION@")
> usage_fatal "Please specify the version you're upgrading from, not" \
> "the version you're upgrading to."
> diff --git a/lib/rpki/scmmain.h b/lib/rpki/scmmain.h
> index 8ae143e..289e7bb 100644
> --- a/lib/rpki/scmmain.h
> +++ b/lib/rpki/scmmain.h
> @@ -8,6 +8,32 @@
>
> #ifdef SCM_DEFINED_HERE
>
> +/**
> + * @brief Column definitions to represent a prefix and maximum length.
> + *
> + * prefix: IPv4 or IPv6 prefix, network byte order, filled with 0s to the
> full
> + * length for the address family. E.g., 1.2.3.0 would be 0x01020300, and
> + * 123:4567:: would be 0x01234567000000000000000000000000
> + *
> + * prefix_length: Number of used bits in prefix.
> + *
> + * prefix_max_length: Maximum length of any sub-prefixes.
Please use a bulleted list here.
> + *
> + * @sa SCM_CHECKS_PREFIX_MAXLEN
> + */
> +#define SCM_COLDEFS_PREFIX_MAXLEN \
> + "prefix VARBINARY(16) NOT NULL," \
> + "prefix_length tinyint unsigned NOT NULL," \
> + "prefix_max_length tinyint unsigned NOT NULL"
A few things:
* Isn't roa_local_id missing here? (nevermind -- it's in the table
definition below, though I'm not yet sure why)
* I think there should be an addr family column here, rather than
assume that length(prefix) == 4 means IPv4 and == 16 means IPv6.
* 'tinyint unsigned' is not capitalized here but is in upgrade.in
and in the rest of this file (style consistency is good)
> +
> +/**
> + * @brief CHECKs for the columns in #SCM_COLDEFS_PREFIX_MAXLEN.
> + */
> +#define SCM_CHECKS_PREFIX_MAXLEN \
> + "CHECK (length(prefix) = 4 OR length(prefix) = 16)," \
> + "CHECK (prefix_length <= prefix_max_length)," \
> + "CHECK (prefix_max_length <= length(prefix) * 8)"
> +
> /*
> * Table definitions
> */
> @@ -107,7 +133,6 @@ static scmtab scmtabbuilder[] = {
> "sig VARCHAR(520) NOT NULL,"
> "sigval INT UNSIGNED DEFAULT 0,"
> "hash VARCHAR(256),"
> - "ip_addrs VARCHAR(32768) NOT NULL,"
> "asn INT UNSIGNED NOT NULL,"
> "flags INT UNSIGNED DEFAULT 0,"
> "local_id INT UNSIGNED NOT NULL UNIQUE,"
> @@ -118,6 +143,18 @@ static scmtab scmtabbuilder[] = {
> " KEY ski (ski)",
> NULL,
> 0},
> + {
> + "rpki_roa_prefix",
> + "ROA_PREFIX",
> + "roa_local_id INT UNSIGNED NOT NULL,"
Don't you want an INDEX on roa_local_id since that's what you use for
the WHERE clause in many queries?
> + SCM_COLDEFS_PREFIX_MAXLEN ","
> + "KEY (roa_local_id, prefix),"
Why create a multi-column INDEX on (roa_local_id, prefix)?
> + "FOREIGN KEY (roa_local_id) REFERENCES rpki_roa (local_id) "
> + " ON DELETE CASCADE "
> + " ON UPDATE CASCADE,"
> + SCM_CHECKS_PREFIX_MAXLEN,
> + NULL,
> + 0},
A few things:
* Does the code properly handle ROAs with multiple entries for the
same prefix? (RFC6482 section 3.3 says that the same prefix can
be listed multiple times in a ROA, but it is NOT RECOMMENDED
because the one with the shorter maxLength can be omitted without
changing the meaning of the ROA.) We should add a test case to
ensure this if we don't have one already.
* Does the code properly handle a ROA with redundant entries? For
example: 10.0.0.0/8-24 and 10.0.0.0/24-24. (As far as I can tell,
RFC6482 doesn't prohibit this case, but it doesn't mention it.)
We should add a test case to ensure this if we don't have one
already.
* Does the code properly handle cross-ROA redundancies? For
example, if ROA #1 has AS foo and 10.0.0/23-24 and ROA #2 has AS
foo and 10.0.0/24-24.
* Why not have a PRIMARY KEY on (roa_local_id, prefix,
prefix_length) since there's no point in having more than one
entry with the same prefix?
* The comment above the rpki_roa table says, "The IP address
information is not stored here; it must be fetched from the file
itself using the ROA read code." Is this still true? Seems like
the IP address info is now in the rpki_roa_prefix table (and was
previously in the ip_addr column).
> { /* RPKI_MANIFEST */
> "rpki_manifest",
> "MANIFEST",
> @@ -207,8 +244,9 @@ static scmtab scmtabbuilder[] = {
> "RTR_FULL",
> "serial_num INT UNSIGNED NOT NULL,"
> "asn INT UNSIGNED NOT NULL,"
> - "ip_addr VARCHAR(50) NOT NULL,"
> - " PRIMARY KEY (serial_num, asn, ip_addr)",
> + SCM_COLDEFS_PREFIX_MAXLEN ","
Ah, this is why SCM_COLDEFS_PREFIX_MAXLEN doesn't have roa_local_id.
> + " PRIMARY KEY (serial_num, asn, prefix, prefix_length,
> prefix_max_length),"
> + SCM_CHECKS_PREFIX_MAXLEN,
> NULL,
> 0},
> { /* RTR_INCREMENTAL */
> @@ -223,12 +261,16 @@ static scmtab scmtabbuilder[] = {
> * x */
> "is_announce BOOLEAN NOT NULL," /* announcement or withdrawal */
> "asn INT UNSIGNED NOT NULL,"
> - "ip_addr VARCHAR(50) NOT NULL,"
> - " PRIMARY KEY (serial_num, asn, ip_addr)",
> + SCM_COLDEFS_PREFIX_MAXLEN ","
> + " PRIMARY KEY (serial_num, asn, prefix, prefix_length,
> prefix_max_length),"
> + SCM_CHECKS_PREFIX_MAXLEN,
> NULL,
> 0},
> };
>
> +#undef SCM_COLDEFS_PREFIX_MAXLEN
> +#undef SCM_CHECKS_PREFIX_MAXLEN
Why undefine these? Seems like they'd be useful for a table upgrade
utility.
> +
> #endif
>
> #endif
>
------------------------------------------------------------------------------
_______________________________________________
rpstir-devel mailing list
rpstir-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rpstir-devel
