Busyloop when installed alongside MetalLB

[imax9000]

Describe the bug

When using LoadBalancer service type, NGF and MetalLB are constantly updating the same Service objects: MetalLB adds metallb.io/ip-allocated-from-pool annotation and NGF immediately deletes it, MetalLB adds it back and so on.

To Reproduce
Steps to reproduce the behavior:

1. Install and configure NGF and MetalLB.
2. Create a Gateway resource.
3. Run kubectl proxy and curl -s "http://127.0.0.1:8001/api/v1/namespaces/${NAMESPACE:-default}/services?watch=1" | jq -c .object.metadata.annotations

{"metallb.io/ip-allocated-from-pool":"extra"}
null
{"metallb.io/ip-allocated-from-pool":"extra"}
null
{"metallb.io/ip-allocated-from-pool":"extra"}
null
{"metallb.io/ip-allocated-from-pool":"extra"}
null
{"metallb.io/ip-allocated-from-pool":"extra"}
null
{"metallb.io/ip-allocated-from-pool":"extra"}
null
{"metallb.io/ip-allocated-from-pool":"extra"}
null

Expected behavior

Peaceful coexistence and proper functioning of both NGF and MetalLB.

Your environment

• nginx-gateway-fabric: 2.1.4
• MetalLB: v0.14.9
• Kubernetes: v1.33.0, on-prem via kubeadm

[nginx-bot]

Hi @imax9000! Welcome to the project! 🎉

Thanks for opening this issue!
Be sure to check out our Contributing Guidelines and the Issue Lifecycle while you wait for someone on the team to take a look at this.

[salonichf5]

Hey @imax9000 — I tested this on my setup and can confirm the bug does exist. I found a workaround that stops the annotation fight.

You can use the patches option
in the NGF Helm chart to add the MetalLB annotation directly to the NGINX Service.
For example:

# values.yaml
patches:
  - type: JSONPatch
    value:
      - op: add
        path: /metadata/annotations/metallb.io~1ip-allocated-from-pool
        value: "default-address-pool"

Then run a helm upgrade for your NGF installation with the updated values file.

After upgrading, the annotation stays on the Service as expected:

kubectl describe svc gateway-nginx

Annotations:              metallb.io/ip-allocated-from-pool: default-address-pool
Type:                     LoadBalancer
LoadBalancer Ingress:     172.18.255.1 (VIP)

This keeps MetalLB’s ip-allocated-from-pool annotation from getting wiped by NGF.

[imax9000]

Thanks, I also thought about that, but decided against it: MetalLB relies on these annotations to keep track of resource state, and I'd prefer to not have a possibility of NGF confusing MetalLB into an invalid configuration.

[salonichf5]

@imax9000 We discussed this issue in our community meeting and decided to dig into it further. I’ll keep you posted on its priority.

[sleungcy]

I can reproduce this issue with purelb as well.

[sjberman]

Thanks, I also thought about that, but decided against it: MetalLB relies on these annotations to keep track of resource state, and I'd prefer to not have a possibility of NGF confusing MetalLB into an invalid configuration.

Trying to understand the concern here. Are these annotations static or dynamic? If it's the former, then you can manually set them in the API per Saloni's suggestion (or this way) and they'll be set on the resource. If it's the latter, then I understand the problem and would love to figure out a solution, but this has been a struggle to figure out for awhile.

The issue with solving this is that the source of truth for the NGINX Service configuration is the NginxProxy API (or a Gateway's spec.infrastructure.annotations/labels field). Our control plane currently disallows out of band configuration changes (i.e. directly on the provisioned resources) to prevent conflicts between manual changes versus what's specified in our API. We want to have a single source of truth in a deterministic way, because if our control plane starts up and sees the NGINX Service config doesn't match what's defined our API (because a user altered it directly on the Service), it will want to correct it to what it thinks it should be. It's not obvious to me how we can essentially allow two separate APIs (ours and k8s) to control the config and not have conflicts.

What would be nice is if MetalLB could have an integration with Gateway API and be able to set these labels directly in the Gateway's infrastructure spec.

[imax9000]

Are these annotations static or dynamic? If it's the former, then you can manually set them in the API per Saloni's suggestion (or this way) and they'll be set on the resource. If it's the latter, then I understand the problem and would love to figure out a solution, but this has been a struggle to figure out for awhile.

They are dynamic: MetalLB detects that there's a new Service object with type: LoadBalancer and attempts to allocate an IP address for it; as a part of that process it selects which pool to allocate from and records that in the annotation. Arguably, a more appropriate place for this information would be under status, but there's no place for it in there.

This also has nothing to do with Gateway API specifically. MetalLB is only concerned with Service objects and (correctly) doesn't care about Gateways or anything else related to those Service objects.

Our control plane currently disallows out of band configuration changes (i.e. directly on the provisioned resources) to prevent conflicts between manual changes versus what's specified in our API. We want to have a single source of truth in a deterministic way, because if our control plane starts up and sees the NGINX Service config doesn't match what's defined our API (because a user altered it directly on the Service), it will want to correct it to what it thinks it should be. It's not obvious to me how we can essentially allow two separate APIs (ours and k8s) to control the config and not have conflicts.

It's easier to solve if you reduce the scope to metadata or metadata.annotations. Nobody expects out-of-band modifications to spec to stick around, so you don't need to worry about preserving them. OTOH, metadata.annotations specifically has a history of being used by various cluster infrastructure bits for configuration and state tracking. That said, I haven't found any explicit API contract describing who and when should/shouldn't modify annotations, so it's probably best to aim for compatibility with existing usage.

I would suggest to not treat unexpected fields in metadata.annotations as a change, and carry them over during updates (documentation actually advises doing that). E.g., you overwrite any annotations that the user specified in the Helm chart values and those that NGF uses internally (if any, with your own prefix), and keep the rest as is.

Note that you already need to be a bit selective about what fields are taken into account during change detection: things like metadata.resourceVersion, metadata.creationTimestamp, spec.clusterIP, spec.ports[].nodePort are set automatically and it would be silly to keep trying to remove them from the Service object.

Also, maybe managedFields was added to help with such situations, but I'm not very clear on how it's supposed to work.