Implement receiving / filtering prefixes from outside #21
Conversation
fedepaol
force-pushed
the
receivetests
branch
3 times, most recently
from
0625cd1
to
81a57b8
Compare
| } | ||
| res.HasV6 = true | ||
| } | ||
| return res |
There was a problem hiding this comment.
should we sort the prefixes here?
| @@ -20,8 +20,7 @@ route-map {{.neighbor.ID}}-out permit {{counter .neighbor.ID}} | |||
| deny all the others.*/ -}} | |||
| {{- define "neighborfilters" -}} | |||
|
|
|||
| route-map {{.neighbor.ID}}-in deny 20 | |||
There was a problem hiding this comment.
was there a reason we went with this approach instead of "deny any" now?
e2etests/pkg/routes/routes.go
Outdated
| if !found { | ||
| return false | ||
| } | ||
| return true |
e2etests/tests/validate.go
Outdated
|
|
||
| func ValidateNodesDoNotHaveRoutes(pods []*v1.Pod, neigh frrcontainer.FRR, prefixes ...string) { | ||
| ginkgo.By(fmt.Sprintf("Checking routes %v not injected from %s", prefixes, neigh.Name)) | ||
| Consistently(func() error { |
There was a problem hiding this comment.
should you add an eventually first? (this might bite us when/if we update the resource as part of the test)
There was a problem hiding this comment.
yep, we always call this after a "doHaveRoutes" but there's no guarantee. I'll add
fedepaol
force-pushed
the
receivetests
branch
6 times, most recently
from
375f619
to
73b9c2a
Compare
| PrefixesV6: sortMap(advsV6), | ||
| } | ||
| return res, nil | ||
| } |
| if n.ToAdvertise.Allowed.Mode == v1beta1.AllowAll { | ||
| res.Outgoing, err = toAdvertiseToFRR(n.ToAdvertise, ipv4Prefixes, ipv6Prefixes) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
nit: format err with the neighbor's name/addr
internal/controller/api_to_config.go
Outdated
| if !ok { // shouldn't happen as we check in previous loop, just in case | ||
| return nil, fmt.Errorf("unexpected err - no community prefix matching %s", p) | ||
| return fmt.Errorf("unexpected err - no community prefix matching %s", p) |
There was a problem hiding this comment.
I think this is misleading now since it seems advs and communitesforPrefix diverged? shouldn't communityPrefixesToMap be aware of the allowed advs when building the map? i.e
return nil, fmt.Errorf("prefix %s with community %s not in allowed list for neighbor %s", p, pfxs.Community, n.Address)
gone (maybe changing the error message here might be enough, in case we want to keep the other func as is)
| } | ||
| return res, nil | ||
| } | ||
| func prefixesToMap(toAdvertise v1beta1.Advertise, ipv4Prefixes, ipv6Prefixes []string) (map[string]*frr.OutgoingFilter, map[string]*frr.OutgoingFilter) { |
There was a problem hiding this comment.
can you add some docs around the new functions (reduce the change of getting lost here)?
| // TODO: check that the prefix matches the passed IPv4/IPv6 prefixes | ||
| advs[p] = &frr.AdvertisementConfig{Prefix: p, IPFamily: family} |
There was a problem hiding this comment.
ah I missed this on my pr, my idea was verifying that the explicit prefix mentioned belongs to the router's "global" ones (ipv4Prefixes, ipv6Prefixes), did you remove it on purpose?
There was a problem hiding this comment.
Nope, I missed it. I will leave a TODO because doing it while we process seems a bit ugly, so it might worth having an ad hoc validate function. Let's wait after we do the merging thing and the dust settles
| @@ -54,10 +53,32 @@ route-map {{$.neighbor.ID}}-out permit {{counter $.neighbor.ID}} | |||
|
|
|||
| {{/* If the neighbor does not have an advertisement, we need to add a prefix to deny | |||
| for when we have a prefix but a given peer is not selected for any prefixes */}} | |||
| {{- if not .neighbor.HasV4Advertisements}} | |||
| {{- if not .neighbor.Outgoing.PrefixesV4 }} | |||
There was a problem hiding this comment.
neat, this returns according to the slice being empty or not?
e2etests/tests/validate.go
Outdated
|
|
||
| // shouldFailConsistentyl checks for the failure to happen | ||
| // and then checks it consistently. | ||
| func shouldFailConsistently(toCheck func() error) { |
fedepaol
force-pushed
the
receivetests
branch
2 times, most recently
from
a4beb97
to
a545a36
Compare
Here we implement the translation and the rendering of the inbound path. Signed-off-by: Federico Paolinelli <[email protected]>
To consume the version where we can inject v4 and v6 prefixes from the external containers. Signed-off-by: Federico Paolinelli <[email protected]>
We add tests to cover incoming routes acceptance / filtering. Signed-off-by: Federico Paolinelli <[email protected]>
…shift-4.14-ose-frr Updating ose-frr images to be consistent with ART
Here we implement the incoming routes accepting (and filtering) and we add e2e tests for it.
This is based on #19 for the testing part.
Also, it will require metallb/metallb#2001 to validate injection on dual stack.