Bug 1747840: dns: add explicit TTL to all wildcard records

Before this commit, wildcard DNS resource record TTLs were inconsistent:

* AWS alias record TTLs are not user-configurable.
* Azure records TTLs were zero (which is essentially undefined and so causes
unpredictable client behavior)
* GCP record TTLs were 120 seconds.

After this commit, all DNS records default to 30 seconds for any provider which
supports setting a TTL for the record type (Azure and GCP at this time). This
brings some consistency, and most importantly, eliminates the possibility of
zero TTLs.

hack/run-local.sh

@@ -8,4 +8,7 @@ oc scale --replicas 0 -n openshift-ingress-operator deployments ingress-operator
 IMAGE=$(oc get -n openshift-ingress-operator deployments/ingress-operator -o json | jq -r '.spec.template.spec.containers[0].env[] | select(.name=="IMAGE").value')
 RELEASE_VERSION=$(oc get clusterversion/version -o json | jq -r '.status.desired.version')
 
-IMAGE="$IMAGE" RELEASE_VERSION="$RELEASE_VERSION" ./ingress-operator
+echo "Image: ${IMAGE}"
+echo "Release version: ${RELEASE_VERSION}"
+
+IMAGE="${IMAGE}" RELEASE_VERSION="${RELEASE_VERSION}" ./ingress-operator

pkg/dns/azure/dns.go

@@ -84,6 +84,7 @@ func (m *provider) Ensure(record *iov1.DNSRecord, zone configv1.DNSZone) error {
 		client.ARecord{
 			Address: record.Spec.Targets[0],
 			Name:    ARecordName,
+			TTL:     record.Spec.RecordTTL,
 		})
 
 	if err == nil {
@@ -111,6 +112,7 @@ func (m *provider) Delete(record *iov1.DNSRecord, zone configv1.DNSZone) error {
 		client.ARecord{
 			Address: record.Spec.Targets[0],
 			Name:    ARecordName,
+			TTL:     record.Spec.RecordTTL,
 		})
 
 	if err == nil {

pkg/dns/azure/dns_test.go

@@ -37,6 +37,7 @@ func TestEnsureDNS(t *testing.T) {
 			DNSName:    "subdomain.dnszone.io.",
 			RecordType: iov1.ARecordType,
 			Targets:    []string{"55.11.22.33"},
+			RecordTTL:  120,
 		},
 	}
 	dnsZone := configv1.DNSZone{

pkg/dns/gcp/provider.go

@@ -72,6 +72,6 @@ func resourceRecordSet(record *iov1.DNSRecord) *gdnsv1.ResourceRecordSet {
 		Name:    record.Spec.DNSName,
 		Rrdatas: record.Spec.Targets,
 		Type:    record.Spec.RecordType,
-		Ttl:     300,
+		Ttl:     record.Spec.RecordTTL,
 	}
 }

pkg/operator/controller/dns/controller.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"time"
 
+	"k8s.io/client-go/tools/record"
+
 	iov1 "github.com/openshift/cluster-ingress-operator/pkg/api/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/dns"
 	logf "github.com/openshift/cluster-ingress-operator/pkg/log"
@@ -39,6 +41,7 @@ func New(mgr manager.Manager, dnsProvider dns.Provider) (runtimecontroller.Contr
 		client:      mgr.GetClient(),
 		cache:       mgr.GetCache(),
 		dnsProvider: dnsProvider,
+		recorder:    mgr.GetEventRecorderFor(controllerName),
 	}
 	c, err := runtimecontroller.New(controllerName, mgr, runtimecontroller.Options{Reconciler: reconciler})
 	if err != nil {
@@ -54,6 +57,7 @@ type reconciler struct {
 	client      client.Client
 	cache       cache.Cache
 	dnsProvider dns.Provider
+	recorder    record.EventRecorder
 }
 
 func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) {
@@ -89,6 +93,15 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 		log.V(2).Info("warning: dnsrecord is missing owner label", "dnsrecord", record, "ingresscontroller", ingressName)
 		return reconcile.Result{RequeueAfter: 1 * time.Minute}, nil
 	}
+
+	// Existing 4.1 records will have a zero TTL. Instead of making all the client implementations guard against
+	// zero TTLs, simply ignore the record until the TTL is updated by the ingresscontroller controller. Report
+	// this through events so we can detect problems with our migration.
+	if record.Spec.RecordTTL <= 0 {
+		r.recorder.Eventf(record, "Warning", "ZeroTTL", "Record is missing TTL and will be temporarily ignored; the TTL will be automatically updated and the record will be retried.")
+		return reconcile.Result{}, nil
+	}
+
 	if err := r.cache.Get(context.TODO(), types.NamespacedName{Namespace: record.Namespace, Name: ingressName}, &operatorv1.IngressController{}); err != nil {
 		if errors.IsNotFound(err) {
 			// TODO: what should we do here? something upstream could detect and delete the orphan? add new conditions?

pkg/operator/controller/ingress/dns.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	iov1 "github.com/openshift/cluster-ingress-operator/pkg/api/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
@@ -16,6 +19,14 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+// defaultRecordTTL is the TTL (in seconds) assigned to all new DNS records.
+//
+// Note that TTL isn't necessarily honored by clouds providers (for example,
+// on AWS TTL is not configurable for alias records[1]).
+//
+// [1] https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/resource-record-sets-choosing-alias-non-alias.html
+const defaultRecordTTL int64 = 30
+
 // ensureDNS will create DNS records for the given LB service. If service is
 // nil, nothing is done.
 func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, service *corev1.Service) (*iov1.DNSRecord, error) {
@@ -28,13 +39,23 @@ func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, s
 	if err != nil {
 		return nil, err
 	}
-	if desired != nil && current == nil {
+
+	switch {
+	case desired != nil && current == nil:
 		if err := r.client.Create(context.TODO(), desired); err != nil {
 			return nil, fmt.Errorf("failed to create dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err)
 		}
 		log.Info("created dnsrecord", "dnsrecord", desired)
-		return desired, nil
+		return r.currentWildcardDNSRecord(ic)
+	case desired != nil && current != nil:
+		if updated, err := r.updateDNSRecord(current, desired); err != nil {
+			return nil, fmt.Errorf("failed to update dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err)
+		} else if updated {
+			log.Info("updated dnsrecord", "dnsrecord", desired)
+			return r.currentWildcardDNSRecord(ic)
+		}
 	}
+
 	return current, nil
 }
 
@@ -110,6 +131,7 @@ func desiredWildcardRecord(ic *operatorv1.IngressController, service *corev1.Ser
 			DNSName:    domain,
 			Targets:    []string{target},
 			RecordType: recordType,
+			RecordTTL:  defaultRecordTTL,
 		},
 	}
 }
@@ -139,3 +161,29 @@ func (r *reconciler) deleteWildcardDNSRecord(ic *operatorv1.IngressController) e
 	}
 	return nil
 }
+
+// updateDNSRecord updates a DNSRecord. Returns a boolean indicating whether
+// the record was updated, and an error value.
+func (r *reconciler) updateDNSRecord(current, desired *iov1.DNSRecord) (bool, error) {
+	changed, updated := dnsRecordChanged(current, desired)
+	if !changed {
+		return false, nil
+	}
+
+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
+// dnsRecordChanged checks if the current DNSRecord spec matches the expected spec and
+// if not returns an updated one.
+func dnsRecordChanged(current, expected *iov1.DNSRecord) (bool, *iov1.DNSRecord) {
+	if cmp.Equal(current.Spec, expected.Spec, cmpopts.EquateEmpty()) {
+		return false, nil
+	}
+
+	updated := current.DeepCopy()
+	updated.Spec = expected.Spec
+	return true, updated
+}

pkg/operator/controller/ingress/dns_test.go

@@ -57,6 +57,7 @@ func TestDesiredWildcardDNSRecord(t *testing.T) {
 				DNSName:    "*.apps.openshift.example.com.",
 				RecordType: iov1.CNAMERecordType,
 				Targets:    []string{"lb.cloud.example.com"},
+				RecordTTL:  defaultRecordTTL,
 			},
 		},
 		{
@@ -70,6 +71,7 @@ func TestDesiredWildcardDNSRecord(t *testing.T) {
 				DNSName:    "*.apps.openshift.example.com.",
 				RecordType: iov1.ARecordType,
 				Targets:    []string{"192.0.2.1"},
+				RecordTTL:  defaultRecordTTL,
 			},
 		},
 	}