Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(backend): fixes healthz response by adding json content type. Fixes #7525 #7532

Merged
merged 1 commit into from
May 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/src/apiserver/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ func startHttpProxy(resourceManager *resource.ResourceManager) {
topMux.HandleFunc("/apis/v1beta1/pipelines/upload", pipelineUploadServer.UploadPipeline)
topMux.HandleFunc("/apis/v1beta1/pipelines/upload_version", pipelineUploadServer.UploadPipelineVersion)
topMux.HandleFunc("/apis/v1beta1/healthz", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
io.WriteString(w, `{"commit_sha":"`+common.GetStringConfigWithDefault("COMMIT_SHA", "unknown")+`", "tag_name":"`+common.GetStringConfigWithDefault("TAG_NAME", "unknown")+`", "multi_user":`+strconv.FormatBool(common.IsMultiUserMode())+`}`)
})

Expand Down
52 changes: 52 additions & 0 deletions backend/src/common/client/api_server/healthz_client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package api_server

import (
"fmt"

"github.com/go-openapi/strfmt"
apiclient "github.com/kubeflow/pipelines/backend/api/go_http_client/healthz_client"
params "github.com/kubeflow/pipelines/backend/api/go_http_client/healthz_client/healthz_service"
model "github.com/kubeflow/pipelines/backend/api/go_http_client/healthz_model"
"github.com/kubeflow/pipelines/backend/src/common/util"
"k8s.io/client-go/tools/clientcmd"
)

type HealthzInterface interface {
GetHealthz() (*params.GetHealthzOK, error)
}

type HealthzClient struct {
apiClient *apiclient.Healthz
}

func NewHealthzClient(clientConfig clientcmd.ClientConfig, debug bool) (*HealthzClient, error) {
runtime, err := NewHTTPRuntime(clientConfig, debug)
if err != nil {
return nil, err
}

apiClient := apiclient.New(runtime, strfmt.Default)

// Creating upload client
return &HealthzClient{
apiClient: apiClient,
}, nil
}

func (c *HealthzClient) GetHealthz() (*model.APIGetHealthzResponse, error) {
parameters := params.NewGetHealthzParamsWithTimeout(apiServerDefaultTimeout)
response, err := c.apiClient.HealthzService.GetHealthz(parameters, PassThroughAuth)
if err != nil {
if defaultError, ok := err.(*params.GetHealthzDefault); ok {
err = CreateErrorFromAPIStatus(defaultError.Payload.Error, defaultError.Payload.Code)
} else {
err = CreateErrorCouldNotRecoverAPIStatus(err)
}

return nil, util.NewUserError(err,
fmt.Sprintf("Failed to get Healthz. Params: '%+v'", parameters),
fmt.Sprintf("Failed to get Healthz. Params: '%+v'", parameters))

}
return response.Payload, nil
}
64 changes: 64 additions & 0 deletions backend/test/integration/healthz_api_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package integration

import (
"testing"

"github.com/golang/glog"
"github.com/kubeflow/pipelines/backend/src/common/client/api_server"
"github.com/kubeflow/pipelines/backend/test"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/suite"
)

type HealthzApiTest struct {
suite.Suite
namespace string
healthzClient *api_server.HealthzClient
}

// Check the namespace have ML job installed and ready
func (s *HealthzApiTest) SetupTest() {
if !*runIntegrationTests {
s.T().SkipNow()
return
}

if !*isDevMode {
err := test.WaitForReady(*namespace, *initializeTimeout)
if err != nil {
glog.Exitf("Failed to initialize test. Error: %v", err)
}
}
s.namespace = *namespace
clientConfig := test.GetClientConfig(*namespace)
var err error
s.healthzClient, err = api_server.NewHealthzClient(clientConfig, false)
if err != nil {
glog.Exitf("Failed to get healthz client. Error: %v", err)
}
s.cleanUp()
}

func (s *HealthzApiTest) TearDownSuite() {
if *runIntegrationTests {
if !*isDevMode {
s.cleanUp()
}
}
}

func (s *HealthzApiTest) cleanUp() {
}

func (s *HealthzApiTest) TestHealthzAPI() {
t := s.T()

/* ---------- Verify healthz response ---------- */
healthzResp, err := s.healthzClient.GetHealthz()
assert.Nil(t, err)
assert.NotNil(t, healthzResp)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check the content within healthzResp are json parsable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To show more about what I mean: For example: you can check the content within healthzResp contains multi_user key and value: https://github.com/kubeflow/pipelines/blob/74c7773ca40decfd0d4ed40dc93a6af591bbc190/backend/api/python_http_client/docs/ApiGetHealthzResponse.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zijianjoy thanks for the reply, but I think I still don't get it.
the question here is healthzResp is a well-defined structure, you can check the structure definition here

type GetHealthzResponse struct {

so the field multi-user will be there.

But I think the question is: are we able to verify the field value multi-user to be true in a multi-tenancy setup. (as the multi-user field is a simple boolean field, we are unable to verify when the value is not presented.)
And still, it is hard to verify during this test (as we should be able to create a test environment w/o multiuser).

}

func TestHealthzAPI(t *testing.T) {
suite.Run(t, new(HealthzApiTest))
}