Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: fix default instance when leaving the hidden state. #18119

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

louis-6wind
Copy link
Contributor

When unconfiguring a default BGP instance with VPN SAFI configurations,
the default BGP structure remains but enters a hidden state. Upon
reconfiguration, the instance name state is incorrect.

  • its name is "VIEW ?" instead of "VRF Default"
  • the process_queue is not recreating meaning the incoming prefixes are never processed

Fix both issues
Fixes: 4d0e7a4 ("bgpd: VRF-Lite fix default bgp delete")

@ton31337
Copy link
Member

@Mergifyio backport dev/10.3 stable/10.2

Copy link

mergify bot commented Feb 12, 2025

backport dev/10.3 stable/10.2

🟠 Waiting for conditions to match

  • merged [📌 backport requirement]

@ton31337
Copy link
Member

Can we cover this stuff with a topotest?

@louis-6wind
Copy link
Contributor Author

Can we cover this stuff with a topotest?

Correct me if I'm wrong, but no topotest was required when the hidden default instance behavior was introduced. Now, just to fix it, a test is suddenly needed?

@ton31337
Copy link
Member

Yes, because nobody expected the issues with these changes (it wasn't/isn't a feature). But now I see we hit not the first time this area, e.g. #17861. Is this somehow related?

When unconfiguring a default BGP instance with VPN SAFI configurations,
the default BGP structure remains but enters a hidden state. Upon
reconfiguration, the instance name incorrectly appears as "VIEW ?"
instead of "VRF default". And the name_pretty pointer

The name_pretty pointer is replaced by another one with the incorrect
name. This also leads to a memory leak as the previous pointer is not
properly freed.

Do not rewrite the instance name.

Fixes: 4d0e7a4 ("bgpd: VRF-Lite fix default bgp delete")
Signed-off-by: Louis Scalbert <[email protected]>
bgp_process_queue_init() is not called in bgp_create() when leaving the
BGP instance hidden state because of the following goto:

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

Upon reconfiguration of the default instance, the prefixes are never set
into a meta queue by mq_add_handler(). They are never processed for
zebra RIB installation and announcements of update/withdraw.

Do not delete the BGP process_queue when hiding.

Fixes: 4d0e7a4 ("bgpd: VRF-Lite fix default bgp delete")
Signed-off-by: Louis Scalbert <[email protected]>
louis-6wind and others added 5 commits February 14, 2025 18:01
This reverts commit 2ff08af.

The fix is obviously wrong.

Link: 2ff08af
Signed-off-by: Louis Scalbert <[email protected]>
'import vrf VRF' could define a hidden bgp instance with
the default AS_UNSPECIFIED (i.e. = 1) value.
When a
	router bgp AS vrf VRF
gets configured later on, replace this AS_UNSPECIFIED setting
with a requested value.

Fixes: 9680831 ("bgpd: fix as_pretty mem leaks when un-hiding")
Signed-off-by: Alexander Skorichenko <[email protected]>
Signed-off-by: Louis Scalbert <[email protected]>
Test that leaving the hidden BGP instance state is working.

Signed-off-by: Louis Scalbert <[email protected]>
Upon configuration of a VRF instance that references an absent default
VRF with "import vrf default", the default instance is created in hidden
state. However, the default instance is not properly un-hidden when
configured.

Restore the behavior prior to commit below.

Fixes: 9f7177a ("bgpd: fix duplicate BGP instance created with unified config")
Signed-off-by: Louis Scalbert <[email protected]>
Creates the default VRF instance after the other VRF instances. The
default VRF instance is created in hidden state. Check that AS number
in show run is correctly written.

Signed-off-by: Louis Scalbert <[email protected]>
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants