From b03e089d470a4cdc806a9f660fc3461f4f88863e Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Tue, 3 Sep 2019 16:35:52 -0400 Subject: [PATCH] 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 | 5 +- pkg/dns/azure/dns.go | 2 + pkg/dns/azure/dns_test.go | 1 + pkg/dns/gcp/provider.go | 2 +- pkg/operator/controller/dns/controller.go | 13 ++++++ pkg/operator/controller/ingress/dns.go | 52 ++++++++++++++++++++- pkg/operator/controller/ingress/dns_test.go | 2 + 7 files changed, 73 insertions(+), 4 deletions(-) diff --git a/hack/run-local.sh b/hack/run-local.sh index 8d7a8492c..1f8743b03 100755 --- a/hack/run-local.sh +++ b/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 diff --git a/pkg/dns/azure/dns.go b/pkg/dns/azure/dns.go index db540e9e5..d70198327 100644 --- a/pkg/dns/azure/dns.go +++ b/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 { diff --git a/pkg/dns/azure/dns_test.go b/pkg/dns/azure/dns_test.go index ec5c27032..1d8bdd3d4 100644 --- a/pkg/dns/azure/dns_test.go +++ b/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{ diff --git a/pkg/dns/gcp/provider.go b/pkg/dns/gcp/provider.go index c8ee08905..b6fb49175 100644 --- a/pkg/dns/gcp/provider.go +++ b/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, } } diff --git a/pkg/operator/controller/dns/controller.go b/pkg/operator/controller/dns/controller.go index 52a3a2e4e..8ac83ab6b 100644 --- a/pkg/operator/controller/dns/controller.go +++ b/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? diff --git a/pkg/operator/controller/ingress/dns.go b/pkg/operator/controller/ingress/dns.go index 64ac95321..92ac6ae7c 100644 --- a/pkg/operator/controller/ingress/dns.go +++ b/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 +} diff --git a/pkg/operator/controller/ingress/dns_test.go b/pkg/operator/controller/ingress/dns_test.go index 43ab3a4e3..6ce0e5559 100644 --- a/pkg/operator/controller/ingress/dns_test.go +++ b/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, }, }, }