internal: Fix check call error being shadowed

Few places in the check call shadow the error declaration which results in some instances the evaluation errors not being included in decision log events. This fix attempts to resolve this. Fixes: #open-policy-agent/opa/issues/6174 Signed-off-by: Ashutosh Narkar <[email protected]>
open-policy-agent · Sep 5, 2023 · 67abebf · 67abebf
1 parent 3d79d78
commit 67abebf
Show file tree

Hide file tree

Showing 2 changed files with 114 additions and 8 deletions.
diff --git a/internal/internal.go b/internal/internal.go
@@ -394,19 +394,21 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
 		return nil, stop, err
 	}
 
-	inputValue, err := ast.InterfaceToValue(input)
+	var inputValue ast.Value
+	inputValue, err = ast.InterfaceToValue(input)
 	if err != nil {
 		return nil, stop, err
 	}
 
-	if err := envoyauth.Eval(ctx, p, inputValue, result); err != nil {
+	if err = envoyauth.Eval(ctx, p, inputValue, result); err != nil {
 		evalErr = err
 		return nil, stop, err
 	}
 
 	resp := &ext_authz_v3.CheckResponse{}
 
-	allowed, err := result.IsAllowed()
+	var allowed bool
+	allowed, err = result.IsAllowed()
 	if err != nil {
 		return nil, stop, errors.Wrap(err, "failed to get response status")
 	}
@@ -419,19 +421,22 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
 
 	switch result.Decision.(type) {
 	case map[string]interface{}:
-		responseHeaders, err := result.GetResponseEnvoyHeaderValueOptions()
+		var responseHeaders []*ext_core_v3.HeaderValueOption
+		responseHeaders, err = result.GetResponseEnvoyHeaderValueOptions()
 		if err != nil {
 			return nil, stop, errors.Wrap(err, "failed to get response headers")
 		}
 
 		if status == int32(code.Code_OK) {
 
-			headersToRemove, err := result.GetRequestHTTPHeadersToRemove()
+			var headersToRemove []string
+			headersToRemove, err = result.GetRequestHTTPHeadersToRemove()
 			if err != nil {
 				return nil, stop, errors.Wrap(err, "failed to get request headers to remove")
 			}
 
-			responseHeadersToAdd, err := result.GetResponseHTTPHeadersToAdd()
+			var responseHeadersToAdd []*ext_core_v3.HeaderValueOption
+			responseHeadersToAdd, err = result.GetResponseHTTPHeadersToAdd()
 			if err != nil {
 				return nil, stop, errors.Wrap(err, "failed to get response headers to send to client")
 			}
@@ -444,12 +449,14 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
 				},
 			}
 		} else {
-			body, err := result.GetResponseBody()
+			var body string
+			body, err = result.GetResponseBody()
 			if err != nil {
 				return nil, stop, errors.Wrap(err, "failed to get response body")
 			}
 
-			httpStatus, err := result.GetResponseEnvoyHTTPStatus()
+			var httpStatus *ext_type_v3.HttpStatus
+			httpStatus, err = result.GetResponseEnvoyHTTPStatus()
 			if err != nil {
 				return nil, stop, errors.Wrap(err, "failed to get response http status")
 			}

diff --git a/internal/internal_test.go b/internal/internal_test.go
@@ -806,6 +806,105 @@ func TestCheckBadDecisionWithLogger(t *testing.T) {
 	}
 }
 
+func TestCheckEvalErrorWithLogger(t *testing.T) {
+	var req ext_authz.CheckRequest
+	if err := util.Unmarshal([]byte(exampleAllowedRequest), &req); err != nil {
+		panic(err)
+	}
+
+	// create custom logger
+	customLogger := &testPlugin{}
+
+	module := `
+		package envoy.authz
+
+		allow := false
+
+        allow:= true`
+
+	server := testAuthzServerWithModule(module, "envoy/authz/allow", nil, withCustomLogger(customLogger))
+	ctx := context.Background()
+	output, err := server.Check(ctx, &req)
+
+	if err == nil {
+		t.Fatal("Expected error but got nil")
+	}
+
+	if output != nil {
+		t.Fatalf("Expected no output but got %v", output)
+	}
+
+	if len(customLogger.events) != 1 {
+		t.Fatal("Unexpected events:", customLogger.events)
+	}
+
+	event := customLogger.events[0]
+
+	if event.Error == nil || event.Path != "envoy/authz/allow" || event.Revision != "" || event.Result != nil ||
+		event.DecisionID == "" || event.Metrics == nil {
+		t.Fatalf("Unexpected events: %+v", customLogger.events)
+	}
+
+	expectedMsg := "eval_conflict_error: complete rules must not produce multiple outputs"
+	if !strings.Contains(event.Error.Error(), expectedMsg) {
+		t.Fatalf("Expected error message %v, but got %v", expectedMsg, event.Error.Error())
+	}
+}
+
+func TestCheckAllowObjectDecisionWithBadReqHeadersToRemoveWithLogger(t *testing.T) {
+	var req ext_authz.CheckRequest
+	if err := util.Unmarshal([]byte(exampleAllowedRequestParsedPath), &req); err != nil {
+		panic(err)
+	}
+
+	module := `
+		package envoy.authz
+
+		default allow = false
+
+		allow {
+			input.parsed_path = ["my", "test", "path"]
+		}
+
+		headers["x"] = "hello"
+		headers["y"] = "world"
+
+		request_headers_to_remove := "foo"
+
+		result["allowed"] = allow
+		result["headers"] = headers
+		result["request_headers_to_remove"] = request_headers_to_remove`
+
+	customLogger := &testPlugin{}
+
+	server := testAuthzServerWithModule(module, "envoy/authz/result", nil, withCustomLogger(customLogger))
+	ctx := context.Background()
+	output, err := server.Check(ctx, &req)
+	if err == nil {
+		t.Fatal("Expected error but got nil")
+	}
+
+	if output != nil {
+		t.Fatalf("Expected no output but got %v", output)
+	}
+
+	if len(customLogger.events) != 1 {
+		t.Fatal("Unexpected events:", customLogger.events)
+	}
+
+	event := customLogger.events[0]
+
+	if event.Error == nil || event.Path != "envoy/authz/result" || event.Revision != "" || event.Result != nil ||
+		event.DecisionID == "" || event.Metrics == nil {
+		t.Fatalf("Unexpected events: %+v", customLogger.events)
+	}
+
+	expectedMsg := "type assertion error"
+	if !strings.Contains(event.Error.Error(), expectedMsg) {
+		t.Fatalf("Expected error message %v, but got %v", expectedMsg, event.Error.Error())
+	}
+}
+
 func TestCheckWithLoggerError(t *testing.T) {
 	// Example Envoy Check Request for input:
 	// curl --user  alice:password  -o /dev/null -s -w "%{http_code}\n" http://${GATEWAY_URL}/api/v1/products