[backend] wrong HTTP status and content type ending up with misleading go-sdk client errors

[ekesken]

Environment

• How did you deploy Kubeflow Pipelines (KFP)?
  Using the kustomize manifests here: https://github.com/kubeflow/manifests/tree/master/apps/pipeline/upstream/base

• KFP version:
  Tested and saw it failing the same way both in

  • Kubeflow 1.5.1, Pipelines 1.8.2
  • Kubeflow 1.8.0, Pipelines 2.0.3

• KFP SDK version:
  Tested by the following go sdk versions:

  • github.com/kubeflow/pipelines@v1.7.0-alpha.3.0.20211030192034-1d53de98c731
  • github.com/kubeflow/pipelines@v0.0.0-20220610193957-d5b428416871 # 1.8.22
  • github.com/kubeflow/pipelines@v0.0.0-20231208180049-c5658f09ec38 # 2.0.3

Steps to reproduce

To test server side issue over curl:

kubectl port-forward -n kubeflow svc/ml-pipeline8888:8888 # change it according to your setup
cat <<EOF >/tmp/w.yml
apiVersion: argoproj.io/v1alpha1
kind: Workflow
EOF
curl -v "http://127.0.0.1:8888/apis/v1beta1/pipelines/upload?name=some_name" -F uploadfile=@/tmp/w.yml
curl -v "http://127.0.0.1:8888/apis/v1beta1/pipelines/upload?name=some_name" -F uploadfile=@/tmp/w.yml

You'll see some output like this:

< HTTP/1.1 500 Internal Server Error
< Date: Wed, 13 Dec 2023 08:19:47 GMT
< Content-Length: 475
< Content-Type: text/plain; charset=utf-8
<
* Connection #0 to host 127.0.0.1 left intact
{"error_message":"Failed to create a pipeline and a pipeline version: Failed to create a pipeline and a pipeline version: Already exist error: Failed to create a new pipeline. The name some_name already exists. Please specify a new name","error_details":"Failed to create a pipeline and a pipeline version: Failed to create a pipeline and a pipeline version: Already exist error: Failed to create a new pipeline. The name some_name already exists. Please specify a new name"}%

It should return 409 Conflict with application/json content type ideally. But it's more about the extra trouble it causes in go sdk side rather than ideals. Because it hides the real problem in client side causing an error like this:

&{0 [] } (*pipeline_upload_model.APIStatus) is not supported by the TextConsumer, can be resolved by supporting TextUnmarshaler interface

Expected result

In HTTP Response, we should have seen:

< HTTP/1.1 409 Conflict
...
< Content-Type: application/json; charset=utf-8

And in go client sdk, we should get Already Exists error instead of an unmarshall error.

Materials and Reference

A test case like the following:

package clients

import (
	"fmt"
	"io"
	"net/http"
	"strings"
	"time"

	openapiRuntime "github.com/go-openapi/runtime"
	"github.com/kubeflow/pipelines/backend/api/go_http_client/pipeline_upload_client"
	"github.com/kubeflow/pipelines/backend/api/go_http_client/pipeline_upload_client/pipeline_upload_service"
	. "github.com/onsi/ginkgo"
	. "github.com/onsi/gomega"
	"github.com/onsi/gomega/ghttp"
)

var _ = Describe("KfpClient", func() {
	var (
		subject     *pipeline_upload_client.PipelineUpload
		err         error
		stubServer  *ghttp.Server
		testTimeout = 10 * time.Second
	)

	Context("given a conflict error", func() {
		BeforeEach(func() {
			stubServer = ghttp.NewServer()
			stubServer.AppendHandlers(
				ghttp.CombineHandlers(
					ghttp.VerifyRequest("POST", "/apis/v1beta1/pipelines/upload"),
					ghttp.RespondWith(
						http.StatusInternalServerError,
						`{"error_message":"Error creating pipeline: Create pipeline failed: Already exist error: Failed to create a new pipeline. The name some_name already exist. Please specify a new name.","error_details":"Error creating pipeline: Create pipeline failed: Already exist error: Failed to create a new pipeline. The name some_name already exist. Please specify a new name."}`,
						http.Header{"Content-Type": []string{"application/json"}},
					),
				),
			)

			subject = pipeline_upload_client.NewHTTPClientWithConfig(
				nil,
				pipeline_upload_client.DefaultTransportConfig().WithHost("127.0.0.1:8888").WithSchemes([]string{"http"}),
			)
		})

		Context("when calling UploadPipeline", func() {
			var uploadPipelineOk *pipeline_upload_service.UploadPipelineOK

			BeforeEach(func() {
				spec := "{\"apiVersion\": \"argoproj.io/v1alpha1\", \"kind\": \"Workflow\"}"
				readerCloser := io.NopCloser(strings.NewReader(spec))
				namedReadCloser := openapiRuntime.NamedReader("name.yaml", readerCloser)
				name := "some_name"
				params := pipeline_upload_service.NewUploadPipelineParams().WithUploadfile(namedReadCloser).WithName(&name).WithTimeout(testTimeout)

				uploadPipelineOk, err = subject.PipelineUploadService.UploadPipeline(params, nil)
			})

			It("should fail due to conflict error", func() {
				fmt.Println(err.Error())
				Expect(uploadPipelineOk).Should(BeNil())
				Expect(err).ShouldNot(BeNil())
				Expect(err.Error()).Should(ContainSubstring("Already exist error"))
			})
		})
	})
})

will end up with an error like this:

      Expected
          <string>: &{0 [] } (*pipeline_upload_model.APIStatus) is not supported by the TextConsumer, can be resolved by supporting TextUnmarshaler interface
      to contain substring
          <string>: Already exist error

until the problem gets fixed.

I see that, a similar problem is fixed for health check endpoints here in the context of this issue, but we need a similar fix everywhere.

I also found this issue which is addressed by this PR that's merged at 2020, probably that fix was covering only GRPC API not the Rest API. So there is still something to do for Rest API as I understand.

And this problem is not just about pipeline uploads I believe, we use misleading 5xx http response status code for client side issues in various endpoints.

---

Impacted by this bug? Give it a 👍.

[rimolive]

Does this issue persist in KFP 2.0.5?

[champon1020]

Does this issue persist in KFP 2.0.5?

I found this occurs also in KFP 2.0.5.