ENH: Better handling of proxy.Instance

Author: thomasjpfan

actions/reconfigure.go

@@ -49,10 +49,9 @@ var NewReconfigure = func(baseData BaseReconfigure, serviceData proxy.Service) R
 func (m *Reconfigure) Execute(reloadAfter bool) error {
 	if strings.EqualFold(os.Getenv("FILTER_PROXY_INSTANCE_NAME"), "true") &&
 		!strings.EqualFold(m.InstanceName, m.Service.ProxyInstanceName) {
+		logPrintf("Filtering %s configuration, with proxyInstanceName: %s", m.ServiceName, m.Service.ProxyInstanceName)
 		return nil
 	}
-	mu.Lock()
-	defer mu.Unlock()
 	if strings.EqualFold(os.Getenv("SKIP_ADDRESS_VALIDATION"), "false") {
 		host := m.ServiceName
 		if len(m.ServiceDest) > 0 && len(m.ServiceDest[0].OutboundHostname) > 0 {
@@ -63,12 +62,9 @@ func (m *Reconfigure) Execute(reloadAfter bool) error {
 			return err
 		}
 	}
-	if err := m.createConfigs(); err != nil {
+	if err := m.createConfigsAddService(); err != nil {
 		return err
 	}
-	if !m.hasTemplate() {
-		proxy.Instance.AddService(m.Service)
-	}
 	if reloadAfter {
 		reload := reload{}
 		if err := reload.Execute(true); err != nil {
@@ -87,6 +83,19 @@ func (m *Reconfigure) Execute(reloadAfter bool) error {
 	return nil
 }
 
+func (m *Reconfigure) createConfigsAddService() error {
+	configProxyMu.Lock()
+	defer configProxyMu.Unlock()
+
+	if err := m.createConfigs(); err != nil {
+		return err
+	}
+	if !m.hasTemplate() {
+		proxy.Instance.AddService(m.Service)
+	}
+	return nil
+}
+
 // GetData returns structure with reconfiguration data and the service
 func (m *Reconfigure) GetData() (BaseReconfigure, proxy.Service) {
 	return m.BaseReconfigure, m.Service

actions/reconfigure_test.go

@@ -1260,8 +1260,9 @@ func (m *ProxyMock) AddService(service proxy.Service) {
 	m.Called(service)
 }
 
-func (m *ProxyMock) RemoveService(service string) {
-	m.Called(service)
+func (m *ProxyMock) RemoveService(service string) bool {
+	params := m.Called(service)
+	return params.Bool(0)
 }
 
 func (m *ProxyMock) GetServices() map[string]proxy.Service {
@@ -1298,7 +1299,7 @@ func getProxyMock(skipMethod string) *ProxyMock {
 		mockObj.On("AddService", mock.Anything)
 	}
 	if skipMethod != "RemoveService" {
-		mockObj.On("RemoveService", mock.Anything)
+		mockObj.On("RemoveService", mock.Anything).Return(true)
 	}
 	if skipMethod != "GetServices" {
 		mockObj.On("GetServices")

actions/remove.go

@@ -1,8 +1,9 @@
 package actions
 
 import (
-	"../proxy"
 	"fmt"
+
+	"../proxy"
 )
 
 // Removable defines functions that must be implemented by any struct in charge of removing services from the proxy.
@@ -33,11 +34,14 @@ var NewRemove = func(serviceName, aclName, configsPath, templatesPath string, in
 // Execute initiates the removal of a service
 func (m *Remove) Execute(args []string) error {
 	logPrintf("Removing %s configuration", m.ServiceName)
-	if err := m.removeFiles(m.TemplatesPath, m.ServiceName, m.AclName); err != nil {
-		logPrintf(err.Error())
+	didRemove, err := m.removeConfigsAndService()
+	if err != nil {
 		return err
 	}
-	proxy.Instance.RemoveService(m.ServiceName)
+	if !didRemove {
+		logPrintf("%s was not configured, no reload required", m.ServiceName)
+		return nil
+	}
 	reload := reload{}
 	if err := reload.Execute(true); err != nil {
 		logPrintf(err.Error())
@@ -46,6 +50,21 @@ func (m *Remove) Execute(args []string) error {
 	return nil
 }
 
+func (m *Remove) removeConfigsAndService() (bool, error) {
+	configProxyMu.Lock()
+	defer configProxyMu.Unlock()
+	didRemove := proxy.Instance.RemoveService(m.ServiceName)
+	if !didRemove {
+		return didRemove, nil
+	}
+
+	if err := m.removeFiles(m.TemplatesPath, m.ServiceName, m.AclName); err != nil {
+		logPrintf(err.Error())
+		return false, err
+	}
+	return didRemove, nil
+}
+
 func (m *Remove) removeFiles(templatesPath, serviceName, aclName string) error {
 	logPrintf("Removing the %s configuration files", serviceName)
 	if len(aclName) == 0 {
@@ -55,8 +74,6 @@ func (m *Remove) removeFiles(templatesPath, serviceName, aclName string) error {
 		fmt.Sprintf("%s/%s-fe.cfg", templatesPath, aclName),
 		fmt.Sprintf("%s/%s-be.cfg", templatesPath, aclName),
 	}
-	//	mu.Lock()
-	//	defer mu.Unlock()
 	for _, path := range paths {
 		osRemove(path)
 	}

actions/remove_test.go

@@ -3,11 +3,12 @@
 package actions
 
 import (
-	"../proxy"
 	"fmt"
+	"testing"
+
+	"../proxy"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/suite"
-	"testing"
 )
 
 type RemoveTestSuite struct {
@@ -112,6 +113,18 @@ func (s RemoveTestSuite) Test_Execute_Invokes_HaProxyReload() {
 	mockObj.AssertCalled(s.T(), "Reload")
 }
 
+func (s RemoveTestSuite) Test_Execute_If_RemoveService_Does_Not_Invokes_HaProxyReload() {
+	proxyOrig := proxy.Instance
+	defer func() { proxy.Instance = proxyOrig }()
+	mockObj := getProxyMock("RemoveService")
+	mockObj.On("RemoveService", mock.Anything).Return(false)
+	proxy.Instance = mockObj
+
+	s.remove.Execute([]string{})
+
+	mockObj.AssertNotCalled(s.T(), "Reload")
+}
+
 func (s RemoveTestSuite) Test_Execute_ReturnsError_WhenHaProxyReloadFails() {
 	proxyOrig := proxy.Instance
 	defer func() { proxy.Instance = proxyOrig }()

actions/util.go

@@ -6,6 +6,7 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"sync"
 )
 
 type executable interface {
@@ -19,3 +20,5 @@ var writeFeTemplate = ioutil.WriteFile
 var writeBeTemplate = ioutil.WriteFile
 var readTemplateFile = ioutil.ReadFile
 var osRemove = os.Remove
+
+var configProxyMu = &sync.Mutex{}

proxy/ha_proxy.go

@@ -201,8 +201,14 @@ func (m HaProxy) AddService(service Service) {
 }
 
 // RemoveService deletes a service from the `dataInstance` map using `ServiceName` as the key
-func (m HaProxy) RemoveService(service string) {
+// Returns false if there are no services to remove
+func (m HaProxy) RemoveService(service string) bool {
+	_, ok := dataInstance.Services[service]
+	if !ok {
+		return false
+	}
 	delete(dataInstance.Services, service)
+	return true
 }
 
 // GetServices returns a map with all the services used by the proxy.

proxy/ha_proxy_test.go

@@ -2890,11 +2890,25 @@ func (s *HaProxyTestSuite) Test_AddService_RemovesService() {
 
 	p.AddService(s1)
 	p.AddService(s2)
-	p.RemoveService("my-service-1")
+	didRemove := p.RemoveService("my-service-1")
 
+	s.True(didRemove)
 	s.Len(dataInstance.Services, 1)
 }
 
+func (s *HaProxyTestSuite) Test_RemovesService_Does_Not_Exist() {
+	s1 := Service{ServiceName: "my-service-1"}
+	s2 := Service{ServiceName: "my-service-2"}
+	p := NewHaProxy("anything", "doesn't").(HaProxy)
+
+	p.AddService(s1)
+	p.AddService(s2)
+	didRemove := p.RemoveService("my-service-3")
+
+	s.False(didRemove)
+	s.Len(dataInstance.Services, 2)
+}
+
 // Util
 
 func (s *HaProxyTestSuite) getTemplateWithLogs() string {

proxy/proxy.go

@@ -17,6 +17,6 @@ type proxy interface {
 	GetCertPaths() []string
 	GetCerts() map[string]string
 	AddService(service Service)
-	RemoveService(service string)
+	RemoveService(service string) bool
 	GetServices() map[string]Service
 }

server/cert_test.go

@@ -719,8 +719,9 @@ func (m *ProxyMock) AddService(service proxy.Service) {
 	m.Called(service)
 }
 
-func (m *ProxyMock) RemoveService(service string) {
-	m.Called(service)
+func (m *ProxyMock) RemoveService(service string) bool {
+	params := m.Called(service)
+	return params.Bool(0)
 }
 
 func (m *ProxyMock) GetServices() map[string]proxy.Service {
@@ -757,7 +758,7 @@ func getProxyMock(skipMethod string) *ProxyMock {
 		mockObj.On("AddService", mock.Anything)
 	}
 	if skipMethod != "RemoveService" {
-		mockObj.On("RemoveService", mock.Anything)
+		mockObj.On("RemoveService", mock.Anything).Return(true)
 	}
 	if skipMethod != "GetServices" {
 		mockObj.On("GetServices").Return(map[string]proxy.Service{})