feat(api): Add multi-domain support with unified domain management #67

nfebe · 2026-01-31T14:31:56Z

Add DomainConfig type and Domains field to ServiceMetadata
Add GetEffectiveDomains helper for backward compatibility
Add domain CRUD API endpoints (list, add, update, delete)
Support "default" ID for legacy networking.domain config
Add SSL manager support for multi-domain certificates
Add nginx manager support for multi-domain virtual hosts
Add comprehensive domain API tests

sourceant · 2026-01-31T14:33:02Z

Code Review Summary

This review covers a significant feature addition introducing multi-domain and multi-database support for deployments. The changes involve updates to service metadata models, API handlers for domain and database management, and extensive refactoring of the Nginx proxy orchestration to support multiple domain configurations and SSL certificates. The new test cases are well-structured and provide good coverage for the added functionality. While many aspects of the implementation are well-considered, a critical architectural flaw and some areas for improved clarity have been identified.

🚀 Key Improvements

Introduction of explicit Domains and Databases arrays in ServiceMetadata for flexible configuration.
Comprehensive helper methods (GetDomains, GetPrimaryDatabase, GetUniqueDomainNames, HasMultipleDomains) for better data access and encapsulation in pkg/models/deployment.go.
New API endpoints for list, add, update, and delete operations on deployment domains, including UUID generation for new domains.
Refactored Nginx proxy configuration to support multiple domains per deployment, with intelligent SSL handling and path-based routing, including text/template for generation.
Robust multi-database provisioning and environment variable generation, supporting shared, existing, and external modes.
Thorough test coverage for new domain and multi-database functionalities.

💡 Minor Suggestions

Consider making the implicit conversion of legacy NetworkingConfig to a 'default' domain in addDomain a more explicit, one-time migration step rather than part of the add flow to prevent unexpected behavior.
Review the ProxyStatus.Domain field in internal/proxy/orchestrator.go; with the Domains array, it might be redundant or require clearer definition as a 'primary' domain.
Add detailed comments to the new Nginx templates (multiRouteHTTPTemplate, multiRouteSSLTemplate, multiRouteMixedTemplate) in internal/nginx/manager.go to explain their complex logic and conditional directives.
Consolidate default path resolution logic in config-related functions for better maintainability.

🚨 Critical Issues

Critical Package Naming Collision and Architectural Flaw: The file internal/nginx/manager.go is incorrectly declared as package ssl, causing a package naming collision with internal/ssl/manager.go, which also declares package ssl. This is a severe architectural issue. internal/nginx/manager.go must be refactored to package nginx and handle only Nginx-specific logic, importing internal/ssl for certificate management. This requires significant refactoring in internal/api/server.go and internal/proxy/orchestrator.go to correctly initialize and use the separated managers. This must be addressed immediately as it will cause compilation errors or incorrect runtime behavior.

sourceant · 2026-01-31T14:33:07Z

🔍 Code Review

💡 1. **internal/api/server.go** (Lines 3462-3471) - REFACTOR

The logic here implicitly adds a "default" domain based on Networking config if deployment.Metadata.Domains is empty. While this handles backward compatibility, it might be more explicit to have a dedicated migration function or a ConvertLegacyNetworkingToDomains method that is called once, possibly during deployment initialization or metadata upgrade, rather than implicitly modifying Domains within addDomain. This prevents unexpected creation of a 'default' entry when a user intends to only add a single new domain via the API.

Suggested Code:

    if deployment.Metadata == nil {
        deployment.Metadata = &models.ServiceMetadata{}
    }

    // TODO: Consider a dedicated migration path for legacy networking config
    // If `deployment.Metadata.Domains` is initially empty but `Networking.Expose` is true, a 'default'
    // domain based on `Networking` will be implicitly created by GetDomains().
    // This could be made explicit here as a one-time conversion if `len(deployment.Metadata.Domains) == 0`
    // and `deployment.Metadata.Networking.Expose` to ensure consistent state before adding new domains.

    deployment.Metadata.Domains = append(deployment.Metadata.Domains, domain)

Current Code:

    if deployment.Metadata == nil {
        deployment.Metadata = &models.ServiceMetadata{}
    }

    if len(deployment.Metadata.Domains) == 0 && deployment.Metadata.Networking.Expose {
        existingDomain := models.DomainConfig{
            ID:            "default",
            Service:       deployment.Metadata.Name,
            ContainerPort: deployment.Metadata.Networking.ContainerPort,
            Domain:        deployment.Metadata.Networking.Domain,
            SSL:           deployment.Metadata.SSL,
        }
        deployment.Metadata.Domains = []models.DomainConfig{existingDomain}
    }

    deployment.Metadata.Domains = append(deployment.Metadata.Domains, domain)

