Handle more BGPConfiguration fields directly, not via confd "cache"#12524
Handle more BGPConfiguration fields directly, not via confd "cache"#12524nelljerram wants to merge 1 commit intoprojectcalico:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Moves handling of several BGPConfiguration service CIDR fields (ClusterIP/ExternalIP/LoadBalancerIP ranges) away from the legacy confd “cache” paths and toward using the v3 BGPConfiguration directly, continuing the #12523 direction.
Changes:
- Stop translating
Service{Cluster,External,LoadBalancer}IPsinto legacy v1 cache keys and instead derive them directly fromglobalBGPConfig. - Thread LoadBalancer CIDRs into
BirdBGPConfigand update BIRD IPAM templates to use that field rather than reading/bgp/v1/global/svc_loadbalancer_ips. - Remove the legacy service-LB key from the confd template watch lists, relying on
/bgpconfiginvalidation instead.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
confd/pkg/backends/types/bird_bgp_config.go |
Adds LoadBalancerIPs to the template config struct. |
confd/pkg/backends/calico/client.go |
Uses v3 globalBGPConfig for service CIDR updates; removes translation to legacy cache keys; updates /bgpconfig invalidation behavior. |
confd/pkg/backends/calico/bgp_processor.go |
Populates BirdBGPConfig.LoadBalancerIPs for template rendering. |
confd/etc/calico/confd/templates/bird_ipam.cfg.template |
Switches RR LB allowlist logic to iterate $config.LoadBalancerIPs instead of reading legacy confd key. |
confd/etc/calico/confd/templates/bird6_ipam.cfg.template |
Same as above for IPv6. |
confd/etc/calico/confd/conf.d/bird_ipam.toml |
Stops watching the legacy svc_loadbalancer key. |
confd/etc/calico/confd/conf.d/bird6_ipam.toml |
Stops watching the legacy svc_loadbalancer key. |
| // Same for cluster CIDRs - except those can be overridden by an env var, so only | ||
| // process cluster IPs in BGPConfiguration if that env var is not set. | ||
| if len(os.Getenv(envAdvertiseClusterIPs)) == 0 { | ||
| c.onClusterIPsUpdate(getServiceClusterIPs(c.globalBGPConfig)) | ||
| } |
There was a problem hiding this comment.
When CALICO_ADVERTISE_CLUSTER_IPS is set, this block skips calling onClusterIPsUpdate entirely. However, needServiceAdvertisementUpdates can be triggered by node label changes (ExcludeServiceAdvertisement), and in that case cluster CIDR routes still need to be reconciled (advertise vs reject/withdraw) even if the CIDR source is the env var. As written, cluster routes can remain in the previous state while external/LB routes get updated.
Suggestion: always call onClusterIPsUpdate, but choose the CIDR list based on precedence (env var vs BGPConfiguration). For example, compute clusterCIDRs from the env var when set, otherwise from BGPConfiguration, then call onClusterIPsUpdate(clusterCIDRs). Adding a regression unit test for "env var set + node label flips exclude-from-external-load-balancers" would help prevent this from regressing.
| // Same for cluster CIDRs - except those can be overridden by an env var, so only | |
| // process cluster IPs in BGPConfiguration if that env var is not set. | |
| if len(os.Getenv(envAdvertiseClusterIPs)) == 0 { | |
| c.onClusterIPsUpdate(getServiceClusterIPs(c.globalBGPConfig)) | |
| } | |
| // Reconcile cluster CIDRs on every service advertisement update. The effective | |
| // source of cluster CIDRs is the env var when set, otherwise BGPConfiguration. | |
| clusterCIDRs := getServiceClusterIPs(c.globalBGPConfig) | |
| if len(os.Getenv(envAdvertiseClusterIPs)) != 0 { | |
| clusterCIDRs = c.clusterCIDRs | |
| } | |
| c.onClusterIPsUpdate(clusterCIDRs) |
- ServiceClusterIPs - ServiceExternalIPs - ServiceLoadBalancerIPs - NodeMeshRestartTime - NodeMeshPassword - IgnoredInterfaces - ProgramClusterRoutes (Following the direction laid out in projectcalico#12523)
8cd55a9 to
b672201
Compare
(Following the direction laid out in #12523)