Add a proposal to move frr as a separate daemonset #1933
Conversation
fedepaol
force-pushed
the
design/splitfrr
branch
2 times, most recently
from
7ff7132
to
090a9a9
Compare
fedepaol
force-pushed
the
design/splitfrr
branch
2 times, most recently
from
3d568c1
to
bdec09d
Compare
|
I like this idea. I've wondered in the past about running PIM in a kubernetes cluster, I think depending on the CNI it could be used to bring native multicast up to pods. This seems like a cool possible path to get frr to do that in addition to BGP. |
design/splitfrr-proposal.md
Outdated
| type AllowedPrefixes struct { | ||
| Prefixes []string `json:"prefixes,omitempty"` | ||
| // +kubebuilder:validation:Enum=all;filtered | ||
| Mode string `json:"mode,omitempty"` |
There was a problem hiding this comment.
nit: I think setting a struct here and putting the validation on it is cleaner (and probably easier to manage), i.e:
Mode AllowMode `json:"mode,omitempty"`
// +kubebuilder:validation:Enum=all;filtered
type AllowMode string
const (
// ...
AllowAll AllowMode = "all"
// ...
AllowRestricted AllowMode = "filtered"
)wdyt?
There was a problem hiding this comment.
sounds good, TIL you can put constraints on a type
| } | ||
|
|
||
| type AllowedPrefixes struct { | ||
| Prefixes []string `json:"prefixes,omitempty"` |
There was a problem hiding this comment.
nit: do we expect this to be an array of CIDRs? leaving this here for now
// +kubebuilder:validation:Format="cidr"
as it might come in handy later 😅
There was a problem hiding this comment.
I'll add it to the actual implementation
design/splitfrr-proposal.md
Outdated
| metadata: | ||
| name:advertisement | ||
| namespace: metallb-system | ||
| spec: peers: peer1 |
design/splitfrr-proposal.md
Outdated
| port: 179 | ||
| toAdvertise: | ||
| allowed: | ||
| - 192.168.10.0/32 |
There was a problem hiding this comment.
nope. The default is filtered (it used to be a bool). We'll explicit the default value in the implementation.
There was a problem hiding this comment.
left comments on the api structure
| } | ||
|
|
||
| type Advertise struct { | ||
| AllowedPrefixes `json:"allowed,omitempty"` |
There was a problem hiding this comment.
what happens when there's only
allowed:
- mode: "all"
does it mean that nothing is actually advertised? (I feel like a few comments around the fields would solve some of these questions 😅
There was a problem hiding this comment.
Nope, it means that all the prefixes configured in the router are actually advertised
| } | ||
|
|
||
| type Neighbor struct { | ||
| Address string `json:"address"` |
There was a problem hiding this comment.
nit: same as the cidr note, might need proper validation later
| On the other hand, the MetalLB Operator will be changed so it can deploy this | ||
| new component too. |
There was a problem hiding this comment.
do you mean here that the operator will be able to deploy FRR as a standalone (i.e a new FRR CRD), or as part of the MetalLB resource?
There was a problem hiding this comment.
as a starter I'd let the operator deploy frr together with metallb, because metallb requires it to function. IIRC, olm allows to specify operator dependencies or something like that, so it might be that in the future this thing has its own operator.
Instead of running frr as a side container, here we propose to move it as a separate daemon with its own API, that can be alimented both by the user and by MetalLB. This would serve all those use cases where the users require to have a FRR-like process running on the cluster's nodes and currently can't because of technical limitations. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
yep there are a few users that asked for extra frr configuration and running two instances is not ideal. There's a good chance the set of features exposed will grow with users' demand. |
Instead of running frr as a side container, here we propose to move it as a separate daemon with its own API, that can be alimented both by the user and by MetalLB.
This would serve all those use cases where the users require to have a FRR-like process running on the cluster's nodes and currently can't because of technical limitations.