Bgp control plane: add route aggregation feature #37275
Conversation
There was a problem hiding this comment.
Hi @romanspb80 Before jumping into the code review of the implementation, I think we need to agree on the API change. I still think user should specify aggregation length instead of dynamically calculating it based on the boolean flag.
In vast majority of cases there is going to be single IP returned from getServicePrefix
, so with single /32 or /128 prefix there will be nothing to aggregate.
Have you tested this change on kubernetes cluster with sample loadbalancer service ? can you provide some example outputs how aggregation will look like?
Hi @harsimran-pabla . Thanks for the comment. In this case the dynamically calculating works: The dynamically calculated mask /28 is more efficiently then if we set the mask /24 manualy. Also there is another case which I noted in my comment : Thanks |
Yes, but the common case is that each service gets single /32 prefix. And if we pick start and end address from various services which have aggregate flag set, resulting aggregated prefix can be very large range - as services can have IPs allocated from different lb-ipam pools. |
This approach was modifying LB Pool with a aggregate flag. Why not introduce new BGP advertisement type which will advertise complete LB IPAM pool range ? Implementation would be much simpler and not change existing service reconcilers. I am trying to understand if the goal is to reduce /32 prefix announcements, can we achieve it with this simpler mechanism. Existing service announcements can be configured if user wants to service with eTP=Local to work. |
Yes, that makes sense. So instead of dynamic calculation use the cidr (or all cidrs from the LB pool) mask right away? |
No, I would still prefer each pool advertisement. Atleast we should start with this common case and see later if it makes sense to improve further upon it. In this approach, feature is not about prefix aggregation. It is more about LB Pool advertisement. |
|
There are two approaches to reduce /32 advertisements, I am not leaning towards one or the other.
I am not yet convinced if we need to jump straight into auto-aggregation, we can look at it in the future. At this stage, I think we need something simpler and deterministic. |
|
Ok. Will do this approach:
|
|
Commit 3a5d0c8 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
3a5d0c8 to
89a459f
Compare
|
Hi @harsimran-pabla
It makes sense to discuss the thresholds of these ranges. Thanks |
There was a problem hiding this comment.
Thanks @romanspb80 for the changes. API looks good to me, there are few other comments regarding the implementation.
- Route policies should be done for all service types ( external IP, cluster IP ), currently it implemented for LB Ingress IP only.
- Unit tests are missing.
- Route policies which are merged here should be sorted based on prefix length. For eg, if multiple advertisements match same service, longer prefix should be first.
There was a problem hiding this comment.
@cilium/sig-k8s CRD changes look good to me. I do see that @harsimran-pabla had a comment regarding the need for the min values.
4e3904e to
4d27eee
Compare
|
@harsimran-pabla , I've updated the PR in accordance with the comments. |
4d27eee to
1af4fc5
Compare
Thank you, @harsimran-pabla . I updated the change |
|
/test |
20fc92b to
27801fa
Compare
|
@romanspb80 I think you'd need to rebase on latest to get the failing test to pass. Check this comment on slack. |
New route aggregation option under the CiliumBGPAdvertisement.
apiVersion: cilium.io/v2alpha1
kind: CiliumBGPAdvertisement
metadata:
name: bgp-advertisements
labels:
advertise: bgp
spec:
advertisements:
- advertisementType: "Service"
service:
aggregationLengthIPv4: 24 <<<<<<<<<<<<<< New Field
aggregationLengthIPv6: 120 <<<<<<<<<<<<<< New Field
Fixes: cilium#36777
Signed-off-by: Roman Ptitcyn <romanspb@yahoo.com>
27801fa to
f61646e
Compare
|
/test |
done. Thanks! |
Add route aggregation feature in BGP Control Plane (RFC 4632)
New route aggregation options under the CiliumBGPAdvertisement.
Fixes: #36777
Signed-off-by: Roman Ptitcyn romanspb@yahoo.com