Operator metrics server

[SamBarker]

Type of change

• Enhancement / new feature

Description

Following on from #1865 this PR starts a HTTP server and registers a handler at /metrics to expose those metrics.

Additional Context

Fixes: #1866, #1896

This PR starts a standard Java HTTP server (this is not public or intended for high volume requests so we don't need anything more sophisticated).

Note with an eye to the future and the need to support kubernetes health monitoring this PR builds its own webserver rather than using the one offered by prometheus-metrics-exporter-httpserver. I investigated using the prometheus one however we can't register our own handlers and it felt excessive to have two different http servers requiring two different ports.

Things this PR doesn't address:

• HTTPS support
• Any configuration of the HTTP server. Such as binding address or port.
• Support any metrics registry apart from Prometheus

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• PR raised from a fork of this repository and made from a branch rather than main.
• Write tests
• Update documentation
• Make sure all unit/integration tests pass
• Make sure all Sonarcloud warnings are addressed or are justifiably ignored.
• If applicable to the change, trigger the system test suite. Make sure tests pass.
• If applicable to the change, trigger the performance test suite. Ensure that any degradations to performance numbers are understood and justified.
• Ensure the PR references relevant issue(s) so they are closed on merging.
• For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

NOTE: You must be a member of @kroxylicious/developers to trigger the system test and performance test suites. If you are not part of this group, comment on the PR requesting a trigger, tagging @kroxylicious/developers.

[k-wall]

Couple of niggles, but otherwise heading in the right direction.

[robobario]

We have to be careful to terminate the exchange correctly after we're done:

Terminating exchanges Exchanges are terminated when both the request InputStream and response OutputStream are closed. Closing the OutputStream, implicitly closes the InputStream (if it is not already closed). However, it is recommended to consume all the data from the InputStream before closing it. The convenience method close() does all of these tasks. Closing an exchange without consuming all of the request body is not an error but may make the underlying TCP connection unusable for following exchanges. The effect of failing to terminate an exchange is undefined, but will typically result in resources failing to be freed/ reused.

The docs are kind of ambiguous there, saying close does all the tasks but I think it doesn't consume the input stream fully before close.

In the delegation case I guess we can leave cleanup to the delegate?

[robobario]

hoping there is JDK one-liner for eating the inputstream, I think we can do is.transferTo(OutputStream.nullOutputStream()) give or take a try with resources.

[SamBarker]

Oh, good find.

[k-wall]

The point of #1039 is to prevent a DoS attack against the HTTP port. The attacker could POST a large amount of data, cause resources to the chewed on the server, preventing service to others.

If I understand is.transferTo(OutputStream.nullOutputStream()) correctly, it will read the data from the network and discard it. Can we do better? Can we just send the response, cause the socket to close (somehow) and avoid reading the response?

[robobario]

It looks like throwing an exception in the HttpHandler before a response has finished being written would close the socket. So maybe we could send headers and throw.

FWIW the prometheus MetricsHandler doesn't look like it drains the input on the successful path, I can see in their HTTPServer implementation (which we don't use) they do have a drainInputAndClose in one unauthorized 403 case, but that looks like the only place.

Having a play around I couldn't provoke any issues by sending multiple GETs with request body at the /metrics endpoint on the same connection.

[SamBarker]

As I read the HttpServer impl code (though I'm not claiming any expertise here) sending the response and leaving the inputStream untouched read will trigger a close as it it will not be EOF.

I'm not sure if we are actually buying anything much here as the InputStream will still be allocated...

[robobario]

As I read the HttpServer impl code (though I'm not claiming any expertise here) sending the response and leaving the inputStream untouched read will trigger a close as it it will not be EOF.

ah cool, so maybe it's fine. probably why my test with multiple curl requests is fine, was probably using a new connection each time.

I'm not sure if we are actually buying anything much here as the InputStream will still be allocated...

The inputstream is some lazy thing though that doesn't buffer all the data from the client.

[robobario]

maybe we should do what the JOSDK extension is doing and create a test namespace that we can delete in one shot

[SamBarker]

A task for a separate PR.

[robobario]

HTTPS support

think we've talked about this before for Kroxy and setting up TLS on the management port appears uncommon so we've not done it in the proxy

[robobario]

Playing around I get:

curl localhost:8080 -> 404
curl localhost:8080/ -> 404
curl localhost:8080/bla -> 404
curl localhost:8080/ -XPOST -> 405 with Allow: GET header
curl localhost:8080 -I (head request) -> 405 with Allow: GET header
curl localhost:8080/metrics -> dumps metrics as text/plain
curl localhost:8080/metrics -XPOST -> 405 with Allow: GET header
curl localhost:8080/metricsaaaargh -> dumps metrics as text/plain
curl localhost:8080/metrics/1/2/3 -> dumps metrics as text/plain

I guess the MetricsHandler doesn't care about the path. Could fix as part of this PR or another.

[SamBarker]

I guess the MetricsHandler doesn't care about the path. Could fix as part of this PR or another.

IIRC upstream (as in Prometheus) doesn't consider that a bug...

[robobario]

another thing spotted in the Prometheus HTTPServer is they're defaulting a couple of system props around the http server timeouts

Looks quite prudent as the defaults are forever https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerConfig.java#L46

[SamBarker]

another thing spotted in the Prometheus HTTPServer is they're defaulting a couple of system props around the http server timeouts

Looks quite prudent as the defaults are forever https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerConfig.java#L46

I've done that though I wasn't quite as generous with the max response time as 600 seconds felt rather long...

[SamBarker]

We should maybe support overriding this from an ENV Var for better support in a k8s env. Thoughts? Maybe we should leave that until we see an actual need. As two mins seems rather generous anyway.

[k-wall]

LGTM thanks @SamBarker

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
96.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud