On Wed, Jan 04, 2023 at 12:21:59PM +0100, Claudio Jeker wrote:
> bgpd already supports Open Policy (RFC 9234) and the role of the router is
> set as part of the `announce policy` config. Now ASPA also needs the role
> so it makes sense to promote setting the role in the config.
>
> I also switched the role to an enum mainly because I want 0 to be the
> default "NONE" role. Roles only matter on ebgp sessions and ibgp sessions
> should just use ROLE_NONE.
>
> announce policy now only has three options: yes, no and enforce
> If announce policy is set to yes but no role is defined the policy
> capability is silently switched to no. I'm not super happy with that
> behaviour but I think it is temporary and at one point 'role' will become
> mandatory for ebgp neighbors.
>
> Similar to turning off announce policy, ibgp sessions are forced to
> ROLE_NONE. Roles make no sense for ibgp and so forcing the role to none
> makes sure they remain consistent.
>
> The automatic changes are done so that groups can inherit default values
> that are fixed up if required.
>
> I did the minimal adjustment in bgpd.conf.5 manpage. I think more is
> needed there.
>
> This breaks the policy intergration test but I have a fix for it ready.
Reads fine.
ok with one small comment below.
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.438
> diff -u -p -r1.438 session.c
> --- session.c 28 Dec 2022 21:30:16 -0000 1.438
> +++ session.c 4 Jan 2023 11:11:45 -0000
[...]
> @@ -1442,9 +1483,12 @@ session_open(struct peer *p)
> errs += session_capa_add(opb, CAPA_REFRESH, 0);
>
> /* BGP open policy, RFC 9234, only for ebgp sessions */
> - if (p->capa.ann.role_ena && p->conf.ebgp) {
> + if (p->conf.ebgp && p->capa.ann.role_ena &&
> + p->capa.ann.role != CAPA_NONE) {
This should be ROLE_NONE
> + uint8_t val;
> + val = role2capa(p->capa.ann.role);
> errs += session_capa_add(opb, CAPA_ROLE, 1);
> - errs += ibuf_add(opb, &p->capa.ann.role, 1);
> + errs += ibuf_add(opb, &val, 1);
> }
>
> /* graceful restart and End-of-RIB marker, RFC 4724 */
