malhuda/alice-lg
1
0
Fork 0
Code Issues 39 Pull Requests 11 Actions Packages Projects Releases 2 Wiki Activity

Add support for per-Source Hidden Neighbors #143

Open
jof wants to merge 1 commits from jof/jof/hidden-neighbors into develop
pull from: jof/jof/hidden-neighbors
merge into: malhuda:develop
Conversation 5 Commits 1 Files Changed 14 +189 -33
jof commented 2024-02-19 08:38:24 +08:00 (Migrated from github.com)
Copy Link

Closes #142

As part of Route Server operations, it is sometimes necessary to configure some BGP Neighbors which are not appropriate for a public Looking Glass interface.
For example, an internal route server peer used to collect routes for flow analytics, but which is not a "normal" Route Server peer. Another example: an internal (route server)-to-(route server) peer used to scale a multi-instance route server.

This PR adds some support to Alice to enable configuring some Neighbors to be hidden on a per-source basis. This filtration could be based on IPs, CIDRs, or Regular Expressions. As BIRD(watcher) has some some NeighborStatus IDs which are potentially not IP addresses, it seemed important to enable filtering either IPs or Neighbor Protocol names.

Closes #142 As part of Route Server operations, it is sometimes necessary to configure some BGP Neighbors which are not appropriate for a public Looking Glass interface. For example, an internal route server peer used to collect routes for flow analytics, but which is not a "normal" Route Server peer. Another example: an internal (route server)-to-(route server) peer used to scale a multi-instance route server. This PR adds some support to Alice to enable configuring some Neighbors to be hidden on a per-source basis. This filtration could be based on IPs, CIDRs, or Regular Expressions. As BIRD(watcher) has some some NeighborStatus IDs which are potentially not IP addresses, it seemed important to enable filtering either IPs or Neighbor Protocol names.
annikahannig commented 2024-02-29 17:33:50 +08:00 (Migrated from github.com)
Copy Link

I'm currently reviewing this.

I'm currently reviewing this.
annikahannig (Migrated from github.com) requested changes 2024-02-29 18:26:47 +08:00
annikahannig (Migrated from github.com) left a comment
Copy Link

Hej! :)

Thanks again for the effort.
I added some notes how this could be improved and reuse already existing code. (To a degree)

Hej! :) Thanks again for the effort. I added some notes how this could be improved and reuse already existing code. (To a degree)
pkg/sources/source.go
@ -38,0 +58,4 @@
}
return excludeCIDRs, excludeIPs, excludePatterns, nil
}
annikahannig (Migrated from github.com) commented 2024-02-29 17:51:29 +08:00
Copy Link

This function has too many return values.
Also I think it should be done during config parsing.

Also, maybe the already existing filtering infrastructure could be reused:

There is api.NeighborFilter which could be extended:

  • api.Filterable this should maybe be split into RouteFilterable and NeighborFilterable
  • NeighborFilter.Match should accept this interface, so this can be implemented for api.Neighbor and api.NeighborStatus
This function has too many return values. Also I think it should be done during config parsing. Also, maybe the already existing filtering infrastructure could be reused: There is `api.NeighborFilter` which could be extended: - `api.Filterable` this should maybe be split into `RouteFilterable` and `NeighborFilterable` - `NeighborFilter.Match` should accept this interface, so this can be implemented for `api.Neighbor` and `api.NeighborStatus`
annikahannig (Migrated from github.com) commented 2024-02-29 18:21:25 +08:00
Copy Link

The NeighborFilter could then include this list of CIDRs / IPs / RE patterns.

The NeighborFilterable then could maybe expect MatchAddress MatchASN, MatchDescription MatchPattern?

something like this :)

