Live PRKI Origin Validation Annotation

[salsh]

• The BGPStream will be extended by Live PRKI Origin Validation Annotation
• All details concerning the provided functions, annotation elements and output format are described in issue CAIDA/bgpstream#19

[alistairking]

@salsh thanks for submitting this Pull Request. I wanted to let you know that I'm working on reviewing the changes and hope to have an update for you early next week.

[salsh]

@alistairking alright. Thank you for letting me know that.

[salsh]

Ping @alistairking

[alistairking]

Sorry guys, I have been totally swamped this month, but I haven't forgotten about you :)

[salsh]

Ping @alistairking

[waehlisch]

@alistairking, ping again ;). I know, PAM deadline is coming soon and IMC will take place next month, however, would be good to get some feedback soonish. Thanks!

[alistairking]

Thank you very much for the call today, it was really helpful. My comments below are mostly things that we talked about, but there are a couple of things that I noticed/remembered after we got off the call. Let me know if anything is unclear.

configure.ac: Currently bgpstream will build with RTR support if the library is installed, but users may want to choose not to build with rtr support even if the library is installed. We usually use something like the following to allow the user to manually prevent a feature:

# shall we build with support for kafka?
AC_MSG_CHECKING([whether to build kafka backend])
AC_ARG_WITH([kafka],
    [AS_HELP_STRING([--without-kafka],
      [do not compile the kafka backend])],
      [],
      [with_kafka=yes])
AC_MSG_RESULT([$with_kafka])

AM_CONDITIONAL([WITH_KAFKA], [test "x$with_kafka" != xno])

if test x"$with_kafka" = xyes; then
AC_CHECK_LIB([rdkafka], [rd_kafka_query_watermark_offsets], ,
               [AC_MSG_ERROR(
                 [librdkafka is required for kafka (--without-kafka to disable)])])

AC_DEFINE([WITH_KAFKA],[1],[Building kafka backend])
fi

in this example, passing --without-kafka will force WITH_KAFKA to be unset.

bgpstream.[ch] (and others) As discussed, please move the rtr_mgr_config structure instance (cfg_tr) inside the bgpstream_t structure (in bgpstream.h) and update all "methods" that need to have access to the config to have bgpstream_t *bs as the first argument.

bgstream_utils_as_path.[ch]

• Move your code into bgpstream_utils_rpki.[ch] (sorry for the confusion)
• Consider using a hash table to map from ASN to prefixes. BGPStream includes a nice hash table implementation (https://github.com/CAIDA/cc-common/blob/3557514be5d777bc554b382389f1606b5e7bb088/khash.h), but it can be a little confusing at first, so I'm happy to have a call to discuss how to use it.
• Consider using prefixes as the key (i.e. pfx->asn rather than asn->pfxes), or even consider maintaining mappings in both directions (as we discussed this really depends on how you expect people to use this data in their applications).

tests Please add a functional test that creates an instance of BGPStream, configures some filters, enables RPKI, and then reads some elems. Since you have to run in live mode, I'm not sure that you can check for correctness, but it would be nice to just make sure things don't return error codes/crash, etc.

bgpreader I think the code that parses the -R option argument can be made simpler (maybe using strsep), but lets consider this comment a "MAY" rather than "SHOULD" or "MUST" :)

general comments

• Be careful to handle error conditions (e.g., malloc, strdup, etc.). Functions that return void should have no possibility of failing.
• Be careful when casting bgpstream_pfx_t * to bgpstream_pfx_storage_t *, the bgpstream_pfx_t may actually be backed by a bgpstream_ipv4_pfx_t structure, and so will result in a buffer overrun condition when it is cast to bgpstream_pfx_storage_t and then copied.
• Please don't use bgpreader as the "primary" user when you are thinking about implementing features, try to consider more generally applications that directly use the C API (bgpreader is a simplistic example of this). E.g., an application may legitimately have multiple (different) instances of BGPStream.

code style (again, my apologies for not already having a document that describes this)

• Please use #ifdef XX and #ifndef (or #else) instead of #if defined(XX) and #if !defined(XX)

[salsh]

@alistairking I have to thank you for the call, the review and the hints. I really appreciate your time and effort. I'm working on your suggestions and will push it as soon as possible. If there are any questions about khash I will gladly accept the offer.

[yswery]

I know this is old and all and is linked back to an issue thats been open since 1+ years ago, But I am still hoping to see this get merged in. Would love to be able to stream the RPKI along side all the rest of the data

[alistairking]

@yswery thanks for your interest. We're working on getting this merged, and hopefully we can get this taken care of in the next few weeks.

[jaredthecoder]

Is this still happening?

[yswery]

I am with @jaredthecoder would still be keen to see this merged (of course after conflicts solved etc etc)

[alistairking]

Yep, it's still happening! Our plan is to merge this functionality in the v2 release of bgpstream which should be out sometime in the next few months. (Hopefully.)