INWX: Let the API (not DNSControl) enforce the RFC 7505 prohibition of mixed regular/null MX records (#3805)

Co-authored-by: Tom Limoncelli <6293917+tlimoncelli@users.noreply.github.com>

documentation/provider/inwx.md

@@ -109,3 +109,7 @@ D("example.com", REG_INWX, DnsProvider(DSP_CF),
 );
 ```
 {% endcode %}
+
+## Notes
+
+INWX enforces the [RFC 7505](https://www.rfc-editor.org/rfc/rfc7505.html#section-3) MUST NOT guidance regarding publishing both null MX and regular MX records. If a push would result in mixed null MX and regular MX records in the zone, the API responds with `FAILURE! (2308) Data management policy violation` and the record will not be persisted.

providers/inwx/inwxProvider.go

@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -231,109 +230,11 @@ func (api *inwxAPI) deleteRecord(RecordID int) error {
 	return api.client.Nameservers.DeleteRecord(RecordID)
 }
 
-// appendDeleteCorrection is a helper function to append delete corrections to the list of corrections
-func (api *inwxAPI) appendDeleteCorrection(corrections []*models.Correction, rec *models.RecordConfig, removals map[string]struct{}) ([]*models.Correction, map[string]struct{}) {
-	// prevent duplicate delete instructions
-	if _, found := removals[rec.ToComparableNoTTL()]; found {
-		return corrections, removals
-	}
-	corrections = append(corrections, &models.Correction{
-		Msg: color.RedString("- DELETE %s %s %s ttl=%d", rec.GetLabelFQDN(), rec.Type, rec.ToComparableNoTTL(), rec.TTL),
-		F: func() error {
-			return api.deleteRecord(rec.Original.(goinwx.NameserverRecord).ID)
-		},
-	})
-	removals[rec.ToComparableNoTTL()] = struct{}{}
-	return corrections, removals
-}
-
 // isNullMX checks if a record is a null MX record.
 func isNullMX(rec *models.RecordConfig) bool {
 	return rec.Type == "MX" && rec.MxPreference == 0 && rec.GetTargetField() == "."
 }
 
-// MXCorrections generates required delete corrections when a MX change can not be applied in an updateRecord call.
-func (api *inwxAPI) MXCorrections(dc *models.DomainConfig, foundRecords models.Records, corrections []*models.Correction) ([]*models.Correction, models.Records, error) {
-
-	// If a null MX is present in the zone, we have to take special care of any
-	// planned MX changes: No non-null MX records can be added until the null
-	// MX is deleted. If a null MX is planned to be added and the diff is
-	// trying to replace an existing regular MX, we need to delete the existing
-	// MX record because an update would be rejected with "2308 Data management policy violation"
-
-	removals := make(map[string]struct{})
-	tempRecords := []*models.RecordConfig{}
-
-	// Detect Null MX in foundRecords
-	nullMXInFound := slices.ContainsFunc(foundRecords.GetByType("MX"), isNullMX)
-
-	// Detect Null MX and regular MX in desired records
-	nullMXInDesired := false
-	regularMXInDesired := false
-	for _, rec := range dc.Records.GetByType("MX") {
-		if isNullMX(rec) {
-			nullMXInDesired = true
-		} else {
-			regularMXInDesired = true
-		}
-	}
-
-	// invalid state. Null MX and regular MX are both present in the configuration
-	if nullMXInDesired && regularMXInDesired {
-		return nil, nil, fmt.Errorf("desired configuration contains both Null MX and regular MX records")
-	}
-
-	if nullMXInFound && !nullMXInDesired {
-		// Null MX exists in foundRecords, but desired configuration contains only regular MX records
-		// Safe to delete the Null MX record
-		for _, rec := range foundRecords {
-			if isNullMX(rec) {
-				corrections, removals = api.appendDeleteCorrection(corrections, rec, removals)
-			}
-		}
-	} else if !nullMXInFound && nullMXInDesired {
-		// Null MX is being added, ensure all existing MX records are deleted
-		for _, rec := range foundRecords {
-			if rec.Type == "MX" {
-				corrections, removals = api.appendDeleteCorrection(corrections, rec, removals)
-			}
-		}
-	}
-
-	mxRecords := foundRecords.GetByType("MX")
-	mxonlyDc, err := dc.Copy()
-	if err != nil {
-		return nil, nil, err
-	}
-	mxonlyDc.Records = mxonlyDc.Records.GetByType("MX")
-
-	mxchanges, _, err := diff2.ByRecord(mxRecords, mxonlyDc, nil)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	for _, change := range mxchanges {
-		if change.Type == diff2.CHANGE {
-			// INWX will not apply a MX preference update of >=1 to 0. The updateRecord
-			// endpoint will not report an error, so the zone and config will be out of
-			// sync unless we handle this as a delete then create
-			if change.New[0].MxPreference == 0 && change.Old[0].MxPreference != 0 {
-				corrections, removals = api.appendDeleteCorrection(corrections, change.Old[0], removals)
-			}
-		}
-	}
-
-	// We need to remove the RRs already in corrections
-	for _, rec := range foundRecords {
-		if _, found := removals[rec.ToComparableNoTTL()]; !found {
-			tempRecords = append(tempRecords, rec)
-		}
-	}
-
-	cleanedRecords := models.Records(tempRecords)
-	return corrections, cleanedRecords, nil
-}
-
 // AutoDnssecToggle enables and disables AutoDNSSEC for INWX domains.
 func (api *inwxAPI) AutoDnssecToggle(dc *models.DomainConfig, corrections []*models.Correction) ([]*models.Correction, error) {
 
@@ -369,47 +270,66 @@ func (api *inwxAPI) AutoDnssecToggle(dc *models.DomainConfig, corrections []*mod
 // GetZoneRecordsCorrections returns a list of corrections that will turn existing records into dc.Records.
 func (api *inwxAPI) GetZoneRecordsCorrections(dc *models.DomainConfig, foundRecords models.Records) ([]*models.Correction, int, error) {
 
+	// INWX support for Null MX requires special handling.  MX preference changes
+	// of >0 to 0 are silently dropped, so if the change includes a null MX record
+	// we have to delete then create.  Corrections are compiled separately for
+	// deletes, changes and creates and then assembled in that order.
 	corrections := []*models.Correction{}
+	deletes := []*models.Correction{}
+	creates := []*models.Correction{}
+	deferred := []*models.Correction{}
 
-	corrections, records, err := api.MXCorrections(dc, foundRecords, corrections)
+	corrections, err := api.AutoDnssecToggle(dc, corrections)
 	if err != nil {
 		return nil, 0, err
 	}
 
-	corrections, err = api.AutoDnssecToggle(dc, corrections)
+	changes, actualChangeCount, err := diff2.ByRecord(foundRecords, dc, nil)
-	if err != nil {
-		return nil, 0, err
-	}
-
-	changes, actualChangeCount, err := diff2.ByRecord(records, dc, nil)
 	if err != nil {
 		return nil, 0, err
 	}
 	for _, change := range changes {
 		changeMsgs := change.MsgsJoined
-		dcName := dc.Name
 		switch change.Type {
 		case diff2.REPORT:
 			corrections = append(corrections, &models.Correction{Msg: changeMsgs})
 		case diff2.CHANGE:
-			recID := change.Old[0].Original.(goinwx.NameserverRecord).ID
+			oldRec := change.Old[0]
-			corrections = append(corrections, &models.Correction{
+			newRec := change.New[0]
-				Msg: changeMsgs,
+			if isNullMX(newRec) || isNullMX(oldRec) {
-				F: func() error {
+				// changing to or from a Null MX has to be delete then create
-					return api.updateRecord(recID, change.New[0])
+				deletes = append(deletes, &models.Correction{
-				},
+					Msg: color.RedString("- DELETE %s %s %s ttl=%d", oldRec.GetLabelFQDN(), oldRec.Type, oldRec.ToComparableNoTTL(), oldRec.TTL),
-			})
+					F: func() error {
+						return api.deleteRecord(oldRec.Original.(goinwx.NameserverRecord).ID)
+					},
+				})
+				deferred = append(deferred, &models.Correction{
+					Msg: color.GreenString("+ CREATE %s %s %s ttl=%d", newRec.GetLabelFQDN(), newRec.Type, newRec.ToComparableNoTTL(), newRec.TTL),
+					F: func() error {
+						return api.createRecord(dc.Name, newRec)
+					},
+				})
+			} else {
+				recID := oldRec.Original.(goinwx.NameserverRecord).ID
+				corrections = append(corrections, &models.Correction{
+					Msg: changeMsgs,
+					F: func() error {
+						return api.updateRecord(recID, newRec)
+					},
+				})
+
+			}
 		case diff2.CREATE:
-			changeNew := change.New[0]
+			creates = append(creates, &models.Correction{
-			corrections = append(corrections, &models.Correction{
 				Msg: changeMsgs,
 				F: func() error {
-					return api.createRecord(dcName, changeNew)
+					return api.createRecord(dc.Name, change.New[0])
 				},
 			})
 		case diff2.DELETE:
 			recID := change.Old[0].Original.(goinwx.NameserverRecord).ID
-			corrections = append(corrections, &models.Correction{
+			deletes = append(deletes, &models.Correction{
 				Msg: changeMsgs,
 				F:   func() error { return api.deleteRecord(recID) },
 			})
@@ -417,6 +337,9 @@ func (api *inwxAPI) GetZoneRecordsCorrections(dc *models.DomainConfig, foundReco
 			panic(fmt.Sprintf("unhandled change.Type %s", change.Type))
 		}
 	}
+	corrections = append(deletes, corrections...)
+	corrections = append(corrections, creates...)
+	corrections = append(corrections, deferred...)
 	return corrections, actualChangeCount, nil
 }