The `NeighborFilter` could then include this list of CIDRs / IPs / RE patterns. The NeighborFilterable then could maybe expect `MatchAddress` `MatchASN`, `MatchDescription` `MatchPattern?` something like this :)
pkg/sources/source.go
@ -38,0 +68,4 @@
}
neighbors:
for _, neighbor := range neighbors {
neighborIP := net.ParseIP(neighbor.Address)
annikahannig (Migrated from github.com) commented 2024-02-29 18:24:04 +08:00
Copy Link

Another reason to do this at config parse time:
If there is a broken pattern in the config, this would fail when routes refresh starts or, if there is no global search enabled, on first request.

Another reason to do this at config parse time: If there is a broken pattern in the config, this would fail when routes refresh starts or, if there is no global search enabled, on first request.
pkg/sources/source.go
@ -38,0 +78,4 @@
continue neighbors
}
}
for _, cidr := range excludeCIDRs {
annikahannig (Migrated from github.com) commented 2024-02-29 18:09:43 +08:00
Copy Link

If the hidden_members config option is parsed into a e.g. []*NeighborFilter / filterset something like

func (filters []*NeighborFilter) Exclude[T NeighborFilterable](n []T) ([]T, error) {
  for filters ... {
     if n.Match(...) skip
  }
}

Then the invocation in the source could be like

neighbors := make(api.Neighbors, 0)
... decode neighbors...
response.Neighbors, err = src.cfg.HiddenNeighbors.Exclude(neighbors)
	if err != nil {
		return nil, err
	}
If the `hidden_members` config option is parsed into a e.g. []*NeighborFilter / filterset something like ``` func (filters []*NeighborFilter) Exclude[T NeighborFilterable](n []T) ([]T, error) { for filters ... { if n.Match(...) skip } } ``` Then the invocation in the source could be like ``` neighbors := make(api.Neighbors, 0) ... decode neighbors... response.Neighbors, err = src.cfg.HiddenNeighbors.Exclude(neighbors) if err != nil { return nil, err } ```
pkg/sources/source.go
@ -38,0 +111,4 @@
for _, ip := range excludeIPs {
if ip.Equal(neighborIDAsIP) {
continue neighbors
}
annikahannig (Migrated from github.com) commented 2024-02-29 18:05:13 +08:00
Copy Link

This should rather be an early return, to reduce nesting.

This should rather be an early return, to reduce nesting.
pkg/sources/source.go
annikahannig (Migrated from github.com) commented 2024-02-29 18:13:15 +08:00
Copy Link

Also this duplicated code would not longer be required.

Additionally: These functions feel a bit out of place here; so this would also be resolved.

Also this duplicated code would not longer be required. Additionally: These functions feel a bit out of place here; so this would also be resolved.
pkg/store/neighbors_store.go
@ -54,6 +54,7 @@ type NeighborsStoreBackend interface {
type NeighborsStore struct {
backend NeighborsStoreBackend
sources *SourcesStore
cfg *config.Config
annikahannig (Migrated from github.com) commented 2024-02-29 18:16:34 +08:00
Copy Link

I think this is an artifact?

I think this is an artifact?
annikahannig (Migrated from github.com) reviewed 2024-02-29 18:29:48 +08:00
pkg/sources/source.go
@ -38,0 +78,4 @@
continue neighbors
}
}
for _, cidr := range excludeCIDRs {
annikahannig (Migrated from github.com) commented 2024-02-29 18:29:48 +08:00
Copy Link

Or: even better: before it is appended during decode:

if cfg.HiddenNeighbors.Match(...) {
  continue
 }
 response.Neighbors = append(...

Then the Exclude function would not even be required. :)

Or: even better: before it is appended during decode: ``` if cfg.HiddenNeighbors.Match(...) { continue } response.Neighbors = append(... ``` Then the Exclude function would not even be required. :)
jof (Migrated from github.com) reviewed 2024-03-01 04:26:20 +08:00
pkg/store/neighbors_store.go
@ -54,6 +54,7 @@ type NeighborsStoreBackend interface {
type NeighborsStore struct {
backend NeighborsStoreBackend
sources *SourcesStore
cfg *config.Config
jof (Migrated from github.com) commented 2024-03-01 04:26:19 +08:00
Copy Link

Indeed, it was.

Indeed, it was.
jof commented 2024-03-09 14:13:57 +08:00 (Migrated from github.com)
Copy Link

I'm still thinking this through, but I understand the desire to have a common filtering interface.

I'll take me a little bit to get up to speed with the other filtering codepaths and interfaces.

I'm still thinking this through, but I understand the desire to have a common filtering interface. I'll take me a little bit to get up to speed with the other filtering codepaths and interfaces.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin jof/jof/hidden-neighbors:jof/jof/hidden-neighbors
git checkout jof/jof/hidden-neighbors
Sign in to join this conversation.
Reviewers
No Reviewers
annikahannig
Labels
Clear labels
bug
Something isn't working
dependencies
Pull requests that update a dependency file
duplicate
This issue or pull request already exists
enhancement
New feature or request
go
Pull requests that update Go code
good first issue
Good for newcomers
help wanted
Extra attention is needed
invalid
This doesn't seem right
javascript
Pull requests that update Javascript code
priority
question
Further information is requested
wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: malhuda/alice-lg#143
No description provided.
Delete Branch "jof/jof/hidden-neighbors"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?