Add timeout for http.send builtin

There is a new optional `timeout` option to specify with `http.send`
which will set a client timeout on the request. This will override the
default 5 second timeout.

This change also corrects the request to use the builtin context. This
means that if the evaluation is canceled the request will also now be
canceled.

There is an environment variable for adjusting the default timeout
which we should aim to remove in future OPA versions (when
appropriate). For now though it is still supported, and will panic if
supplied with an invalid value rather than previously ignoring it and
effectively disable the timeout (0 == unlimited).

Fixes: open-policy-agent#2099
Signed-off-by: Patrick East <east.patrick@gmail.com>

docs/content/policy-reference.md

@@ -543,7 +543,7 @@ The `request` object parameter may contain the following fields:
 | `tls_client_key_env_variable` | no | `string` | Environment variable containing a client key in PEM encoded format. |
 | `tls_client_cert_file` | no | `string` | Path to file containing a client certificate in PEM encoded format. |
 | `tls_client_key_file` | no | `string` | Path to file containing a key  in PEM encoded format. |
-
+| `timeout` | no | `string` or `number` | Timeout for the HTTP request with a default of 5 seconds (`5s`). Numbers provided are in nanoseconds. Strings must be a valid duration string where a duration string is a possibly signed sequence of decimal numbers, each with optional fraction and a unit suffix, such as "300ms", "-1.5h" or "2h45m". Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h". A zero timeout means no timeout.|
 
 To trigger the use of HTTPs the user must provide one of the following combinations:
 

topdown/http.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net/url"
 	"strconv"
 
 	"github.com/open-policy-agent/opa/internal/version"
@@ -24,7 +25,9 @@ import (
 	"github.com/open-policy-agent/opa/topdown/builtins"
 )
 
