diff --git a/controllers/network/bgpconfiguration_controller.go b/controllers/network/bgpconfiguration_controller.go index aad71c86..1391241c 100644 --- a/controllers/network/bgpconfiguration_controller.go +++ b/controllers/network/bgpconfiguration_controller.go @@ -243,38 +243,61 @@ func (r *BGPConfigurationReconciler) SetupWithManager(ctx context.Context, mgr c // Skip if // * no NAD annotation was configured on old object OR // * no NAD annotation was configured on new object AND + // * no predictableip label was configured on old object OR + // * no predictableip label was configured on new object AND // * the resourceVersion has not changed - oldConfigured := true + oldNADConfigured := true if val, ok := e.ObjectOld.GetAnnotations()[k8s_networkv1.NetworkAttachmentAnnot]; !ok || len(val) == 0 { - oldConfigured = false + oldNADConfigured = false } - newConfigured := true + newNADConfigured := true if val, ok := e.ObjectNew.GetAnnotations()[k8s_networkv1.NetworkAttachmentAnnot]; !ok || len(val) == 0 { - newConfigured = false + newNADConfigured = false } - return (oldConfigured || newConfigured) && e.ObjectOld.GetResourceVersion() != e.ObjectNew.GetResourceVersion() + oldPredictableIPConfigured := true + if val, ok := e.ObjectOld.GetLabels()["predictableip"]; !ok || len(val) == 0 { + oldPredictableIPConfigured = false + } + newPredictableIPConfigured := true + if val, ok := e.ObjectNew.GetLabels()["predictableip"]; !ok || len(val) == 0 { + newPredictableIPConfigured = false + } + + return (oldNADConfigured || newNADConfigured || oldPredictableIPConfigured || newPredictableIPConfigured) && e.ObjectOld.GetResourceVersion() != e.ObjectNew.GetResourceVersion() }, DeleteFunc: func(e event.DeleteEvent) bool { // Skip if - // * NAD annotation key is missing + // * NAD annotation key is missing AND predictableip label is missing // * there is no additional network configured - if val, ok := e.Object.GetAnnotations()[k8s_networkv1.NetworkAttachmentAnnot]; !ok || len(val) == 0 { - return false + nadConfigured := false + if val, ok := e.Object.GetAnnotations()[k8s_networkv1.NetworkAttachmentAnnot]; ok && len(val) > 0 { + nadConfigured = true } - return true + predictableIPConfigured := false + if val, ok := e.Object.GetLabels()["predictableip"]; ok && len(val) > 0 { + predictableIPConfigured = true + } + + return nadConfigured || predictableIPConfigured }, CreateFunc: func(e event.CreateEvent) bool { // Skip if - // * NAD annotation key is missing + // * NAD annotation key is missing AND predictableip label is missing // * there is no additional network configured - if val, ok := e.Object.GetAnnotations()[k8s_networkv1.NetworkAttachmentAnnot]; !ok || len(val) == 0 { - return false + nadConfigured := false + if val, ok := e.Object.GetAnnotations()[k8s_networkv1.NetworkAttachmentAnnot]; ok && len(val) > 0 { + nadConfigured = true } - return true + predictableIPConfigured := false + if val, ok := e.Object.GetLabels()["predictableip"]; ok && len(val) > 0 { + predictableIPConfigured = true + } + + return nadConfigured || predictableIPConfigured }, } @@ -315,7 +338,7 @@ func (r *BGPConfigurationReconciler) SetupWithManager(ctx context.Context, mgr c return ctrl.NewControllerManagedBy(mgr). For(&networkv1.BGPConfiguration{}). - // Watch pods which have additional networks configured with k8s_networkv1.NetworkAttachmentAnnot annotation in the same namespace + // Watch pods which have additional networks configured with k8s_networkv1.NetworkAttachmentAnnot annotation or predictableip label in the same namespace Watches(&corev1.Pod{}, podFN, builder.WithPredicates(pPod)). @@ -479,7 +502,7 @@ func (r *BGPConfigurationReconciler) deleteStaleFRRConfigurations(ctx context.Co // getPodNetworkDetails - returns the podDetails for a list of pods in status.phase: Running // where the pod has the multus k8s_networkv1.NetworkAttachmentAnnot annotation -// and its value is not '[]' +// and its value is not '[]' OR has a predictableip label func getPodNetworkDetails( ctx context.Context, h *helper.Helper, @@ -499,14 +522,24 @@ func getPodNetworkDetails( if pod.Status.Phase != corev1.PodRunning { continue } - if netAttachString, ok := pod.Annotations[k8s_networkv1.NetworkAttachmentAnnot]; ok && netAttachString != "[]" { - // get the elements from val to validate the status annotation has the right length - netAttach := []k8s_networkv1.NetworkSelectionElement{} - err := json.Unmarshal([]byte(netAttachString), &netAttach) - if err != nil { - return nil, fmt.Errorf("failed to decode networks %s: %w", netAttachString, err) - } + // Check for pods with predictableip label + hasPredictableIP := false + predictableIP := "" + if ipValue, ok := pod.Labels["predictableip"]; ok && ipValue != "" { + hasPredictableIP = true + predictableIP = ipValue + } + + hasNetworkAttachment := false + var netAttachString string + if netAttach, ok := pod.Annotations[k8s_networkv1.NetworkAttachmentAnnot]; ok && netAttach != "[]" { + hasNetworkAttachment = true + netAttachString = netAttach + } + + // Process pod if it has either network attachments or predictableip label + if hasNetworkAttachment || hasPredictableIP { // verify the nodeName information is already present in the pod spec, otherwise report an error to reconcile if pod.Spec.NodeName == "" { return detailList, fmt.Errorf("empty spec.nodeName on pod %s", pod.Name) @@ -518,55 +551,121 @@ func getPodNetworkDetails( Node: pod.Spec.NodeName, } - netsStatus, err := nad.GetNetworkStatusFromAnnotation(pod.Annotations) - if err != nil { - return detailList, fmt.Errorf("failed to get netsStatus from pod annoation - %v: %w", pod.Annotations, err) - } - // on pod start it can happen that the network status annotation does not yet - // reflect all requested networks. return with an error to reconcile if the length - // is <= the status. Note: the status also has the pod network - if len(netsStatus) <= len(netAttach) { - return detailList, fmt.Errorf("metadata.Annotations['k8s.ovn.org/pod-networks'] %s on pod %s, does not match requested networks %s", - pod.GetAnnotations()[k8s_networkv1.NetworkStatusAnnot], pod.Name, netAttachString) - } + var netsStatus []k8s_networkv1.NetworkStatus + + // Handle pods with network attachments + if hasNetworkAttachment { + // get the elements from val to validate the status annotation has the right length + netAttach := []k8s_networkv1.NetworkSelectionElement{} - netsStatusCopy := make([]k8s_networkv1.NetworkStatus, len(netsStatus)) - copy(netsStatusCopy, netsStatus) - // verify there are IP information for all networks in the status, otherwise report an error to reconcile - for idx, netStat := range netsStatusCopy { - // remove status for the pod interface - // it should always be ovn-kubernetes, but if not, remove status for eth0 which is the pod network - if netStat.Name == "ovn-kubernetes" || netStat.Interface == "eth0" { - removeIndex(netsStatus, idx) + // Validate that the annotation is not empty and contains valid JSON + if netAttachString == "" { + Log.Info(fmt.Sprintf("Skipping pod %s: network attachment annotation is empty", pod.Name)) continue } - // get ipam configuration from NAD - nadName := strings.TrimPrefix(netStat.Name, pod.Namespace+"/") - netAtt, err := nad.GetNADWithName( - ctx, h, nadName, pod.Namespace) - if err != nil { - return detailList, err + // Check if the annotation looks like JSON (starts with [ or {) or is a simple string reference + trimmed := strings.TrimSpace(netAttachString) + if !strings.HasPrefix(trimmed, "[") && !strings.HasPrefix(trimmed, "{") && !strings.HasPrefix(trimmed, "\"") { + Log.Info(fmt.Sprintf("Skipping pod %s: network attachment annotation does not appear to be valid JSON or string reference: %s", pod.Name, netAttachString)) + continue + } + + // Log the raw annotation for debugging + Log.Info(fmt.Sprintf("Processing pod %s with network attachment annotation: %s", pod.Name, netAttachString)) + + // Handle different annotation formats + var err error + if strings.HasPrefix(trimmed, "\"") && strings.HasSuffix(trimmed, "\"") { + // Simple string reference like "bgpnet-worker-3" + var nadName string + err = json.Unmarshal([]byte(netAttachString), &nadName) + if err != nil { + Log.Error(err, fmt.Sprintf("Failed to decode network attachment string reference for pod %s. Raw annotation: %s", pod.Name, netAttachString)) + return nil, fmt.Errorf("failed to decode network string reference %s for pod %s: %w", netAttachString, pod.Name, err) + } + // Create a NetworkSelectionElement from the string reference + netAttach = []k8s_networkv1.NetworkSelectionElement{ + { + Name: nadName, + Namespace: pod.Namespace, + }, + } + Log.Info(fmt.Sprintf("Converted string reference %s to NetworkSelectionElement for pod %s", nadName, pod.Name)) + } else { + // Full JSON array format + err = json.Unmarshal([]byte(netAttachString), &netAttach) + if err != nil { + Log.Error(err, fmt.Sprintf("Failed to decode network attachment annotation for pod %s. Raw annotation: %s", pod.Name, netAttachString)) + return nil, fmt.Errorf("failed to decode networks %s for pod %s: %w", netAttachString, pod.Name, err) + } } - ipam, err := nad.GetJSONPathFromConfig(*netAtt, ".ipam") + netsStatus, err = nad.GetNetworkStatusFromAnnotation(pod.Annotations) if err != nil { - return detailList, err + return detailList, fmt.Errorf("failed to get netsStatus from pod annoation - %v: %w", pod.Annotations, err) + } + Log.Info(fmt.Sprintf("Retrieved network statuses for pod %s: %+v", pod.Name, netsStatus)) + // on pod start it can happen that the network status annotation does not yet + // reflect all requested networks. return with an error to reconcile if the length + // is <= the status. Note: the status also has the pod network + if len(netsStatus) <= len(netAttach) { + return detailList, fmt.Errorf("metadata.Annotations['k8s.ovn.org/pod-networks'] %s on pod %s, does not match requested networks %s", + pod.GetAnnotations()[k8s_networkv1.NetworkStatusAnnot], pod.Name, netAttachString) } - // if the NAD has no ipam configured, skip it as there will be no IP - if ipam == "{}" { - Log.Info(fmt.Sprintf("removing netsStatus for NAD %s for %s, IPAM configuration is empty: %s", netAtt.Name, pod.Name, ipam)) - removeIndex(netsStatus, idx) - continue + netsStatusCopy := make([]k8s_networkv1.NetworkStatus, len(netsStatus)) + copy(netsStatusCopy, netsStatus) + // verify there are IP information for all networks in the status, otherwise report an error to reconcile + for idx, netStat := range netsStatusCopy { + // remove status for the pod interface + // it should always be ovn-kubernetes, but if not, remove status for eth0 which is the pod network + if netStat.Name == "ovn-kubernetes" || netStat.Interface == "eth0" { + removeIndex(netsStatus, idx) + continue + } + + // get ipam configuration from NAD + nadName := strings.TrimPrefix(netStat.Name, pod.Namespace+"/") + netAtt, err := nad.GetNADWithName( + ctx, h, nadName, pod.Namespace) + if err != nil { + return detailList, err + } + + ipam, err := nad.GetJSONPathFromConfig(*netAtt, ".ipam") + if err != nil { + return detailList, err + } + + // if the NAD has no ipam configured, skip it as there will be no IP + if ipam == "{}" { + Log.Info(fmt.Sprintf("removing netsStatus for NAD %s for %s, IPAM configuration is empty: %s", netAtt.Name, pod.Name, ipam)) + removeIndex(netsStatus, idx) + continue + } + + // verify there is IP information for the network, otherwise report an error to reconcile + if len(netStat.IPs) == 0 { + return detailList, fmt.Errorf("no IP information for network %s on pod %s", netStat.Name, pod.Name) + } } + } - // verify there is IP information for the network, otherwise report an error to reconcile - if len(netStat.IPs) == 0 { - return detailList, fmt.Errorf("no IP information for network %s on pod %s", netStat.Name, pod.Name) + // Handle pods with predictableip label + if hasPredictableIP { + // Create synthetic NetworkStatus for predictable IP + predictableNetStatus := k8s_networkv1.NetworkStatus{ + Name: "predictableip", + Interface: "predictableip", + IPs: []string{predictableIP}, } + netsStatus = append(netsStatus, predictableNetStatus) + Log.Info(fmt.Sprintf("Added predictable IP %s for pod %s", predictableIP, pod.Name)) } + // Log all network statuses for debugging + Log.Info(fmt.Sprintf("Pod %s final network statuses: %+v", pod.Name, netsStatus)) detail.NetworkStatus = netsStatus detailList = append(detailList, detail) @@ -594,6 +693,7 @@ func (r *BGPConfigurationReconciler) createOrPatchFRRConfiguration( Log.Info("Reconciling createOrUpdateFRRConfiguration") podPrefixes := bgp.GetFRRPodPrefixes(podDtl.NetworkStatus) + Log.Info(fmt.Sprintf("Generated prefixes for pod %s: %v", podDtl.Name, podPrefixes)) nodeFRRCfg := nodeFRRCfgs[podDtl.Node] diff --git a/tests/functional/bgpconfiguration_controller_test.go b/tests/functional/bgpconfiguration_controller_test.go index 6c1ab45a..fc85f58a 100644 --- a/tests/functional/bgpconfiguration_controller_test.go +++ b/tests/functional/bgpconfiguration_controller_test.go @@ -25,6 +25,7 @@ import ( frrk8sv1 "github.com/metallb/frr-k8s/api/v1beta1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) var _ = Describe("BGPConfiguration controller", func() { @@ -386,4 +387,105 @@ var _ = Describe("BGPConfiguration controller", func() { } }) }) + + When("a pod with predictableip label gets created", func() { + var podFrrName types.NamespacedName + var podName types.NamespacedName + var metallbNS *corev1.Namespace + + BeforeEach(func() { + metallbNS = th.CreateNamespace(frrCfgNamespace + "-" + namespace) + // create a FRR configuration for a node + meallbFRRCfgName = types.NamespacedName{Namespace: metallbNS.Name, Name: "worker-0"} + meallbFRRCfg := CreateFRRConfiguration(meallbFRRCfgName, GetMetalLBFRRConfigurationSpec("worker-0")) + Expect(meallbFRRCfg).To(Not(BeNil())) + + bgpcfg := CreateBGPConfiguration(namespace, GetBGPConfigurationSpec(metallbNS.Name)) + bgpcfgName.Name = bgpcfg.GetName() + bgpcfgName.Namespace = bgpcfg.GetNamespace() + + podName = types.NamespacedName{Namespace: namespace, Name: uuid.New().String()} + // create pod with predictableip label but no NAD annotation + podLabels := map[string]string{ + "predictableip": "172.67.0.102", + } + + // Create pod directly with labels + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName.Name, + Namespace: podName.Namespace, + Labels: podLabels, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "foo", + Image: "foo:latest", + Ports: []corev1.ContainerPort{ + { + ContainerPort: 80, + }, + }, + }, + }, + TerminationGracePeriodSeconds: func() *int64 { i := int64(0); return &i }(), + NodeName: "worker-0", + }, + } + Expect(k8sClient.Create(ctx, pod)).Should(Succeed()) + th.SimulatePodPhaseRunning(podName) + + podFrrName.Name = podName.Namespace + "-" + podName.Name + podFrrName.Namespace = metallbNS.Name + + DeferCleanup(th.DeleteInstance, bgpcfg) + DeferCleanup(th.DeleteInstance, meallbFRRCfg) + DeferCleanup(th.DeleteInstance, pod) + }) + + It("should have created a FRRConfiguration for the pod with predictableip", func() { + pod := th.GetPod(podName) + Expect(pod).To(Not(BeNil())) + Expect(pod.Labels["predictableip"]).To(Equal("172.67.0.102")) + + podFrrName := podName.Namespace + "-" + podName.Name + Eventually(func(g Gomega) { + frr := GetFRRConfiguration(types.NamespacedName{Namespace: metallbNS.Name, Name: podFrrName}) + g.Expect(frr).To(Not(BeNil())) + g.Expect(frr.Spec.BGP.Routers[0].Prefixes).To(ContainElement("172.67.0.102/32")) + }, timeout, interval).Should(Succeed()) + }) + + When("NAD annotation gets added to the pod with predictableip", func() { + BeforeEach(func() { + // create a nad config with gateway + nad := th.CreateNAD(types.NamespacedName{Namespace: namespace, Name: "internalapi"}, GetNADSpec()) + DeferCleanup(th.DeleteInstance, nad) + + pod := th.GetPod(podName) + Expect(pod).To(Not(BeNil())) + + pod.Annotations = GetPodAnnotation(namespace) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Update(ctx, pod)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + }) + + It("should have created a FRRConfiguration with both NAD and predictableip prefixes", func() { + pod := th.GetPod(podName) + Expect(pod).To(Not(BeNil())) + Expect(pod.Labels["predictableip"]).To(Equal("172.67.0.102")) + + podFrrName := podName.Namespace + "-" + podName.Name + Eventually(func(g Gomega) { + frr := GetFRRConfiguration(types.NamespacedName{Namespace: metallbNS.Name, Name: podFrrName}) + g.Expect(frr).To(Not(BeNil())) + // Should have both the NAD IP and the predictable IP + g.Expect(frr.Spec.BGP.Routers[0].Prefixes).To(ContainElement("172.17.0.40/32")) // NAD IP + g.Expect(frr.Spec.BGP.Routers[0].Prefixes).To(ContainElement("172.67.0.102/32")) // Predictable IP + }, timeout, interval).Should(Succeed()) + }) + }) + }) })