diff --git a/pkg/operator/controller/dns/controller.go b/pkg/operator/controller/dns/controller.go index 16201fe4e3..c0b0f1f0a6 100644 --- a/pkg/operator/controller/dns/controller.go +++ b/pkg/operator/controller/dns/controller.go @@ -62,6 +62,8 @@ const ( kubeCloudConfigName = "kube-cloud-config" // cloudCABundleKey is the key in the kube cloud config ConfigMap where the custom CA bundle is located cloudCABundleKey = "ca-bundle.pem" + // dnsRecordIndexFieldName is the key for the DNSRecord index, used to identify conflicting domain names. + dnsRecordIndexFieldName = "Spec.DNSName" ) var log = logf.Logger.WithName(controllerName) @@ -81,6 +83,17 @@ func New(mgr manager.Manager, config Config) (runtimecontroller.Controller, erro if err := c.Watch(source.Kind[client.Object](operatorCache, &iov1.DNSRecord{}, &handler.EnqueueRequestForObject{}, predicate.GenerationChangedPredicate{})); err != nil { return nil, err } + // When a DNS record is deleted, there may be a conflicting record that should be published. Add a second watch + // exclusively for deletes so that in addition to the normal on-delete cleanup, queue a reconcile for the next + // record with the same domain name (if one exists) so that it can be published. + if err := c.Watch(source.Kind[client.Object](operatorCache, &iov1.DNSRecord{}, handler.EnqueueRequestsFromMapFunc(reconciler.mapOnRecordDelete), predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { return false }, + DeleteFunc: func(e event.DeleteEvent) bool { return true }, + UpdateFunc: func(e event.UpdateEvent) bool { return false }, + GenericFunc: func(e event.GenericEvent) bool { return false }, + })); err != nil { + return nil, err + } if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.DNS{}, handler.EnqueueRequestsFromMapFunc(reconciler.ToDNSRecords))); err != nil { return nil, err } @@ -102,6 +115,12 @@ func New(mgr manager.Manager, config Config) (runtimecontroller.Controller, erro })); err != nil { return nil, err } + if err := operatorCache.IndexField(context.Background(), &iov1.DNSRecord{}, dnsRecordIndexFieldName, func(o client.Object) []string { + dnsRecord := o.(*iov1.DNSRecord) + return []string{dnsRecord.Spec.DNSName} + }); err != nil { + return nil, err + } return c, nil } @@ -337,17 +356,33 @@ func (r *reconciler) publishRecordToZones(zones []configv1.DNSZone, record *iov1 var err error var condition iov1.DNSZoneCondition + var isDomainPublished bool if dnsPolicy == iov1.UnmanagedDNS { - log.Info("DNS record not published", "record", record.Spec) + log.Info("DNS record not published: DNS management policy is unmanaged", "record", record.Spec) condition = iov1.DNSZoneCondition{ - Message: "DNS record is currently not being managed by the operator", - Reason: "UnmanagedDNS", - Status: string(operatorv1.ConditionUnknown), - Type: iov1.DNSRecordPublishedConditionType, - LastTransitionTime: metav1.Now(), + Message: "DNS record is currently not being managed by the operator", + Reason: "UnmanagedDNS", + Status: string(operatorv1.ConditionUnknown), + Type: iov1.DNSRecordPublishedConditionType, } } else if isRecordPublished { condition, err = r.replacePublishedRecord(zones[i], record) + } else if isDomainPublished, err = domainIsAlreadyPublishedToZone(context.Background(), r.cache, record, &zones[i]); err != nil { + log.Error(err, "failed to validate DNS record", "record", record.Spec) + condition = iov1.DNSZoneCondition{ + Message: "Pre-publish validation failed", + Reason: "InternalError", + Status: string(operatorv1.ConditionFalse), + Type: iov1.DNSRecordPublishedConditionType, + } + } else if isDomainPublished { + log.Info("DNS record not published: domain name already used by another DNS record", "record", record.Spec) + condition = iov1.DNSZoneCondition{ + Message: "Domain name is already in use", + Reason: "DomainAlreadyInUse", + Status: string(operatorv1.ConditionFalse), + Type: iov1.DNSRecordPublishedConditionType, + } } else { condition, err = r.publishRecord(zones[i], record) } @@ -386,6 +421,34 @@ func recordIsAlreadyPublishedToZone(record *iov1.DNSRecord, zoneToPublish *confi return false } +// domainIsAlreadyPublishedToZone returns true if the domain name in the provided DNSRecord is already published by +// another existing dnsRecord. +func domainIsAlreadyPublishedToZone(ctx context.Context, cache cache.Cache, record *iov1.DNSRecord, zone *configv1.DNSZone) (bool, error) { + records := iov1.DNSRecordList{} + if err := cache.List(ctx, &records, client.MatchingFields{dnsRecordIndexFieldName: record.Spec.DNSName}); err != nil { + return false, err + } + + if len(records.Items) == 0 { + return false, nil + } + + for _, existingRecord := range records.Items { + // we only care if the domain name is published by a different record, so ignore the matching record if it + // already exists. + if record.UID == existingRecord.UID { + continue + } + if record.Spec.DNSName != existingRecord.Spec.DNSName { + continue + } + if recordIsAlreadyPublishedToZone(&existingRecord, zone) { + return true, nil + } + } + return false, nil +} + func (r *reconciler) delete(record *iov1.DNSRecord) error { var errs []error for i := range record.Status.Zones { @@ -576,6 +639,44 @@ func (r *reconciler) ToDNSRecords(ctx context.Context, o client.Object) []reconc return requests } +// mapOnRecordDelete finds another DNSRecord (if any) that uses the same domain name as the deleted record, and queues a +// reconcile for that record, so that it can be published. If multiple matching records are found, a request for the +// oldest record is returned. +func (r *reconciler) mapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request { + deletedRecord, ok := o.(*iov1.DNSRecord) + if !ok { + log.Error(nil, "Got unexpected type of object", "expected", "DNSRecord", "actual", fmt.Sprintf("%T", o)) + return []reconcile.Request{} + } + otherRecords := iov1.DNSRecordList{} + if err := r.cache.List(ctx, &otherRecords, client.MatchingFields{dnsRecordIndexFieldName: deletedRecord.Spec.DNSName}); err != nil { + log.Error(err, "failed to list DNS records") + return []reconcile.Request{} + } + if len(otherRecords.Items) == 0 { + // Nothing to do. + return []reconcile.Request{} + } + oldestExistingRecord := iov1.DNSRecord{} + for _, existingRecord := range otherRecords.Items { + // Exclude records that are marked for deletion. + if !existingRecord.DeletionTimestamp.IsZero() { + continue + } + if oldestExistingRecord.CreationTimestamp.IsZero() || existingRecord.CreationTimestamp.Before(&oldestExistingRecord.CreationTimestamp) { + oldestExistingRecord = existingRecord + } + } + return []reconcile.Request{ + { + NamespacedName: types.NamespacedName{ + Name: oldestExistingRecord.Name, + Namespace: oldestExistingRecord.Namespace, + }, + }, + } +} + // createDNSProvider creates a DNS manager compatible with the given cluster // configuration. func (r *reconciler) createDNSProvider(dnsConfig *configv1.DNS, platformStatus *configv1.PlatformStatus, infraStatus *configv1.InfrastructureStatus, creds *corev1.Secret, AzureWorkloadIdentityEnabled bool) (dns.Provider, error) { diff --git a/pkg/operator/controller/dns/controller_test.go b/pkg/operator/controller/dns/controller_test.go index 4cf2f2b71b..eb0f8e3e35 100644 --- a/pkg/operator/controller/dns/controller_test.go +++ b/pkg/operator/controller/dns/controller_test.go @@ -9,9 +9,12 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" iov1 "github.com/openshift/api/operatoringress/v1" "github.com/openshift/cluster-ingress-operator/pkg/dns" + testutil "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/test/util" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/cache/informertest" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -117,6 +120,7 @@ func Test_publishRecordToZones(t *testing.T) { r := &reconciler{ // TODO To write a fake provider that can return errors and add more test cases. dnsProvider: &dns.FakeProvider{}, + cache: buildFakeCache(t), } _, actual := r.publishRecordToZones(test.zones, record) @@ -128,9 +132,9 @@ func Test_publishRecordToZones(t *testing.T) { } } -// TestPublishRecordToZonesMergesStatus verifies that publishRecordToZones +// Test_publishRecordToZonesMergesStatus verifies that publishRecordToZones // correctly merges status updates. -func TestPublishRecordToZonesMergesStatus(t *testing.T) { +func Test_publishRecordToZonesMergesStatus(t *testing.T) { testCases := []struct { description string oldZoneStatuses []iov1.DNSZoneStatus @@ -215,7 +219,10 @@ func TestPublishRecordToZonesMergesStatus(t *testing.T) { }, Status: iov1.DNSRecordStatus{Zones: tc.oldZoneStatuses}, } - r := &reconciler{dnsProvider: &dns.FakeProvider{}} + r := &reconciler{ + dnsProvider: &dns.FakeProvider{}, + cache: buildFakeCache(t), + } zone := []configv1.DNSZone{{ID: "zone2"}} oldStatuses := record.Status.DeepCopy().Zones _, newStatuses := r.publishRecordToZones(zone, record) @@ -840,3 +847,28 @@ func Test_customCABundle(t *testing.T) { }) } } + +// buildFakeCache returns a fake cache, with the necessary schema and index function(s) to mimic the parts of the cache +// that the dns controller interacts with. +func buildFakeCache(t *testing.T) *testutil.FakeCache { + t.Helper() + scheme := runtime.NewScheme() + iov1.AddToScheme(scheme) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithIndex(&iov1.DNSRecord{}, dnsRecordIndexFieldName, func(o client.Object) []string { + dnsRecord := o.(*iov1.DNSRecord) + return []string{dnsRecord.Spec.DNSName} + }). + Build() + cl := &testutil.FakeClientRecorder{ + Client: fakeClient, + T: t, + Added: []client.Object{}, + Updated: []client.Object{}, + Deleted: []client.Object{}, + } + informer := informertest.FakeInformers{Scheme: scheme} + cache := testutil.FakeCache{Informers: &informer, Reader: cl} + return &cache +}