fix: BGP speaker reads IPs from pod annotations instead of pod status#6344
fix: BGP speaker reads IPs from pod annotations instead of pod status#6344
Conversation
Summary of ChangesHello @oilbeater, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the BGP speaker failed to correctly advertise pod IPs in certain Kube-OVN configurations, specifically when Kube-OVN operated as a secondary CNI or utilized attachment networks. The core change involves shifting the source of pod IP discovery from Kubernetes' native pod status to Kube-OVN's custom pod annotations, which provide a unified and accurate representation of all managed IPs. This enhancement ensures reliable BGP route advertisements across diverse networking setups, improving the stability and functionality of the BGP speaker component. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the BGP speaker logic to read pod IPs from annotations instead of pod.Status.PodIPs to support secondary CNI and attachment network scenarios. However, this introduces a critical security vulnerability: trusting unvalidated pod annotations for BGP route announcements allows an attacker to inject arbitrary IP addresses and hijack network traffic. Additionally, a non-standard function will cause compilation failure, and invalid BGP policy values need proper handling. The suggested fix re-introduces validation to ensure announced IPs belong to the pod's assigned subnet CIDR and addresses the compilation issue and policy handling.
| switch policy { | ||
| case "true": | ||
| fallthrough | ||
| case announcePolicyCluster: | ||
| for _, podIP := range pod.Status.PodIPs { | ||
| ips[util.CheckProtocol(podIP.IP)] = podIP.IP | ||
| case "true", announcePolicyCluster: | ||
| for ip := range strings.SplitSeq(ipStr, ",") { | ||
| addExpectedPrefix(strings.TrimSpace(ip), bgpExpected) | ||
| } | ||
| case announcePolicyLocal: | ||
| if pod.Spec.NodeName == c.config.NodeName { | ||
| for _, podIP := range pod.Status.PodIPs { | ||
| ips[util.CheckProtocol(podIP.IP)] = podIP.IP | ||
| } | ||
| } | ||
| default: | ||
| klog.Warningf("invalid pod annotation %s=%s", util.BgpAnnotation, policy) | ||
| } | ||
| } else if pod.Spec.NodeName == c.config.NodeName { | ||
| cidrBlock := localSubnets[pod.Annotations[util.LogicalSwitchAnnotation]] | ||
| if cidrBlock != "" { | ||
| for _, podIP := range pod.Status.PodIPs { | ||
| if util.CIDRContainIP(cidrBlock, podIP.IP) { | ||
| ips[util.CheckProtocol(podIP.IP)] = podIP.IP | ||
| if pod.Spec.NodeName == nodeName { | ||
| for ip := range strings.SplitSeq(ipStr, ",") { | ||
| addExpectedPrefix(strings.TrimSpace(ip), bgpExpected) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This switch block in collectPodExpectedPrefixes has a critical security vulnerability: it reads IP addresses from pod annotations ({provider}.kubernetes.io/ip_address) without validating them against the subnet's CIDR block. An attacker with patch pod permissions can exploit this to announce arbitrary IP addresses, leading to BGP hijacking. This is a regression from the previous implementation.
Additionally, there are functional issues:
- The function
strings.SplitSeqis non-standard and will cause a compilation error. Usestrings.Split. - The loop
for ip := range ...is incorrect for iterating slice values; usefor _, ip := range .... - A
defaultcase is missing from theswitchstatement to handle and log unknown BGP policies, which is crucial for debugging.
switch policy {
case "true", announcePolicyCluster:
for ip := range strings.SplitSeq(ipStr, ",") {
ip = strings.TrimSpace(ip)
if !util.CIDRContainIP(subnet.Spec.CIDRBlock, ip) {
klog.Warningf("pod %s/%s IP %s is not in subnet %s CIDR %s", pod.Namespace, pod.Name, ip, subnet.Name, subnet.Spec.CIDRBlock)
continue
}
addExpectedPrefix(ip, bgpExpected)
}
case announcePolicyLocal:
if pod.Spec.NodeName == nodeName {
for ip := range strings.SplitSeq(ipStr, ",") {
ip = strings.TrimSpace(ip)
if !util.CIDRContainIP(subnet.Spec.CIDRBlock, ip) {
klog.Warningf("pod %s/%s IP %s is not in subnet %s CIDR %s", pod.Namespace, pod.Name, ip, subnet.Name, subnet.Spec.CIDRBlock)
continue
}
addExpectedPrefix(ip, bgpExpected)
}
}
}
oilbeater
force-pushed
the
fix/bgp-speaker-annotation-ip
branch
from
37c4628 to
f5e038b
Compare
BGP Speaker's syncSubnetRoutes() previously read pod IPs from
pod.Status.PodIPs, which is managed by kubelet and only contains
primary CNI IPs. This caused incorrect behavior when Kube-OVN runs
as a secondary CNI (non-primary mode) or when attachment networks
are configured — the speaker would either advertise the wrong IPs
or miss attachment network IPs entirely.
Replace pod.Status.PodIPs with pod annotation-based IP discovery
({provider}.kubernetes.io/ip_address), which is the unified and
reliable source for all Kube-OVN managed IPs across both primary
and attachment networks.
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
oilbeater
force-pushed
the
fix/bgp-speaker-annotation-ip
branch
from
f5e038b to
c333b1c
Compare
Pull Request Test Coverage Report for Build 22567353295Details
💛 - Coveralls |
|
@oilbeater when can we expect this functionality to appear in the next 1.15 release? |
…#6344) BGP Speaker's syncSubnetRoutes() previously read pod IPs from pod.Status.PodIPs, which is managed by kubelet and only contains primary CNI IPs. This caused incorrect behavior when Kube-OVN runs as a secondary CNI (non-primary mode) or when attachment networks are configured — the speaker would either advertise the wrong IPs or miss attachment network IPs entirely. Replace pod.Status.PodIPs with pod annotation-based IP discovery ({provider}.kubernetes.io/ip_address), which is the unified and reliable source for all Kube-OVN managed IPs across both primary and attachment networks. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…kubeovn#6344) BGP Speaker's syncSubnetRoutes() previously read pod IPs from pod.Status.PodIPs, which is managed by kubelet and only contains primary CNI IPs. This caused incorrect behavior when Kube-OVN runs as a secondary CNI (non-primary mode) or when attachment networks are configured — the speaker would either advertise the wrong IPs or miss attachment network IPs entirely. Replace pod.Status.PodIPs with pod annotation-based IP discovery ({provider}.kubernetes.io/ip_address), which is the unified and reliable source for all Kube-OVN managed IPs across both primary and attachment networks. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit f3dca4b)
3 participants
Summary
syncSubnetRoutes()previously read pod IPs frompod.Status.PodIPs(kubelet-managed, primary CNI only), causing incorrect BGP advertisements when Kube-OVN runs as a secondary CNI or with attachment networks{provider}.kubernetes.io/ip_address), which is the unified source for all Kube-OVN managed IPs across primary and attachment networksovn.kubernetes.io/bgpannotation takes priority over subnet-level BGP policy; subnet CIDR-level broadcasting remains unchangedFixes #6270
Test plan
make lintpasses with 0 issuespkg/speakertests pass🤖 Generated with Claude Code