replace calls to panic, have functions return error value

README.md

@@ -384,7 +384,6 @@ Rules:
 * The lambda function does not handle OverSizedChangeNotification
 * The lambda function name is hard-coded in the Makefile
 * Region is hard-coded to us-east-1 for GetValueFromS3
-* Replace calls to panic with better error reporting
 * Invoke should be a POST, not a GET, and it should probably include the payload
 * Use type switch as more idiomatic way to handle multiple types in match.go
 * Start using go testing coverage tools

assertion/assertion.go

@@ -4,49 +4,61 @@ import (
 	"fmt"
 )
 
-func searchAndMatch(assertion Assertion, resource Resource, log LoggingFunction) bool {
+func searchAndMatch(assertion Assertion, resource Resource, log LoggingFunction) (bool, error) {
 	v, err := SearchData(assertion.Key, resource.Properties)
 	if err != nil {
-		panic(err)
+		return false, err
 	}
-	match := isMatch(v, assertion.Op, assertion.Value)
+	match, err := isMatch(v, assertion.Op, assertion.Value)
 	log(fmt.Sprintf("Key: %s Output: %s Looking for %s %s", assertion.Key, v, assertion.Op, assertion.Value))
 	log(fmt.Sprintf("ResourceID: %s Type: %s %t",
 		resource.ID,
 		resource.Type,
 		match))
-	return match
+	return match, err
 }
 
-func orOperation(assertions []Assertion, resource Resource, log LoggingFunction) bool {
+func orOperation(assertions []Assertion, resource Resource, log LoggingFunction) (bool, error) {
 	for _, childAssertion := range assertions {
-		if booleanOperation(childAssertion, resource, log) {
-			return true
+		b, err := booleanOperation(childAssertion, resource, log)
+		if err != nil {
+			return b, err
+		}
+		if b {
+			return true, nil
 		}
 	}
-	return false
+	return false, nil
 }
 
-func andOperation(assertions []Assertion, resource Resource, log LoggingFunction) bool {
+func andOperation(assertions []Assertion, resource Resource, log LoggingFunction) (bool, error) {
 	for _, childAssertion := range assertions {
-		if !booleanOperation(childAssertion, resource, log) {
-			return false
+		b, err := booleanOperation(childAssertion, resource, log)
+		if err != nil {
+			return b, err
+		}
+		if !b {
+			return false, nil
 		}
 	}
-	return true
+	return true, nil
 }
 
-func notOperation(assertions []Assertion, resource Resource, log LoggingFunction) bool {
+func notOperation(assertions []Assertion, resource Resource, log LoggingFunction) (bool, error) {
 	// more than one child filter treated as not any
 	for _, childAssertion := range assertions {
-		if booleanOperation(childAssertion, resource, log) {
-			return false
+		b, err := booleanOperation(childAssertion, resource, log)
+		if err != nil {
+			return false, err
+		}
+		if b {
+			return false, nil
 		}
 	}
-	return true
+	return true, nil
 }
 
-func booleanOperation(assertion Assertion, resource Resource, log LoggingFunction) bool {
+func booleanOperation(assertion Assertion, resource Resource, log LoggingFunction) (bool, error) {
 	if assertion.Or != nil && len(assertion.Or) > 0 {
 		return orOperation(assertion.Or, resource, log)
 	}
@@ -84,10 +96,14 @@ func FilterResourceExceptions(rule Rule, resources []Resource) []Resource {
 }
 
 // CheckAssertion validates a single Resource using a single Assertion
-func CheckAssertion(rule Rule, assertion Assertion, resource Resource, log LoggingFunction) string {
+func CheckAssertion(rule Rule, assertion Assertion, resource Resource, log LoggingFunction) (string, error) {
 	status := "OK"
-	if !booleanOperation(assertion, resource, log) {
+	b, err := booleanOperation(assertion, resource, log)
+	if err != nil {
+		return "FAILURE", err
+	}
+	if !b {
 		status = rule.Severity
 	}
-	return status
+	return status, nil
 }

assertion/assertion_test.go

@@ -8,7 +8,13 @@ import (
 func testLogging(s string) {
 }
 
-func testsimple(t *testing.T) {
+func failTestIfError(err error, message string, t *testing.T) {
+	if err != nil {
+		t.Error(message + ":" + err.Error())
+	}
+}
+
+func Testsimple(t *testing.T) {
 	rule := Rule{
 		ID:       "test1",
 		Message:  "test rule",
@@ -29,7 +35,8 @@ func testsimple(t *testing.T) {
 		Properties: map[string]interface{}{"instance_type": "t2.micro"},
 		Filename:   "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestSimple", t)
 	if status != "OK" {
 		t.Error("Expecting simple rule to match")
 	}
@@ -66,7 +73,8 @@ func TestOrToMatch(t *testing.T) {
 		Properties: map[string]interface{}{"instance_type": "t2.micro"},
 		Filename:   "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestOrToMatch", t)
 	if status != "OK" {
 		t.Error("Expecting or to return OK")
 	}
@@ -103,7 +111,8 @@ func TestOrToNotMatch(t *testing.T) {
 		Properties: map[string]interface{}{"instance_type": "m3.medium"},
 		Filename:   "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestOrToNotMatch", t)
 	if status != "FAILURE" {
 		t.Error("Expecting or to return FAILURE")
 	}
@@ -143,7 +152,8 @@ func TestAndToMatch(t *testing.T) {
 		},
 		Filename: "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestAndToMatch", t)
 	if status != "OK" {
 		t.Error("Expecting and to return OK")
 	}
@@ -183,7 +193,8 @@ func TestAndToNotMatch(t *testing.T) {
 		},
 		Filename: "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestAndToNotMatch", t)
 	if status != "FAILURE" {
 		t.Error("Expecting and to return FAILURE")
 	}
@@ -216,7 +227,8 @@ func TestNotToMatch(t *testing.T) {
 		},
 		Filename: "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestNotToMatch", t)
 	if status != "OK" {
 		t.Error("Expecting no to return OK")
 	}
@@ -249,7 +261,8 @@ func TestNotToNotMatch(t *testing.T) {
 		},
 		Filename: "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestNotToNotMatch", t)
 	if status != "FAILURE" {
 		t.Error("Expecting no to return FAILURE")
 	}
@@ -292,7 +305,8 @@ func TestNestedNot(t *testing.T) {
 		},
 		Filename: "test.tf",
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestNestedNot", t)
 	if status != "FAILURE" {
 		t.Error("Expecting nested boolean to return FAILURE")
 	}
@@ -356,7 +370,8 @@ func TestNestedBooleans(t *testing.T) {
 	if err != nil {
 		t.Error("Error parsing resource JSON")
 	}
-	status := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	status, err := CheckAssertion(rule, rule.Assertions[0], resource, testLogging)
+	failTestIfError(err, "TestNestedBoolean", t)
 	if status != "NOT_COMPLIANT" {
 		t.Error("Expecting nested boolean to return NOT_COMPLIANT")
 	}

assertion/engine.go

@@ -19,7 +19,7 @@ type ValueFrom struct {
 
 // ValueSource interface to fetch values
 type ValueSource interface {
-	GetValue(Assertion) string
+	GetValue(Assertion) (string, error)
 }
 
 // InvokeRuleAPI describes parameters for calling an external API
@@ -75,5 +75,5 @@ type Resource struct {
 
 // ExternalRuleInvoker defines an interface for invoking an external API
 type ExternalRuleInvoker interface {
-	Invoke(Rule, Resource) (string, []Violation)
+	Invoke(Rule, Resource) (string, []Violation, error)
 }

assertion/invoke.go

@@ -23,14 +23,14 @@ type StandardExternalRuleInvoker struct {
 }
 
 // Invoke an external API to validate a Resource
-func (e StandardExternalRuleInvoker) Invoke(rule Rule, resource Resource) (string, []Violation) {
+func (e StandardExternalRuleInvoker) Invoke(rule Rule, resource Resource) (string, []Violation, error) {
 	status := "OK"
 	violations := make([]Violation, 0)
 	payload := resource.Properties
 	if rule.Invoke.Payload != "" {
 		p, err := SearchData(rule.Invoke.Payload, resource.Properties)
 		if err != nil {
-			panic(err)
+			return status, violations, err
 		}
 		payload = p
 	}
@@ -48,7 +48,7 @@ func (e StandardExternalRuleInvoker) Invoke(rule Rule, resource Resource) (strin
 				Message:      fmt.Sprintf("Invoke failed: %s", err.Error()),
 			},
 		}
-		return rule.Severity, violations
+		return rule.Severity, violations, err
 	}
 	if httpResponse.StatusCode != 200 {
 		violations := []Violation{
@@ -61,7 +61,7 @@ func (e StandardExternalRuleInvoker) Invoke(rule Rule, resource Resource) (strin
 				Message:      fmt.Sprintf("Invoke failed, StatusCode: %d", httpResponse.StatusCode),
 			},
 		}
-		return rule.Severity, violations
+		return rule.Severity, violations, nil
 	}
 	defer httpResponse.Body.Close()
 	body, err := ioutil.ReadAll(httpResponse.Body)
@@ -76,7 +76,7 @@ func (e StandardExternalRuleInvoker) Invoke(rule Rule, resource Resource) (strin
 				Message:      "Invoke response cannot be read",
 			},
 		}
-		return rule.Severity, violations
+		return rule.Severity, violations, nil
 	}
 	e.Log(string(body))
 	var invokeResponse InvokeResponse
@@ -92,7 +92,7 @@ func (e StandardExternalRuleInvoker) Invoke(rule Rule, resource Resource) (strin
 				Message:      "Invoke response cannot be parsed",
 			},
 		}
-		return rule.Severity, violations
+		return rule.Severity, violations, nil
 	}
 	for _, violation := range invokeResponse.Violations {
 		status = rule.Severity
@@ -105,5 +105,5 @@ func (e StandardExternalRuleInvoker) Invoke(rule Rule, resource Resource) (strin
 			Message:      violation.Message,
 		})
 	}
-	return status, violations
+	return status, violations, nil
 }

assertion/match.go

@@ -29,88 +29,88 @@ func isObject(data interface{}) bool {
 	return ok
 }
 
-func isMatch(data interface{}, op string, value string) bool {
+func isMatch(data interface{}, op string, value string) (bool, error) {
 	searchResult, err := JSONStringify(data)
 	if err != nil {
-		panic(err)
+		return false, err
 	}
 	searchResult = unquoted(searchResult)
 	switch op {
 	case "eq":
 		if searchResult == value {
-			return true
+			return true, nil
 		}
 	case "ne":
 		if searchResult != value {
-			return true
+			return true, nil
 		}
 	case "lt":
 		if searchResult < value {
-			return true
+			return true, nil
 		}
 	case "le":
 		if searchResult <= value {
-			return true
+			return true, nil
 		}
 	case "gt":
 		if searchResult > value {
-			return true
+			return true, nil
 		}
 	case "ge":
 		if searchResult >= value {
-			return true
+			return true, nil
 		}
 	case "in":
 		for _, v := range strings.Split(value, ",") {
 			if v == searchResult {
-				return true
+				return true, nil
 			}
 		}
 	case "not-in":
 		for _, v := range strings.Split(value, ",") {
 			if v == searchResult {
-				return false
+				return false, nil
 			}
 		}
-		return true
+		return true, nil
 	case "absent":
 		if isAbsent(searchResult) {
-			return true
+			return true, nil
 		}
 	case "present":
 		if isPresent(searchResult) {
-			return true
+			return true, nil
 		}
 	case "null":
-		return isNil(data)
+		return isNil(data), nil
 	case "not-null":
-		return !isNil(data)
+		return !isNil(data), nil
 	case "empty":
-		return isEmpty(data)
+		return isEmpty(data), nil
 	case "intersect":
 		if jsonListsIntersect(searchResult, value) {
-			return true
+			return true, nil
 		}
 	case "contains":
 		if s, isString := convertToString(data); isString {
 			if strings.Contains(s, value) {
-				return true
+				return true, nil
 			}
 		}
 		if c, isSlice := convertToSliceOfStrings(data); isSlice {
 			for _, element := range c {
 				if element == value {
-					return true
+					return true, nil
 				}
 			}
-			return false
+			return false, nil
 		}
-		return strings.Contains(searchResult, value)
+		return strings.Contains(searchResult, value), nil
 	case "regex":
-		if regexp.MustCompile(value).MatchString(searchResult) {
-			return true
+		if regexp.MustCompile(value).MatchString(searchResult) { // FIXME don't use MustCompile
+			return true, nil
 		}
-		return false
+		return false, nil
 	}
-	return false
+	return false, nil
 }