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 */

Reply via email to