Feature/fixing prometheus

[jkiddo]

Fixing prometheus so that it actually works. Also seems related to micrometer-metrics/micrometer#5093

[dotasek]

What is the testable result of this change?

From the Prometheus docs, when this is running, there should be an endpoint available that exposes metrics at /actuator/prometheus or similar. If this is enabled by default, we could also include a smoke test to verify that this is running.

[jkiddo]

@dotasek I've added this: dfce153

[jkiddo]

... and now a smoketest for the promethus endpoint as well

[dotasek]

This looks good so far. When I get a chance later today, I'll spin up the project and check if it's reporting metrics too, and approve if everything looks OK.

Thanks Jens!

[dotasek]

I do not see actuator/info appearing as part of the default config.

prometheus, health, and metrics appear, but I need to explicitly set up info to get that endpoint, in both the main and test application configs:

  endpoint:
    ...
    info:
      enabled: true
    ...

Is leaving info out intended?

It would also not take much to extend that IT test to cover all these endpoints. I tried the following, and it worked for me:

        @ParameterizedTest
	@ValueSource(strings = {"prometheus", "info", "health", "metrics"})
	void testActuatorEndpointExists(String endpoint) throws IOException {

		CloseableHttpClient httpclient = HttpClients.createDefault();
		CloseableHttpResponse response = httpclient.execute(new HttpGet("http://localhost:" + port + "/actuator/" + endpoint));
		int statusCode = response.getStatusLine().getStatusCode();
		assertEquals(200, statusCode);

	}

Could you add that as well?

[XcrigX]

Does the prometheus endpoint expose anything potentially sensitive?
If I read this correctly, only /health is enabled by default over HTTP: https://docs.spring.io/spring-boot/reference/actuator/endpoints.html#actuator.endpoints.security

[dotasek]

@jkiddo I think @XcrigX may have a very valid point. This should be most secure by default, and we would have to review each of these endpoints to check what HAPI produces.

Maybe it's best to only configure /health, and comment the rest with a warning about security concerns.

[XcrigX]

@jkiddo I think @XcrigX may have a very valid point. This should be most secure by default, and we would have to review each of these endpoints to check what HAPI produces.

Maybe it's best to only configure /health, and comment the rest with a warning about security concerns.

Agree with this. I'd include them commented out so it's easy to enable, but not as default. The good news is that base for these is at /actuator which is a sibling of the /fhir baseurl. So it's easy to enable them internally and prevent public access through a reverse proxy/gateway.

[jkiddo]

While I can comment them out I feel I do need to say a couple of things here:

1. I consider the JPA starter project as an example project. It shows of some of the options available using the HAPI stack. Some functionality more secure than others. I would recommend not to ship it to production with its current feature set.
2. None of the endpoints to all the actual FHIR resources - meaning the clinical data is not protected in any way - AT ALL. It is completely unsecured by default.
3. The configuration is a default configuration. By default it ships with H2. It is not intended for production - in any way - as I see it. The configuration needs change if you take it beyond the sandbox anyways.
4. If you still consider it bad practice to have the prometheus parts exposed as part of this project that shows how HAPI can be used, I would have to find my peace with it, but due to the points above, I would fail to understand the security concerns / leak of data when the above is not adressed first.

I may be making some wrong assumptions here, but that is how I see it.

[XcrigX]

I don't disagree with you in general, @jkiddo. But consider this scenario. I am deploying my own application.yml file as a file with my server deployment. My application.yml doesn't mention the actuator endpoints at all.

Tomorrow, I pull the latest code and build it which now includes these options in the classpath/jar copy of application.yaml. I re-deploy my server (not changing my locally deployed properties file), and now the endpoints are suddenly enabled because these properties are not explicitly overridden in my deployed properties but are present on the classpath/jar.

[XcrigX]

Here are the relevant docs on property loading order:
https://docs.spring.io/spring-boot/reference/features/external-config.html

I'm pretty sure any property in the compiled/jar copy of the properties file will but picked up if it's not explicitly overridden in a deployment - so it makes sense to me to be careful and only have safe/reasonable defaults enabled.

[jkiddo]

@XcrigX - that's a fair point, though I experience a different behavour. When using --spring.profiles.active=diff --spring.config.location=...application-diff.yaml whatever is stated in that config file is what gets loaded. It does not take into account what was stated in the bundled one unless I'm doing something wrong.

[XcrigX]

@jkiddo - I just ran a quick test to verify my understanding. Here's what I did.

1. Pulled latest master branch.
2. Updated the application.yaml, changed the server.port property to 8888.
3. compiled w/ "mvn clean package spring-boot:repackage -DskipTests=true -Pboot"
4. took the compiled uber jar - ROOT.war file, copied it to a different directory on my machine
5. copied the application.yaml file to the same directory, edited it and commented out the server.port property.
6. ran the server (java -jar ROOT.war)

The server came up on 8888, despite my local application.yaml not specifying that port and it not being the Spring default- it's picking up the default value specified in the compiled jar.

To double-check myself I then stopped the server, uncommented server.port and changed it to 8081, reran and it came up on 8081.

[jkiddo]

I guess that the reason that you @XcrigX experience a different behaviour than me is because I use the spring.config.location:

[jkiddo]

To get the ball rolling I've removed the prometheus and metrics parts by default.

[dotasek]

@jkiddo but why re-introduce /info? I thought only /health was recommended?

[jkiddo]

@dotasek that was a blunder from my side. It's fixed now.

[dotasek]

This looks good. The test config has everything enabled, so we can verify the endpoints work with our rest, but when run with the default config, only the health endpoint appears.

One minor nitpicky change, and I think this is done. As always, thanks for the patience, Jens.

I may follow up by adding an HTTPClient test to verify the default config, but that can wait for after the merge.