Răsfoiți Sursa

Atm/internal server error fix (#3142)

Signed-off-by: Alex Meijer <alexander.meijer@ibm.com>
Alex Meijer 1 an în urmă
părinte
comite
852b0ae745

+ 5 - 5
.github/workflows/integration-testing.yaml

@@ -54,7 +54,7 @@ jobs:
         runs-on: ubuntu-latest
         permissions: {}
         needs: check_actor_permissions
-        if: ${{ (always() && !cancelled()) && ( github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
+        if: ${{ (always() && !cancelled()) && ( github.event.event_name == 'schedule' || github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
         outputs:
             IMAGE_TAG: ${{ steps.set_image_tags.outputs.IMAGE_TAG }}
             NAMESPACE: ${{ steps.set_image_tags.outputs.NAMESPACE }}
@@ -117,7 +117,7 @@ jobs:
                 
     build-test-stack:
         needs: wait_for_image_ready
-        if: ${{ (always() && !cancelled()) && ( github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
+        if: ${{ (always() && !cancelled()) && ( github.event.event_name == 'schedule'  || github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
         uses: opencost/opencost-infra/.github/workflows/build-stack.yaml@main
         secrets: inherit
         with:
@@ -127,7 +127,7 @@ jobs:
     wait-for-dns:
         needs: [wait_for_image_ready, build-test-stack]
         runs-on: ubuntu-latest
-        if: ${{ (always() && !cancelled()) && ( github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
+        if: ${{ (always() && !cancelled()) && ( github.event.event_name == 'schedule'  || github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
         permissions: {}
         steps:
           - name: Wait for DNS to resolve
@@ -149,7 +149,7 @@ jobs:
 
     run-tests:
         needs: [wait_for_image_ready, build-test-stack, wait-for-dns]
-        if: ${{ (always() && !cancelled()) && ( github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
+        if: ${{ (always() && !cancelled()) && ( github.event.event_name == 'schedule'  || github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
         permissions: {}
         uses: opencost/opencost-infra/.github/workflows/test-stack.yaml@main
         secrets: inherit
@@ -159,7 +159,7 @@ jobs:
     
     teardown-test-stack:
         needs: [wait_for_image_ready, run-tests]
-        if: ${{ (always() && !cancelled()) && ( github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
+        if: ${{ (always() && !cancelled()) && ( github.event.event_name == 'schedule'  || github.event_name == 'push' || github.event_name == 'merge_group' || (github.event_name == 'pull_request_target'  && needs.check_actor_permissions.outputs.ismaintainer == 'true')) }}
         uses: opencost/opencost-infra/.github/workflows/destroy-stack.yaml@main
         secrets: inherit 
         permissions: {}

+ 3 - 1
core/go.mod

@@ -12,6 +12,7 @@ require (
 	github.com/patrickmn/go-cache v2.1.0+incompatible
 	github.com/rs/zerolog v1.26.1
 	github.com/spf13/viper v1.8.1
+	github.com/stretchr/testify v1.9.0
 	golang.org/x/exp v0.0.0-20221031165847-c99f073a8326
 	golang.org/x/sync v0.6.0
 	golang.org/x/text v0.14.0
@@ -41,11 +42,11 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/oklog/run v1.1.0 // indirect
 	github.com/pelletier/go-toml v1.9.3 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/spf13/afero v1.6.0 // indirect
 	github.com/spf13/cast v1.3.1 // indirect
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
-	github.com/stretchr/testify v1.8.4 // indirect
 	github.com/subosito/gotenv v1.2.0 // indirect
 	golang.org/x/net v0.23.0 // indirect
 	golang.org/x/sys v0.18.0 // indirect
@@ -53,6 +54,7 @@ require (
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
 	k8s.io/klog/v2 v2.80.0 // indirect
 	k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed // indirect
 	sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect

+ 2 - 2
core/go.sum

@@ -281,8 +281,8 @@ github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5
 github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.2/go.mod h1:R6va5+xMeoiuVRoj+gSkQ7d3FALtqAAGI1FQKckRals=
-github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
-github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
+github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
+github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
 github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s=
 github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=
 github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=

+ 20 - 4
core/pkg/protocol/http.go

@@ -12,6 +12,8 @@ import (
 // HTTPProtocol is a struct used as a selector for request/response protocol utility methods
 type HTTPProtocol struct{}
 
+const internalServerErrorJSON = `{"code":500,"message":"Internal Server Error"}`
+
 // HTTPError represents an http error response
 type HTTPError struct {
 	StatusCode int
@@ -123,14 +125,14 @@ func (hp HTTPProtocol) WriteJSONData(w http.ResponseWriter, data interface{}) {
 	w.WriteHeader(status)
 	if err := json.NewEncoder(w).Encode(data); err != nil {
 		log.Error("Failed to encode JSON response: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }
 
 // WriteRawError uses json content-type and outputs raw error message for backwards compatibility to existing
 // frontend expectations.
 func (hp HTTPProtocol) WriteRawError(w http.ResponseWriter, httpStatusCode int, err string) {
-	// I know this isn't json, but its what we've done and don't want to break frontned while we fix CWE
-	w.Header().Set("Content-Type", "application/json")
 	http.Error(w, err, httpStatusCode)
 }
 
@@ -140,6 +142,8 @@ func (hp HTTPProtocol) WriteEncodedError(w http.ResponseWriter, httpStatusCode i
 	w.WriteHeader(httpStatusCode)
 	if err := json.NewEncoder(w).Encode(errorResponse); err != nil {
 		log.Error("Failed to encode error response: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }
 
@@ -151,6 +155,8 @@ func (hp HTTPProtocol) WriteData(w http.ResponseWriter, data interface{}) {
 	w.WriteHeader(status)
 	if err := json.NewEncoder(w).Encode(data); err != nil {
 		log.Error("Failed to encode response: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }
 
@@ -166,6 +172,8 @@ func (hp HTTPProtocol) WriteDataWithWarning(w http.ResponseWriter, data interfac
 	w.WriteHeader(status)
 	if err := json.NewEncoder(w).Encode(resp); err != nil {
 		log.Error("Failed to encode response with warning: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }
 
@@ -181,6 +189,8 @@ func (hp HTTPProtocol) WriteDataWithMessage(w http.ResponseWriter, data interfac
 	w.WriteHeader(status)
 	if err := json.NewEncoder(w).Encode(resp); err != nil {
 		log.Error("Failed to encode response with message: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }
 
@@ -218,6 +228,8 @@ func (hp HTTPProtocol) WriteDataWithMessageAndWarning(w http.ResponseWriter, dat
 	w.WriteHeader(status)
 	if err := json.NewEncoder(w).Encode(resp); err != nil {
 		log.Error("Failed to encode response with message and warning: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }
 
@@ -230,12 +242,14 @@ func (hp HTTPProtocol) WriteError(w http.ResponseWriter, err HTTPError) {
 	}
 	w.WriteHeader(status)
 
-	resp, _ := json.Marshal(&HTTPResponse{
+	resp := &HTTPResponse{
 		Code:    status,
 		Message: err.Body,
-	})
+	}
 	if err := json.NewEncoder(w).Encode(resp); err != nil {
 		log.Error("Failed to encode error response: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }
 
@@ -246,5 +260,7 @@ func (hp HTTPProtocol) WriteResponse(w http.ResponseWriter, r *HTTPResponse) {
 	w.WriteHeader(status)
 	if err := json.NewEncoder(w).Encode(r); err != nil {
 		log.Error("Failed to encode response: " + err.Error())
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(internalServerErrorJSON))
 	}
 }

+ 170 - 0
core/pkg/protocol/http_test.go

@@ -0,0 +1,170 @@
+package protocol
+
+import (
+	"errors"
+	"log"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestHTTPError_Error(t *testing.T) {
+	err := HTTPError{StatusCode: 400, Body: "bad request"}
+	assert.Equal(t, "bad request", err.Error())
+}
+
+func TestHTTPProtocol_BadRequest(t *testing.T) {
+	hp := HTTPProtocol{}
+	err := hp.BadRequest("bad req")
+	assert.Equal(t, http.StatusBadRequest, err.StatusCode)
+	assert.Equal(t, "bad req", err.Body)
+}
+
+func TestHTTPProtocol_UnprocessableEntity(t *testing.T) {
+	hp := HTTPProtocol{}
+	err := hp.UnprocessableEntity("")
+	assert.Equal(t, http.StatusUnprocessableEntity, err.StatusCode)
+	assert.Equal(t, "Unprocessable Entity", err.Body)
+	err2 := hp.UnprocessableEntity("custom")
+	assert.Equal(t, "custom", err2.Body)
+}
+
+func TestHTTPProtocol_InternalServerError(t *testing.T) {
+	hp := HTTPProtocol{}
+	err := hp.InternalServerError("")
+	assert.Equal(t, http.StatusInternalServerError, err.StatusCode)
+	assert.Equal(t, "Internal Server Error", err.Body)
+	err2 := hp.InternalServerError("custom")
+	assert.Equal(t, "custom", err2.Body)
+}
+
+func TestHTTPProtocol_NotImplemented(t *testing.T) {
+	hp := HTTPProtocol{}
+	err := hp.NotImplemented("")
+	assert.Equal(t, http.StatusNotImplemented, err.StatusCode)
+	assert.Equal(t, "Not Implemented", err.Body)
+	err2 := hp.NotImplemented("custom")
+	assert.Equal(t, "custom", err2.Body)
+}
+
+func TestHTTPProtocol_Forbidden(t *testing.T) {
+	hp := HTTPProtocol{}
+	err := hp.Forbidden("")
+	assert.Equal(t, http.StatusForbidden, err.StatusCode)
+	assert.Equal(t, "Forbidden", err.Body)
+	err2 := hp.Forbidden("custom")
+	assert.Equal(t, "custom", err2.Body)
+}
+
+func TestHTTPProtocol_NotFound(t *testing.T) {
+	hp := HTTPProtocol{}
+	err := hp.NotFound()
+	assert.Equal(t, http.StatusNotFound, err.StatusCode)
+	assert.Equal(t, "Not Found", err.Body)
+}
+
+func TestHTTPProtocol_ToResponse(t *testing.T) {
+	hp := HTTPProtocol{}
+	resp := hp.ToResponse("data", nil)
+	assert.Equal(t, http.StatusOK, resp.Code)
+	assert.Equal(t, "data", resp.Data)
+	resp2 := hp.ToResponse("data", errors.New("fail"))
+	assert.Equal(t, http.StatusInternalServerError, resp2.Code)
+	assert.Equal(t, "fail", resp2.Message)
+}
+
+func TestHTTPProtocol_WriteRawOK(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteRawOK(rw)
+	assert.Equal(t, http.StatusOK, rw.Code)
+	assert.Equal(t, "application/json", rw.Header().Get("Content-Type"))
+	assert.Equal(t, "0", rw.Header().Get("Content-Length"))
+}
+
+func TestHTTPProtocol_WriteRawNoContent(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteRawNoContent(rw)
+	assert.Equal(t, http.StatusNoContent, rw.Code)
+	assert.Equal(t, "application/json", rw.Header().Get("Content-Type"))
+}
+
+func TestHTTPProtocol_WriteJSONData(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteJSONData(rw, map[string]string{"foo": "bar"})
+	assert.Equal(t, http.StatusOK, rw.Code)
+	assert.Contains(t, rw.Body.String(), "foo")
+}
+
+func TestHTTPProtocol_WriteRawError(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteRawError(rw, http.StatusBadRequest, "bad")
+	assert.Equal(t, http.StatusBadRequest, rw.Code)
+	assert.Equal(t, "text/plain; charset=utf-8", rw.Header().Get("Content-Type"))
+	assert.Contains(t, rw.Body.String(), "bad")
+}
+
+func TestHTTPProtocol_WriteEncodedError(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteEncodedError(rw, http.StatusBadRequest, map[string]string{"err": "bad"})
+	assert.Equal(t, http.StatusBadRequest, rw.Code)
+	assert.Contains(t, rw.Body.String(), "bad")
+}
+
+func TestHTTPProtocol_WriteData(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteData(rw, map[string]string{"foo": "bar"})
+	assert.Equal(t, http.StatusOK, rw.Code)
+	assert.Contains(t, rw.Body.String(), "foo")
+}
+
+func TestHTTPProtocol_WriteDataWithWarning(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteDataWithWarning(rw, map[string]string{"foo": "bar"}, "warn")
+	assert.Equal(t, http.StatusOK, rw.Code)
+	assert.Contains(t, rw.Body.String(), "warn")
+}
+
+func TestHTTPProtocol_WriteDataWithMessage(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteDataWithMessage(rw, map[string]string{"foo": "bar"}, "msg")
+	assert.Equal(t, http.StatusOK, rw.Code)
+	assert.Contains(t, rw.Body.String(), "msg")
+}
+
+func TestHTTPProtocol_WriteDataWithMessageAndWarning(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteDataWithMessageAndWarning(rw, map[string]string{"foo": "bar"}, "msg", "warn")
+	assert.Equal(t, http.StatusOK, rw.Code)
+	assert.Contains(t, rw.Body.String(), "msg")
+	assert.Contains(t, rw.Body.String(), "warn")
+}
+
+func TestHTTPProtocol_WriteError(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	hp.WriteError(rw, HTTPError{StatusCode: 400, Body: "fail"})
+	assert.Equal(t, 400, rw.Code)
+	body := rw.Body.String()
+	log.Println("body: " + body)
+	assert.Contains(t, body, "fail")
+}
+
+func TestHTTPProtocol_WriteResponse(t *testing.T) {
+	hp := HTTPProtocol{}
+	rw := httptest.NewRecorder()
+	resp := &HTTPResponse{Code: 200, Data: "foo"}
+	hp.WriteResponse(rw, resp)
+	assert.Equal(t, 200, rw.Code)
+	assert.Contains(t, rw.Body.String(), "foo")
+}