💡 2. **internal/api/server.go** (Lines 3551-3590) - REFACTOR

The logic for handling the 'default' domain deletion and subsequent proxy teardown/setup is complex. It involves checking len(deployment.Metadata.Domains) == 0 and deployment.Metadata.Networking.Expose. This indicates an intricate interaction between the new explicit Domains array and the legacy Networking configuration. While functional, explicitly separating the migration/cleanup of legacy Networking config from the deletion of user-defined explicit domains would improve clarity and reduce potential edge cases. For instance, after deleting the last explicit domain, a function could be called to 'normalize' the Networking settings if they still implicitly defined a domain.

Suggested Code:

func (s *Server) deleteDomain(c *gin.Context) {
	name := c.Param("name")
	domainID := c.Param("domainId")

	deployment, err := s.manager.GetDeployment(name)
	if err != nil {
		c.JSON(http.StatusNotFound, gin.H{"error": "Deployment not found"})
		return
	}

	if deployment.Metadata == nil {
		c.JSON(http.StatusNotFound, gin.H{"error": "Domain not found"})
		return
	}

	// Attempt to delete a legacy "default" domain first if applicable.
	// This ensures that the old networking config can be cleared.
	wasLegacyDefaultDeleted := false
	if domainID == "default" && len(deployment.Metadata.Domains) == 0 && deployment.Metadata.Networking.Expose && deployment.Metadata.Networking.Domain != "" {
		// Clear the legacy networking config
		deployment.Metadata.Networking.Expose = false
		deployment.Metadata.Networking.Domain = ""
		deployment.Metadata.SSL.Enabled = false
		deployment.Metadata.SSL.AutoCert = false
		wasLegacyDefaultDeleted = true
	} else if domainID == "default" {
		// If domainId is "default" but no legacy domain exists to clear, treat as not found
		c.JSON(http.StatusNotFound, gin.H{"error": "Default legacy domain not found or not applicable."})
		return
	}

	// If not a legacy default deletion, proceed with explicit domain deletion
	found := false
	newDomains := make([]models.DomainConfig, 0)
	for _, d := range deployment.Metadata.Domains {
		if d.ID == domainID {
			found = true
			continue
		}
		newDomains = append(newDomains, d)
	}

	if !found && !wasLegacyDefaultDeleted {
		c.JSON(http.StatusNotFound, gin.H{"error": "Domain not found"})
		return
	}

	// Update domains in metadata only if explicit domains were modified
	if found {
		if len(newDomains) == 0 {
			deployment.Metadata.Domains = nil
		} else {
			deployment.Metadata.Domains = newDomains
		}
	}

	// Save metadata after changes
	if err := s.manager.SaveMetadata(name, deployment.Metadata); err != nil {
		c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save metadata: " + err.Error()})
		return
	}

	// Reconfigure proxy based on the new state
	if s.proxyOrchestrator != nil {
		// If all explicit domains are gone AND legacy expose is false, teardown completely.
		if len(deployment.Metadata.Domains) == 0 && !deployment.Metadata.Networking.Expose {
			if err := s.proxyOrchestrator.TeardownDeployment(name); err != nil {
				log.Printf("Warning: failed to teardown proxy for %s after last domain deleted: %v", name, err)
			}
		} else {
			// Otherwise, re-setup proxy for remaining explicit domains or potentially newly exposed legacy domain.
			if _, err := s.proxyOrchestrator.SetupDeployment(deployment); err != nil {
				log.Printf("Warning: failed to update proxy for %s after domain deletion: %v", name, err)
			}
		}
	}

	c.JSON(http.StatusOK, gin.H{
		"message": "Domain deleted successfully",
	})
}

