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: Revalidate locally originated routes also when RPKI state changes #16483

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

Conversation

ton31337
Copy link
Member

@ton31337 ton31337 commented Jul 26, 2024

If we have something like:

router bgp 65001
 network 10.10.10.0/24 route-map rpki
!
route-map rpki permit 10
 match rpki valid
 set local-preference 150
route-map rpki permit 20
 match rpki notfound
 set local-preference 50
!

Then 10.10.10.0/24 is never revalidated when RPKI state changes. E.g. if it was valid, and moves to notfound => local-preference remains 150, but should be 50.

With this patch we force BGP network (static) routes to be revalidated when revalidation event is triggered from RPKI module.

Fixes #16474

@frrbot frrbot bot added the bgp label Jul 26, 2024
@ton31337 ton31337 marked this pull request as draft July 27, 2024 11:05
@github-actions github-actions bot added size/M and removed size/XS labels Jul 27, 2024
@ton31337 ton31337 force-pushed the fix/validate_rpki_network_routes branch 3 times, most recently from d11efc5 to d1c7c65 Compare July 27, 2024 13:53
@ton31337 ton31337 marked this pull request as ready for review July 28, 2024 09:45
@donaldsharp
Copy link
Member

I know we have other places in the code where we have timers to regenerate data in BGP but I am personally not happy about that approach as that it adds unnecessary load to already loaded systems when there are a bunch of prefixes in the system. Is there a way we can have routes revalidated automatically? what is going on that we have to set a timer?

@ton31337
Copy link
Member Author

what is going on that we have to set a timer?

Routes are never revalidated actually (including static routes (network ...), redistributed routes, ...). When a callback is received from librtr, we validate per-prefix and per-neighbor (using soft inbound) only.

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.

this might need to be redesigned so it's not timer based ...

@riw777
Copy link
Member

riw777 commented Jul 30, 2024

waiting on #16474 to be tested to make certain it resolves this issue

@riw777 riw777 self-requested a review July 30, 2024 15:29
@ton31337
Copy link
Member Author

#16474 is fixed using this approach. @donaldsharp do you have any other solution or we can go with this?

@ton31337
Copy link
Member Author

ton31337 commented Jul 31, 2024

this might need to be redesigned so it's not timer based ...

It's not really possible easily. RPKI cache server sends only changed prefixes. E.g. using StayRTR:

{
  "roas": [
    {
      "prefix": "100.100.100.100/32",
      "maxLength": 32,
      "asn": "AS65001"
    }
  ]
}

If I change to:

{
  "roas": [
    {
      "prefix": "100.100.100.101/32",
      "maxLength": 32,
      "asn": "AS65001"
    }
  ]
}

Then 100.100.100.101/32 will be sent as a prefix to be validated. If this route does not exist in the RIB, then no changes will be triggered and again we have a stale state.

With the timer, it's deterministic, we always know we have the right state eventually. It can be controlled with high timer values and does not need to be every 30 seconds.

UPDATE: Opened an issue, but I doubt it's even possible because it's more like up to the RPKI cache server to send those removed prefixes.

Copy link
Contributor

@pushpasis pushpasis 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 to me.

@riw777
Copy link
Member

riw777 commented Aug 27, 2024

@donaldsharp are we still looking into another way to do this, or should we go with this commit for the moment?

@donaldsharp
Copy link
Member

I would really really prefer that we fix this problem the correct way the first time. I don't think we are in too much of a hurry since this is a day one problem with the rpki code.

Copy link

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

If we have something like:

```
router bgp 65001
 network 10.10.10.0/24 route-map rpki
!
route-map rpki permit 10
 match rpki valid
 set local-preference 150
route-map rpki permit 20
 match rpki notfound
 set local-preference 50
!
```

Then 10.10.10.0/24 is never revalidated when RPKI state changes. E.g. if it was
valid, and moves to notfound => local-preference remains 150, but should be 50.

With this patch we force BGP network (static) routes to be revalidated when
revalidation event is triggered from RPKI module.

Signed-off-by: Donatas Abraitis <[email protected]>
If we have something like:

```
router bgp 65001
 redistribute connected route-map rpki
!
route-map rpki permit 10
 match rpki valid
 set local-preference 150
route-map rpki permit 20
 match rpki notfound
 set local-preference 50
!
```

Then connected routes are never revalidated when RPKI state changes. E.g. if it was
valid, and moves to notfound => local-preference remains 150, but should be 50.

With this patch we force redistributed routes to be revalidated when
revalidation event is triggered from RPKI module.

Signed-off-by: Donatas Abraitis <[email protected]>
@ton31337 ton31337 force-pushed the fix/validate_rpki_network_routes branch from d1c7c65 to 77476b6 Compare September 18, 2024 15:04
@riw777
Copy link
Member

riw777 commented Nov 5, 2024

Where are we on this? Still waiting on a better way to resolve it?

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.

path selection in bgp is not updated accordingly to rpki updates
4 participants