[Apply comments] Remove SetExternalEndpoint

This function is almost not needed at all with the changes
on how the OVNDBCluster is created.

api/v1beta1/ovndbcluster_types.go

@@ -31,6 +31,10 @@ const (
 	// SBDBType - Southbound database type
 	SBDBType = "SB"
 
+	// TODO: retrieve it from environment
+	// DNS Suffix is a hardcoded value on how DNSCore domain is configured
+	DNSSuffix = "cluster.local"
+
 	// Container image fall-back defaults
 
 	// OvnNBContainerImage is the fall-back container image for OVNDBCluster NB

controllers/ovndbcluster_controller.go

@@ -23,9 +23,7 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -51,6 +49,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // OVNDBClusterReconciler reconciles a OVNDBCluster object
@@ -471,8 +470,8 @@ func (r *OVNDBClusterReconciler) reconcileNormal(ctx context.Context, instance *
 
 			// Filter out headless services
 			if svc.Spec.ClusterIP != "None" {
-				internalDbAddress = append(internalDbAddress, fmt.Sprintf("tcp:%s.%s.svc.cluster.local:%d", svc.Name, svc.Namespace, svcPort))
-				raftAddress = append(raftAddress, fmt.Sprintf("tcp:%s.%s.svc.cluster.local:%d", svc.Name, svc.Namespace, svc.Spec.Ports[1].Port))
+				internalDbAddress = append(internalDbAddress, fmt.Sprintf("tcp:%s.%s.svc.%s:%d", svc.Name, svc.Namespace, v1beta1.DNSSuffix, svcPort))
+				raftAddress = append(raftAddress, fmt.Sprintf("tcp:%s.%s.svc.%s:%d", svc.Name, svc.Namespace, v1beta1.DNSSuffix, svc.Spec.Ports[1].Port))
 			}
 		}
 
@@ -485,6 +484,41 @@ func (r *OVNDBClusterReconciler) reconcileNormal(ctx context.Context, instance *
 	return ctrl.Result{}, nil
 }
 
+func getPodIPv4InNetwork(ovnPod corev1.Pod, namespace string, networkAttachment string) (string, error) {
+	netStat, err := nad.GetNetworkStatusFromAnnotation(ovnPod.Annotations)
+	if err != nil {
+		err = fmt.Errorf("Error while getting the Network Status for pod %s: %v", ovnPod.Name, err)
+		return "", err
+	}
+	for _, v := range netStat {
+		if v.Name == namespace + "/" + networkAttachment {
+			for i := 0; i < len(v.IPs); i++ {
+				if !strings.Contains(v.IPs[i], ":") {
+					return v.IPs[i], nil
+				}
+			}
+		}
+	}
+	// If this is reached it means that no IP was found, construct error and return
+	err = fmt.Errorf("Error while getting IPv4 address from pod %s in network %s, IP is empty", ovnPod.Name, networkAttachment)
+	return "", err
+}
+
+func deleteDNSData(ctx context.Context, helper *helper.Helper, dnsName string, namespace string) error {
+	// Delete DNS records for deleted services/pods
+	dnsData := &infranetworkv1.DNSData{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      dnsName,
+			Namespace: namespace,
+		},
+	}
+	err := helper.GetClient().Delete(ctx, dnsData)
+	if err != nil && !k8s_errors.IsNotFound(err) {
+		return fmt.Errorf("Error while cleaning up DNS record %s: %w", dnsName, err)
+	}
+	return nil
+}
+
 func (r *OVNDBClusterReconciler) reconcileServices(
 	ctx context.Context,
 	instance *ovnv1.OVNDBCluster,
@@ -558,9 +592,10 @@ func (r *OVNDBClusterReconciler) reconcileServices(
 	)
 	if err == nil && len(svcList.Items) > int(*(instance.Spec.Replicas)) {
 		for i := len(svcList.Items) - 1; i >= int(*(instance.Spec.Replicas)); i-- {
+			fullServiceName := fmt.Sprintf("%s-%d", serviceName, i)
 			svcLabels := map[string]string{
 				common.AppSelector:                   serviceName,
-				"statefulset.kubernetes.io/pod-name": serviceName + fmt.Sprintf("-%d", i),
+				"statefulset.kubernetes.io/pod-name": fullServiceName,
 			}
 			err = service.DeleteServicesWithLabel(
 				ctx,
@@ -569,47 +604,31 @@ func (r *OVNDBClusterReconciler) reconcileServices(
 				svcLabels,
 			)
 			if err != nil {
+				err = fmt.Errorf("Error while deleting service with name %s: %w", serviceName, err)
 				return ctrl.Result{}, err
 			}
 			// Delete DNS records for deleted services/pods
-			dnsData := &infranetworkv1.DNSData{}
-			dnsName := fmt.Sprintf("dns-%s-%d", serviceName, i)
-			dnsNamespace := helper.GetBeforeObject().GetNamespace()
-			err = helper.GetClient().Get(ctx, types.NamespacedName{Name: dnsName, Namespace: dnsNamespace}, dnsData)
-			if err != nil && !k8s_errors.IsNotFound(err) {
-				Log.Info(fmt.Sprintf("Error while getting DNS object: %v", err))
-			}
-			err := helper.GetClient().Delete(ctx, dnsData)
+			namespace := helper.GetBeforeObject().GetNamespace()
+			err = deleteDNSData(ctx, helper, fullServiceName, namespace)
 			if err != nil {
-				Log.Info(fmt.Sprintf("Error while cleaning up the DNS record %s: %v", dnsName, err))
+				return ctrl.Result{}, err
 			}
-
 		}
 	}
 
 	podList, err = ovndbcluster.OVNDBPods(ctx, instance, helper, serviceLabels)
 	if err != nil {
+		err = fmt.Errorf("Error while retrieving pods on %s cluster: %w", instance.Spec.DBType, err)
 		return ctrl.Result{}, err
 	}
-	// Assuming that the replicas is greater than 0 AND networkAttachment != ""
-	// dbAddress will contain ovsdbserver-(nb|sb).openstack.svc
-	// if replicas == 0 or networkAttachment == "" dbAddress will be empty.
-	// Meaning that Status.DBAddress is not ready.
-	dbAddress := ""
 
-	// DNSData is needed to provide DNS outside the cluster (edpm nodes) which needs to have
-	// spec.NetworkAttachment != ""
+	var svc *corev1.Service
+
+	// When the cluster is attached to an external network, create DNS record for every
+	// cluster member so it can be resolved from outside cluster (edpm nodes)
 	if instance.Spec.NetworkAttachment != "" {
-		for i, ovnPod := range podList.Items {
-			if i >= int(*(instance.Spec.Replicas)) {
-				break
-			}
-			var dnsName string
-			var dnsIP string
-			var hostnames []string
-			dnsName = "dns-" + ovnPod.Name
-			// Get Hostname
-			svc, err := service.GetServiceWithName(
+		for _, ovnPod := range podList.Items[:*(instance.Spec.Replicas)] {
+			svc, err = service.GetServiceWithName(
 				ctx,
 				helper,
 				ovnPod.Name,
@@ -618,75 +637,31 @@ func (r *OVNDBClusterReconciler) reconcileServices(
 			if err != nil {
 				return ctrl.Result{}, err
 			}
-			hostname := svc.ObjectMeta.Annotations[infranetworkv1.AnnotationHostnameKey]
-			hostnames = append(hostnames, hostname)
 
-			// Get IP
-			net_stat, err := nad.GetNetworkStatusFromAnnotation(ovnPod.Annotations)
+			// Currently only IPv4 is supported
+			dnsIP, err := getPodIPv4InNetwork(ovnPod, instance.Namespace, instance.Spec.NetworkAttachment)
 			if err != nil {
-				Log.Info(fmt.Sprintf("Error while getting the NAD for pod %s: %v", ovnPod.Name, err))
 				return ctrl.Result{}, err
 			}
-			for _, v := range net_stat {
-				if v.Interface == instance.Spec.NetworkAttachment {
-					// Return only IPv4 at the moment
-					if !strings.Contains(v.IPs[0], ":") {
-						dnsIP = v.IPs[0]
-						break
-					}
-				}
-			}
-
-			if len(dnsIP) == 0 {
-				Log.Info(fmt.Sprintf("Error while getting pod %s %s IP, IP is empty", ovnPod.Name, instance.Spec.NetworkAttachment))
-				// TODO: uncommenting this 'continue' will break some tests.
-				// working on fixing it
-				//continue
-			}
 
-			// Create DNSRecord
-			var DNSHosts []infranetworkv1.DNSHost
-			// ovsdbserver-(nb|sb)-n entry
-			DNSHost := infranetworkv1.DNSHost{}
-			DNSHost.IP = dnsIP
-			DNSHost.Hostnames = hostnames
-			// ovsdbserver-(sb|nb) entry
-			headlessDnsHostname := ovndbcluster.ServiceNameSB + "." + instance.Namespace + ".svc"
-			if instance.Spec.DBType == v1beta1.NBDBType {
-				headlessDnsHostname = ovndbcluster.ServiceNameNB + "." + instance.Namespace + ".svc"
-			}
-			dbAddress = fmt.Sprintf("tcp:%s:%d", headlessDnsHostname, svc.Spec.Ports[0].Port)
-			DNSHostCname := infranetworkv1.DNSHost{}
-			DNSHostCname.IP = dnsIP
-			DNSHostCname.Hostnames = append(DNSHostCname.Hostnames, headlessDnsHostname)
-			DNSHosts = append(DNSHosts, DNSHost)
-			DNSHosts = append(DNSHosts, DNSHostCname)
-
-			// Create DNSData
-			DNSData := &infranetworkv1.DNSData{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      dnsName,
-					Namespace: ovnPod.Namespace,
-				},
-			}
-			_, err = controllerutil.CreateOrPatch(ctx, helper.GetClient(), DNSData, func() error {
-				DNSData.Spec.Hosts = DNSHosts
-				DNSData.Spec.DNSDataLabelSelectorValue = "dnsdata"
-				err := controllerutil.SetControllerReference(helper.GetBeforeObject(), DNSData, helper.GetScheme())
-				if err != nil {
-					return err
-				}
-				return nil
-			})
+			// Create DNSData CR
+			err = ovndbcluster.DnsData(
+				ctx,
+				helper,
+				svc,
+				dnsIP,
+				instance,
+				ovnPod,
+				serviceLabels,
+			)
 			if err != nil {
-				Log.Error(err, "Error Creating DNSData.")
 				return ctrl.Result{}, err
 			}
-
 		}
-
 	}
-	instance.Status.DBAddress = dbAddress
+	// dbAddress will contain ovsdbserver-(nb|sb).openstack.svc or empty
+	// IPv6 is not handled
+	instance.Status.DBAddress = ovndbcluster.GetDBAddress(instance, svc)
 
 	Log.Info("Reconciled OVN DB Cluster Service successfully")
 	return ctrl.Result{}, nil

pkg/ovndbcluster/dnsdata.go

@@ -0,0 +1,74 @@
+package ovndbcluster
+
+import (
+	"context"
+	"fmt"
+
+	infranetworkv1 "github.com/openstack-k8s-operators/infra-operator/apis/network/v1beta1"
+	"github.com/openstack-k8s-operators/lib-common/modules/common/helper"
+	"github.com/openstack-k8s-operators/ovn-operator/api/v1beta1"
+	ovnv1 "github.com/openstack-k8s-operators/ovn-operator/api/v1beta1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+func DnsData(
+	ctx context.Context,
+	helper *helper.Helper,
+	svc *corev1.Service,
+	ip string,
+	instance *ovnv1.OVNDBCluster,
+	ovnPod corev1.Pod,
+	serviceLabels map[string]string,
+) error {
+	// ovsdbserver-(sb|nb) entry
+	headlessDnsHostname := ServiceNameSB + "." + instance.Namespace + ".svc"
+	if instance.Spec.DBType == v1beta1.NBDBType {
+		headlessDnsHostname = ServiceNameNB + "." + instance.Namespace + ".svc"
+	}
+	dnsHostCname := infranetworkv1.DNSHost{
+		IP: ip,
+		Hostnames: []string{
+			headlessDnsHostname,
+		},
+	}
+
+	// Create DNSData object
+	dnsData := &infranetworkv1.DNSData{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      ovnPod.Name,
+			Namespace: ovnPod.Namespace,
+			Labels:    serviceLabels,
+		},
+	}
+	dnsHosts := []infranetworkv1.DNSHost{dnsHostCname}
+
+	_, err := controllerutil.CreateOrPatch(ctx, helper.GetClient(), dnsData, func() error {
+		dnsData.Spec.Hosts = dnsHosts
+		// TODO: use value from DNSMasq instance instead of hardcode
+		dnsData.Spec.DNSDataLabelSelectorValue = "dnsdata"
+		err := controllerutil.SetControllerReference(helper.GetBeforeObject(), dnsData, helper.GetScheme())
+		if err != nil {
+			return err
+		}
+		return nil
+	})
+	if err != nil {
+		return fmt.Errorf("Error creating DNSData %s: %w", dnsData.Name, err)
+	}
+	return nil
+}
+
+func GetDBAddress(instance *ovnv1.OVNDBCluster, svc *corev1.Service) string {
+	if svc == nil {
+		return ""
+	}
+	headlessDnsHostname := ""
+	if instance.Spec.DBType == v1beta1.NBDBType {
+		headlessDnsHostname = ServiceNameNB + "." + instance.Namespace + ".svc"
+	} else {
+		headlessDnsHostname = ServiceNameSB + "." + instance.Namespace + ".svc"
+	}
+	return fmt.Sprintf("tcp:%s:%d", headlessDnsHostname, svc.Spec.Ports[0].Port)
+}