Skip to content

Commit

Permalink
icinga2: Check errors in HTTP status and JSON
Browse files Browse the repository at this point in the history
The Icinga 2 Event Stream connection did not validate the HTTP status
code returned by the server. Consequently, HTTP errors were not
identified and their error messages were incorrectly interpreted as
valid messages. This occurred in two places: firstly, with regard to the
HTTP status code, and secondly, in relation to the error field, which
may be present within a JSON struct.

This modification introduces a brief HTTP status validation method that
adheres to the specifications outlined in the Icinga 2 API documentation
and advanced error checks in UnmarshalEventStreamResponse.
  • Loading branch information
oxzi committed May 31, 2024
1 parent b535ab3 commit 0d3e2ea
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 5 deletions.
14 changes: 11 additions & 3 deletions internal/icinga2/api_responses.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,14 +282,22 @@ func UnmarshalEventStreamResponse(bytes []byte) (any, error) {
// decompositions in different variables will result in multiple manual fixes. Thus, a two-way deserialization
// was chosen which selects the target type based on the first parsed type field.

var responseType string
var (
responseType string
responseError interface{}
)
err := json.Unmarshal(bytes, &struct {
Type *string `json:"type"`
}{&responseType})
Type *string `json:"type"`
Error interface{} `json:"error"`
}{&responseType, &responseError})
if err != nil {
return nil, err
}

if responseError != nil {
return nil, fmt.Errorf("error field is present with message %v", responseError)
}

var resp any
switch responseType {
case typeStateChange:
Expand Down
20 changes: 20 additions & 0 deletions internal/icinga2/api_responses_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,11 +294,31 @@ func TestApiResponseUnmarshal(t *testing.T) {
isError bool
expected any
}{
{
name: "empty",
jsonData: ``,
isError: true,
},
{
name: "invalid-json",
jsonData: `{":}"`,
isError: true,
},
{
name: "empty-json-struct",
jsonData: `{}`,
isError: true,
},
{
name: "error-field",
jsonData: `{"error":401,"status":"Unauthorized. Please check your user credentials."}`,
isError: true,
},
{
name: "error-field-with-valid-type",
jsonData: `{"type":"StateChange","error":401,"status":"Unauthorized. Please check your user credentials."}`,
isError: true,
},
{
name: "unknown-type",
jsonData: `{"type": "ihopethisstringwillneverappearinicinga2asavalidtype"}`,
Expand Down
32 changes: 30 additions & 2 deletions internal/icinga2/client_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,25 @@ import (

// This file contains Icinga 2 API related methods.

// checkHTTPResponseStatusCode compares an HTTP response status code against supported Icinga 2 API codes.
//
// An error will be returned for invalid status codes according to the documentation. Undefined status codes are going
// to result in errors as well.
//
// https://icinga.com/docs/icinga-2/latest/doc/12-icinga2-api/#http-statuses
func checkHTTPResponseStatusCode(status int) error {
switch {
case 200 <= status && status <= 299:
return nil
case 400 <= status && status <= 499:
return fmt.Errorf("invalid request, HTTP error %d", status)
case 500 <= status && status <= 599:
return fmt.Errorf("server error, HTTP error %d", status)
default:
return fmt.Errorf("unknown HTTP error %d", status)
}
}

// transport wraps http.Transport and overrides http.RoundTripper to set a custom User-Agent for all requests.
type transport struct {
http.Transport
Expand Down Expand Up @@ -91,10 +110,11 @@ func (client *Client) queryObjectsApi(
return nil, err
}

if res.StatusCode != http.StatusOK {
err = checkHTTPResponseStatusCode(res.StatusCode)
if err != nil {
_, _ = io.Copy(io.Discard, res.Body)
_ = res.Body.Close()
return nil, fmt.Errorf("unexpected HTTP status code %d", res.StatusCode)
return nil, err
}

return res.Body, nil
Expand Down Expand Up @@ -347,6 +367,14 @@ func (client *Client) connectEventStream(esTypes []string) (io.ReadCloser, error
return
}

err = checkHTTPResponseStatusCode(res.StatusCode)
if err != nil {
client.Logger.Errorw("Establishing an Event Stream API connection failed with an HTTP error, will be retried",
zap.Error(err),
zap.Duration("delay", retryDelay))
return
}

select {
case <-reqCtx.Done():
// This case might happen when this httpClient.Do and the time.After in the select below finish at round
Expand Down
28 changes: 28 additions & 0 deletions internal/icinga2/client_api_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package icinga2

import (
"github.com/stretchr/testify/assert"
"net/http"
"testing"
)

func TestCheckHTTPResponseStatusCode(t *testing.T) {
tests := []struct {
name string
status int
wantErr assert.ErrorAssertionFunc
}{
{"http-200", http.StatusOK, assert.NoError},
{"http-299", 299, assert.NoError},
{"http-204", http.StatusNoContent, assert.NoError},
{"http-401", http.StatusUnauthorized, assert.Error},
{"http-500", http.StatusInternalServerError, assert.Error},
{"http-900", 900, assert.Error},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
tt.wantErr(t, checkHTTPResponseStatusCode(tt.status), "checkHTTPResponseStatusCode(%v)", tt.status)
})
}
}

0 comments on commit 0d3e2ea

Please sign in to comment.