kibanaResponseFactory should support entirely custom responses

[gmmorris]

Our Acceptance Tests rely on fixtures which simulate 3rd party services that we need to interact with, such as the public Slack or Pageduty APIs.

These APIs implement http responses which are different than a standard Kibana response and at times that means that I need my plugin to respond with an http error code and some custom body.

I was hoping that kibanaResponseFactory.customError and kibanaResponseFactory.custom will be enough, but there are various assumptions baked in (such as an error response code receiving an Error type with a message prop - assumptions which aren’t true in the Slack or Pageduty APIs.

After discussing this on Slack with @rudolf and @restrry we agreed that there should probably be a way (either via a new API or by relaxing kibanaResponseFactory.custom) to allow a response with an error code (such as 429) and an entirely custom object (that isn’t an Error type as currently required).

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[rudolf]

It's ugly and undocumented, but at least as a workaround returning a Buffer or Stream body will skip the validation and error wrapping.

return response.custom({
  body: Buffer.from('ok'),
  statusCode: 429,
})

kibana/src/core/server/http/router/response_adapter.ts

Lines 119 to 126 in 952b61e

	// Special case for when we are proxying requests and want to enable streaming back error responses opaquely.
	if (Buffer.isBuffer(payload) || payload instanceof stream.Readable) {
	const response = this.responseToolkit
	.response(kibanaResponse.payload)
	.code(kibanaResponse.status);
	setHeaders(response, kibanaResponse.options.headers);
	return response;
	}

[pgayvallet]

Follow-up of rudolf's snippet is:

kibana/src/core/server/http/router/response_adapter.ts

Lines 128 to 144 in 952b61e

	// we use for BWC with Boom payload for error responses - {error: string, message: string, statusCode: string}
	const error = new Boom('', {
	statusCode: kibanaResponse.status,
	});
	error.output.payload.message = getErrorMessage(payload);
	const attributes = getErrorAttributes(payload);
	if (attributes) {
	error.output.payload.attributes = attributes;
	}
	const headers = kibanaResponse.options.headers;
	if (headers) {
	// Hapi typings for header accept only strings, although string[] is a valid value
	error.output.headers = headers as any;
	}

We would need a way to opt-out from this conversion.

Maybe a new option on CustomHttpResponseOptions + HttpResponseOptions, such as convertErrors? that would default to true?

[mshustov]

Maybe a new option on CustomHttpResponseOptions + HttpResponseOptions, such as convertErrors? that would default to true?

We will have to maintain it in the long term. We are going to formalize error response in the future #57296 but this case requires full freedom to customize error format. I don't think we want to give such a possibility to any plugin.
Probably we shouldn't provide such API in whole Kibana, but extend test utils somehow? Or adopt alerting plugin tests to use another HTTP server (Hapi?) instead of relying on Kibana API?

[pgayvallet]

I don't think we want to give such a possibility to any plugin.

For standard use cases, I agree 100%.

But in some cases, we might need it? In example, a plugin used as a proxy to any arbitrary API/endpoint and forwarding responses as-is to the client-side is something that should not be permitted?

[rudolf]

Alerting's use case is just for testing and I think that could easily be converted not to use Core API's but rather a standalone hapi server for instance. However, the Console app needs to proxy requests to Elasticsearch and should return responses in the original error format. We could "wrap" these errors in Core and let the frontend unwrap the errors to expose the real ES error. But there's a balance between enforcing consistent error responses and the complexity of having to work around it for valid use cases.

I lean towards promoting strong conventions, but having some escape hatches for the few cases that might come up where it makes sense to break the convention.

[joshdover]

I generally agree, these tests should use a different HTTP server API than Core.

For the proxying use case I think we should provide proxying out-of-the-box directly, rather than adding an escape hatch. Not that I'm 100% against an escape hatch, I'd just rather not add it unless we really need it and there's other good reasons to introduce proxying as a first-class feature.

Should we close this issue if we're in agreement on these tests being ported to a different HTTP server API?

[mshustov]

Closed. The recommended solution for this particular use-case is to switch to another HTTP server.