fix: nil pointer dereference in HealthStrategy

[massenz]

What does this PR do?

Fixes a panic caused by a nil pointer dereference in the HealthStrategy.WaitUntilReady function.
See Issue #801

Why is it important?

Anyone using the HealthStrategy for their tests would see them failing with the panic.

Related issues

• Closes [Bug]: HealthStrategy tries to dereference a null Health pointer #801

How to test this PR

I have added the following tests, to cover the various scenarios:

PASS wait.TestWaitForHealthTimesOutForUnhealthy (0.20s)
PASS wait.TestWaitForHealthSucceeds (0.00s)
PASS wait.TestWaitForHealthWithNil (0.30s)

The TestWaitForHealthWithNil test will cause the panic with the original code, but it does not after the PR is applied.

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	701b216
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/63e488f30e81ca000748aa35
😎 Deploy Preview	https://deploy-preview-802--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[mdelapenya]

I'm not sure about this 🤔 Do we want to eventually pass the test if the health is nil? Which comes with a second question: what do we want to do in the health strategy when the health is nil? Fail/Continue/Log?

EDIT: the above line was for this block: https://github.com/testcontainers/testcontainers-go/pull/802/files#diff-62f25e9df2042653e9484951d6537c4088f6580206cbaef6bb30d610bfdd6befR69-R74. So please do not get me wrong 🙏 I think we should check for nil, although considering different states.

In the current implementation we are considering that not having declared a Health in the container is exactly the same as not being healthy (we sleep). I'm not opposed to that, but what to discuss with you about it: should be consider not having Health in the state as healthy or not?

[massenz]

I'm not sure about this thinking Do we want to eventually pass the test if the health is nil? Which comes with a second question: what do we want to do in the health strategy when the health is nil? Fail/Continue/Log?

Hey, thanks for super-quick review and sorry for not explaining the tests better (I've now added comments, but please let me know if still not sufficient).
And, no, absolutely, the thinking is that if Health is nil the strategy will (eventually, after the timeout) fail

EDIT: the above line was for this block: https://github.com/testcontainers/testcontainers-go/pull/802/files#diff-62f25e9df2042653e9484951d6537c4088f6580206cbaef6bb30d610bfdd6befR69-R74. So please do not get me wrong pray I think we should check for nil, although considering different states.

Indeed - in fact that test is meant to "emulate" observed container behavior: initially Health is nil and then it becomes healthy: in the original code, this would have caused a panic, but now it waits and (if the state becomes healthy then it succeeds).

I've added another test (TestWaitFailsForNilHealth) where Health stays nil and the strategy eventually times out and returns the (expected) context.DeadlineExceeded error.

In the current implementation we are considering that not having declared a Health in the container is exactly the same as not being healthy (we sleep). I'm not opposed to that, but what to discuss with you about it: should be consider not having Health in the state as healthy or not?

Yes, this follows the same principle, as mentioned above, but it waits for it to either become non-nil, and if not it fails - if it does become non-nil, then it checks the state.

Hopefully that clarifies, and the additional test proves that.

[mdelapenya]

Just a tiny suggestion in the new test method. Once we discuss on that, LGTM

[mdelapenya]

I'm approving the PR even without the suggestions I added on the style on how to assert.

Will merge it once you approve them or discard them for a different preference in the assert style

Thanks for t¡your time and patience contributing to the project 🚀

[massenz]

Just a tiny suggestion in the new test method. Once we discuss on that, LGTM

oh, I'm so sorry!
I thought you meant only that "new test method" and didn't realize I should make the changes everywhere
(I usually use Gingko/Gomega assertions, not really used to the "vanilla" Go testing)

On an unrelated note - I have the impression that some containers (most? all?) do not really set the Health value: I can see the running is true, but Health stays nil.
The code is still valid, but I'm now wondering about the value of this particular Strategy.

BTW - while working on this, I was wondering adding an HealthEndpointStrategy which checks on one of the mapppedPorts for a particular endpoint (default: /health) makes a GET request and expects a given status code (default http.StatusOk)
What do you think?

[mdelapenya]

Just a tiny suggestion in the new test method. Once we discuss on that, LGTM

oh, I'm so sorry! I thought you meant only that "new test method" and didn't realize I should make the changes everywhere (I usually use Gingko/Gomega assertions, not really used to the "vanilla" Go testing)

No worries, I totally understand :) I was not explicit, so not an issue at all

On an unrelated note - I have the impression that some containers (most? all?) do not really set the Health value: I can see the running is true, but Health stays nil. The code is still valid, but I'm now wondering about the value of this particular Strategy.

The State function, when it's called from a container, inspects the container, so it will take whatever it's in the Dockerfile. If the container you use does not have a Health it's because it's not there.

// State returns container's running state
func (c *DockerContainer) State(ctx context.Context) (*types.ContainerState, error) {
	inspect, err := c.inspectRawContainer(ctx)
	if err != nil {
		if c.raw != nil {
			return c.raw.State, err
		}
		return nil, err
	}
	return inspect.State, nil
}

BTW - while working on this, I was wondering adding an HealthEndpointStrategy which checks on one of the mapppedPorts for a particular endpoint (default: /health) makes a GET request and expects a given status code (default http.StatusOk) What do you think?

For this, you can use wait.ForHTTP https://golang.testcontainers.org/features/wait/http/ which, I think, comes with what you expect: check for status code, check in an HTTP endpoint, and check for a response. Is that what you are looking for?

[massenz]

The State function, when it's called from a container, inspects the container, so it will take whatever it's in the Dockerfile. If the container you use does not have a Health it's because it's not there.

Thanks, Manuel - I didn't know of the HEALTHCHECK directive, thanks for making me discover it (I thought the health was somehow defined by the docker daemon according to some internal state).
As a matter of fact, OPA (Open Policy Agent) doesn't set it.

For this, you can use wait.ForHTTP https://golang.testcontainers.org/features/wait/http/ which, I think, comes with what you expect: check for status code, check in an HTTP endpoint, and check for a response. Is that what you are looking for?

Well, honestly I have no idea how I missed that one 🤦‍♂️ it's not even like there's a want of options!!! 👍
Thanks!

Have committed your suggestion too.

[mdelapenya]

Great job during the review! I very much appreciate your effort in contributing the fix