fix errors from client side

Pix4D · Sep 26, 2024 · fe42ea1 · fe42ea1
1 parent 71b7bf7
commit fe42ea1
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 11 deletions.
diff --git a/atc/policy/opa/opa.go b/atc/policy/opa/opa.go
@@ -60,23 +60,23 @@ func (c opa) Check(input policy.PolicyCheckInput) (policy.PolicyCheckResult, err
 	client.Timeout = c.config.Timeout
 	resp, err := client.Do(req)
 	if err != nil {
-		return nil, fmt.Errorf("opa server connection error: %s", err)
+		return nil, fmt.Errorf("OPA server: connecting: %s", err)
 	}
 	defer resp.Body.Close()
 
 	statusCode := resp.StatusCode
 	if statusCode != http.StatusOK {
-		return nil, fmt.Errorf("opa server returned status: %d", statusCode)
+		return nil, fmt.Errorf("OPA server returned status: %d", statusCode)
 	}
 
 	body, err := io.ReadAll(resp.Body)
 	if err != nil {
-		return nil, fmt.Errorf("opa server returned no response: %s", err.Error())
+		return nil, fmt.Errorf("OPA server: reading response body: %s", err)
 	}
 
 	result, err := ParseOpaResult(body, c.config)
 	if err != nil {
-		return nil, fmt.Errorf("parsing opa response: %w", err)
+		return nil, fmt.Errorf("parsing OPA results: %w", err)
 	}
 
 	return result, nil

diff --git a/atc/policy/opa/opa_test.go b/atc/policy/opa/opa_test.go
@@ -177,7 +177,7 @@ var _ = Describe("OPA Policy Checker", func() {
 		It("should return error", func() {
 			result, err := agent.Check(policy.PolicyCheckInput{})
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(MatchRegexp("opa server connection error: .* connection refused"))
+			Expect(err.Error()).To(MatchRegexp("OPA server: connecting: .* connection refused"))
 			Expect(result).To(BeNil())
 		})
 	})
@@ -190,7 +190,7 @@ var _ = Describe("OPA Policy Checker", func() {
 		It("should return error", func() {
 			result, err := agent.Check(policy.PolicyCheckInput{})
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(Equal("opa server returned status: 404"))
+			Expect(err.Error()).To(Equal("OPA server returned status: 404"))
 			Expect(result).To(BeNil())
 		})
 	})
@@ -205,7 +205,7 @@ var _ = Describe("OPA Policy Checker", func() {
 		It("should return error", func() {
 			result, err := agent.Check(policy.PolicyCheckInput{})
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("JSON parsing: error: invalid character 'h' looking for beginning of value. Hint: response from opa server is not a valid JSON"))
+			Expect(err.Error()).To(ContainSubstring("parsing JSON: invalid character 'h' looking for beginning of value"))
 			Expect(result).To(BeNil())
 		})
 	})

diff --git a/atc/policy/opa/result.go b/atc/policy/opa/result.go
@@ -30,7 +30,7 @@ func ParseOpaResult(bytesResult []byte, opaConfig OpaConfig) (opaResult, error)
 	var results vars.StaticVariables
 	err := json.Unmarshal(bytesResult, &results)
 	if err != nil {
-		return opaResult{}, fmt.Errorf("JSON parsing: error: %s. Hint: response from opa server is not a valid JSON", err)
+		return opaResult{}, fmt.Errorf("parsing JSON: %w", err)
 	}
 
 	var allowed, shouldBlock, ok bool

diff --git a/go-concourse/concourse/configs.go b/go-concourse/concourse/configs.go
@@ -96,7 +96,11 @@ func (team *team) CreateOrUpdatePipelineConfig(pipelineRef atc.PipelineRef, conf
 		var validationErr atc.SaveConfigResponse
 		err = json.Unmarshal(body, &validationErr)
 		if err != nil {
-			return false, false, []ConfigWarning{}, err
+			return false, false, []ConfigWarning{}, internal.UnexpectedResponseError{
+				StatusCode: response.StatusCode,
+				Status:     response.Status,
+				Body:       string(body),
+			}
 		}
 		return false, false, []ConfigWarning{}, InvalidConfigError{Errors: validationErr.Errors}
 	case http.StatusForbidden:

diff --git a/go-concourse/concourse/configs_test.go b/go-concourse/concourse/configs_test.go
@@ -378,6 +378,7 @@ var _ = Describe("ATC Handler Configs", func() {
 				It("returns an error", func() {
 					_, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials)
 					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(ContainSubstring("bad-json"))
 				})
 			})
 		})

diff --git a/go-concourse/concourse/internal/connection.go b/go-concourse/concourse/internal/connection.go
@@ -204,7 +204,10 @@ func (connection *connection) populateResponse(response *http.Response, returnRe
 	}
 
 	if response.StatusCode == http.StatusForbidden {
-		return ErrForbidden
+		body, _ := io.ReadAll(response.Body)
+		return ForbiddenError{
+			Reason: string(body),
+		}
 	}
 
 	if response.StatusCode < 200 || response.StatusCode >= 300 {

diff --git a/go-concourse/concourse/internal/connection_test.go b/go-concourse/concourse/internal/connection_test.go
@@ -353,7 +353,7 @@ var _ = Describe("ATC Connection", func() {
 				})
 			})
 
-			Describe("403 response", func() {
+			Describe("HTTPAgent: 403 response", func() {
 				BeforeEach(func() {
 					atcServer = ghttp.NewServer()
 
@@ -381,6 +381,34 @@ var _ = Describe("ATC Connection", func() {
 				})
 			})
 
+			Describe("Connection: 403 response", func() {
+				BeforeEach(func() {
+					atcServer = ghttp.NewServer()
+
+					connection = NewConnection(atcServer.URL(), nil, tracing)
+
+					atcServer.AppendHandlers(
+						ghttp.CombineHandlers(
+							ghttp.VerifyRequest("DELETE", "/api/v1/teams/main/pipelines/foo"),
+							ghttp.RespondWith(http.StatusForbidden, "problem"),
+						),
+					)
+				})
+
+				It("returns back ForbiddenError", func() {
+					err := connection.Send(Request{
+						RequestName: atc.DeletePipeline,
+						Params: rata.Params{
+							"pipeline_name": "foo",
+							"team_name":     atc.DefaultTeamName,
+						},
+					}, nil)
+					fe, ok := err.(ForbiddenError)
+					Expect(ok).To(BeTrue())
+					Expect(fe.Reason).To(Equal("problem"))
+				})
+			})
+
 			Describe("404 response", func() {
 				Context("when the response does not contain JSONAPI errors", func() {
 					BeforeEach(func() {