Merge pull request #115 from datum-cloud/feature/track-cert-health-on-proxy

Add hostname and proxy status conditions to show cert health

Author: mattdjenkinson

api/v1alpha/httpproxy_types.go

@@ -261,6 +261,9 @@ const (
 	// This condition is true when connector metadata has been programmed
 	// via the downstream EnvoyPatchPolicy.
 	HTTPProxyConditionConnectorMetadataProgrammed = "ConnectorMetadataProgrammed"
+
+	// This condition is true when all HTTPS hostnames have ready TLS certificates.
+	HTTPProxyConditionCertificatesReady = "CertificatesReady"
 )
 
 const (
@@ -283,6 +286,25 @@ const (
 	// HostnameConditionAvailable indicates whether the hostname was successfully
 	// claimed by this resource or is already in use by another resource.
 	HostnameConditionAvailable = "Available"
+
+	// HostnameConditionCertificateReady tracks whether a TLS certificate has been
+	// provisioned for this hostname (cert-manager Certificate in the downstream cluster).
+	HostnameConditionCertificateReady = "CertificateReady"
+)
+
+// Reasons for HostnameConditionCertificateReady.
+const (
+	// CertificateReadyReasonCertificateIssued indicates the certificate has been issued and is ready.
+	CertificateReadyReasonCertificateIssued = "CertificateIssued"
+
+	// CertificateReadyReasonPending indicates the certificate is not yet ready (e.g. not found or provisioning).
+	CertificateReadyReasonPending = "Pending"
+
+	// CertificateReadyReasonProvisioningFailed indicates certificate provisioning failed.
+	CertificateReadyReasonProvisioningFailed = "ProvisioningFailed"
+
+	// CertificateReadyReasonChallengeInProgress indicates an ACME challenge is in progress.
+	CertificateReadyReasonChallengeInProgress = "ChallengeInProgress"
 )
 
 // Reasons for HostnameConditionAvailable.
@@ -316,9 +338,16 @@ const (
 	DNSRecordReasonZoneNotReady = "DNSZoneNotReady"
 
 	// DNSRecordReasonDomainNotVerified indicates the Domain resource for this
-	// hostname has not been verified via a DNSZone.
+	// hostname has not been verified.
 	DNSRecordReasonDomainNotVerified = "DomainNotVerified"
 
+	// DNSRecordReasonDNSAuthorityMissing indicates the Domain is verified but
+	// Datum DNS does not have authority over the domain. This happens when:
+	// - The DNSZone is not ready (Accepted/Programmed conditions not True)
+	// - The DNSZone has no nameservers assigned yet
+	// - The domain's nameservers don't include the DNSZone's nameservers
+	DNSRecordReasonDNSAuthorityMissing = "DNSAuthorityMissing"
+
 	// DNSRecordReasonConflict indicates an existing DNSRecordSet for this
 	// hostname is managed by a different actor.
 	DNSRecordReasonConflict = "ConflictWithUserRecord"
@@ -353,6 +382,18 @@ const (
 	DNSRecordsProgrammedReasonPartialFailure = "PartialFailure"
 )
 
+// Reasons for HTTPProxyConditionCertificatesReady.
+const (
+	// CertificatesReadyReasonAllCertificatesReady indicates all HTTPS hostnames have ready certificates.
+	CertificatesReadyReasonAllCertificatesReady = "AllCertificatesReady"
+
+	// CertificatesReadyReasonCertificatesPending indicates one or more certificates are pending or in progress.
+	CertificatesReadyReasonCertificatesPending = "CertificatesPending"
+
+	// CertificatesReadyReasonCertificatesFailed indicates one or more certificate provisioning attempts failed.
+	CertificatesReadyReasonCertificatesFailed = "CertificatesFailed"
+)
+
 const (
 
 	// HTTPProxyReasonAccepted indicates that the HTTP proxy has been accepted.
@@ -394,6 +435,7 @@ const (
 //
 // +kubebuilder:printcolumn:name="Hostname",type=string,JSONPath=`.status.hostnames[*]`
 // +kubebuilder:printcolumn:name="Programmed",type=string,JSONPath=`.status.conditions[?(@.type=="Programmed")].status`
+// +kubebuilder:printcolumn:name="Certificates",type=string,JSONPath=`.status.conditions[?(@.type=="CertificatesReady")].status`
 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
 type HTTPProxy struct {
 	metav1.TypeMeta   `json:",inline"`

api/v1alpha/httpproxy_types_test.go

@@ -216,6 +216,7 @@ func TestConditionConstants(t *testing.T) {
 	assert.Equal(t, "DNSZoneNotFound", networkingv1alpha.DNSRecordReasonZoneNotFound)
 	assert.Equal(t, "DNSZoneNotReady", networkingv1alpha.DNSRecordReasonZoneNotReady)
 	assert.Equal(t, "DomainNotVerified", networkingv1alpha.DNSRecordReasonDomainNotVerified)
+	assert.Equal(t, "DNSAuthorityMissing", networkingv1alpha.DNSRecordReasonDNSAuthorityMissing)
 	assert.Equal(t, "ConflictWithUserRecord", networkingv1alpha.DNSRecordReasonConflict)
 	assert.Equal(t, "RecordCreationFailed", networkingv1alpha.DNSRecordReasonFailed)
 	assert.Equal(t, "RetryPending", networkingv1alpha.DNSRecordReasonRetryPending)

config/rbac_downstream/role.yaml

@@ -134,6 +134,14 @@ rules:
   - httproutes/status
   verbs:
   - get
+- apiGroups:
+  - cert-manager.io
+  resources:
+  - certificates
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - gateway.envoyproxy.io
   resources:

internal/controller/domain_controller.go

@@ -33,6 +33,7 @@ import (
 	"go.datum.net/network-services-operator/internal/config"
 	"go.datum.net/network-services-operator/internal/registrydata"
 	conditionutil "go.datum.net/network-services-operator/internal/util/condition"
+	dnsutil "go.datum.net/network-services-operator/internal/util/dns"
 )
 
 // DomainReconciler reconciles a Domain object
@@ -320,34 +321,6 @@ var dnsZoneListGVK = schema.GroupVersionKind{
 	Kind:    "DNSZoneList",
 }
 
-func normalizeNameserverHostname(s string) string {
-	return strings.ToLower(strings.TrimSuffix(strings.TrimSpace(s), "."))
-}
-
-func hasStringOverlapNormalized(a, b []string) bool {
-	if len(a) == 0 || len(b) == 0 {
-		return false
-	}
-	set := make(map[string]struct{}, len(a))
-	for _, v := range a {
-		n := normalizeNameserverHostname(v)
-		if n == "" {
-			continue
-		}
-		set[n] = struct{}{}
-	}
-	for _, v := range b {
-		n := normalizeNameserverHostname(v)
-		if n == "" {
-			continue
-		}
-		if _, ok := set[n]; ok {
-			return true
-		}
-	}
-	return false
-}
-
 func (r *DomainReconciler) attemptDNSZoneVerification(
 	ctx context.Context,
 	reader client.Reader,
@@ -439,7 +412,7 @@ func (r *DomainReconciler) attemptDNSZoneVerification(
 			verifiedDNSZoneCondition.Message = fmt.Sprintf("Waiting for Domain status.nameservers before verifying via DNSZone %q", zoneName)
 			continue
 		}
-		if !hasStringOverlapNormalized(domainNS, zoneNS) {
+		if !dnsutil.HasNameserverOverlap(domainNS, zoneNS) {
 			verifiedDNSZoneCondition.Reason = networkingv1alpha.DomainReasonDNSZoneNameserverMismatch
 			verifiedDNSZoneCondition.Message = fmt.Sprintf("Domain nameservers do not match DNSZone %q nameservers yet", zoneName)
 			continue

internal/controller/gateway_controller.go

@@ -797,6 +797,14 @@ func (r *GatewayReconciler) finalizeGateway(
 ) (result Result) {
 	logger := log.FromContext(ctx)
 	logger.Info("finalizing gateway")
+
+	// Clean up DNS records created by this gateway
+	if r.Config.Gateway.EnableDNSIntegration {
+		if cleanupResult := r.cleanupDNSRecordSets(ctx, upstreamClient, upstreamGateway); cleanupResult.ShouldReturn() {
+			return cleanupResult
+		}
+	}
+
 	// Go through downstream http routes that are attached to the downstream
 	// gateway and remove the parentRef from the status. If it's the last parent
 	// ref, delete the downstream route. If there's a race condition on delete/create,
@@ -870,6 +878,54 @@ func (r *GatewayReconciler) finalizeGateway(
 	return result
 }
 
+// cleanupDNSRecordSets deletes all DNSRecordSet resources that were created by
+// this gateway. This is called during gateway finalization to ensure DNS records
+// are cleaned up when the gateway is deleted, since owner reference-based garbage
+// collection doesn't work (the dns-operator sets itself as the controller owner).
+func (r *GatewayReconciler) cleanupDNSRecordSets(
+	ctx context.Context,
+	upstreamClient client.Client,
+	upstreamGateway *gatewayv1.Gateway,
+) (result Result) {
+	logger := log.FromContext(ctx)
+
+	var recordSetList dnsv1alpha1.DNSRecordSetList
+	if err := upstreamClient.List(ctx, &recordSetList,
+		client.InNamespace(upstreamGateway.Namespace),
+		client.MatchingLabels{
+			labelDNSManaged:    "true",
+			labelManagedBy:     labelManagedByValue,
+			labelDNSSourceKind: KindGateway,
+			labelDNSSourceName: upstreamGateway.Name,
+			labelDNSSourceNS:   upstreamGateway.Namespace,
+		},
+	); err != nil {
+		// If the CRD doesn't exist, there's nothing to clean up
+		if apimeta.IsNoMatchError(err) {
+			return result
+		}
+		result.Err = fmt.Errorf("failed listing DNSRecordSets for cleanup: %w", err)
+		return result
+	}
+
+	for _, rs := range recordSetList.Items {
+		logger.Info("deleting DNSRecordSet during gateway finalization",
+			"name", rs.Name,
+			"hostname", rs.Annotations[annotationDNSHostname],
+		)
+		if err := upstreamClient.Delete(ctx, &rs); err != nil && !apierrors.IsNotFound(err) {
+			result.Err = fmt.Errorf("failed to delete DNSRecordSet %q: %w", rs.Name, err)
+			return result
+		}
+	}
+
+	if len(recordSetList.Items) > 0 {
+		logger.Info("deleted DNSRecordSets during gateway finalization", "count", len(recordSetList.Items))
+	}
+
+	return result
+}
+
 // TODO(jreese) revisit the parameters here to clean them up. It's a bit messy
 // as this function is used against both the upstream and downstream resources.
 func (r *GatewayReconciler) detachHTTPRoutes(

internal/controller/gateway_dns_controller.go

@@ -25,6 +25,7 @@ import (
 	mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile"
 
 	networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha"
+	dnsutil "go.datum.net/network-services-operator/internal/util/dns"
 	dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1"
 )
 
@@ -102,7 +103,7 @@ func (r *GatewayReconciler) ensureDNSRecordSets(
 			continue
 		}
 
-		// Find the most specific Domain + DNSZone combination.
+		// Find the most specific Domain + DNSZone combination where Datum DNS has authority.
 		var domain *networkingv1alpha.Domain
 		var dnsZone *dnsv1alpha1.DNSZone
 		var matchedZoneName string
@@ -114,10 +115,9 @@ func (r *GatewayReconciler) ensureDNSRecordSets(
 				continue
 			}
 
-			// Check if the Domain has VerifiedDNSZone=True.
-			if !apimeta.IsStatusConditionTrue(d.Status.Conditions, networkingv1alpha.DomainConditionVerifiedDNSZone) {
-				// Domain exists but isn't verified; record this for potential error message
-				// but keep looking for a more specific verified domain.
+			// Check if the Domain is verified (ownership proven via any method).
+			if !apimeta.IsStatusConditionTrue(d.Status.Conditions, networkingv1alpha.DomainConditionVerified) {
+				// Domain exists but isn't verified; keep looking.
 				continue
 			}
 
@@ -135,36 +135,87 @@ func (r *GatewayReconciler) ensureDNSRecordSets(
 				continue
 			}
 
-			// Found a matching Domain + DNSZone.
+			// Check if Datum DNS has authority (DNSZone ready + nameservers match).
+			zone := &dnsZoneList.Items[0]
+			if !dnsutil.HasDNSAuthority(&d, zone) {
+				// Domain verified but Datum DNS doesn't have authority yet.
+				continue
+			}
+
+			// Found a matching Domain + DNSZone where Datum DNS has authority.
 			domain = &d
-			dnsZone = &dnsZoneList.Items[0]
+			dnsZone = zone
 			matchedZoneName = zoneName
 			break
 		}
 
 		// If no matching Domain + DNSZone found, check why and set appropriate status.
 		if domain == nil || dnsZone == nil {
 			// Try to provide a helpful message by checking what we found.
-			var unverifiedDomain *networkingv1alpha.Domain
+			var (
+				unverifiedDomain  *networkingv1alpha.Domain
+				noAuthorityDomain *networkingv1alpha.Domain
+				noAuthorityZone   *dnsv1alpha1.DNSZone
+			)
 			for _, zoneName := range zoneNames {
 				d, found := findDomainByName(domainList.Items, zoneName)
-				if found {
-					if !apimeta.IsStatusConditionTrue(d.Status.Conditions, networkingv1alpha.DomainConditionVerifiedDNSZone) {
-						unverifiedDomain = &d
-						break
-					}
+				if !found {
+					continue
+				}
+
+				// Check if domain is verified
+				if !apimeta.IsStatusConditionTrue(d.Status.Conditions, networkingv1alpha.DomainConditionVerified) {
+					unverifiedDomain = &d
+					break
+				}
+
+				// Domain is verified; check if there's a DNSZone
+				var dnsZoneList dnsv1alpha1.DNSZoneList
+				if err := upstreamClient.List(ctx, &dnsZoneList,
+					client.InNamespace(upstreamGateway.Namespace),
+					client.MatchingFields{dnsZoneDomainNameIndex: zoneName},
+				); err != nil {
+					continue
 				}
+				if len(dnsZoneList.Items) == 0 {
+					continue
+				}
+
+				// DNSZone exists but Datum DNS doesn't have authority
+				noAuthorityDomain = &d
+				noAuthorityZone = &dnsZoneList.Items[0]
+				break
 			}
 
-			if unverifiedDomain != nil {
+			switch {
+			case unverifiedDomain != nil:
 				apimeta.SetStatusCondition(&hs.Conditions, metav1.Condition{
 					Type:               networkingv1alpha.HostnameConditionDNSRecordProgrammed,
 					Status:             metav1.ConditionFalse,
 					Reason:             networkingv1alpha.DNSRecordReasonDomainNotVerified,
-					Message:            fmt.Sprintf("Domain %q has not been verified via a DNSZone (VerifiedDNSZone condition is not True)", unverifiedDomain.Name),
+					Message:            fmt.Sprintf("Domain %q ownership has not been verified", unverifiedDomain.Name),
+					ObservedGeneration: upstreamGateway.Generation,
+				})
+			case noAuthorityDomain != nil:
+				msg := fmt.Sprintf("Domain %q is verified but Datum DNS does not have authority", noAuthorityDomain.Name)
+				if noAuthorityZone != nil {
+					if !apimeta.IsStatusConditionTrue(noAuthorityZone.Status.Conditions, "Accepted") ||
+						!apimeta.IsStatusConditionTrue(noAuthorityZone.Status.Conditions, "Programmed") {
+						msg = fmt.Sprintf("DNSZone %q is not ready (waiting for Accepted and Programmed conditions)", noAuthorityZone.Name)
+					} else if len(noAuthorityZone.Status.Nameservers) == 0 {
+						msg = fmt.Sprintf("DNSZone %q has no nameservers assigned yet", noAuthorityZone.Name)
+					} else {
+						msg = fmt.Sprintf("Domain %q nameservers do not include DNSZone %q nameservers; update your registrar's NS records", noAuthorityDomain.Name, noAuthorityZone.Name)
+					}
+				}
+				apimeta.SetStatusCondition(&hs.Conditions, metav1.Condition{
+					Type:               networkingv1alpha.HostnameConditionDNSRecordProgrammed,
+					Status:             metav1.ConditionFalse,
+					Reason:             networkingv1alpha.DNSRecordReasonDNSAuthorityMissing,
+					Message:            msg,
 					ObservedGeneration: upstreamGateway.Generation,
 				})
-			} else {
+			default:
 				apimeta.SetStatusCondition(&hs.Conditions, metav1.Condition{
 					Type:               networkingv1alpha.HostnameConditionDNSRecordProgrammed,
 					Status:             metav1.ConditionTrue,