bgpd: update AS value of a hidden bgp instance

[askorichenko]

'import vrf CUSTOM' could define a hidden bgp instance with the default AS_UNSPECIFIED (i.e. = 1) value.
When a
router bgp AS vrf CUSTOM
gets configured later on, replace this AS_UNSPECIFIED setting with a requested value.

[askorichenko]

Let config have

• a custom "vrf D" defined and
• a bgp router in that vrf that imports default vrf

frr# show run
Current configuration:
...
!
vrf D
exit-vrf
!
router bgp 65001 vrf D
 !
 address-family ipv4 unicast
  import vrf default
 exit-address-family
exit
!

This operation defines a 'hidden bgp' in the default vrf

• it has bgp->as = AS_UNSPECIFIED (i.e = 1)
• a flag (bgp->flags & BGP_FLAG_INSTANCE_HIDDEN) == BGP_FLAG_INSTANCE_HIDDEN

To define a real bgp router in the default vrf now, this hidden structure would be selected for update.
But bgp_create() function would miss to update "bgp->as, bgp->as_pretty" values.
The router would remain in AS = 1.

frr# conf
frr(config)# router bgp 65001 
frr(config-router)# exit
frr(config)# exit
frr#
frr#
frr# show run 
Current configuration:
...
!
vrf D
exit-vrf
!
router bgp 65001 vrf D
 !
 address-family ipv4 unicast
  import vrf default
 exit-address-family
exit
!
router bgp 1
exit

This patch of bgp_create() adds an overwrite of as, as_pretty values for a hidden bgp instance.

[donaldsharp]

I'd like to see a topotest for this situation... Unless @ton31337 doesn't think we need it.

[ton31337]

Agree, a topotest would be great 👍

[askorichenko]

Hi,
Submitted the topotest
tests: bgp routers with vrf cross import #17907

[askorichenko]

The current code has

if (hidden) {
  bgp = bgp_old;
  goto peer_init;
}

The logic of this change adds setting 3 values from the function arguments

if (hidden) {
  bgp = bgp_old;
  bgp->as = *as;
  bgp->as_pretty = as_pretty;
  bgp->asnotation = asnotation;
  goto peer_init;
}

It's only written slightly different to utilize identical code for the '! hidden' case.

Please review

[askorichenko]

Hello,
If the change was approved, could it get merged, please?
Thank you!

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[ton31337]

@askorichenko could you rebase please?

[askorichenko]

Done

[louis-6wind]

You are duplicating code that are just after the peer_init label

4358f12#diff-2a485107bd513dd693e99b63de03350c46f55affadfda5082728e7060a0ce021R3479

Duplication is incorrect and it creates some memory leaks.

I have found the same issue and here is my fix. I am doing the same thing without duplicating code.
4ea376f

[ton31337]

So, can you both combine two PRs in a single one, but have a topotest which is here?

[louis-6wind]

yes

[askorichenko]

Commit collides with

commit 2ff08af78e315c69795417d150cd23649f68c655
Author: Chirag Shah <[email protected]>
Date:   Mon Feb 10 18:56:15 2025 -0800

    bgpd: fix bgp vrf instance creation from implicit

merged 3 days ago

Here is the above patch by @chiragshah6
https://github.com/FRRouting/frr/pull/18081/files,
And that patch's code obviously zlog_debugs a null string in line 3425 of the new version of the file bgpd/bgpd.c.

Reverting to the previous version of my patch (this preserves log messaging order)

[louis-6wind]

Feel free to provide your own patch. But use a working topotest. For example: 0b2b477

[louis-6wind]

And that patch's code obviously zlog_debugs a null string in line 3425 of the new version of the file bgpd/bgpd.c.

the null pointer issue is fixed in #18119

[louis-6wind]

Continuing in #18119

The pull-request fix is incorrect and the topotest even passes without the fix.

The fixes for the described issue:
9680831
9e343e3
and its topotest:

0b2b477

[louis-6wind]

Your fix was obviously wrong and the test was not testing anything.

I am fixing your issue in #18119

If it is not fixing your issue, please open a new pull-request and we will look at it.