fixes for MX checks. Optimizations for status updates. Less load on m…

…ailgun

api/domain/v1/domain_types.go

@@ -112,7 +112,6 @@ type DomainStatus struct {
 
 // +kubebuilder:object:root=true
 // +kubebuilder:subresource:status
-
 // Domain is the Schema for the domains API
 type Domain struct {
 	metav1.TypeMeta   `json:",inline"`

internal/controller/domain/domain_controller.go

@@ -29,6 +29,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/external-dns/endpoint"
 
 	domainv1 "github.com/amoniacou/mailgun-operator/api/domain/v1"
@@ -59,15 +60,6 @@ type DomainReconciler struct {
 // +kubebuilder:rbac:groups=externaldns.k8s.io,resources=dnsendpoints,verbs=get;list;create;update;patch;delete;watch
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete
 
-// Reconcile is part of the main kubernetes reconciliation loop which aims to
-// move the current state of the cluster closer to the desired state.
-// TODO(user): Modify the Reconcile function to compare the state specified by
-// the Domain object against the actual cluster state, and then
-// perform operations to make the cluster state reflect the state specified by
-// the user.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.19.0/pkg/reconcile
 func (r *DomainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := log.FromContext(ctx)
 	var mailgunDomain = &domainv1.Domain{}
@@ -144,13 +136,16 @@ func (r *DomainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	}
 
 	// No need to continue if domain already failed or activated
-	if mailgunDomain.Status.State == domainv1.DomainStateFailed || mailgunDomain.Status.State == domainv1.DomainStateActivated {
+	if mailgunDomain.Status.State == domainv1.DomainStateFailed ||
+		mailgunDomain.Status.State == domainv1.DomainStateActivated {
 		log.V(1).Info("Domain state check", "domain", domainName, "state", mailgunDomain.Status.State)
 		return ctrl.Result{}, nil
 	}
 
 	// we should to try create external DNS records if they are not created yet
-	if mailgunDomain.Spec.ExternalDNS != nil && *mailgunDomain.Spec.ExternalDNS && !mailgunDomain.Status.DnsEntrypointCreated {
+	if mailgunDomain.Spec.ExternalDNS != nil &&
+		*mailgunDomain.Spec.ExternalDNS &&
+		!mailgunDomain.Status.DnsEntrypointCreated {
 		log.V(1).Info("Create external dns records", "domain", domainName)
 		err := r.createExternalDNSEntity(ctx, mailgunDomain)
 		if err != nil {
@@ -203,20 +198,6 @@ func (r *DomainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		} else {
 			mailgunDomain.Status.State = domainv1.DomainStateActivated
 		}
-
-		// If domain is not activated and we need to force MX checks - we need to requeue after some time
-		if mailgunDomain.Status.State != domainv1.DomainStateActivated {
-			log.V(1).Info("Domain is not activated on mailgun - calling for a next tick", "domain", domainName)
-
-			// Store the status
-			if err := r.Status().Update(ctx, mailgunDomain); err != nil {
-				log.Error(err, "unable to update Domain status")
-				return ctrl.Result{}, err
-			}
-			return ctrl.Result{
-				RequeueAfter: time.Duration(r.Config.DomainVerifyDuration) * time.Second,
-			}, nil
-		}
 	}
 
 	// Store the status
@@ -234,7 +215,9 @@ func (r *DomainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *DomainReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	pred := predicate.GenerationChangedPredicate{}
 	return ctrl.NewControllerManagedBy(mgr).
+		WithEventFilter(pred).
 		For(&domainv1.Domain{}).
 		Complete(r)
 }
@@ -300,9 +283,18 @@ func (r *DomainReconciler) checkMXRecordsAndSetState(ctx context.Context, mg *ma
 		log.Error(err, "unable to get domain from mailgun")
 		return
 	}
+
+	log.V(1).Info("Receiving response from mailgun", "domain", domainResponse.ReceivingDNSRecords)
+
+	if len(domainResponse.ReceivingDNSRecords) == 0 {
+		log.Error(err, "no receiving DNS records for domain in Mailgun response")
+		return
+	}
+
 	for _, record := range domainResponse.ReceivingDNSRecords {
+		log.V(1).Info("Checking MX record", "record", record.RecordType, "value", record.Value, "valid", record.Valid)
 		if record.Valid != "valid" {
-			log.V(1).Info("MX record is not valid", "record", record.RecordType, "value", record.Value)
+			return
 		}
 	}
 	log.V(1).Info("All MX records are valid", "domain", mailgunDomain.Spec.Domain)

internal/controller/domain/domain_controller_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package domain
 
 import (
+	"fmt"
 	"time"
 
 	domainv1 "github.com/amoniacou/mailgun-operator/api/domain/v1"
@@ -189,13 +190,14 @@ var _ = Describe("Domain Controller", func() {
 
 			mgm.ActivateDomain(domainName)
 
-			Eventually(func() bool {
+			Eventually(func() int {
 				err := k8sClient.Get(ctx, doDomainLookup, createdDODomain)
 				if err == nil {
-					return createdDODomain.Status.State == domainv1.DomainStateActivated
+					By("Domain validation count by " + fmt.Sprintf("%d", *createdDODomain.Status.DomainValidationCount))
+					return *createdDODomain.Status.DomainValidationCount
 				}
-				return false
-			}, timeout, interval).Should(BeFalse())
+				return 0
+			}, timeout, interval).Should(And(SatisfyAll(BeNumerically(">=", 2), BeNumerically("<", 5))))
 			Expect(createdDODomain.Status.State).To(Equal(domainv1.DomainStateCreated))
 			By("Activate mx records on fake mailgun")
 

internal/utils/mailgun_mock_server.go

@@ -57,8 +57,8 @@ func (m *MailgunMockServer) AddDomain(domainName string) {
 	m.mutex.Lock()
 	newDomain := m.newMGDomainFor(domainName)
 
-	receiveRecords := m.newMGDnsRecordsFor(domainName, false)
-	sendingRecords := m.newMGDnsRecordsFor(domainName, true)
+	receiveRecords := m.newMGDnsRecordsFor(domainName, false, slices.Contains(m.activeDomains, domainName), slices.Contains(m.activeMXDomains, domainName))
+	sendingRecords := m.newMGDnsRecordsFor(domainName, true, slices.Contains(m.activeDomains, domainName), slices.Contains(m.activeMXDomains, domainName))
 
 	m.domainList = append(m.domainList, mailgun.DomainContainer{
 		Domain:              newDomain,
@@ -124,8 +124,8 @@ func (m *MailgunMockServer) createDomain(w http.ResponseWriter, r *http.Request)
 
 	newDomain := m.newMGDomainFor(domainName)
 
-	receiveRecords := m.newMGDnsRecordsFor(domainName, false)
-	sendingRecords := m.newMGDnsRecordsFor(domainName, true)
+	receiveRecords := m.newMGDnsRecordsFor(domainName, false, slices.Contains(m.activeDomains, domainName), slices.Contains(m.activeMXDomains, domainName))
+	sendingRecords := m.newMGDnsRecordsFor(domainName, true, slices.Contains(m.activeDomains, domainName), slices.Contains(m.activeMXDomains, domainName))
 
 	m.domainList = append(m.domainList, mailgun.DomainContainer{
 		Domain:              newDomain,
@@ -148,11 +148,14 @@ func (m *MailgunMockServer) getDomain(w http.ResponseWriter, r *http.Request) {
 	defer m.mutex.Unlock()
 	m.mutex.Lock()
 	domainName := r.PathValue("domain")
-	for _, domain := range m.domainList {
+	for i, domain := range m.domainList {
 		if domainName == domain.Domain.Name {
+			m.domainList[i].ReceivingDNSRecords = m.newMGDnsRecordsFor(domainName, false, slices.Contains(m.activeDomains, domainName), slices.Contains(m.activeMXDomains, domainName))
 			toJSON(w, map[string]interface{}{
-				"message": "Domain DNS records have been retrieved",
-				"domain":  domain,
+				"message":               "Domain DNS records have been retrieved",
+				"domain":                m.domainList[i].Domain,
+				"receiving_dns_records": m.domainList[i].ReceivingDNSRecords,
+				"sending_dns_records":   m.domainList[i].SendingDNSRecords,
 			}, http.StatusOK)
 			return
 		}
@@ -274,17 +277,17 @@ func (m *MailgunMockServer) newMGDomainFor(domainName string) mailgun.Domain {
 	}
 }
 
-func (m *MailgunMockServer) newMGDnsRecordsFor(domainName string, sendingRecords bool) []mailgun.DNSRecord {
+func (m *MailgunMockServer) newMGDnsRecordsFor(domainName string, sendingRecords, activeDomain, activeMXRecord bool) []mailgun.DNSRecord {
 	var records []mailgun.DNSRecord
 
 	dnsValid := unknownDNS
 	mxDnsValid := unknownDNS
 
-	if slices.Contains(m.activeDomains, domainName) {
+	if activeDomain {
 		dnsValid = validDNS
 	}
 
-	if slices.Contains(m.activeMXDomains, domainName) && sendingRecords {
+	if activeMXRecord {
 		mxDnsValid = validDNS
 	}