Current Code:

    // Handle legacy "default" domain from networking config
    if domainID == "default" && len(deployment.Metadata.Domains) == 0 {
        if !deployment.Metadata.Networking.Expose || deployment.Metadata.Networking.Domain == "" {
            c.JSON(http.StatusNotFound, gin.H{"error": "Domain not found"})
            return
        }
        // Clear the legacy networking config
        deployment.Metadata.Networking.Expose = false
        deployment.Metadata.Networking.Domain = ""
        deployment.Metadata.SSL.Enabled = false
        deployment.Metadata.SSL.AutoCert = false

        if err := s.manager.SaveMetadata(name, deployment.Metadata); err != nil {
            c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save metadata: " + err.Error()})
            return
        }

        if s.proxyOrchestrator != nil {
            if err := s.proxyOrchestrator.TeardownDeployment(name); err != nil {
                log.Printf("Warning: failed to teardown proxy for %s: %v", name, err)
            }
        }

        c.JSON(http.StatusOK, gin.H{"message": "Domain deleted successfully"})
        return
    }

    if len(deployment.Metadata.Domains) == 0 {
        c.JSON(http.StatusNotFound, gin.H{"error": "Domain not found"})
        return
    }

    found := false
    newDomains := make([]models.DomainConfig, 0)
    for _, d := range deployment.Metadata.Domains {
        if d.ID == domainID {
            found = true
            continue
        }
        newDomains = append(newDomains, d)
    }

    if !found {
        c.JSON(http.StatusNotFound, gin.H{"error": "Domain not found"})
        return
    }

    if len(newDomains) == 0 {
        deployment.Metadata.Domains = nil
    } else {
        deployment.Metadata.Domains = newDomains
    }

    if err := s.manager.SaveMetadata(name, deployment.Metadata); err != nil {
        c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save metadata: " + err.Error()})
        return
    }

    if s.proxyOrchestrator != nil {
        if len(newDomains) == 0 {
            if deployment.Metadata.Networking.Expose {
                if _, err := s.proxyOrchestrator.SetupDeployment(deployment); err != nil {
                    log.Printf("Warning: failed to setup legacy proxy for %s: %v", name, err)
                }
            } else {
                if err := s.proxyOrchestrator.TeardownDeployment(name); err != nil {
                    log.Printf("Warning: failed to teardown proxy for %s: %v", name, err)
                }
            }
        } else {
            if _, err := s.proxyOrchestrator.SetupDeployment(deployment); err != nil {
                log.Printf("Warning: failed to update proxy for %s: %v", name, err)
            }
        }
    }

    c.JSON(http.StatusOK, gin.H{
        "message": "Domain deleted successfully",
    })

💡 3. **internal/nginx/manager.go** (Line 19) - REFACTOR

The Manager struct in internal/nginx/manager.go is currently defined with SSL/Certbot related fields (config, certsPath, webRoot, containerWebRoot, mu). These fields, along with the associated methods (e.g., RequestCertificate, RenewCertificates, DeleteCertificate), belong in the internal/ssl/Manager struct. This file should focus purely on Nginx management and depend on the ssl package for certificate-related operations. Additionally, the config field should use config.NginxConfig.

Suggested Code:

type Manager struct {
	config        *config.NginxConfig
	configPath    string
	webrootPath   string
	sslManager    *ssl.Manager // Dependency for SSL certificate operations
	mu            sync.RWMutex
	securityRules *SecurityRuleManager // If Nginx security features are part of this manager
}

Current Code:

type Manager struct {
	config           *config.CertbotConfig
	certsPath        string
	webRoot          string
	containerWebRoot string
	mu               sync.RWMutex
}

💡 4. **internal/nginx/manager.go** (Lines 26-48) - REFACTOR

The NewManager function in internal/nginx/manager.go should initialize an Nginx-specific manager. Its parameters and the fields it sets (certsPath, webRoot, containerWebRoot) are currently tied to Certbot/SSL. This function needs to be refactored to accept an *config.NginxConfig and potentially an *ssl.Manager dependency, and initialize only Nginx-related paths and configurations. The certbotWebrootPath is also incorrectly passed as a string, it should be derived from the ssl.Manager.

Suggested Code:

func NewManager(cfg *config.NginxConfig, deploymentsPath, certbotWebrootPath string) *Manager {
	configPath := cfg.ConfigPath
	if configPath == "" {
		configPath = filepath.Join(deploymentsPath, "nginx", "conf.d")
	}

	// Ensure nginx config path exists
	if err := os.MkdirAll(configPath, 0755); err != nil {
		log.Printf("Warning: failed to create nginx config directory %s: %v", configPath, err)
	}

	// The webroot path needs to be consistent for Certbot ACME challenges
	// It's passed from the proxy orchestrator which has access to certbot config.

	return &Manager{
		config:      cfg,
		configPath:  configPath,
		webrootPath: certbotWebrootPath,
	}
}

Current Code:

