Skip to content

Commit 932fccc

Browse files
oilbeaterclaude
oilbeater
and
claude
authored
fix(daemon): skip invalid policy routing rules when pod lacks matching protocol IP (#6522)
* fix(daemon): skip invalid policy routing rules when pod lacks matching protocol IP In getPolicyRouting, when ExternalEgressGateway is dual-stack but a pod only has single-stack IP, the loop generated a rule with nil IP and zero mask (matching all traffic). This could cause unexpected traffic hijacking via policy routing. Fix both the distributed gateway branch (skip when pod has no IP for the protocol) and the centralized gateway branch (skip when CIDR count is less than protocol count). Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> * fix(daemon): address review feedback for getPolicyRouting - Add error handling for net.ParseCIDR in centralized gateway branch, log and skip malformed CIDRs instead of silently producing nil rules - Add test case for centralized dual-stack EGW with single-stack CIDR Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> --------- Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 2c28bfa commit 932fccc

File tree

2 files changed

+332
-2
lines changed

2 files changed

+332
-2
lines changed

‎pkg/daemon/controller_linux.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -813,16 +813,25 @@ func (c *Controller) getPolicyRouting(subnet *kubeovnv1.Subnet) ([]netlink.Rule,
813813
}
814814
}
815815

816+
if ip == nil {
817+
continue
818+
}
816819
rule.Src = &net.IPNet{IP: ip, Mask: net.CIDRMask(maskBits, maskBits)}
817820
rules = append(rules, *rule)
818821
}
819822
}
820823
} else {
821824
for i := range protocols {
822825
rule.Family, _ = util.ProtocolToFamily(protocols[i])
823-
if len(cidr) == len(protocols) {
824-
_, rule.Src, _ = net.ParseCIDR(cidr[i])
826+
if i >= len(cidr) {
827+
continue
828+
}
829+
_, ipNet, err := net.ParseCIDR(cidr[i])
830+
if err != nil {
831+
klog.Errorf("failed to parse CIDR %q for subnet %s policy routing: %v", cidr[i], subnet.Name, err)
832+
continue
825833
}
834+
rule.Src = ipNet
826835
rules = append(rules, *rule)
827836
}
828837
}

‎pkg/daemon/controller_linux_test.go‎

Lines changed: 321 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,321 @@
1+
package daemon
2+
3+
import (
4+
"net"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
"github.com/vishvananda/netlink"
9+
"golang.org/x/sys/unix"
10+
v1 "k8s.io/api/core/v1"
11+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
listerv1 "k8s.io/client-go/listers/core/v1"
13+
"k8s.io/client-go/tools/cache"
14+
15+
kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1"
16+
"github.com/kubeovn/kube-ovn/pkg/util"
17+
)
18+
19+
func newPodForPolicyRouting(name, namespace, subnetName, podIP string, podIPs []v1.PodIP) *v1.Pod {
20+
return &v1.Pod{
21+
ObjectMeta: metav1.ObjectMeta{
22+
Name: name,
23+
Namespace: namespace,
24+
Annotations: map[string]string{
25+
util.LogicalSwitchAnnotation: subnetName,
26+
},
27+
},
28+
Status: v1.PodStatus{
29+
PodIP: podIP,
30+
PodIPs: podIPs,
31+
},
32+
}
33+
}
34+
35+
func TestGetPolicyRouting(t *testing.T) {
36+
t.Parallel()
37+
38+
const (
39+
clusterRouter = "ovn-cluster"
40+
nodeName = "test-node"
41+
subnetName = "test-subnet"
42+
tableID = 100
43+
priority = 200
44+
)
45+
46+
tests := []struct {
47+
name string
48+
subnet *kubeovnv1.Subnet
49+
pods []*v1.Pod
50+
expectedRules int
51+
expectedRtns int
52+
validateRules func(t *testing.T, rules []netlink.Rule)
53+
}{
54+
{
55+
name: "nil subnet returns nil",
56+
subnet: nil,
57+
expectedRules: 0,
58+
expectedRtns: 0,
59+
},
60+
{
61+
name: "subnet without ExternalEgressGateway returns nil",
62+
subnet: &kubeovnv1.Subnet{
63+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
64+
Spec: kubeovnv1.SubnetSpec{
65+
Vpc: clusterRouter,
66+
GatewayType: kubeovnv1.GWDistributedType,
67+
},
68+
},
69+
expectedRules: 0,
70+
expectedRtns: 0,
71+
},
72+
{
73+
name: "subnet with different VPC returns nil",
74+
subnet: &kubeovnv1.Subnet{
75+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
76+
Spec: kubeovnv1.SubnetSpec{
77+
Vpc: "other-vpc",
78+
ExternalEgressGateway: "10.0.0.1",
79+
GatewayType: kubeovnv1.GWDistributedType,
80+
PolicyRoutingTableID: tableID,
81+
PolicyRoutingPriority: priority,
82+
},
83+
},
84+
expectedRules: 0,
85+
expectedRtns: 0,
86+
},
87+
{
88+
name: "distributed: single-stack IPv4 EGW + IPv4 Pod",
89+
subnet: &kubeovnv1.Subnet{
90+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
91+
Spec: kubeovnv1.SubnetSpec{
92+
Vpc: clusterRouter,
93+
CIDRBlock: "10.16.0.0/24",
94+
ExternalEgressGateway: "10.0.0.1",
95+
GatewayType: kubeovnv1.GWDistributedType,
96+
PolicyRoutingTableID: tableID,
97+
PolicyRoutingPriority: priority,
98+
},
99+
},
100+
pods: []*v1.Pod{
101+
newPodForPolicyRouting("pod1", "default", subnetName, "10.16.0.5",
102+
[]v1.PodIP{{IP: "10.16.0.5"}}),
103+
},
104+
expectedRules: 1,
105+
expectedRtns: 1,
106+
validateRules: func(t *testing.T, rules []netlink.Rule) {
107+
require.Equal(t, unix.AF_INET, rules[0].Family)
108+
require.Equal(t, net.ParseIP("10.16.0.5"), rules[0].Src.IP)
109+
ones, _ := rules[0].Src.Mask.Size()
110+
require.Equal(t, 32, ones)
111+
},
112+
},
113+
{
114+
name: "distributed: dual-stack EGW + dual-stack Pod",
115+
subnet: &kubeovnv1.Subnet{
116+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
117+
Spec: kubeovnv1.SubnetSpec{
118+
Vpc: clusterRouter,
119+
CIDRBlock: "10.16.0.0/24,fd00::/120",
120+
ExternalEgressGateway: "10.0.0.1,fd00::1",
121+
GatewayType: kubeovnv1.GWDistributedType,
122+
PolicyRoutingTableID: tableID,
123+
PolicyRoutingPriority: priority,
124+
},
125+
},
126+
pods: []*v1.Pod{
127+
newPodForPolicyRouting("pod1", "default", subnetName, "10.16.0.5",
128+
[]v1.PodIP{{IP: "10.16.0.5"}, {IP: "fd00::5"}}),
129+
},
130+
expectedRules: 2,
131+
expectedRtns: 2,
132+
validateRules: func(t *testing.T, rules []netlink.Rule) {
133+
require.Equal(t, unix.AF_INET, rules[0].Family)
134+
require.Equal(t, net.ParseIP("10.16.0.5"), rules[0].Src.IP)
135+
ones, _ := rules[0].Src.Mask.Size()
136+
require.Equal(t, 32, ones)
137+
138+
require.Equal(t, unix.AF_INET6, rules[1].Family)
139+
require.Equal(t, net.ParseIP("fd00::5"), rules[1].Src.IP)
140+
ones, _ = rules[1].Src.Mask.Size()
141+
require.Equal(t, 128, ones)
142+
},
143+
},
144+
{
145+
name: "distributed: dual-stack EGW + IPv4-only Pod should skip IPv6 rule",
146+
subnet: &kubeovnv1.Subnet{
147+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
148+
Spec: kubeovnv1.SubnetSpec{
149+
Vpc: clusterRouter,
150+
CIDRBlock: "10.16.0.0/24,fd00::/120",
151+
ExternalEgressGateway: "10.0.0.1,fd00::1",
152+
GatewayType: kubeovnv1.GWDistributedType,
153+
PolicyRoutingTableID: tableID,
154+
PolicyRoutingPriority: priority,
155+
},
156+
},
157+
pods: []*v1.Pod{
158+
newPodForPolicyRouting("pod1", "default", subnetName, "10.16.0.5",
159+
[]v1.PodIP{{IP: "10.16.0.5"}}),
160+
},
161+
expectedRules: 1,
162+
expectedRtns: 2,
163+
validateRules: func(t *testing.T, rules []netlink.Rule) {
164+
require.Equal(t, unix.AF_INET, rules[0].Family)
165+
require.Equal(t, net.ParseIP("10.16.0.5"), rules[0].Src.IP)
166+
},
167+
},
168+
{
169+
name: "distributed: dual-stack EGW + IPv6-only Pod should skip IPv4 rule",
170+
subnet: &kubeovnv1.Subnet{
171+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
172+
Spec: kubeovnv1.SubnetSpec{
173+
Vpc: clusterRouter,
174+
CIDRBlock: "10.16.0.0/24,fd00::/120",
175+
ExternalEgressGateway: "10.0.0.1,fd00::1",
176+
GatewayType: kubeovnv1.GWDistributedType,
177+
PolicyRoutingTableID: tableID,
178+
PolicyRoutingPriority: priority,
179+
},
180+
},
181+
pods: []*v1.Pod{
182+
newPodForPolicyRouting("pod1", "default", subnetName, "fd00::5",
183+
[]v1.PodIP{{IP: "fd00::5"}}),
184+
},
185+
expectedRules: 1,
186+
expectedRtns: 2,
187+
validateRules: func(t *testing.T, rules []netlink.Rule) {
188+
require.Equal(t, unix.AF_INET6, rules[0].Family)
189+
require.Equal(t, net.ParseIP("fd00::5"), rules[0].Src.IP)
190+
ones, _ := rules[0].Src.Mask.Size()
191+
require.Equal(t, 128, ones)
192+
},
193+
},
194+
{
195+
name: "distributed: multiple pods with mixed stacks",
196+
subnet: &kubeovnv1.Subnet{
197+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
198+
Spec: kubeovnv1.SubnetSpec{
199+
Vpc: clusterRouter,
200+
CIDRBlock: "10.16.0.0/24,fd00::/120",
201+
ExternalEgressGateway: "10.0.0.1,fd00::1",
202+
GatewayType: kubeovnv1.GWDistributedType,
203+
PolicyRoutingTableID: tableID,
204+
PolicyRoutingPriority: priority,
205+
},
206+
},
207+
pods: []*v1.Pod{
208+
// dual-stack pod: should generate 2 rules
209+
newPodForPolicyRouting("pod1", "default", subnetName, "10.16.0.5",
210+
[]v1.PodIP{{IP: "10.16.0.5"}, {IP: "fd00::5"}}),
211+
// IPv4-only pod: should generate 1 rule
212+
newPodForPolicyRouting("pod2", "default", subnetName, "10.16.0.6",
213+
[]v1.PodIP{{IP: "10.16.0.6"}}),
214+
// pod in different subnet: should be skipped
215+
newPodForPolicyRouting("pod3", "default", "other-subnet", "10.16.0.7",
216+
[]v1.PodIP{{IP: "10.16.0.7"}}),
217+
// pod without IP: should be skipped
218+
newPodForPolicyRouting("pod4", "default", subnetName, "",
219+
nil),
220+
},
221+
expectedRules: 3, // 2 from pod1 + 1 from pod2
222+
expectedRtns: 2,
223+
validateRules: func(t *testing.T, rules []netlink.Rule) {
224+
for _, r := range rules {
225+
require.NotNil(t, r.Src, "rule.Src must not be nil")
226+
require.NotNil(t, r.Src.IP, "rule.Src.IP must not be nil")
227+
}
228+
},
229+
},
230+
{
231+
name: "centralized: dual-stack EGW + dual-stack CIDR",
232+
subnet: &kubeovnv1.Subnet{
233+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
234+
Spec: kubeovnv1.SubnetSpec{
235+
Vpc: clusterRouter,
236+
CIDRBlock: "10.16.0.0/24,fd00::/120",
237+
ExternalEgressGateway: "10.0.0.1,fd00::1",
238+
GatewayType: kubeovnv1.GWCentralizedType,
239+
GatewayNode: nodeName,
240+
PolicyRoutingTableID: tableID,
241+
PolicyRoutingPriority: priority,
242+
},
243+
},
244+
expectedRules: 2,
245+
expectedRtns: 2,
246+
validateRules: func(t *testing.T, rules []netlink.Rule) {
247+
require.Equal(t, unix.AF_INET, rules[0].Family)
248+
require.NotNil(t, rules[0].Src)
249+
require.Equal(t, "10.16.0.0/24", rules[0].Src.String())
250+
251+
require.Equal(t, unix.AF_INET6, rules[1].Family)
252+
require.NotNil(t, rules[1].Src)
253+
require.Equal(t, "fd00::/120", rules[1].Src.String())
254+
},
255+
},
256+
{
257+
name: "centralized: dual-stack EGW + single-stack CIDR skips missing protocol",
258+
subnet: &kubeovnv1.Subnet{
259+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
260+
Spec: kubeovnv1.SubnetSpec{
261+
Vpc: clusterRouter,
262+
CIDRBlock: "10.16.0.0/24",
263+
ExternalEgressGateway: "10.0.0.1,fd00::1",
264+
GatewayType: kubeovnv1.GWCentralizedType,
265+
GatewayNode: nodeName,
266+
PolicyRoutingTableID: tableID,
267+
PolicyRoutingPriority: priority,
268+
},
269+
},
270+
expectedRules: 1,
271+
expectedRtns: 2,
272+
validateRules: func(t *testing.T, rules []netlink.Rule) {
273+
require.Equal(t, unix.AF_INET, rules[0].Family)
274+
require.NotNil(t, rules[0].Src)
275+
require.Equal(t, "10.16.0.0/24", rules[0].Src.String())
276+
},
277+
},
278+
}
279+
280+
for _, tt := range tests {
281+
t.Run(tt.name, func(t *testing.T) {
282+
t.Parallel()
283+
284+
// Build pod indexer
285+
podIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
286+
for _, pod := range tt.pods {
287+
require.NoError(t, podIndexer.Add(pod))
288+
}
289+
290+
// Build node indexer for centralized gateway tests
291+
nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
292+
require.NoError(t, nodeIndexer.Add(&v1.Node{
293+
ObjectMeta: metav1.ObjectMeta{Name: nodeName},
294+
}))
295+
296+
c := &Controller{
297+
podsLister: listerv1.NewPodLister(podIndexer),
298+
nodesLister: listerv1.NewNodeLister(nodeIndexer),
299+
config: &Configuration{
300+
ClusterRouter: clusterRouter,
301+
NodeName: nodeName,
302+
},
303+
}
304+
305+
rules, routes, err := c.getPolicyRouting(tt.subnet)
306+
require.NoError(t, err)
307+
require.Len(t, rules, tt.expectedRules)
308+
require.Len(t, routes, tt.expectedRtns)
309+
310+
// Validate all rules have non-nil Src.IP
311+
for i, r := range rules {
312+
require.NotNil(t, r.Src, "rule[%d].Src must not be nil", i)
313+
require.NotNil(t, r.Src.IP, "rule[%d].Src.IP must not be nil", i)
314+
}
315+
316+
if tt.validateRules != nil {
317+
tt.validateRules(t, rules)
318+
}
319+
})
320+
}
321+
}

0 commit comments

Comments
 (0)