Merge pull request #155 from die-van-de-boekhouding/prevent-saving-invalid-state-on-deploy

Prevent saving invalid configuration on deploy

Author: kevinmcconnell

internal/server/router_test.go

@@ -1,8 +1,10 @@
 package server
 
 import (
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -207,7 +209,7 @@ func TestRouter_UpdatingOptions(t *testing.T) {
 	assert.Empty(t, body)
 }
 
-func TestRouter_DeploymmentsWithErrorsDoNotUpdateService(t *testing.T) {
+func TestRouter_DeploymentsWithErrorsDoNotUpdateService(t *testing.T) {
 	router := testRouter(t)
 	_, target := testBackend(t, "first", http.StatusOK)
 
@@ -220,21 +222,43 @@ func TestRouter_DeploymmentsWithErrorsDoNotUpdateService(t *testing.T) {
 	serviceOptions := defaultServiceOptions
 	serviceOptions.Hosts = []string{"example.com"}
 
+	assert.NoFileExists(t, router.statePath)
 	require.NoError(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, serviceOptions, defaultTargetOptions, DefaultDeployTimeout, DefaultDrainTimeout))
 	ensureServiceIsHealthy()
+	require.FileExists(t, router.statePath)
+
+	ensureStateWasNotSaved := func() {
+		f, err := os.OpenFile(router.statePath, os.O_RDONLY, 0600)
+		require.NoError(t, err)
+
+		var services []*Service
+		require.NoError(t, json.NewDecoder(f).Decode(&services), "if this test failed it means an invalid config prevents the system from booting")
+
+		sm := NewServiceMap()
+		for _, service := range services {
+			sm.Set(service)
+		}
+		persistedOptions := sm.Get("service1").options
+		assert.Equal(t, serviceOptions.TLSPrivateKeyPath, persistedOptions.TLSPrivateKeyPath)
+		assert.Equal(t, serviceOptions.TLSCertificatePath, persistedOptions.TLSCertificatePath)
+		assert.Equal(t, serviceOptions.TLSEnabled, persistedOptions.TLSEnabled)
+		assert.Equal(t, serviceOptions.ErrorPagePath, persistedOptions.ErrorPagePath)
+	}
 
 	t.Run("custom TLS that is not valid", func(t *testing.T) {
-		serviceOptions := ServiceOptions{TLSEnabled: true, TLSCertificatePath: "not valid", TLSPrivateKeyPath: "not valid"}
-		require.Error(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, serviceOptions, defaultTargetOptions, DefaultDeployTimeout, DefaultDrainTimeout))
+		newServiceOptions := ServiceOptions{TLSEnabled: true, TLSCertificatePath: "not valid", TLSPrivateKeyPath: "not valid"}
+		require.Error(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, newServiceOptions, defaultTargetOptions, DefaultDeployTimeout, DefaultDrainTimeout))
 
 		ensureServiceIsHealthy()
+		ensureStateWasNotSaved()
 	})
 
 	t.Run("custom error pages that are not valid", func(t *testing.T) {
-		serviceOptions := ServiceOptions{ErrorPagePath: "not valid"}
-		require.Error(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, serviceOptions, defaultTargetOptions, DefaultDeployTimeout, DefaultDrainTimeout))
+		newServiceOptions := ServiceOptions{ErrorPagePath: "not valid"}
+		require.Error(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, newServiceOptions, defaultTargetOptions, DefaultDeployTimeout, DefaultDrainTimeout))
 
 		ensureServiceIsHealthy()
+		ensureStateWasNotSaved()
 	})
 }
 

internal/server/service.go

@@ -134,19 +134,20 @@ type Service struct {
 func NewService(name string, options ServiceOptions, targetOptions TargetOptions) (*Service, error) {
 	service := &Service{
 		name:            name,
-		options:         options,
 		targetOptions:   targetOptions,
 		pauseController: NewPauseController(),
 	}
 
-	return service, service.initialize()
+	if err := service.initialize(options); err != nil {
+		return nil, err
+	}
+	return service, nil
 }
 
 func (s *Service) UpdateOptions(options ServiceOptions, targetOptions TargetOptions) error {
-	s.options = options
 	s.targetOptions = targetOptions
 
-	return s.initialize()
+	return s.initialize(options)
 }
 
 func (s *Service) Dispose() {
@@ -265,26 +266,25 @@ func (s *Service) UnmarshalJSON(data []byte) error {
 	s.name = ms.Name
 	s.pauseController = ms.PauseController
 	s.rolloutController = ms.RolloutController
-	s.options = ms.Options
 	s.targetOptions = ms.TargetOptions
 
 	activeTargets, err := NewTargetList(ms.ActiveTargets, ms.ActiveReaders, ms.TargetOptions)
 	if err != nil {
 		return err
 	}
-	s.active = NewLoadBalancer(activeTargets, s.options.WriterAffinityTimeout, s.options.ReadTargetsAcceptWebsockets)
+	s.active = NewLoadBalancer(activeTargets, ms.Options.WriterAffinityTimeout, ms.Options.ReadTargetsAcceptWebsockets)
 	s.active.MarkAllHealthy()
 
 	rolloutTargets, err := NewTargetList(ms.RolloutTargets, ms.RolloutReaders, ms.TargetOptions)
 	if err != nil {
 		return err
 	}
 	if len(rolloutTargets) > 0 {
-		s.rollout = NewLoadBalancer(rolloutTargets, s.options.WriterAffinityTimeout, s.options.ReadTargetsAcceptWebsockets)
+		s.rollout = NewLoadBalancer(rolloutTargets, ms.Options.WriterAffinityTimeout, ms.Options.ReadTargetsAcceptWebsockets)
 		s.rollout.MarkAllHealthy()
 	}
 
-	return s.initialize()
+	return s.initialize(ms.Options)
 }
 
 func (s *Service) Stop(drainTimeout time.Duration, message string) error {
@@ -325,17 +325,18 @@ func (s *Service) Resume() error {
 
 // Private
 
-func (s *Service) initialize() error {
-	certManager, err := s.createCertManager(s.options)
+func (s *Service) initialize(options ServiceOptions) error {
+	certManager, err := s.createCertManager(options)
 	if err != nil {
 		return err
 	}
 
-	middleware, err := s.createMiddleware(s.options, certManager)
+	middleware, err := s.createMiddleware(options, certManager)
 	if err != nil {
 		return err
 	}
 
+	s.options = options
 	s.certManager = certManager
 	s.middleware = middleware