func NewManager(cfg *config.CertbotConfig, deploymentsPath string) *Manager {
	certsPath := cfg.CertsPath
	if certsPath == "" {
		certsPath = filepath.Join(deploymentsPath, "nginx", "certs", "live")
	}

	webRoot := cfg.WebrootPath
	if webRoot == "" {
		webRoot = filepath.Join(deploymentsPath, "nginx", "html")
	}

	containerWebRoot := cfg.ContainerWebrootPath
	if containerWebRoot == "" {
		containerWebRoot = "/var/www/certbot"
	}

	return &Manager{
		config:           cfg,
		certsPath:        certsPath,
		webRoot:          webRoot,
		containerWebRoot: containerWebRoot,
	}
}

💡 5. **internal/nginx/manager.go** (Lines 54-77) - REFACTOR

The UpdateConfig method in internal/nginx/manager.go is currently updating SSL/Certbot related configurations. This method should be refactored to only update Nginx-specific configurations, and the SSL configuration updates should be handled by the internal/ssl/Manager.

Suggested Code:

func (m *Manager) UpdateConfig(cfg *config.NginxConfig, deploymentsPath, certbotWebrootPath string) {
	m.mu.Lock()
	defer m.mu.Unlock()

	m.config = cfg

	configPath := cfg.ConfigPath
	if configPath == "" {
		configPath = filepath.Join(deploymentsPath, "nginx", "conf.d")
	}
	m.configPath = configPath
	m.webrootPath = certbotWebrootPath
}

Current Code:

func (m *Manager) UpdateConfig(cfg *config.CertbotConfig, deploymentsPath string) {
	m.mu.Lock()
	defer m.mu.Unlock()

	m.config = cfg

	certsPath := cfg.CertsPath
	if certsPath == "" {
		certsPath = filepath.Join(deploymentsPath, "nginx", "certs", "live")
	}
	m.certsPath = certsPath

	webRoot := cfg.WebrootPath
	if webRoot == "" {
		webRoot = filepath.Join(deploymentsPath, "nginx", "html")
	}
	m.webRoot = webRoot

	containerWebRoot := cfg.ContainerWebrootPath
	if containerWebRoot == "" {
		containerWebRoot = "/var/www/certbot"
	}
	m.containerWebRoot = containerWebRoot
}

💡 6. **internal/proxy/orchestrator.go** (Line 20) - REFACTOR

The NewOrchestrator function should initialize o.nginx using nginx.NewManager (from the nginx package) and o.ssl using ssl.NewManager (from the ssl package). This requires nginx.NewManager to accept an *config.NginxConfig and ssl.NewManager to accept an *config.CertbotConfig. The current ssl.NewManager call for o.nginx is incorrect due to the package confusion. Additionally, nginx.NewManager needs the webrootPath from the SSL manager, which should be available via a method on ssl.Manager.

Suggested Code:

func NewOrchestrator(cfg *config.Config) *Orchestrator {
	sslMgr := ssl.NewManager(&cfg.Certbot, cfg.DeploymentsPath)
	return &Orchestrator{
		nginx: nginx.NewManager(&cfg.Nginx, cfg.DeploymentsPath, sslMgr.WebRoot()), // Pass webroot path from SSL manager
		ssl:   sslMgr,
	}
}

Current Code:

func NewOrchestrator(cfg *config.Config) *Orchestrator {
	return &Orchestrator{
		nginx: nginx.NewManager(&cfg.Nginx, cfg.DeploymentsPath, cfg.Certbot.WebrootPath),
		ssl:   ssl.NewManager(&cfg.Certbot, cfg.DeploymentsPath),
	}
}

💡 7. **internal/proxy/orchestrator.go** (Lines 42-43) - REFACTOR

The UpdateConfig method should correctly update both the Nginx manager's configuration and the SSL manager's configuration, passing the relevant parts of the main config.Config to each. Ensure nginx.Manager.UpdateConfig expects an *config.NginxConfig and ssl.Manager.UpdateConfig expects an *config.CertbotConfig. The WebRoot() method on o.ssl would be needed to pass the correct webroot path to the Nginx manager.

Suggested Code:

func (o *Orchestrator) UpdateConfig(cfg *config.Config) {
	o.nginx.UpdateConfig(&cfg.Nginx, cfg.DeploymentsPath, o.ssl.WebRoot())
	o.ssl.UpdateConfig(&cfg.Certbot, cfg.DeploymentsPath)
}

Current Code:

func (o *Orchestrator) UpdateConfig(cfg *config.Config) {
	o.nginx.UpdateConfig(&cfg.Nginx, cfg.DeploymentsPath, cfg.Certbot.WebrootPath)
	o.ssl.UpdateConfig(&cfg.Certbot, cfg.DeploymentsPath)
}

