Skip to content

Commit b29a0ab

Browse files
zbb88888zbb88888
committed
fix(vpc-nat-gw): use consistent provider for all pod annotations (#6204)
The VPC NAT gateway custom routes were not being applied because of inconsistent annotation key prefixes. The `GenNatGwPodAnnotations` function was hardcoding the "ovn" prefix for annotations like `ovn.kubernetes.io/logical_switch`, while `setPodRoutesAnnotation` was using the actual subnet provider (e.g., "subnet.namespace.ovn"). This caused the CNI daemon to look for routes annotation with a different key than what was set by the controller, resulting in routes being ignored. Changes: - Add `provider` parameter to `GenNatGwPodAnnotations` function - Use annotation templates instead of hardcoded constants to generate annotation keys dynamically based on the provider - Remove redundant annotation settings in `EnableNonPrimaryCNI` branch since `GenNatGwPodAnnotations` now handles all cases uniformly - Update unit tests to cover both "ovn" and NAD provider scenarios Fixes #6202 --------- Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>
1 parent b3d96f7 commit b29a0ab

File tree

3 files changed

+107
-37
lines changed

3 files changed

+107
-37
lines changed

‎pkg/controller/vpc_nat_gateway.go‎

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -849,40 +849,28 @@ func (c *Controller) genNatGwStatefulSet(gw *kubeovnv1.VpcNatGateway, oldSts *v1
849849
}
850850

851851
externalNadNamespace, externalNadName := c.getExternalSubnetNad(gw)
852-
podAnnotations := util.GenNatGwPodAnnotations(gw, externalNadNamespace, externalNadName)
853-
854-
// Restart logic to fix #5072
855-
if oldSts != nil && len(oldSts.Spec.Template.Annotations) != 0 {
856-
if _, ok := oldSts.Spec.Template.Annotations[util.VpcNatGatewayContainerRestartAnnotation]; !ok && natGwPodContainerRestartCount > 0 {
857-
podAnnotations[util.VpcNatGatewayContainerRestartAnnotation] = ""
858-
}
859-
}
860852

861853
eth0SubnetProvider, err := c.GetSubnetProvider(gw.Spec.Subnet)
862854
if err != nil {
863855
klog.Errorf("failed to get gw eth0 valid subnet provider: %v", err)
864856
return nil, err
865857
}
866-
if c.config.EnableNonPrimaryCNI {
867-
// We specify NAD using annotations when Kube-OVN is running as a secondary CNI
868-
var attachedNetworks string
869-
// Get NetworkAttachmentDefinition if specified by user from pod annotations
870-
if gw.Annotations != nil && gw.Annotations[nadv1.NetworkAttachmentAnnot] != "" {
871-
attachedNetworks = gw.Annotations[nadv1.NetworkAttachmentAnnot] + ", "
872-
}
873-
// Attach the external network to attachedNetworks
874-
attachedNetworks += fmt.Sprintf("%s/%s", externalNadNamespace, externalNadName)
875-
// Check if we have a subnet provider, if so, use it to set the routes annotation
876-
// This is useful when running in secondary CNI mode, as the subnet provider will be the
877-
// one that has the routes to the subnet
878-
vpcNatGwNameAnnotation := fmt.Sprintf(util.VpcNatGatewayAnnotationTemplate, eth0SubnetProvider)
879-
logicalSwitchAnnotation := fmt.Sprintf(util.LogicalSwitchAnnotationTemplate, eth0SubnetProvider)
880-
ipAddressAnnotation := fmt.Sprintf(util.IPAddressAnnotationTemplate, eth0SubnetProvider)
881-
// Merge new annotations with existing ones
882-
podAnnotations[nadv1.NetworkAttachmentAnnot] = attachedNetworks
883-
podAnnotations[vpcNatGwNameAnnotation] = gw.Name
884-
podAnnotations[logicalSwitchAnnotation] = gw.Spec.Subnet
885-
podAnnotations[ipAddressAnnotation] = gw.Spec.LanIP
858+
859+
// Get additional networks specified by user in gw.Annotations (for secondary CNI mode)
860+
// TODO: the EnableNonPrimaryCNI check may not be necessary, as additional NADs could also
861+
// be useful in primary CNI mode. Consider removing this condition in the future.
862+
var additionalNetworks string
863+
if c.config.EnableNonPrimaryCNI && gw.Annotations != nil {
864+
additionalNetworks = gw.Annotations[nadv1.NetworkAttachmentAnnot]
865+
}
866+
867+
podAnnotations := util.GenNatGwPodAnnotations(gw, externalNadNamespace, externalNadName, eth0SubnetProvider, additionalNetworks)
868+
869+
// Restart logic to fix #5072
870+
if oldSts != nil && len(oldSts.Spec.Template.Annotations) != 0 {
871+
if _, ok := oldSts.Spec.Template.Annotations[util.VpcNatGatewayContainerRestartAnnotation]; !ok && natGwPodContainerRestartCount > 0 {
872+
podAnnotations[util.VpcNatGatewayContainerRestartAnnotation] = ""
873+
}
886874
}
887875
klog.V(3).Infof("%s podAnnotations:%v", gw.Name, podAnnotations)
888876

‎pkg/util/vpc_nat_gateway.go‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,21 @@ func GenNatGwSelectors(selectors []string) map[string]string {
5959
}
6060

6161
// GenNatGwPodAnnotations returns the Pod annotations for a NAT gateway
62-
func GenNatGwPodAnnotations(gw *kubeovnv1.VpcNatGateway, externalNadNamespace, externalNadName string) map[string]string {
62+
// additionalNetworks is optional, used when user specifies extra NADs in gw.Annotations
63+
func GenNatGwPodAnnotations(gw *kubeovnv1.VpcNatGateway, externalNadNamespace, externalNadName, provider, additionalNetworks string) map[string]string {
64+
p := provider
65+
if p == "" {
66+
p = OvnProvider
67+
}
68+
attachedNetworks := fmt.Sprintf("%s/%s", externalNadNamespace, externalNadName)
69+
if additionalNetworks != "" {
70+
attachedNetworks = additionalNetworks + ", " + attachedNetworks
71+
}
6372
return map[string]string{
64-
VpcNatGatewayAnnotation: gw.Name,
65-
nadv1.NetworkAttachmentAnnot: fmt.Sprintf("%s/%s", externalNadNamespace, externalNadName),
66-
LogicalSwitchAnnotation: gw.Spec.Subnet,
67-
IPAddressAnnotation: gw.Spec.LanIP,
73+
fmt.Sprintf(VpcNatGatewayAnnotationTemplate, p): gw.Name,
74+
nadv1.NetworkAttachmentAnnot: attachedNetworks,
75+
fmt.Sprintf(LogicalSwitchAnnotationTemplate, p): gw.Spec.Subnet,
76+
fmt.Sprintf(IPAddressAnnotationTemplate, p): gw.Spec.LanIP,
6877
}
6978
}
7079

‎pkg/util/vpc_nat_gateway_test.go‎

Lines changed: 77 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package util
22

33
import (
4+
"fmt"
45
"reflect"
56
"testing"
67

@@ -265,10 +266,12 @@ func TestGenNatGwPodAnnotations(t *testing.T) {
265266
gw v1.VpcNatGateway
266267
externalNadNamespace string
267268
externalNadName string
269+
provider string
270+
additionalNetworks string
268271
expected map[string]string
269272
}{
270273
{
271-
name: "All fields provided",
274+
name: "Empty provider defaults to ovn",
272275
gw: v1.VpcNatGateway{
273276
ObjectMeta: metav1.ObjectMeta{
274277
Name: "test-gateway",
@@ -279,14 +282,82 @@ func TestGenNatGwPodAnnotations(t *testing.T) {
279282
},
280283
},
281284
externalNadName: "external-subnet",
282-
externalNadNamespace: "kube-system",
285+
externalNadNamespace: metav1.NamespaceSystem,
286+
provider: "",
287+
additionalNetworks: "",
283288
expected: map[string]string{
284289
VpcNatGatewayAnnotation: "test-gateway",
285290
nadv1.NetworkAttachmentAnnot: "kube-system/external-subnet",
286291
LogicalSwitchAnnotation: "internal-subnet",
287292
IPAddressAnnotation: "10.20.30.40",
288293
},
289294
},
295+
{
296+
name: "All fields provided with ovn provider",
297+
gw: v1.VpcNatGateway{
298+
ObjectMeta: metav1.ObjectMeta{
299+
Name: "test-gateway",
300+
},
301+
Spec: v1.VpcNatGatewaySpec{
302+
Subnet: "internal-subnet",
303+
LanIP: "10.20.30.40",
304+
},
305+
},
306+
externalNadName: "external-subnet",
307+
externalNadNamespace: metav1.NamespaceSystem,
308+
provider: OvnProvider,
309+
additionalNetworks: "",
310+
expected: map[string]string{
311+
VpcNatGatewayAnnotation: "test-gateway",
312+
nadv1.NetworkAttachmentAnnot: "kube-system/external-subnet",
313+
LogicalSwitchAnnotation: "internal-subnet",
314+
IPAddressAnnotation: "10.20.30.40",
315+
},
316+
},
317+
{
318+
name: "All fields provided with NAD provider",
319+
gw: v1.VpcNatGateway{
320+
ObjectMeta: metav1.ObjectMeta{
321+
Name: "test-gateway",
322+
},
323+
Spec: v1.VpcNatGatewaySpec{
324+
Subnet: "internal-subnet",
325+
LanIP: "10.20.30.40",
326+
},
327+
},
328+
externalNadName: "external-subnet",
329+
externalNadNamespace: metav1.NamespaceSystem,
330+
provider: "subnet.namespace.ovn",
331+
additionalNetworks: "",
332+
expected: map[string]string{
333+
fmt.Sprintf(VpcNatGatewayAnnotationTemplate, "subnet.namespace.ovn"): "test-gateway",
334+
nadv1.NetworkAttachmentAnnot: "kube-system/external-subnet",
335+
fmt.Sprintf(LogicalSwitchAnnotationTemplate, "subnet.namespace.ovn"): "internal-subnet",
336+
fmt.Sprintf(IPAddressAnnotationTemplate, "subnet.namespace.ovn"): "10.20.30.40",
337+
},
338+
},
339+
{
340+
name: "With additional networks for secondary CNI",
341+
gw: v1.VpcNatGateway{
342+
ObjectMeta: metav1.ObjectMeta{
343+
Name: "test-gateway",
344+
},
345+
Spec: v1.VpcNatGatewaySpec{
346+
Subnet: "internal-subnet",
347+
LanIP: "10.20.30.40",
348+
},
349+
},
350+
externalNadName: "external-subnet",
351+
externalNadNamespace: metav1.NamespaceSystem,
352+
provider: "subnet.namespace.ovn",
353+
additionalNetworks: "default/extra-net1, default/extra-net2",
354+
expected: map[string]string{
355+
fmt.Sprintf(VpcNatGatewayAnnotationTemplate, "subnet.namespace.ovn"): "test-gateway",
356+
nadv1.NetworkAttachmentAnnot: "default/extra-net1, default/extra-net2, kube-system/external-subnet",
357+
fmt.Sprintf(LogicalSwitchAnnotationTemplate, "subnet.namespace.ovn"): "internal-subnet",
358+
fmt.Sprintf(IPAddressAnnotationTemplate, "subnet.namespace.ovn"): "10.20.30.40",
359+
},
360+
},
290361
{
291362
name: "No static LAN IP",
292363
gw: v1.VpcNatGateway{
@@ -299,7 +370,9 @@ func TestGenNatGwPodAnnotations(t *testing.T) {
299370
},
300371
},
301372
externalNadName: "external-subnet",
302-
externalNadNamespace: "kube-system",
373+
externalNadNamespace: metav1.NamespaceSystem,
374+
provider: OvnProvider,
375+
additionalNetworks: "",
303376
expected: map[string]string{
304377
VpcNatGatewayAnnotation: "test-gateway",
305378
nadv1.NetworkAttachmentAnnot: "kube-system/external-subnet",
@@ -311,7 +384,7 @@ func TestGenNatGwPodAnnotations(t *testing.T) {
311384

312385
for _, tc := range tests {
313386
t.Run(tc.name, func(t *testing.T) {
314-
result := GenNatGwPodAnnotations(&tc.gw, tc.externalNadNamespace, tc.externalNadName)
387+
result := GenNatGwPodAnnotations(&tc.gw, tc.externalNadNamespace, tc.externalNadName, tc.provider, tc.additionalNetworks)
315388
if !reflect.DeepEqual(tc.expected, result) {
316389
t.Errorf("got %v, but want %v", result, tc.expected)
317390
}

0 commit comments

Comments
 (0)