CR Converstion tool, failed to generate resources: invalid aggregation length 24

[jonwilliams84]

Take this configmap:

apiVersion: v1
kind: ConfigMap
metadata:
  namespace: metallb-system
  name: config
data:
  config: |
    peers:
  config: |
    peers:
    - peer-address: 10.32.100.1
      peer-asn: 64588
      my-asn: 64500
    address-pools:
    - name: default
      protocol: bgp
      addresses:
      - 10.0.8.0/24
      avoid-buggy-ips: true
      bgp-advertisements:
      - aggregation-length: 32
        localpref: 100
      - aggregation-length: 24

When running it through the CM to CR conversion tool during upgrade of MetalLB 12.1 > 13.3, I get this error:

failed to generate resources: invalid aggregation length 24: prefix 26 in this pool is more specific than the aggregation length for addresses 10.0.8.1-10.0.8.254

Commenting out the /24 aggregation-length line the CR is generated:

# This was autogenerated by MetalLB's custom resource generator.
apiVersion: metallb.io/v1beta2
kind: BGPPeer
metadata:
  creationTimestamp: null
  name: peer1
  namespace: metallb-system
spec:
  holdTime: 1m30s
  keepaliveTime: 30s
  myASN: 64500
  passwordSecret: {}
  peerASN: 64588
  peerAddress: 10.32.100.1
status: {}
---
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
  creationTimestamp: null
  name: default
  namespace: metallb-system
spec:
  addresses:
  - 10.0.8.1-10.0.8.254
status: {}
---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
  creationTimestamp: null
  name: bgpadvertisement1
  namespace: metallb-system
spec:
  aggregationLength: 32
  ipAddressPools:
  - default
  localPref: 100
status: {}
---

Functionally, I realise this will work but is there any reason it doesn't like the /24 aggregation on a pool that is /24?

[fedepaol]

10.0.8.1-10.0.8.254

I see what the problem is, still need to find the best way to address it.
Your avoidBuggyIPs is effectively translating the /24 pool to a pool that is 10.0.8.1-10.0.8.254.
The cidrs corresponding to that range are at most /26 so the engine (which is the same that validates the configuration in metallb) complains about the fact that it's advertising a cidr larger than the addresses avaliable.
Again, I understand you did nothing wrong, just writing down what happened. We'll need to find the best way to address it, either understanding that it is a masked /24 , or relaxing the check (or something else).

[jonwilliams84]

Of course...I understand. Do CRs allow CIDR denotation for IPAddresses or does it require a range? If it will take a CIDR then I'll adjust the IPAddresses CR to CIDR and add a new advertisement CR for the /24 aggregation manually.

[jonwilliams84]

Ok....found another issue with the generated CR:
Error from server (peer 10.32.100.1 has keepalive-time set on native bgp mode): error when creating "metallb-cr.yaml": admission webhook "bgppeersvalidationwebhook.metallb.io" denied the request: peer 10.32.100.1 has keepalive-time set on native bgp mode

I have commented out the keepaliveTime field for now so here the latest CRs and all seems well:

# This was autogenerated by MetalLB's custom resource generator.
apiVersion: metallb.io/v1beta2
kind: BGPPeer
metadata:
  creationTimestamp: null
  name: peer1
  namespace: metallb-system
spec:
  holdTime: 1m30s
  #keepaliveTime: 30s
  myASN: 64500
  passwordSecret: {}
  peerASN: 64588
  peerAddress: 10.32.100.1
status: {}
---
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
  creationTimestamp: null
  name: default
  namespace: metallb-system
spec:
  addresses:
  - 10.0.8.0/24
status: {}
---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
  creationTimestamp: null
  name: bgpadvertisement1
  namespace: metallb-system
spec:
  aggregationLength: 32
  ipAddressPools:
  - default
  localPref: 100
status: {}
---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
  creationTimestamp: null
  name: bgpadvertisement2
  namespace: metallb-system
spec:
  aggregationLength: 24
  ipAddressPools:
  - default
status: {}

[fedepaol]

Right, that's another issue. The converter tool is using the validation function for the native bgp implementation (which doesn't support the keepalive time field). We'll make it configurable via a parameter.

[fedepaol]

Closing this as both PRs are merged.
TL;DR: we are bringing avoidbuggyips back and we won't set keepalivetime if it's not set. A new release is coming soon