-const defaultHTTPRequestTimeout = time.Second * 5
+const defaultHTTPRequestTimeoutEnv = "HTTP_SEND_TIMEOUT"
+
+var defaultHTTPRequestTimeout = time.Second * 5
 
 var allowedKeyNames = [...]string{
 	"method",
@@ -41,13 +44,12 @@ var allowedKeyNames = [...]string{
 	"tls_client_key_env_variable",
 	"tls_client_cert_file",
 	"tls_client_key_file",
+	"timeout",
 }
 var allowedKeys = ast.NewSet()
 
 var requiredKeys = ast.NewSet(ast.StringTerm("method"), ast.StringTerm("url"))
 
-var client *http.Client
-
 func builtinHTTPSend(bctx BuiltinContext, args []*ast.Term, iter func(*ast.Term) error) error {
 
 	req, err := validateHTTPRequestOperand(args[0], 1)
@@ -57,31 +59,40 @@ func builtinHTTPSend(bctx BuiltinContext, args []*ast.Term, iter func(*ast.Term)
 
 	resp, err := executeHTTPRequest(bctx, req)
 	if err != nil {
-		return handleBuiltinErr(ast.HTTPSend.Name, bctx.Location, err)
+		return handleHTTPSendErr(bctx, err)
 	}
 
 	return iter(ast.NewTerm(resp))
 }
 
 func init() {
 	createAllowedKeys()
-	createHTTPClient()
+	initDefaults()
 	RegisterBuiltinFunc(ast.HTTPSend.Name, builtinHTTPSend)
 }
 
-func createHTTPClient() {
-	timeout := defaultHTTPRequestTimeout
-	timeoutDuration := os.Getenv("HTTP_SEND_TIMEOUT")
-	if timeoutDuration != "" {
-		timeout, _ = time.ParseDuration(timeoutDuration)
+func handleHTTPSendErr(bctx BuiltinContext, err error) error {
+	// Return HTTP client timeout errors in a generic error message to avoid confusion about what happened.
+	// Do not do this if the builtin context was cancelled and is what caused the request to stop.
+	if urlErr, ok := err.(*url.Error); ok && urlErr.Timeout() && bctx.Context.Err() == nil {
+		err = fmt.Errorf("%s %s: request timed out", urlErr.Op, urlErr.URL)
 	}
+	return handleBuiltinErr(ast.HTTPSend.Name, bctx.Location, err)
+}
 
-	// create a http client with redirects disabled
-	client = &http.Client{
-		Timeout: timeout,
-		CheckRedirect: func(*http.Request, []*http.Request) error {
-			return http.ErrUseLastResponse
-		},
+func initDefaults() {
+	timeoutDuration := os.Getenv(defaultHTTPRequestTimeoutEnv)
+	if timeoutDuration != "" {
+		var err error
+		defaultHTTPRequestTimeout, err = time.ParseDuration(timeoutDuration)
+		if err != nil {
+			// If it is set to something not valid don't let the process continue in a state
+			// that will almost definitely give unexpected results by having it set at 0
+			// which means no timeout..
+			// This environment variable isn't considered part of the public API.
+			// TODO(patrick-east): Remove the environment variable
+			panic(fmt.Sprintf("invalid value for HTTP_SEND_TIMEOUT: %s", err))
+		}
 	}
 }
 
@@ -139,6 +150,8 @@ func executeHTTPRequest(bctx BuiltinContext, obj ast.Object) (ast.Value, error)
 	var tlsConfig tls.Config
 	var clientCerts []tls.Certificate
 	var customHeaders map[string]interface{}
+	var timeout = defaultHTTPRequestTimeout
+
 	for _, val := range obj.Keys() {
 		key, err := ast.JSON(val.Value)
 		if err != nil {
@@ -149,7 +162,7 @@ func executeHTTPRequest(bctx BuiltinContext, obj ast.Object) (ast.Value, error)
 		switch key {
 		case "method":
 			method = obj.Get(val).String()
-			method = strings.Trim(method, "\"")
+			method = strings.ToUpper(strings.Trim(method, "\""))
 		case "url":
 			url = obj.Get(val).String()
 			url = strings.Trim(url, "\"")
@@ -218,11 +231,20 @@ func executeHTTPRequest(bctx BuiltinContext, obj ast.Object) (ast.Value, error)
 			if !ok {
 				return nil, fmt.Errorf("invalid type for headers key")
 			}
+		case "timeout":
+			timeout, err = parseTimeout(obj.Get(val).Value)
+			if err != nil {
+				return nil, err
+			}
 		default:
 			return nil, fmt.Errorf("invalid parameter %q", key)
 		}
 	}
 
+	client := &http.Client{
+		Timeout: timeout,
+	}
+
 	if tlsClientCertFile != "" && tlsClientKeyFile != "" {
 		clientCertFromFile, err := tls.LoadX509KeyPair(tlsClientCertFile, tlsClientKeyFile)
 		if err != nil {
@@ -253,8 +275,10 @@ func executeHTTPRequest(bctx BuiltinContext, obj ast.Object) (ast.Value, error)
 	}
 
 	// check if redirects are enabled
-	if enableRedirect {
-		client.CheckRedirect = nil
+	if !enableRedirect {
+		client.CheckRedirect = func(*http.Request, []*http.Request) error {
+			return http.ErrUseLastResponse
+		}
 	}
 
 	if rawBody != nil {
@@ -269,11 +293,13 @@ func executeHTTPRequest(bctx BuiltinContext, obj ast.Object) (ast.Value, error)
 		return cachedResponse, nil
 	}
 
-	// create the http request
-	req, err := http.NewRequest(strings.ToUpper(method), url, body)
+	// create the http request, use the builtin context's context to ensure
+	// the request is cancelled if evaluation is cancelled.
+	req, err := http.NewRequest(method, url, body)
 	if err != nil {
 		return nil, err
 	}
+	req = req.WithContext(bctx.Context)
 
 	// Add custom headers passed from CLI
 
@@ -358,3 +384,31 @@ func createAllowedKeys() {
 		allowedKeys.Add(ast.StringTerm(element))
 	}
 }
+
+func parseTimeout(timeoutVal ast.Value) (time.Duration, error) {
+	var timeout time.Duration
+	switch t := timeoutVal.(type) {
+	case ast.Number:
+		timeoutInt, ok := t.Int64()
+		if !ok {
+			return timeout, fmt.Errorf("invalid timeout number value %v, must be int64", timeoutVal)
+		}
+		return time.Duration(timeoutInt), nil
+	case ast.String:
+		// Support strings without a unit, treat them the same as just a number value (ns)
+		var err error
+		timeoutInt, err := strconv.ParseInt(string(t), 10, 64)
+		if err == nil {
+			return time.Duration(timeoutInt), nil
+		}
+
+		// Try parsing it as a duration (requires a supported units suffix)
+		timeout, err = time.ParseDuration(string(t))
+		if err != nil {
+			return timeout, fmt.Errorf("invalid timeout value %v: %s", timeoutVal, err)
+		}
+		return timeout, nil
+	default:
+		return timeout, builtins.NewOperandErr(1, "'timeout' must be one of {string, number} but got %s", ast.TypeName(t))
+	}
+}