Panic in http_response and confusing behaivour on error states

[mirath]

Bug report

The input plugin http_response sets the result_type fields to "response_string_mismatch" when the plugin fails to read the body of the response when using the response_string_match option, additionally logging the error.

This is prone to cause confusion and extended debug times. It would be more appropriate to set the result_type to a dedicated value that signals a failure to process the body with a regex, something along the lines of "regex_failure" or "error_processing_body"

Additionally, if regex fails to compile the plugin raises a panic that kills Telegraf, though the way that is written suggests that it was intended, as whith a failure to read the response body, to log a message to stderr and set result_type to "response_string_mismatch".

Relevant telegraf.conf:

[[inputs.http_response]]
address = "http://localhost:5000"
response_string_match = "]]"

System info:

Telegraf commit df80fa6

Expected behavior:

The plugin should never cause a panic killing the server, and the "response_string_mismatch" result_type should be reserved for regex mismatches

Actual behavior:

The plugin should panic upon receiving an invalid regex, and the "response_string_mismatch" result_type is used in error states, instead of failing hard and hinting a configuration issue

EDIT:

Relevant code:

	// Start Timer
	start := time.Now()
	resp, err := h.client.Do(request)

	if err != nil {
		if netErr, ok := err.(net.Error); ok && netErr.Timeout() {
			fields["result_type"] = "timeout"
			return fields, nil
		}
		fields["result_type"] = "connection_failed"
		if h.FollowRedirects {
			return fields, nil
		}
		if urlError, ok := err.(*url.Error); ok &&
			urlError.Err == ErrRedirectAttempted {
			err = nil
		} else {
			return fields, nil
		}
	}
	defer func() {
		io.Copy(ioutil.Discard, resp.Body)
		resp.Body.Close()
	}()

	fields["response_time"] = time.Since(start).Seconds()
	fields["http_response_code"] = resp.StatusCode

	// Check the response for a regex match.
	if h.ResponseStringMatch != "" {

		// Compile once and reuse
		if h.compiledStringMatch == nil {
			h.compiledStringMatch = regexp.MustCompile(h.ResponseStringMatch)
			if err != nil {
				log.Printf("E! Failed to compile regular expression %s : %s", h.ResponseStringMatch, err)
				fields["result_type"] = "response_string_mismatch"
				return fields, nil
			}
		}

		bodyBytes, err := ioutil.ReadAll(resp.Body)
		if err != nil {
			log.Printf("E! Failed to read body of HTTP Response : %s", err)
			fields["result_type"] = "response_string_mismatch"
			fields["response_string_match"] = 0
			return fields, nil
		}

		if h.compiledStringMatch.Match(bodyBytes) {
			fields["result_type"] = "success"
			fields["response_string_match"] = 1
		} else {
			fields["result_type"] = "response_string_mismatch"
			fields["response_string_match"] = 0
		}
	} else {
		fields["result_type"] = "success"
	}

[mirath]

If possible, I would like to work on this issue

[danielnelson]

Thanks for the help @mirath, I think if the regex can't be compiled then the plugin should return an error at the start of Gather, not return any metrics, and telegraf will log the error. In a future version we will refuse to start if this regex is invalid.

If the body can't be read we can report body_read_error as the result_type.