On Thu, Nov 25, 2021 at 08:18:10PM +0100, Sebastian Benoit wrote:
> Job Snijders([email protected]) on 2021.11.25 16:13:51 +0000:
> > It might be advantageous to permit operators to optionally specify the
> > maximum number of publication points with which rpki-client will
> > synchronize.
> >
> > For example: "doas rpki-client -m 1 -t /etc/rpki/ripe.tal" has as effect
> > that only RIPE NCC's repository is contacted, but none of the delegated
> > repositories.
> >
> > This flag perhaps permits us to start shipping with a more conservative
> > default than 1000, like 100 or 200. It is clear that encouraging the
> > ecosystem to embrace 'Publish in Parent' is a saner model than everyone
> > running their own delegation.
> >
> > Thoughts?
>
> Yes, if we dont have these configurable, we will have users running into
> them eventually and be sad.
I'm not a big fan of this config option. This is for sure not something
that needs to change between runs and abusing it for the -m 1 case seems
strange.
Will we end up with a option salad for each and every limit? I think this
is a bad direction. rpki-client should just work and the limits need to be
chosen with that in mind.
> maxpubpoints sounds a bit funny, but i have no other idea.
>
> code reads ok.
The way the limit is implemented in this patch is not correct.
More inline
> > Index: main.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > retrieving revision 1.166
> > diff -u -p -r1.166 main.c
> > --- main.c 25 Nov 2021 14:03:40 -0000 1.166
> > +++ main.c 25 Nov 2021 16:08:10 -0000
> > @@ -69,6 +69,7 @@ int verbose;
> > int noop;
> > int rrdpon = 1;
> > int repo_timeout = 15*60;
> > +unsigned int maxpubpoints = 1000;
> >
> > struct stats stats;
> >
> > @@ -714,7 +715,7 @@ main(int argc, char *argv[])
> > "proc exec unveil", NULL) == -1)
> > err(1, "pledge");
> >
> > - while ((c = getopt(argc, argv, "b:Bcd:e:f:jnorRs:t:T:vV")) != -1)
> > + while ((c = getopt(argc, argv, "b:Bcd:e:f:jm:norRs:t:T:vV")) != -1)
> > switch (c) {
> > case 'b':
> > bind_addr = optarg;
> > @@ -738,6 +739,11 @@ main(int argc, char *argv[])
> > case 'j':
> > outformats |= FORMAT_JSON;
> > break;
> > + case 'm':
> > + maxpubpoints = strtonum(optarg, 0, 100000, &errs);
> > + if (errs)
> > + errx(1, "-m: %s", errs);
> > + break;
Why allow 0 as minimum? Will that even work?
> > case 'n':
> > noop = 1;
> > break;
> > @@ -1220,7 +1226,7 @@ usage:
> > fprintf(stderr,
> > "usage: rpki-client [-BcjnoRrVv] [-b sourceaddr] [-d cachedir]"
> > " [-e rsync_prog]\n"
> > - " [-s timeout] [-T table] [-t tal]"
> > - " [outputdir]\n");
> > + " [-m maxpubpoints] [-s timeout] [-T table] "
> > + "[-t tal] [outputdir]\n");
> > return 1;
> > }
> > Index: repo.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/repo.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 repo.c
> > --- repo.c 25 Nov 2021 14:03:40 -0000 1.14
> > +++ repo.c 25 Nov 2021 16:08:17 -0000
> > @@ -41,6 +41,7 @@ extern struct stats stats;
> > extern int noop;
> > extern int rrdpon;
> > extern int repo_timeout;
> > +extern unsigned int maxpubpoints;
> >
> > enum repo_state {
> > REPO_LOADING = 0,
> > @@ -1100,12 +1101,14 @@ ta_lookup(int id, struct tal *tal)
> > if ((rp->repouri = strdup(tal->descr)) == NULL)
> > err(1, NULL);
> >
> > - if (++talrepocnt[id] >= MAX_REPO_PER_TAL) {
> > - if (talrepocnt[id] == MAX_REPO_PER_TAL)
> > - warnx("too many repositories under %s", tals[id]);
> > + if (talrepocnt[id] >= maxpubpoints + 1) {
> > + if (talrepocnt[id] == maxpubpoints)
This can not be right. If the first if is true the 2nd can never be true.
> > + warnx("too many publication points under %s", tals[id]);
> > nofetch = 1;
> > }
> >
> > + talrepocnt[id]++;
> > +
> > rp->ta = ta_get(tal, nofetch);
> >
> > return rp;
> > @@ -1146,11 +1149,12 @@ repo_lookup(int id, const char *uri, con
> > if ((rp->notifyuri = strdup(notify)) == NULL)
> > err(1, NULL);
> >
> > - if (++talrepocnt[id] >= MAX_REPO_PER_TAL) {
> > - if (talrepocnt[id] == MAX_REPO_PER_TAL)
> > - warnx("too many repositories under %s", tals[id]);
> > + if (talrepocnt[id] >= maxpubpoints + 1) {
> > + if (talrepocnt[id] == maxpubpoints)
Same here.
> > + warnx("too many publication points under %s", tals[id]);
> > nofetch = 1;
> > }
> > + talrepocnt[id]++;
> >
> > /* try RRDP first if available */
> > if (notify != NULL)
--
:wq Claudio
