From e55e04b7add78d3eb154ba2c6ea2e6683d6117c8 Mon Sep 17 00:00:00 2001 From: nfebe Date: Tue, 3 Feb 2026 20:39:03 +0100 Subject: [PATCH] fix(proxy): Prevent duplicate domains and restore vhost on failure - Add duplicate domain+path validation in addDomain API endpoint - Add nginx warning tolerance (ssl_stapling warnings no longer fail) - Add location deduplication in config generator to prevent duplicate location "/" errors - Add vhost backup/restore on proxy setup failure instead of deletion - Add WriteVirtualHost and GetVirtualHost to NginxManager interface Fixes issue where adding domains would fail due to ssl_stapling warnings and corrupt vhost config by deleting it on failure. Signed-off-by: nfebe --- internal/api/domains_test.go | 70 ++++++++++++++++++ internal/api/server.go | 9 +++ internal/nginx/manager.go | 29 +++++++- internal/nginx/manager_test.go | 108 ++++++++++++++++++++++++++++ internal/proxy/interfaces.go | 2 + internal/proxy/orchestrator.go | 19 ++++- internal/proxy/orchestrator_test.go | 20 ++++++ 7 files changed, 255 insertions(+), 2 deletions(-) diff --git a/internal/api/domains_test.go b/internal/api/domains_test.go index 72bfbe8..06e9d18 100644 --- a/internal/api/domains_test.go +++ b/internal/api/domains_test.go @@ -381,4 +381,74 @@ func TestAddDomain(t *testing.T) { t.Error("expected domain ID to be generated") } }) + + t.Run("rejects duplicate domain", func(t *testing.T) { + createTestDeployment(t, tmpDir, "duplicate-domain", &models.ServiceMetadata{ + Name: "duplicate-domain", + Type: "web", + Domains: []models.DomainConfig{ + { + ID: "existing-1", + Service: "web", + ContainerPort: 80, + Domain: "existing.example.com", + }, + }, + }) + + duplicateDomain := models.DomainConfig{ + Service: "web", + ContainerPort: 8080, + Domain: "existing.example.com", + } + body, _ := json.Marshal(duplicateDomain) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "name", Value: "duplicate-domain"}} + c.Request = httptest.NewRequest("POST", "/", bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + + server.addDomain(c) + + if w.Code != http.StatusConflict { + t.Errorf("expected status 409 Conflict, got %d: %s", w.Code, w.Body.String()) + } + }) + + t.Run("allows same domain with different path prefix", func(t *testing.T) { + createTestDeployment(t, tmpDir, "different-path", &models.ServiceMetadata{ + Name: "different-path", + Type: "web", + Domains: []models.DomainConfig{ + { + ID: "existing-1", + Service: "api", + ContainerPort: 8080, + Domain: "app.example.com", + PathPrefix: "/api", + }, + }, + }) + + newDomain := models.DomainConfig{ + Service: "web", + ContainerPort: 80, + Domain: "app.example.com", + PathPrefix: "/web", + } + body, _ := json.Marshal(newDomain) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "name", Value: "different-path"}} + c.Request = httptest.NewRequest("POST", "/", bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + + server.addDomain(c) + + if w.Code != http.StatusCreated { + t.Errorf("expected status 201 Created, got %d: %s", w.Code, w.Body.String()) + } + }) } diff --git a/internal/api/server.go b/internal/api/server.go index a4011e2..1ad2501 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -3470,6 +3470,15 @@ func (s *Server) addDomain(c *gin.Context) { deployment.Metadata.Domains = []models.DomainConfig{existingDomain} } + for _, existing := range deployment.Metadata.Domains { + if existing.Domain == domain.Domain && existing.PathPrefix == domain.PathPrefix { + c.JSON(http.StatusConflict, gin.H{ + "error": fmt.Sprintf("Domain %s%s already exists", domain.Domain, domain.PathPrefix), + }) + return + } + } + deployment.Metadata.Domains = append(deployment.Metadata.Domains, domain) if err := s.manager.SaveMetadata(name, deployment.Metadata); err != nil { diff --git a/internal/nginx/manager.go b/internal/nginx/manager.go index ae1e917..6309731 100644 --- a/internal/nginx/manager.go +++ b/internal/nginx/manager.go @@ -3,6 +3,7 @@ package nginx import ( "bytes" "fmt" + "log" "os" "os/exec" "path/filepath" @@ -120,6 +121,14 @@ func (m *Manager) GetVirtualHost(deploymentName string) (string, error) { return string(data), nil } +func (m *Manager) WriteVirtualHost(deploymentName string, content string) error { + m.mu.Lock() + defer m.mu.Unlock() + + configFile := filepath.Join(m.configPath, deploymentName+".conf") + return os.WriteFile(configFile, []byte(content), 0644) +} + func (m *Manager) UpdateVirtualHost(deployment *models.Deployment) error { return m.CreateVirtualHost(deployment) } @@ -229,12 +238,23 @@ func (m *Manager) TestConfig() error { cmd := exec.Command("docker", "exec", m.config.ContainerName, "nginx", "-t") output, err := cmd.CombinedOutput() if err != nil { - return fmt.Errorf("nginx config test failed: %s - %w", string(output), err) + outputStr := string(output) + if isNginxConfigValid(outputStr) { + log.Printf("nginx config test passed with warnings: %s", outputStr) + return nil + } + return fmt.Errorf("nginx config test failed: %s - %w", outputStr, err) } return nil } +func isNginxConfigValid(output string) bool { + hasError := strings.Contains(output, "[emerg]") || strings.Contains(output, "[error]") + hasSuccess := strings.Contains(output, "syntax is ok") || strings.Contains(output, "test is successful") + return !hasError && hasSuccess +} + func (m *Manager) waitForContainerReady(maxRetries int) error { for i := 0; i < maxRetries; i++ { cmd := exec.Command("docker", "inspect", "-f", "{{.State.Status}}", m.config.ContainerName) @@ -433,6 +453,7 @@ func (m *Manager) groupDomainsByHost(domains []models.DomainConfig, deploymentNa }) var locations []locationData + seenPaths := make(map[string]bool) hasSSL := false sslDomain := host var serverAliases []string @@ -443,6 +464,12 @@ func (m *Manager) groupDomainsByHost(domains []models.DomainConfig, deploymentNa path = "/" } + if seenPaths[path] { + log.Printf("warning: skipping duplicate location %q for host %q", path, host) + continue + } + seenPaths[path] = true + service := d.Service if service == "" { service = deploymentName diff --git a/internal/nginx/manager_test.go b/internal/nginx/manager_test.go index 16df67d..f9228da 100644 --- a/internal/nginx/manager_test.go +++ b/internal/nginx/manager_test.go @@ -1719,3 +1719,111 @@ func TestUpdateDeploymentRateLimits(t *testing.T) { t.Error("rate_limits.conf should still contain app2") } } + +func TestWriteVirtualHost(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "nginx-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + m := NewManager(&config.NginxConfig{ + ConfigPath: tmpDir, + }, "/deployments", "") + + content := "# test nginx config\nserver { listen 80; }" + if err := m.WriteVirtualHost("test-app", content); err != nil { + t.Fatalf("WriteVirtualHost failed: %v", err) + } + + configFile := filepath.Join(tmpDir, "test-app.conf") + data, err := os.ReadFile(configFile) + if err != nil { + t.Fatalf("failed to read config file: %v", err) + } + + if string(data) != content { + t.Errorf("expected content %q, got %q", content, string(data)) + } + + readContent, err := m.GetVirtualHost("test-app") + if err != nil { + t.Fatalf("GetVirtualHost failed: %v", err) + } + + if readContent != content { + t.Errorf("GetVirtualHost returned %q, expected %q", readContent, content) + } +} + +func TestIsNginxConfigValid(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + { + name: "valid config no warnings", + output: "nginx: the configuration file /etc/nginx/nginx.conf syntax is ok\nnginx: configuration file /etc/nginx/nginx.conf test is successful", + want: true, + }, + { + name: "valid config with ssl_stapling warning", + output: "2026/02/03 17:33:27 [warn] 2572#2572: \"ssl_stapling\" ignored\nnginx: the configuration file /etc/nginx/nginx.conf syntax is ok\nnginx: configuration file /etc/nginx/nginx.conf test is successful", + want: true, + }, + { + name: "invalid config with emerg error", + output: "nginx: [emerg] unknown directive \"invalid\" in /etc/nginx/conf.d/test.conf:1\nnginx: configuration file /etc/nginx/nginx.conf test failed", + want: false, + }, + { + name: "invalid config with error", + output: "nginx: [error] cannot load certificate\nnginx: configuration file /etc/nginx/nginx.conf test failed", + want: false, + }, + { + name: "no success indicator", + output: "[warn] some warning", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isNginxConfigValid(tt.output) + if got != tt.want { + t.Errorf("isNginxConfigValid() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestGroupDomainsByHost_DeduplicatesLocations(t *testing.T) { + m := NewManager(&config.NginxConfig{}, "/deployments", "") + + domains := []models.DomainConfig{ + {ID: "1", Domain: "example.com", PathPrefix: "", ContainerPort: 80, Service: "web"}, + {ID: "2", Domain: "example.com", PathPrefix: "", ContainerPort: 8080, Service: "api"}, + {ID: "3", Domain: "example.com", PathPrefix: "/api", ContainerPort: 3000, Service: "backend"}, + } + + servers := m.groupDomainsByHost(domains, "test-app") + + if len(servers) != 1 { + t.Fatalf("expected 1 server, got %d", len(servers)) + } + + if len(servers[0].Locations) != 2 { + t.Errorf("expected 2 unique locations, got %d", len(servers[0].Locations)) + } + + pathCounts := make(map[string]int) + for _, loc := range servers[0].Locations { + pathCounts[loc.Path]++ + } + + if pathCounts["/"] != 1 { + t.Errorf("expected exactly 1 location for '/', got %d", pathCounts["/"]) + } +} diff --git a/internal/proxy/interfaces.go b/internal/proxy/interfaces.go index f531d67..ae3d2b3 100644 --- a/internal/proxy/interfaces.go +++ b/internal/proxy/interfaces.go @@ -10,6 +10,8 @@ type NginxManager interface { UpdateVirtualHost(deployment *models.Deployment) error DeleteVirtualHost(deploymentName string) error VirtualHostExists(deploymentName string) bool + GetVirtualHost(deploymentName string) (string, error) + WriteVirtualHost(deploymentName string, content string) error TestConfig() error Reload() error } diff --git a/internal/proxy/orchestrator.go b/internal/proxy/orchestrator.go index fff111d..6164fff 100644 --- a/internal/proxy/orchestrator.go +++ b/internal/proxy/orchestrator.go @@ -88,13 +88,30 @@ func (o *Orchestrator) setupMultiDomainDeployment(deployment *models.Deployment, } deployment.Metadata.Domains = domains + var previousConfig string + hadPreviousConfig := o.nginx.VirtualHostExists(deployment.Name) + if hadPreviousConfig { + var err error + previousConfig, err = o.nginx.GetVirtualHost(deployment.Name) + if err != nil { + log.Printf("warning: failed to backup previous vhost config: %v", err) + hadPreviousConfig = false + } + } + if err := o.nginx.CreateVirtualHost(deployment); err != nil { return nil, fmt.Errorf("failed to create virtual host: %w", err) } result.VirtualHostCreated = true if err := o.nginx.TestConfig(); err != nil { - _ = o.nginx.DeleteVirtualHost(deployment.Name) + if hadPreviousConfig { + if restoreErr := o.nginx.WriteVirtualHost(deployment.Name, previousConfig); restoreErr != nil { + log.Printf("warning: failed to restore previous vhost config: %v", restoreErr) + } + } else { + _ = o.nginx.DeleteVirtualHost(deployment.Name) + } return nil, fmt.Errorf("nginx config validation failed: %w", err) } diff --git a/internal/proxy/orchestrator_test.go b/internal/proxy/orchestrator_test.go index cee00d3..6811af4 100644 --- a/internal/proxy/orchestrator_test.go +++ b/internal/proxy/orchestrator_test.go @@ -12,9 +12,12 @@ type mockNginxManager struct { createVirtualHostCalls []string updateVirtualHostCalls []string deleteVirtualHostCalls []string + getVirtualHostCalls []string + writeVirtualHostCalls []string testConfigCalls int reloadCalls int virtualHostExistsReturns map[string]bool + virtualHostContents map[string]string createVirtualHostErr error testConfigErr error testConfigErrCount int @@ -46,6 +49,23 @@ func (m *mockNginxManager) VirtualHostExists(deploymentName string) bool { return m.virtualHostExistsReturns[deploymentName] } +func (m *mockNginxManager) GetVirtualHost(deploymentName string) (string, error) { + m.getVirtualHostCalls = append(m.getVirtualHostCalls, deploymentName) + if m.virtualHostContents == nil { + return "", nil + } + return m.virtualHostContents[deploymentName], nil +} + +func (m *mockNginxManager) WriteVirtualHost(deploymentName string, content string) error { + m.writeVirtualHostCalls = append(m.writeVirtualHostCalls, deploymentName) + if m.virtualHostContents == nil { + m.virtualHostContents = make(map[string]string) + } + m.virtualHostContents[deploymentName] = content + return nil +} + func (m *mockNginxManager) TestConfig() error { m.testConfigCalls++ if m.testConfigErrCount > 0 && m.testConfigCalls <= m.testConfigErrCount {