💡 8. **pkg/models/deployment.go** (Line 33) - IMPROVEMENT

The addition of Domains []DomainConfig and Databases []DatabaseConfig to ServiceMetadata is a key structural improvement, allowing for more flexible and explicit configuration of multiple domains and databases per deployment.

Suggested Code:

    CredentialID string                    `yaml:"credential_id,omitempty" json:"credential_id,omitempty"`
    Domains      []DomainConfig            `yaml:"domains,omitempty" json:"domains,omitempty"`
    Databases    []DatabaseConfig          `yaml:"databases,omitempty" json:"databases,omitempty"}`

Current Code:

    CredentialID string                    `yaml:"credential_id,omitempty" json:"credential_id,omitempty"`

💡 9. **pkg/models/deployment.go** (Lines 36-64) - IMPROVEMENT

The new GetDomains(), GetUniqueDomainNames(), HasMultipleDomains(), GetDatabases(), GetPrimaryDatabase(), and HasMultipleDatabases() methods on ServiceMetadata provide robust and encapsulated logic for accessing and managing domain and database configurations. The GetDomains() method's fallback to Networking configuration is well-implemented for backward compatibility.

Suggested Code:

type DomainConfig struct {
	ID            string    `yaml:"id" json:"id"`
	Service       string    `yaml:"service" json:"service"`
	ContainerPort int       `yaml:"container_port" json:"container_port"`
	Domain        string    `yaml:"domain" json:"domain"`
	PathPrefix    string    `yaml:"path_prefix,omitempty" json:"path_prefix,omitempty"`
	StripPrefix   bool      `yaml:"strip_prefix,omitempty" json:"strip_prefix,omitempty"`
	SSL           SSLConfig `yaml:"ssl" json:"ssl"`
	Aliases       []string  `yaml:"aliases,omitempty" json:"aliases,omitempty"`
}

type DatabaseConfig struct {
	ID           string `yaml:"id" json:"id"`
	Alias        string `yaml:"alias" json:"alias"`
	Type         string `yaml:"type" json:"type"`
	Mode         string `yaml:"mode" json:"mode"`
	Service      string `yaml:"service,omitempty" json:"service,omitempty"`
	Host         string `yaml:"host,omitempty" json:"host,omitempty"`
	Port         int    `yaml:"port,omitempty" json:"port,omitempty"`
	Container    string `yaml:"container,omitempty" json:"container,omitempty"`
	DatabaseName string `yaml:"database_name,omitempty" json:"database_name,omitempty"`
	Username     string `yaml:"username,omitempty" json:"username,omitempty"`
	EnvPrefix    string `yaml:"env_prefix,omitempty" json:"env_prefix,omitempty"`
	IsShared     bool   `yaml:"is_shared,omitempty" json:"is_shared,omitempty"`
}

func (m *ServiceMetadata) GetDomains() []DomainConfig {
	if len(m.Domains) > 0 {
		return m.Domains
	}
	if !m.Networking.Expose || m.Networking.Domain == "" {
		return nil
	}
	return []DomainConfig{{
		ID:            "default",
		Service:       m.Name,
		ContainerPort: m.Networking.ContainerPort,
		Domain:        m.Networking.Domain,
		SSL:           m.SSL,
	}}
}

func (m *ServiceMetadata) GetUniqueDomainNames() []string {
	domains := m.GetDomains()
	domainSet := make(map[string]struct{})
	for _, d := range domains {
		domainSet[d.Domain] = struct{}{}
		for _, alias := range d.Aliases {
			domainSet[alias] = struct{}{}
		}
	}
	result := make([]string, 0, len(domainSet))
	for name := range domainSet {
		result = append(result, name)
	}
	return result
}

func (m *ServiceMetadata) HasMultipleDomains() bool {
	return len(m.Domains) > 1
}

func (m *ServiceMetadata) GetDatabases() []DatabaseConfig {
	return m.Databases
}

func (m *ServiceMetadata) GetPrimaryDatabase() *DatabaseConfig {
	if len(m.Databases) == 0 {
		return nil
	}
	for i := range m.Databases {
		if m.Databases[i].Alias == "primary" {
			return &m.Databases[i]
		}
	}
	return &m.Databases[0]
}

func (m *ServiceMetadata) HasMultipleDatabases() bool {
	return len(m.Databases) > 1
}

Verdict: APPROVE

Posted as a comment because posting a review failed.

sourceant