Hi,
On Thu, May 14, 2015 at 09:44:22PM +1000, David Gwynne wrote:
> i want relayd to check teh availability of some services and inject
> routes when the service is available. if it is available, i want
> to advertise the routes using ospfd, but i also want the local
> machine to be able to contact the service even if it isnt the carp
> master.
>
> to do that i need to inject the routes twice, once on my real
> interface and again on my carp interfaces. the route on the real
> interface needs to be a higher priority than the carp route.
>
> this shuffles relayd to accomodate this.
>
> it mostly adds stuff to the route statements in routers, but i also
> take priorities away from hosts in tables so i can specify them on
> routes.
>
I'm generally ok with extending the use cases of router, but it seems
that you're replacing one narrow use case with another one.
The router code was mostly done for something like "link balancing",
you have two or more uplink gateways and want to select the route
based on their availability. I'm afraid that moving the priority from
hosts to routes, and changing the semantics, breaks this original use
case somehow.
table <gateways> {
$gw1 ip ttl 1 priority 8,
$gw2 ip ttl 1 priority 52
}
router "uplinks" {
route 0.0.0.0/0
forward to <gateways> check icmp
}
But maybe even multiple routes for something internal:
router "srvlan" {
route 10.10.0.0/16
route 10.40.0.0/16
forward to <srvgateways> check icmp
}
Sometimes different gateways can even be connected via the same interface!
(e.g. with an intermediate switch and a single cable to the OpenBSD box)
> this is an example config:
>
> rns_nogal=130.102.71.227
> rns_neem=130.102.71.229
>
> table <rns> { $rns_nogal ip ttl 1, $rns_neem ip ttl 1 }
>
> router "rns" {
> route 130.102.71.160/31 interface vlan888 priority 8
> route 130.102.71.160/31 interface carp40888 priority 16
> forward to <rns> check icmp
> }
>
I'm wondering, how would you translate my examples above?
> redirect "dns" {
> listen on 130.102.71.160 tcp port 53
> listen on 130.102.71.160 udp port 53
> listen on 130.102.71.161 tcp port 53
> listen on 130.102.71.161 udp port 53
>
> match pftag rns
> forward to <rns> port 53 check icmp
> }
>
> i wish redirects took a prefix. maybe thats a diff for another day.
>
Would make sense.
> anyway, here's the diff.
>
> thoughts? tweaks? ok?
>
In addition to my general concern, see comments inline below.
Reyk
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/parse.y,v
> retrieving revision 1.204
> diff -u -p -r1.204 parse.y
> --- parse.y 2 May 2015 13:15:24 -0000 1.204
> +++ parse.y 14 May 2015 11:30:28 -0000
> @@ -173,6 +173,7 @@ typedef struct {
> %token ROUTER RTLABEL TRANSPARENT TRAP UPDATES URL VIRTUAL WITH TTL
> RTABLE
> %token MATCH PARAMS RANDOM LEASTSTATES SRCHASH KEY CERTIFICATE
> PASSWORD ECDH
> %token EDH CURVE
> +%token NONE LOCAL CONNECTED STATIC OSPF ISIS RIP BGP DEFAULT
A lot of keywords for basically an "enum". See below.
> %token <v.string> STRING
> %token <v.number> NUMBER
> %type <v.string> hostname interface table value optstring
> @@ -182,6 +183,7 @@ typedef struct {
> %type <v.number> redirect_proto relay_proto match
> %type <v.number> action ruleaf key_option
> %type <v.number> tlsdhparams tlsecdhcurve
> +%type <v.number> rtprio
> %type <v.port> port
> %type <v.host> host
> %type <v.addr> address
> @@ -1864,8 +1866,8 @@ router : ROUTER STRING {
> router = rt;
>
> tableport = -1;
> - } '{' optnl routeopts_l '}' {
> - if (!router->rt_conf.nroutes) {
> + } '{' optnl routeropts_l '}' {
> + if (TAILQ_EMPTY(&router->rt_netroutes)) {
> yyerror("router %s without routes",
> router->rt_conf.name);
> free(router);
> @@ -1881,11 +1883,11 @@ router : ROUTER STRING {
> }
> ;
>
> -routeopts_l : routeopts_l routeoptsl nl
> - | routeoptsl optnl
> +routeropts_l : routeropts_l routeroptsl nl
> + | routeroptsl optnl
> ;
>
> -routeoptsl : ROUTE address '/' NUMBER {
> +routeroptsl : ROUTE address '/' NUMBER {
> struct netroute *nr;
>
> if (router->rt_conf.af == AF_UNSPEC)
> @@ -1914,15 +1916,15 @@ routeoptsl : ROUTE address '/' NUMBER {
> YYERROR;
> }
> nr->nr_conf.prefixlen = $4;
> + nr->nr_conf.priority = RTP_DEFAULT;
> nr->nr_conf.routerid = router->rt_conf.id;
> nr->nr_router = router;
> bcopy(&$2.ss, &nr->nr_conf.ss, sizeof($2.ss));
>
> - router->rt_conf.nroutes++;
> - conf->sc_routecount++;
> TAILQ_INSERT_TAIL(&router->rt_netroutes, nr, nr_entry);
> + conf->sc_routecount++;
> TAILQ_INSERT_TAIL(conf->sc_routes, nr, nr_route);
> - }
> + } routeopts_l
> | FORWARD TO tablespec {
> free(hashkey);
> hashkey = NULL;
> @@ -1950,18 +1952,74 @@ routeoptsl : ROUTE address '/' NUMBER {
> }
> router->rt_conf.rtable = $2;
> }
> - | RTLABEL STRING {
> - if (strlcpy(router->rt_conf.label, $2,
> - sizeof(router->rt_conf.label)) >=
> - sizeof(router->rt_conf.label)) {
> - yyerror("route label truncated");
> + | DISABLE { rlay->rl_conf.flags |= F_DISABLE; }
> + | include
> + ;
> +
> +routeopts_l : routeopts_l routeoptsl
> + | routeoptsl
> + | /* empty */
> + ;
> +
> +routeoptsl : INTERFACE STRING {
> + struct netroute *nr =
> + TAILQ_LAST(&router->rt_netroutes, netroutelist);
> + struct netroute_config *c = &nr->nr_conf;
> + size_t len;
> +
> + if (strlen(c->ifname)) {
> + yyerror("interface name already defined");
> free($2);
> YYERROR;
> }
> +
> + len = strlcpy(c->ifname, $2, sizeof(c->ifname));
> free($2);
> +
> + if (len >= sizeof(c->ifname)) {
> + yyerror("interface name truncated");
> + YYERROR;
> + }
> + }
> + | PRIORITY rtprio {
> + struct netroute *nr =
> + TAILQ_LAST(&router->rt_netroutes, netroutelist);
> + struct netroute_config *c = &nr->nr_conf;
> +
> + c->priority = $2;
> + }
> + | RTLABEL STRING {
> + struct netroute *nr =
> + TAILQ_LAST(&router->rt_netroutes, netroutelist);
> + struct netroute_config *c = &nr->nr_conf;
> + size_t len;
> +
> + len = strlcpy(c->label, $2, sizeof(c->label));
> + free($2);
> +
> + if (len >= sizeof(c->label)) {
> + yyerror("route label truncated");
> + YYERROR;
> + }
> + }
> + ;
> +
> +rtprio : NONE { $$ = RTP_NONE; }
> + | LOCAL { $$ = RTP_LOCAL; }
> + | CONNECTED { $$ = RTP_CONNECTED; }
> + | STATIC { $$ = RTP_STATIC; }
> + | OSPF { $$ = RTP_OSPF; }
> + | ISIS { $$ = RTP_ISIS; }
> + | RIP { $$ = RTP_RIP; }
> + | BGP { $$ = RTP_BGP; }
> + | DEFAULT { $$ = RTP_DEFAULT; }
We try to avoid such lists of keywords for enums. With our common
parse.y, every keyword adds a potential conflict with strings that
will have to be quoted. So such enums should use STRING+strcmp instead:
rtprio : STRING {
if (strcasecmp("none", $1) == 0) {
...
} else if ( ...
free($1);
}
> + | NUMBER {
> + if ($1 < 0 || $1 > RTP_MAX) {
> + yyerror("invalid priority value: %d\n", $1);
> + YYERROR;
> + }
> + $$ = $1;
> }
> - | DISABLE { rlay->rl_conf.flags |= F_DISABLE; }
> - | include
> ;
>
> dstaf : /* empty */ {
> @@ -2039,17 +2097,6 @@ hostflags : RETRY NUMBER {
> }
> hst->conf.parentid = $2;
> }
> - | PRIORITY NUMBER {
> - if (hst->conf.priority) {
> - yyerror("priority already set");
> - YYERROR;
> - }
> - if ($2 < 0 || $2 > RTP_MAX) {
> - yyerror("invalid priority value: %d\n", $2);
> - YYERROR;
> - }
> - hst->conf.priority = $2;
> - }
> | IP TTL NUMBER {
> if (hst->conf.ttl) {
> yyerror("ttl value already set");
> @@ -2160,6 +2207,7 @@ lookup(char *s)
> { "append", APPEND },
> { "backlog", BACKLOG },
> { "backup", BACKUP },
> + { "bgp", BGP },
> { "block", BLOCK },
> { "buffer", BUFFER },
> { "ca", CA },
> @@ -2168,8 +2216,10 @@ lookup(char *s)
> { "check", CHECK },
> { "ciphers", CIPHERS },
> { "code", CODE },
> + { "connected", CONNECTED },
> { "cookie", COOKIE },
> { "curve", CURVE },
> + { "default", DEFAULT },
> { "demote", DEMOTE },
> { "destination", DESTINATION },
> { "digest", DIGEST },
> @@ -2192,11 +2242,13 @@ lookup(char *s)
> { "interface", INTERFACE },
> { "interval", INTERVAL },
> { "ip", IP },
> + { "isis", ISIS },
> { "key", KEY },
> { "label", LABEL },
> { "least-states", LEASTSTATES },
> { "listen", LISTEN },
> { "loadbalance", LOADBALANCE },
> + { "local", LOCAL },
> { "log", LOG },
> { "lookup", LOOKUP },
> { "match", MATCH },
> @@ -2205,8 +2257,10 @@ lookup(char *s)
> { "nat", NAT },
> { "no", NO },
> { "nodelay", NODELAY },
> + { "none", NONE },
> { "nothing", NOTHING },
> { "on", ON },
> + { "ospf", OSPF },
> { "params", PARAMS },
> { "parent", PARENT },
> { "pass", PASS },
> @@ -2228,6 +2282,7 @@ lookup(char *s)
> { "response", RESPONSE },
> { "retry", RETRY },
> { "return", RETURN },
> + { "rip", RIP },
> { "roundrobin", ROUNDROBIN },
> { "route", ROUTE },
> { "router", ROUTER },
> @@ -2243,6 +2298,7 @@ lookup(char *s)
> { "source-hash", SRCHASH },
> { "splice", SPLICE },
> { "ssl", SSL },
> + { "static", STATIC },
> { "sticky-address", STICKYADDR },
> { "style", STYLE },
> { "table", TABLE },
> Index: pfe_route.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/pfe_route.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 pfe_route.c
> --- pfe_route.c 22 Jan 2015 17:42:09 -0000 1.9
> +++ pfe_route.c 14 May 2015 11:30:28 -0000
> @@ -21,6 +21,7 @@
> #include <sys/socket.h>
>
> #include <netinet/in.h>
> +#include <net/if_dl.h>
> #include <net/route.h>
> #include <arpa/inet.h>
>
> @@ -39,13 +40,11 @@ struct relay_rtmsg {
> struct sockaddr_in rm_dst;
> struct sockaddr_in rm_gateway;
> struct sockaddr_in rm_netmask;
> - struct sockaddr_rtlabel rm_label;
> } u4;
> struct {
> struct sockaddr_in6 rm_dst;
> struct sockaddr_in6 rm_gateway;
> struct sockaddr_in6 rm_netmask;
> - struct sockaddr_rtlabel rm_label;
> } u6;
> } rm_u;
> };
> @@ -90,7 +89,7 @@ sync_routes(struct relayd *env, struct r
> rt->rt_conf.name, buf, nr->nr_conf.prefixlen,
> host->conf.name,
> HOST_ISUP(host->up) ? "up" : "down",
> - host->conf.priority);
> + nr->nr_conf.priority);
>
> crt.up = host->up;
> memcpy(&crt.nr, &nr->nr_conf, sizeof(nr->nr_conf));
> @@ -106,44 +105,35 @@ sync_routes(struct relayd *env, struct r
> int
> pfe_route(struct relayd *env, struct ctl_netroute *crt)
> {
> + struct iovec iov[3];
> + int iovcnt = 0;
> +
> struct relay_rtmsg rm;
> struct sockaddr_rtlabel sr;
> + struct sockaddr_dl sdl;
> struct sockaddr_storage *gw;
> struct sockaddr_in *s4;
> struct sockaddr_in6 *s6;
> - size_t len = 0;
> + size_t len;
> char *gwname;
> int i = 0;
>
> gw = &crt->host.ss;
> gwname = crt->host.name;
>
> - bzero(&rm, sizeof(rm));
> - bzero(&sr, sizeof(sr));
> + memset(&rm, 0, sizeof(rm));
>
> - rm.rm_hdr.rtm_msglen = len;
> rm.rm_hdr.rtm_version = RTM_VERSION;
> rm.rm_hdr.rtm_type = HOST_ISUP(crt->up) ? RTM_ADD : RTM_DELETE;
> rm.rm_hdr.rtm_flags = RTF_STATIC | RTF_GATEWAY | RTF_MPATH;
> rm.rm_hdr.rtm_seq = env->sc_rtseq++;
> - rm.rm_hdr.rtm_addrs = RTA_DST | RTA_GATEWAY;
> + rm.rm_hdr.rtm_addrs = RTA_DST | RTA_GATEWAY | RTA_NETMASK;
> rm.rm_hdr.rtm_tableid = crt->rt.rtable;
> - rm.rm_hdr.rtm_priority = crt->host.priority;
> -
> - if (strlen(crt->rt.label)) {
> - rm.rm_hdr.rtm_addrs |= RTA_LABEL;
> - sr.sr_len = sizeof(sr);
> - if (snprintf(sr.sr_label, sizeof(sr.sr_label),
> - "%s", crt->rt.label) == -1)
> - goto bad;
> - }
> -
> - if (crt->nr.ss.ss_family == AF_INET) {
> - rm.rm_hdr.rtm_msglen = len =
> - sizeof(rm.rm_hdr) + sizeof(rm.rm_u.u4);
> -
> - bcopy(&sr, &rm.rm_u.u4.rm_label, sizeof(sr));
> + rm.rm_hdr.rtm_priority = crt->nr.priority;
> + len = sizeof(rm.rm_hdr);
>
> + switch (crt->nr.ss.ss_family) {
> + case AF_INET:
> s4 = &rm.rm_u.u4.rm_dst;
> s4->sin_family = AF_INET;
> s4->sin_len = sizeof(rm.rm_u.u4.rm_dst);
> @@ -156,7 +146,6 @@ pfe_route(struct relayd *env, struct ctl
> s4->sin_addr.s_addr =
> ((struct sockaddr_in *)gw)->sin_addr.s_addr;
>
> - rm.rm_hdr.rtm_addrs |= RTA_NETMASK;
> s4 = &rm.rm_u.u4.rm_netmask;
> s4->sin_family = AF_INET;
> s4->sin_len = sizeof(rm.rm_u.u4.rm_netmask);
> @@ -165,12 +154,11 @@ pfe_route(struct relayd *env, struct ctl
> htonl(0xffffffff << (32 - crt->nr.prefixlen));
> else if (crt->nr.prefixlen < 0)
> rm.rm_hdr.rtm_flags |= RTF_HOST;
> - } else if (crt->nr.ss.ss_family == AF_INET6) {
> - rm.rm_hdr.rtm_msglen = len =
> - sizeof(rm.rm_hdr) + sizeof(rm.rm_u.u6);
>
> - bcopy(&sr, &rm.rm_u.u6.rm_label, sizeof(sr));
> + len += sizeof(rm.rm_u.u4);
> + break;
>
> + case AF_INET6:
> s6 = &rm.rm_u.u6.rm_dst;
> bcopy(((struct sockaddr_in6 *)&crt->nr.ss),
> s6, sizeof(*s6));
> @@ -182,7 +170,6 @@ pfe_route(struct relayd *env, struct ctl
> s6->sin6_family = AF_INET6;
> s6->sin6_len = sizeof(*s6);
>
> - rm.rm_hdr.rtm_addrs |= RTA_NETMASK;
> s6 = &rm.rm_u.u6.rm_netmask;
> s6->sin6_family = AF_INET6;
> s6->sin6_len = sizeof(*s6);
> @@ -195,11 +182,64 @@ pfe_route(struct relayd *env, struct ctl
> / 8] = 0xff00 >> i;
> } else if (crt->nr.prefixlen < 0)
> rm.rm_hdr.rtm_flags |= RTF_HOST;
> - } else
> +
> + len += sizeof(rm.rm_u.u6);
> + break;
> +
> + default:
> fatal("pfe_route: invalid address family");
> + }
> +
> + iov[iovcnt].iov_base = &rm;
> + iov[iovcnt].iov_len = len;
> + iovcnt++;
> +
> + if (strlen(crt->nr.ifname)) {
> + i = if_nametoindex(crt->nr.ifname);
> + if (i == 0) {
> + log_debug("%s: gateway %s: interface %s does not exist",
> + __func__, gwname, crt->nr.ifname);
> + return (-1);
> + }
> +
> + memset(&sdl, 0, sizeof(sdl));
> +
> + sdl.sdl_family = AF_LINK;
> + sdl.sdl_len = sizeof(sdl);
> + sdl.sdl_index = i;
> +
> + rm.rm_hdr.rtm_addrs |= RTA_IFP;
> + iov[iovcnt].iov_base = &sdl;
> + iov[iovcnt].iov_len = sizeof(sdl);
> + iovcnt++;
> +
> + len += sizeof(sdl);
> + }
> +
> + if (strlen(crt->nr.label)) {
> + memset(&sr, 0, sizeof(sr));
> +
> + sr.sr_family = AF_UNSPEC;
> + sr.sr_len = sizeof(sr);
> + if (strlcpy(sr.sr_label, crt->nr.label,
> + sizeof(sr.sr_label)) >= sizeof(sr.sr_label)) {
> + log_debug("%s: gateway %s: label is too long",
> + __func__, gwname);
> + return (-1);
> + }
> +
> + rm.rm_hdr.rtm_addrs |= RTA_LABEL;
> + iov[iovcnt].iov_base = &sr;
> + iov[iovcnt].iov_len = sizeof(sr);
> + iovcnt++;
> +
> + len += sizeof(sr);
> + }
> +
> + rm.rm_hdr.rtm_msglen = len;
>
> retry:
> - if (write(env->sc_rtsock, &rm, len) == -1) {
> + if (writev(env->sc_rtsock, iov, iovcnt) == -1) {
> switch (errno) {
> case EEXIST:
> case ESRCH:
> Index: relayd.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v
> retrieving revision 1.161
> diff -u -p -r1.161 relayd.conf.5
> --- relayd.conf.5 9 Mar 2015 17:20:38 -0000 1.161
> +++ relayd.conf.5 14 May 2015 11:30:28 -0000
> @@ -204,12 +204,6 @@ starting with 1; it can be shown with th
> .Xr relayctl 8
> .Ic show summary
> commands.
> -.It Ic priority Ar number
> -Change the route priority used when adding a route.
> -If not specified, the kernel will set a priority of 8 (RTP_STATIC).
> -In ordinary use, a fallback route should be added statically with a very
> -high (e.g. 52) priority.
> -Unused in all other modes.
> .It Ic retry Ar number
> The optional retry option adds a tolerance for failed host checks;
> the check will be retried for
> @@ -1384,10 +1378,16 @@ This entry is mandatory and must be spec
> .It Xo
> .Ic route
> .Ar address Ns Li / Ns Ar prefix
> +.Op Ic interface Ar name
> +.Op Ic priority Ar number
> +.Op Ic rtlabel Ar label
> .Xc
> Specify the network address and prefix length of a route destination
> that is reachable via the active gateways.
> -This entry must be specified at least once in a router directive.
> +An interface, a priority, and a route label can optionally be specified
> +for the route.
> +.Pp
> +At least one route must be specified in a router directive.
You replaced it with a very brief description of "priority".
> .It Ic rtable Ar id
> Add the routes to the kernel routing table with the specified
> .Ar id .
> Index: relayd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
> retrieving revision 1.209
> diff -u -p -r1.209 relayd.h
> --- relayd.h 2 May 2015 13:15:24 -0000 1.209
> +++ relayd.h 14 May 2015 11:30:28 -0000
> @@ -398,7 +399,6 @@ struct host_config {
> char name[HOST_NAME_MAX+1];
> struct sockaddr_storage ss;
> int ttl;
> - int priority;
> };
>
> struct host {
> @@ -803,6 +803,9 @@ struct netroute_config {
> objid_t id;
> struct sockaddr_storage ss;
> int prefixlen;
> + char ifname[IFNAMSIZ];
> + char label[RT_LABEL_SIZE];
> + int priority;
> objid_t routerid;
> };
>
> @@ -820,8 +823,6 @@ struct router_config {
> objid_t id;
> u_int32_t flags;
> char name[HOST_NAME_MAX+1];
> - char label[RT_LABEL_SIZE];
> - int nroutes;
Why are you removing nroutes? It is good for statistics etc.
> objid_t gwtable;
> in_port_t gwport;
> int rtable;
> Index: snmp.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/snmp.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 snmp.c
> --- snmp.c 22 Jan 2015 17:42:09 -0000 1.23
> +++ snmp.c 14 May 2015 11:30:28 -0000
> @@ -1380,8 +1380,7 @@ snmp_router(struct relayd *env, struct s
> break;
> case 5: /* pf label */
> if (snmp_agentx_varbind(resp, oid,
> - AGENTX_OCTET_STRING, router->rt_conf.label,
> - strlen(router->rt_conf.label)) == -1)
> + AGENTX_OCTET_STRING, "", 0) == -1)
The snmp part would need more work.
> return (-1);
> break;
> case 6: /* rtable */
>
--
