Fix pool update job

The pool update job was not launched because it had the same name.
As Kubernetes requires names be unique across all API versions of the same
resource, the jobs were only triggered once.

This patch adds the jobs a unix timestamp suffix to overcome that
problem.

It also sorts the pool resources to create a comparable hash between
jobs.

Author: omersch381

controllers/designate_controller.go

@@ -833,7 +833,6 @@ func (r *DesignateReconciler) reconcileNormal(ctx context.Context, instance *des
 	}
 
 	Log.Info("Bind configmap was created successfully")
-
 	if len(nsRecordsConfigMap.Data) > 0 && instance.Status.DesignateCentralReadyCount > 0 {
 		Log.Info("len(nsRecordsConfigMap.Data) > 0")
 		poolsYamlConfigMap := &corev1.ConfigMap{
@@ -844,11 +843,12 @@ func (r *DesignateReconciler) reconcileNormal(ctx context.Context, instance *des
 			},
 			Data: make(map[string]string),
 		}
-		Log.Info("before designate.GeneratePoolsYamlData")
-		poolsYaml, err := designate.GeneratePoolsYamlData(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
+
+		poolsYaml, poolsYamlHash, err := designate.GeneratePoolsYamlDataAndHash(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
+
 		Log.Info(fmt.Sprintf("pools.yaml content is\n%v", poolsYaml))
 		updatedPoolsYaml := make(map[string]string)
 		updatedPoolsYaml[designate.PoolsYamlContent] = poolsYaml
@@ -862,40 +862,32 @@ func (r *DesignateReconciler) reconcileNormal(ctx context.Context, instance *des
 			Log.Info("Unable to create config map for pools.yaml file")
 			return ctrl.Result{}, err
 		}
-		configMaps := []interface{}{
-			poolsYamlConfigMap.Data,
-		}
 
-		poolsYamlsEnvVars := make(map[string]env.Setter)
-		_, changed, err := r.createHashOfInputHashes(ctx, instance, designate.PoolsYamlHash, poolsYamlsEnvVars, configMaps)
-		if err != nil {
-			return ctrl.Result{}, err
+		if instance.Status.Hash == nil {
+			instance.Status.Hash = make(map[string]string)
 		}
-		if changed {
-			Log.Info("PoolsYamlHash has changed, creating a pool update job")
-
-			var poolUpdateHash string
-			var ok bool
-			if poolUpdateHash, ok = instance.Status.Hash[designatev1beta1.PoolUpdateHash]; !ok {
-				instance.Status.Hash[designatev1beta1.PoolUpdateHash] = ""
-				poolUpdateHash = ""
+
+		oldHash := instance.Status.Hash[designatev1beta1.PoolUpdateHash]
+		if oldHash != poolsYamlHash {
+			Log.Info(fmt.Sprintf("Old poolsYamlHash %s is different than new poolsYamlHash %s.\nLaunching pool update job", oldHash, poolsYamlHash))
+
+			instance.Status.Hash[designatev1beta1.PoolUpdateHash] = poolsYamlHash
+			if err := r.Client.Status().Update(ctx, instance); err != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to update status hash: %w", err)
 			}
 			jobDef := designate.PoolUpdateJob(instance, serviceLabels, serviceAnnotations)
 
 			Log.Info("Initializing pool update job")
+
 			poolUpdatejob := job.NewJob(
 				jobDef,
 				designatev1beta1.PoolUpdateHash,
 				instance.Spec.PreserveJobs,
 				time.Duration(15)*time.Second,
-				poolUpdateHash,
+				oldHash,
 			)
-			_, err = poolUpdatejob.DoJob(ctx, helper)
-			if err != nil {
-				return ctrl.Result{}, err
-			}
-			instance.Status.Hash[designatev1beta1.PoolUpdateHash] = poolUpdatejob.GetHash()
-			err = r.Client.Status().Update(ctx, instance)
+
+			_, err := poolUpdatejob.DoJob(ctx, helper)
 			if err != nil {
 				return ctrl.Result{}, err
 			}

pkg/designate/generate_bind9_pools_yaml.go

@@ -15,9 +15,12 @@ package designate
 
 import (
 	"bytes"
+	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"path"
+	"sort"
 	"text/template"
 
 	"gopkg.in/yaml.v2"
@@ -70,49 +73,73 @@ type CatalogZone struct {
 	Refresh int    `yaml:"refresh"`
 }
 
-func GeneratePoolsYamlData(BindMap, MdnsMap, NsRecordsMap map[string]string) (string, error) {
-	// Create ns_records
+// We sort all pool resources to get the correct hash every time
+func GeneratePoolsYamlDataAndHash(BindMap, MdnsMap, NsRecordsMap map[string]string) (string, string, error) {
 	nsRecords := []NSRecord{}
 	for _, data := range NsRecordsMap {
 		err := yaml.Unmarshal([]byte(data), &nsRecords)
 		if err != nil {
-			return "", fmt.Errorf("error unmarshalling yaml: %w", err)
+			return "", "", fmt.Errorf("error unmarshalling yaml: %w", err)
 		}
 	}
+	sort.Slice(nsRecords, func(i, j int) bool {
+		if nsRecords[i].Hostname != nsRecords[j].Hostname {
+			return nsRecords[i].Hostname < nsRecords[j].Hostname
+		}
+		return nsRecords[i].Priority < nsRecords[j].Priority
+	})
+
+	bindIPs := make([]string, 0, len(BindMap))
+	for _, ip := range BindMap {
+		bindIPs = append(bindIPs, ip)
+	}
+	sort.Strings(bindIPs)
 
-	// Create targets and nameservers
-	nameservers := []Nameserver{}
-	targets := []Target{}
-	rndcKeyNum := 0
+	masterHosts := make([]string, 0, len(MdnsMap))
+	for _, host := range MdnsMap {
+		masterHosts = append(masterHosts, host)
+	}
+	sort.Strings(masterHosts)
 
-	for _, bindIP := range BindMap {
-		nameservers = append(nameservers, Nameserver{
+	nameservers := make([]Nameserver, len(bindIPs))
+	for i, bindIP := range bindIPs {
+		nameservers[i] = Nameserver{
 			Host: bindIP,
 			Port: 53,
-		})
+		}
+	}
 
-		masters := []Master{}
-		for _, masterHost := range MdnsMap {
-			masters = append(masters, Master{
+	targets := make([]Target, len(bindIPs))
+	for i, bindIP := range bindIPs {
+		masters := make([]Master, len(masterHosts))
+		for j, masterHost := range masterHosts {
+			masters[j] = Master{
 				Host: masterHost,
 				Port: 5354,
-			})
+			}
 		}
 
-		target := Target{
+		targets[i] = Target{
 			Type:        "bind9",
-			Description: fmt.Sprintf("BIND9 Server %d (%s)", rndcKeyNum, bindIP),
+			Description: fmt.Sprintf("BIND9 Server %d (%s)", i, bindIP),
 			Masters:     masters,
 			Options: Options{
 				Host:        bindIP,
 				Port:        53,
 				RNDCHost:    bindIP,
 				RNDCPort:    953,
-				RNDCKeyFile: fmt.Sprintf("%s/%s-%d", RndcConfDir, DesignateRndcKey, rndcKeyNum),
+				RNDCKeyFile: fmt.Sprintf("%s/%s-%d", RndcConfDir, DesignateRndcKey, i),
 			},
 		}
-		targets = append(targets, target)
-		rndcKeyNum++
+	}
+
+	sort.Slice(targets, func(i, j int) bool {
+		return targets[i].Options.Host < targets[j].Options.Host
+	})
+
+	for i := range targets {
+		targets[i].Description = fmt.Sprintf("BIND9 Server %d (%s)", i, targets[i].Options.Host)
+		targets[i].Options.RNDCKeyFile = fmt.Sprintf("%s/%s-%d", RndcConfDir, DesignateRndcKey, i)
 	}
 
 	// Catalog zone is an optional section
@@ -132,25 +159,32 @@ func GeneratePoolsYamlData(BindMap, MdnsMap, NsRecordsMap map[string]string) (st
 		CatalogZone: nil, // set to catalogZone if this section should be presented
 	}
 
+	poolBytes, err := yaml.Marshal(pool)
+	if err != nil {
+		return "", "", fmt.Errorf("error marshalling pool for hash: %w", err)
+	}
+	hasher := sha256.New()
+	hasher.Write(poolBytes)
+	poolHash := hex.EncodeToString(hasher.Sum(nil))
+
 	opTemplates, err := util.GetTemplatesPath()
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	poolsYamlPath := path.Join(opTemplates, PoolsYamlPath)
 	poolsYaml, err := os.ReadFile(poolsYamlPath)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	tmpl, err := template.New("pool").Parse(string(poolsYaml))
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
-
 	var buf bytes.Buffer
 	err = tmpl.Execute(&buf, pool)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 
-	return buf.String(), nil
+	return buf.String(), poolHash, nil
 }

pkg/designate/pool_update.go

@@ -7,6 +7,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"time"
 )
 
 const (
@@ -85,9 +86,10 @@ func PoolUpdateJob(
 		},
 	)
 
+	jobName := fmt.Sprintf("%s-pool-update-%d", ServiceName, time.Now().Unix())
 	job := &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      ServiceName + "-pool-update",
+			Name:      jobName,
 			Namespace: instance.Namespace,
 			Labels:    labels,
 		},
@@ -101,7 +103,7 @@ func PoolUpdateJob(
 					ServiceAccountName: instance.RbacResourceName(),
 					Containers: []corev1.Container{
 						{
-							Name:  ServiceName + "-pool-update",
+							Name:  jobName,
 							Image: instance.Spec.DesignateCentral.ContainerImage,
 							Env: []corev1.EnvVar{
 								{

tests/functional/designate_controller_test.go

@@ -644,7 +644,6 @@ var _ = Describe("Designate controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 
 			validate := validator.New()
-
 			for _, pool := range pools {
 				Expect(pool.Name).ToNot(BeEmpty(), "Pool name should not be an empty string")
 				Expect(pool.Description).ToNot(BeEmpty(), "Pool description should not be an empty string")
@@ -726,5 +725,24 @@ var _ = Describe("Designate controller", func() {
 				}
 			}
 		})
+
+		It("should create the same pools.yaml hash when provided the same designate configmaps", func() {
+			bindConfigMap := th.GetConfigMap(types.NamespacedName{
+				Name:      designate.BindPredIPConfigMap,
+				Namespace: namespace})
+			mdnsConfigMap := th.GetConfigMap(types.NamespacedName{
+				Name:      designate.MdnsPredIPConfigMap,
+				Namespace: namespace})
+			nsRecordsConfigMap := th.GetConfigMap(types.NamespacedName{
+				Name:      designate.NsRecordsConfigMap,
+				Namespace: namespace})
+			_, poolsYamlHash, err := designate.GeneratePoolsYamlDataAndHash(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
+			Expect(err).ToNot(HaveOccurred())
+			for i := 0; i < 10; i++ {
+				_, newPoolsYamlHash, err := designate.GeneratePoolsYamlDataAndHash(bindConfigMap.Data, mdnsConfigMap.Data, nsRecordsConfigMap.Data)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(poolsYamlHash).Should(Equal(newPoolsYamlHash))
+			}
+		})
 	})
 })