[e2e] Tests should check status code before attempting to unmarshal into json

[nrfox]

Many e2e tests attempt to unmarshal the kiali api response before checking the status code resulting in an unmarshalling error. This can mask the response from the server making it harder to debug the tests. The status code should first be checked before unmarshalling.

See: https://github.com/kiali/kiali/actions/runs/6413559944/job/18045431679 for an example of the error.

[PrinsonF77]

Hey, is this issue solved yet?

[jshaughn]

hi @PrinsonF77 , I'm going to say no just based on the fact that this is Open, still in the Backlog there are no PRs... Did you want to contribute here?

[PrinsonF77]

Yes. Can you assign it to me?
Also according to what I've seen at https://github.com/kiali/kiali/blob/master/tests/integration/tests/kiali_urls_test.go ,
func TestGrafana(t *testing.T) already implements this. Something similar has to be done for the tests that do not do this check right?

[jshaughn]

Thanks, @PrinsonF77 ! @nrfox , please provide any guidance, thanks.

[nrfox]

Sure the issue is here inside of the Grafana() call (and everywhere else this happens):

kiali/tests/integration/utils/kiali/kiali_client.go

Lines 605 to 616 in 9cb961f

	body, code, _, err := httpGETWithRetry(url, client.GetAuth(), TIMEOUT, nil, client.kialiCookies)
	if err == nil {
	status := new(models.GrafanaInfo)
	err = json.Unmarshal(body, &status)
	if err == nil {
	return status, code, nil
	} else {
	return nil, code, err
	}
	} else {
	return nil, code, err
	}

Until we check the status code, we can't safely unmarshal into status := new(models.GrafanaInfo) since the server response for 404s for non 200 is usually just a string. When the unmarshal fails, it masks the actual error message from the server that resides in the response.Body.

What we need is to stop unmarshaling non-200 responses inside of the client calls. It looks like some tests still need to check the status code so I wouldn't return an error here for non-200 responses. It doesn't appear that anything uses the response body directly though so you can simply log the string of response.Body for non-200 responses.

[PrinsonF77]

Thanks for the explanation. Will start working on it

[jshaughn]

@PrinsonF77 Will you be completing this effort?

[nrfox]

The kiali client methods can now use this method:

kiali/tests/integration/utils/kiali/kiali_client.go

Lines 647 to 663 in 470b68b

	func getRequestAndUnmarshalInto[T any](url string, response *T) error {
	body, code, _, err := httpGETWithRetry(url, client.GetAuth(), TIMEOUT, nil, client.kialiCookies)
	if err != nil {
	return err
	}
	if code != http.StatusOK {
	return fmt.Errorf("non 200 response code: %d from url: %s. Body: %s", code, url, body)
	}
	err = json.Unmarshal(body, response)
	if err != nil {
	return fmt.Errorf("unable to unmarshal body into response: %T. Body: %s", response, body)
	}
	return nil
	}

which will check the status code before unmarshaling. We just need to update the kiali client methods to use this new method.

[PrinsonF77]

Hey @jshaughn @nrfox . Will be taking this up. Sorry for the delay.
@nrfox since getRequestandUnmarshallInto only returns the error right now, there is no way to get the status code as well, which is supposed to be returned from the kiali client methods. Will be modifying this function to also return the code. Does that sound good?

[nrfox]

@PrinsonF77 I would say just update the methods that don't return a status code to use that getRequestAndUnmarshalInto function and leave the methods that already return a status code as they are. I'm guessing the status code is being returned because some tests want to check for a non-200 status. Probably there should be some lower level method like DoRequest that doesn't do any unmarshaling that those tests should use instead of one of the "client methods". If you're feeling very ambitious you can try to implement that otherwise just update the existing client methods that don't return a status to use getRequestAndUnmarshalInto.

[PrinsonF77]

@nrfox Most of the kiali client methods return a status code. Even the Grafana method that was given as an example in #6777 (comment) .
By modifying getRequestAndUnmarshalInto to also return the code, I can rewrite the Grafana() and other methods as such

func Grafana() (*models.GrafanaInfo, int, error) {
	url := fmt.Sprintf("%s/api/grafana", client.kialiURL)
	status := new(models.GrafanaInfo)
	code, err := getRequestAndUnmarshalInto(url, status)
	if err == nil {
		return status, code, err
	} else {
		return nil, code, err
	}
}

I think this would allow for most of the kiali client methods to be rewritten in this way which reduces the redundancy and helps with returning better error messages. This won't affect any methods that return status codes.
I'm done with most of the changes and will raise a PR soon. We can discuss about this over there as well by looking at the changes. If that does not look good, I can go ahead with just rewriting the methods that don't return a status

[nrfox]

I think this would allow for most of the kiali client methods to be rewritten in this way which reduces the redundancy and helps with returning better error messages.

That sounds good.

[jshaughn]

Closing on merge of #7215