Feature/advanced error handling

pkg/controller/endpointmonitor/endpointmonitor_controller.go

@@ -117,10 +117,17 @@ func (r *ReconcileEndpointMonitor) Reconcile(request reconcile.Request) (reconci
 	delay := time.Until(createTime.Add(config.GetControllerConfig().CreationDelay))
 
 	for index := 0; index < len(r.monitorServices); index++ {
-		monitor := findMonitorByName(r.monitorServices[index], monitorName)
+		monitor, err := findMonitorByName(r.monitorServices[index], monitorName)
+		// if there was some error while getting a monitor, re-queue it to try on the next reconcile iteration
+		if err != nil {
+			return reconcile.Result{RequeueAfter: RequeueTime}, err
+		}
 		if monitor != nil {
 			// Monitor already exists, update if required
 			err = r.handleUpdate(request, instance, *monitor, r.monitorServices[index])
+			if err != nil {
+				log.Errorf("Error while handling update: %s", err)
+			}
 		} else {
 			// Monitor doesn't exist, create monitor
 			if delay.Nanoseconds() > 0 {
@@ -129,18 +136,16 @@ func (r *ReconcileEndpointMonitor) Reconcile(request reconcile.Request) (reconci
 				return reconcile.Result{RequeueAfter: delay}, nil
 			}
 			err = r.handleCreate(request, instance, monitorName, r.monitorServices[index])
+			if err != nil {
+				log.Errorf("Error while handling create: %s", err)
+			}
 		}
 	}
 
 	return reconcile.Result{RequeueAfter: RequeueTime}, err
 }
 
-func findMonitorByName(monitorService monitors.MonitorServiceProxy, monitorName string) *models.Monitor {
-
-	monitor, _ := monitorService.GetByName(monitorName)
-	// Monitor Exists
-	if monitor != nil {
-		return monitor
-	}
-	return nil
+func findMonitorByName(monitorService monitors.MonitorServiceProxy, monitorName string) (*models.Monitor, error) {
+	monitor, err := monitorService.GetByName(monitorName)
+	return monitor, err
 }

pkg/controller/endpointmonitor/endpointmonitor_controller_test.go

@@ -123,8 +123,8 @@ func TestEndpointMonitorReconcileUpdate(t *testing.T) {
 
 	monitorCount = 0
 	for index := 0; index < len(r.monitorServices); index++ {
-		monitor := findMonitorByName(r.monitorServices[index], monitorName)
-		if monitor != nil && monitor.URL == testURLFacebook {
+		monitor, err := findMonitorByName(r.monitorServices[index], monitorName)
+		if monitor != nil && err == nil && monitor.URL == testURLFacebook {
 			log.Info("Found Updated Monitor for Provider: " + r.monitorServices[index].GetType())
 			monitorCount++
 		}
@@ -185,7 +185,10 @@ func TestEndpointMonitorReconcileDelete(t *testing.T) {
 
 	monitorCount = 0
 	for index := 0; index < len(r.monitorServices); index++ {
-		monitor := findMonitorByName(r.monitorServices[index], monitorName)
+		monitor, err := findMonitorByName(r.monitorServices[index], monitorName)
+		if err != nil {
+			t.Error(err, "Could not findMonitorByName")
+		}
 		if monitor == nil {
 			monitorCount++
 		}
@@ -232,7 +235,10 @@ func setupReconcilerAndCreateResource() (client.Client, *ReconcileEndpointMonito
 func getMonitorCount(monitorName string, monitorServices []monitors.MonitorServiceProxy) int {
 	monitorCount := 0
 	for index := 0; index < len(monitorServices); index++ {
-		monitor := findMonitorByName(monitorServices[index], monitorName)
+		monitor, err := findMonitorByName(monitorServices[index], monitorName)
+		if err != nil {
+			log.Error(err, "Could not findMonitorByName")
+		}
 		if monitor != nil {
 			log.Info("Found Monitor for Provider: " + monitorServices[index].GetType())
 			monitorCount++

pkg/monitors/appinsights/appinsights-monitor.go

@@ -189,8 +189,10 @@ func (aiService *AppinsightsMonitorService) GetByName(monitorName string) (*mode
 	log.Println("AppInsights Monitor's GetByName method has been called")
 	webtest, err := aiService.insightsClient.Get(aiService.ctx, aiService.resourceGroup, monitorName)
 	if err != nil {
+		// if not found let's mark it as non-error
 		if webtest.Response.StatusCode == http.StatusNotFound {
-			return nil, fmt.Errorf("Application Insights WebTest %s was not found in Resource Group %s", monitorName, aiService.resourceGroup)
+			log.Printf("Application Insights WebTest %s was not found in Resource Group %s", monitorName, aiService.resourceGroup)
+			return nil, nil
 		}
 		return nil, fmt.Errorf("Error retrieving Application Insights WebTests %s (Resource Group %s): %v", monitorName, aiService.resourceGroup, err)
 	}

pkg/monitors/gcloud/gcloud-monitor.go

@@ -60,8 +60,9 @@ func (service *MonitorService) GetByName(name string) (monitor *models.Monitor,
 			return &localMonitor, nil
 		}
 	}
-
-	return nil, fmt.Errorf("Unable to locate monitor with name %v", name)
+	// monitor not found and no errors raised
+	log.Printf("There is no gcloud MonitorService with name %s found.", name)
+	return nil, nil
 }
 
 func (service *MonitorService) GetAll() (monitors []models.Monitor) {

pkg/monitors/pingdom/pingdom-monitor.go

@@ -27,7 +27,7 @@ type PingdomMonitorService struct {
 	client            *pingdom.Client
 }
 
-func (monitor *PingdomMonitorService) Equal(oldMonitor models.Monitor, newMonitor models.Monitor) bool {
+func (service *PingdomMonitorService) Equal(oldMonitor models.Monitor, newMonitor models.Monitor) bool {
 	// TODO: Retrieve oldMonitor config and compare it here
 	return false
 }
@@ -45,21 +45,23 @@ func (service *PingdomMonitorService) Setup(p config.Provider) {
 		BaseURL:  service.url,
 	})
 	if err != nil {
-		log.Println("Error Seting Up Monitor Service: ", err.Error())
+		log.Println("Error Setting Up Monitor Service: ", err.Error())
 	}
 }
 
 func (service *PingdomMonitorService) GetByName(name string) (*models.Monitor, error) {
-	var match *models.Monitor
-
-	monitors := service.GetAll()
+	monitors, err := service.GetAllWithAPIErrorHandler()
+	if err != nil {
+		return nil, fmt.Errorf("error received while getting all Pingdom monitors: %s", err.Error())
+	}
 	for _, mon := range monitors {
 		if mon.Name == name {
 			return &mon, nil
 		}
 	}
-
-	return match, fmt.Errorf("Unable to locate monitor with name %v", name)
+	// neither error and any matched monitor
+	log.Printf("There is no PingdomMonitorService with name %s found.", name)
+	return nil, nil
 }
 
 func (service *PingdomMonitorService) GetAll() []models.Monitor {
@@ -82,6 +84,26 @@ func (service *PingdomMonitorService) GetAll() []models.Monitor {
 	return monitors
 }
 
+// Function that gets all monitors with an error handling to avoid duplicate checks creation
+func (service *PingdomMonitorService) GetAllWithAPIErrorHandler() ([]models.Monitor, error) {
+	var monitors []models.Monitor
+
+	checks, err := service.client.Checks.List()
+	if err != nil {
+		return nil, err
+	}
+	for _, mon := range checks {
+		newMon := models.Monitor{
+			URL:  mon.Hostname,
+			ID:   fmt.Sprintf("%v", mon.ID),
+			Name: mon.Name,
+		}
+		monitors = append(monitors, newMon)
+	}
+
+	return monitors, nil
+}
+
 func (service *PingdomMonitorService) Add(m models.Monitor) {
 	httpCheck := service.createHttpCheck(m)
 

pkg/monitors/pingdom/pingdom-monitor_test.go

@@ -2,7 +2,9 @@ package pingdom
 
 import (
 	log "github.com/sirupsen/logrus"
+	"net/url"
 	"testing"
+	"time"
 
 	"github.com/stakater/IngressMonitorController/pkg/config"
 	"github.com/stakater/IngressMonitorController/pkg/models"
@@ -21,22 +23,33 @@ func TestAddMonitorWithCorrectValues(t *testing.T) {
 		return
 	}
 	service.Setup(*provider)
-	m := models.Monitor{Name: "google-test", URL: "https://google1.com"}
+	urlToTest := "https://google1.com"
+	parsedUrl, err := url.Parse(urlToTest)
+	if err != nil {
+		t.Errorf("Error: %s", err)
+	}
+	m := models.Monitor{Name: "google-test", URL: urlToTest}
 	service.Add(m)
-
+	//Creation delay
+	time.Sleep(5 * time.Second)
 	mRes, err := service.GetByName("google-test")
 
 	if err != nil {
 		t.Error("Error: " + err.Error())
 	}
-	if mRes.Name != m.Name || mRes.URL != m.URL {
-		t.Error("URL and name should be the same")
+	// mRes.URL is a domain name, without scheme, so we parsed URL previously
+	if mRes.Name != m.Name || mRes.URL != parsedUrl.Host {
+		t.Errorf("URL and name should be the same %+v", mRes)
 	}
 	service.Remove(*mRes)
+	//Deletion delay
+	time.Sleep(5 * time.Second)
 	monitor, err := service.GetByName(mRes.Name)
-
+	if err != nil {
+		t.Error("Error: " + err.Error())
+	}
 	if monitor != nil {
-		t.Error("Monitor should've been deleted ", monitor, err)
+		t.Error("Monitor should've been deleted ", monitor)
 	}
 }
 
@@ -53,16 +66,21 @@ func TestUpdateMonitorWithCorrectValues(t *testing.T) {
 		return
 	}
 	service.Setup(*provider)
-
-	m := models.Monitor{Name: "google-test", URL: "https://google.com"}
+	urlToTest := "https://google.com"
+	parsedUrl, err := url.Parse(urlToTest)
+	if err != nil {
+		t.Errorf("Error: %s", err)
+	}
+	m := models.Monitor{Name: "google-test", URL: urlToTest}
 	service.Add(m)
-
+	//Creation delay
+	time.Sleep(5 * time.Second)
 	mRes, err := service.GetByName("google-test")
 
 	if err != nil {
 		t.Error("Error: " + err.Error())
 	}
-	if mRes.Name != m.Name || mRes.URL != m.URL {
+	if mRes.Name != m.Name || mRes.URL != parsedUrl.Host {
 		t.Error("URL and name should be the same")
 	}
 
@@ -75,15 +93,17 @@ func TestUpdateMonitorWithCorrectValues(t *testing.T) {
 	if err != nil {
 		t.Error("Error: " + err.Error())
 	}
-	if mRes.URL != "https://facebook.com" {
+	if mRes.URL != "facebook.com" {
 		t.Error("URL and name should be the same")
 	}
 
 	service.Remove(*mRes)
 
 	monitor, err := service.GetByName(mRes.Name)
-
+	if err != nil {
+		t.Error("Error: " + err.Error())
+	}
 	if monitor != nil {
-		t.Error("Monitor should've been deleted ", monitor, err)
+		t.Error("Monitor should've been deleted ", monitor)
 	}
 }

pkg/monitors/statuscake/statuscake-monitor.go

@@ -3,7 +3,6 @@ package statuscake
 import (
 	"bytes"
 	"encoding/json"
-	"errors"
 	"io/ioutil"
 	"net/http"
 	"net/url"
@@ -202,8 +201,8 @@ func (service *StatusCakeMonitorService) GetByName(name string) (*models.Monitor
 			return &monitor, nil
 		}
 	}
-	errorString := "GetByName Request failed for name: " + name
-	return nil, errors.New(errorString)
+	log.Printf("There is no StatusCakeMonitorService with name %s found.", name)
+	return nil, nil
 }
 
 // GetAll function will fetch all monitors

pkg/monitors/statuscake/statuscake-monitor_test.go

@@ -38,8 +38,11 @@ func TestAddMonitorWithCorrectValues(t *testing.T) {
 
 	monitor, err := service.GetByName(mRes.Name)
 
+	if err != nil {
+		t.Error("Error: " + err.Error())
+	}
 	if monitor != nil {
-		t.Error("Monitor should've been deleted ", monitor, err)
+		t.Error("Monitor should've been deleted ", monitor)
 	}
 }
 
@@ -83,9 +86,11 @@ func TestUpdateMonitorWithCorrectValues(t *testing.T) {
 	service.Remove(*mRes)
 
 	monitor, err := service.GetByName(mRes.Name)
-
+	if err != nil {
+		t.Error("Error: " + err.Error())
+	}
 	if monitor != nil {
-		t.Error("Monitor should've been deleted ", monitor, err)
+		t.Error("Monitor should've been deleted ", monitor)
 	}
 }
 

pkg/monitors/updown/updown-monitor.go

@@ -2,7 +2,6 @@
 package updown
 
 import (
-	"fmt"
 	"net/http"
 	"net/url"
 
@@ -94,8 +93,8 @@ func (updownService *UpdownMonitorService) GetByName(monitorName string) (*model
 			return &updownMonitor, nil
 		}
 	}
-
-	return nil, fmt.Errorf("Unable to locate %v monitor", monitorName)
+	log.Printf("There is no UpdownMonitorService with name %s found.", monitorName)
+	return nil, nil
 }
 
 // Add function method will add a monitor (updown check)

pkg/monitors/uptime/uptime-monitor.go

@@ -2,7 +2,6 @@ package uptime
 
 import (
 	"encoding/json"
-	"errors"
 	"strconv"
 	"strings"
 
@@ -44,9 +43,8 @@ func (monitor *UpTimeMonitorService) GetByName(name string) (*models.Monitor, er
 		}
 	}
 
-	errorString := name + " not found"
-	log.Println(errorString)
-	return nil, errors.New(errorString)
+	log.Printf("There is no UpTimeMonitorService with name %s found.", name)
+	return nil, nil
 }
 
 func (monitor *UpTimeMonitorService) GetAll() []models.Monitor {

pkg/monitors/uptime/uptime-monitor_test.go

@@ -163,9 +163,12 @@ func TestAddMonitorWithIncorrectValues(t *testing.T) {
 
 	service.Add(m)
 
-	_, err := service.GetByName("google-test")
+	monitor, err := service.GetByName("google-test")
 
-	if err == nil {
+	if err != nil {
+		t.Error("Error: " + err.Error())
+	}
+	if monitor != nil {
 		t.Error("google-test should not have existed")
 	}
 }