Skip to content

zebra: Do not flush an existing vni configuration trying to remove wrong vni #18108

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.

Sign up for GitHub

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

Jump to bottom
Merged
donaldsharp merged 1 commit into from
Feb 19, 2025
Merged

zebra: Do not flush an existing vni configuration trying to remove wrong vni #18108

donaldsharp merged 1 commit into from
Feb 19, 2025

Conversation

@ton31337
Copy link
Member

@ton31337 ton31337 commented Feb 11, 2025

Closes #17952

@frrbot frrbot bot added the zebra label Feb 11, 2025
@piotrsuchy
Copy link
Contributor

piotrsuchy commented Feb 12, 2025 •
edited
Loading

Hey @ton31337, thanks for this PR this does indeed fix the issue I described, however there is one edge case:

root@bd6e65241d7a:/# vtysh

Hello, this is FRRouting (version 10.4-dev).
Copyright 1996-2005 Kunihiro Ishiguro, et al.

bd6e65241d7a# configure
bd6e65241d7a(config)# vni 1
bd6e65241d7a(config)# sh run
% Unknown command: sh run
bd6e65241d7a(config)# do sh run
Building configuration...

Current configuration:
!
frr version 10.4-dev
frr defaults traditional
hostname bd6e65241d7a
log syslog informational
vni 1
no ipv6 forwarding
service integrated-vtysh-config
!
end
bd6e65241d7a(config)# no vni 2
bd6e65241d7a(config)# do sh run
Building configuration...

Current configuration:
!
frr version 10.4-dev
frr defaults traditional
hostname bd6e65241d7a
log syslog informational
vni 1
no ipv6 forwarding
service integrated-vtysh-config
!
end
#### EVERYTHING CORRECT, THIS PATCH FIXES THE ABOVE SCENARIO, HOWEVER:

bd6e65241d7a(config)# vrf vrf-test
bd6e65241d7a(config-vrf)# vni 2
bd6e65241d7a(config-vrf)# exit
bd6e65241d7a(config)# do show run
Building configuration...

Current configuration:
!
frr version 10.4-dev
frr defaults traditional
hostname bd6e65241d7a
log syslog informational
vni 1
no ipv6 forwarding
service integrated-vtysh-config
!
vrf vrf-test
 vni 2
exit-vrf
!
end
bd6e65241d7a(config)# no vni 2
bd6e65241d7a(config)# do show run
Building configuration...

Current configuration:
!
frr version 10.4-dev
frr defaults traditional
hostname bd6e65241d7a
log syslog informational
no ipv6 forwarding
service integrated-vtysh-config
!
vrf vrf-test
 vni 2
exit-vrf
!
end

If there is a vrf with that VNI it will still delete the default one, without checking if that's the one specified, so it's like a false positive. We say 'delete 2', it deletes the default one, so '1' in this case, because there is a '2' in the vrf. (the value has to match the value in the VRF). Do you think there is a way to guard against that as well? Because that seems to be our case (in an environment where there are a lot of vrfs).

@piotrsuchy piotrsuchy mentioned this pull request Feb 12, 2025
Closed
2 tasks
@ton31337
zebra: Do not flush an existing vni configuration trying to remove wr…
44fe398 
…ong vni

Before:

```
pc.donatas.net(config)# do sh run | include vni
vni 1
pc.donatas.net(config)# no vni 2
pc.donatas.net(config)# do sh run | include vni
pc.donatas.net(config)#
```

Signed-off-by: Donatas Abraitis <[email protected]>
@ton31337 ton31337 force-pushed the fix/zebra_no_vni_validation branch from d8161fd to 44fe398 Compare February 12, 2025 21:37
@ton31337
Copy link
Member Author

ton31337 commented Feb 12, 2025

@piotrsuchy could you retest with the latest changes?

@piotrsuchy
Copy link
Contributor

piotrsuchy commented Feb 13, 2025

It does work exactly how I thought it should now! Thanks!

@donaldsharp donaldsharp merged commit 66434fc into FRRouting:master Feb 19, 2025
13 checks passed
@donaldsharp
Copy link
Member

donaldsharp commented Feb 19, 2025

@mergifiyio backport dev/10.3 stable/10.0 stable/10.1

@ton31337
Copy link
Member Author

ton31337 commented Mar 22, 2025

@Mergifyio backport stable/10.3 stable/10.2 stable/10.1 stable/10.0

@ton31337 ton31337 deleted the fix/zebra_no_vni_validation branch March 22, 2025 10:41
@mergify
Copy link

mergify bot commented Mar 22, 2025 •
edited
Loading

backport stable/10.3 stable/10.2 stable/10.1 stable/10.0

✅ Backports have been created

Details

This was referenced Mar 22, 2025
Merged
Merged
Merged
Merged
donaldsharp added a commit that referenced this pull request Mar 22, 2025
@donaldsharp
Merge pull request #18455 from FRRouting/mergify/bp/stable/10.3/pr-18108
588462f 
zebra: Do not flush an existing vni configuration trying to remove wrong vni (backport #18108)
donaldsharp added a commit that referenced this pull request Mar 22, 2025
@donaldsharp
Merge pull request #18456 from FRRouting/mergify/bp/stable/10.2/pr-18108
fcbace3 
zebra: Do not flush an existing vni configuration trying to remove wrong vni (backport #18108)
donaldsharp added a commit that referenced this pull request Mar 22, 2025
@donaldsharp
Merge pull request #18457 from FRRouting/mergify/bp/stable/10.1/pr-18108
2ede906 
zebra: Do not flush an existing vni configuration trying to remove wrong vni (backport #18108)
donaldsharp added a commit that referenced this pull request Mar 22, 2025
@donaldsharp
Merge pull request #18458 from FRRouting/mergify/bp/stable/10.0/pr-18108
24e5188 
zebra: Do not flush an existing vni configuration trying to remove wrong vni (backport #18108)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

backport master size/S zebra

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deleting the default VNI instead of saying a VNI like this doesn't exist for 'no vni <VNI>' vtysh command

3 participants

@ton31337 @piotrsuchy @donaldsharp