fix: healthcheck middleware not working with route group

[luk3skyw4lker]

Fixing PR #2860

I have changed the checking method of routes to use a regex which checks if the route ends with the specified liveness or readiness route and included tests for the strict routing mode, all passing with no issues.

As far as the benchmark, here are the results with the changes in this PR:

And without the changes in this PR:

[luk3skyw4lker]

@ReneWerner87 @gaby could you guys take a look at this for me? I don't think I have messed up the benchmark that badly but maybe something else can be worked out.

[gaby]

@luk3skyw4lker I can see regex making performance drop substantially

[gaby]

Yeah, this definitely needs a different approach between Sprintf + Regex it will make the middleware super slow.

[luk3skyw4lker]

@gaby the only other thing I could think of is splitting the route by slashes and checking the last route to see if it's either one of the set up routes. I don't see any other way out of this

[ReneWerner87]

https://github.com/gofiber/fiber/issues/2860#issuecomment-1949132294

prefixCount := len(utils.TrimRight(c.Route().Path, '/'))
if len(c.Path()) >= prefixCount {
	switch c.Path()[prefixCount:] {
	case cfg.ReadinessEndpoint:
		return isReadyHandler(c)
	case cfg.LivenessEndpoint:
		return isLiveHandler(c)
	}
}

Have you ever tested this?

[gaby]

#2860 (comment)

prefixCount := len(utils.TrimRight(c.Route().Path, '/'))
if len(c.Path()) >= prefixCount {
	switch c.Path()[prefixCount:] {
	case cfg.ReadinessEndpoint:
		return isReadyHandler(c)
	case cfg.LivenessEndpoint:
		return isLiveHandler(c)
	}
}

Have you ever tested this?

I was about to comment something similar:

        // Extract the last segment of the path
        pathSegments := strings.Split(c.Path(), "/")
        lastSegment := pathSegments[len(pathSegments)-1]

        // Direct string comparison for the last segment of the path
        if lastSegment == strings.TrimPrefix(cfg.ReadinessEndpoint, "/") {
            return isReadyHandler(c)
        } else if lastSegment == strings.TrimPrefix(cfg.LivenessEndpoint, "/") {
            return isLiveHandler(c)
        }

[luk3skyw4lker]

@ReneWerner87 yours fail in the strict routing test but @gaby solution passed it no prob, benchmark results:

[luk3skyw4lker]

It was kind of what I wanted to do at first, but maybe I misunderstood @ReneWerner87 saying he wanted a different approach than that

[gaby]

@ReneWerner87 yours fail in the strict routing test but @gaby solution passed it no prob, benchmark results:

Hmmm that still drops performance in half. The ns/op went from 100 to 200 ns/op

[ReneWerner87]

then let's find a way without allocations, have an idea

[luk3skyw4lker]

I managed to extract 7 ns/op of @gaby solution by using switch case, let me see if I can do more

[gaby]

@luk3skyw4lker Try this:

// Find the start of the last segment
lastSlashIndex := strings.LastIndex(c.Path(), "/")
lastSegment := c.Path()[lastSlashIndex+1:] // +1 to skip the slash itself

// Direct string comparison for the last segment of the path
if lastSegment == cfg.ReadinessEndpoint {
    return isReadyHandler(c)
} else if lastSegment == cfg.LivenessEndpoint {
    return isLiveHandler(c)
}

Try with and without "+1"

Edit it has to be without "+1" since we need to slash to match the path

[luk3skyw4lker]

@gaby without the +1 looks better, because then we wouldn't have to trim the already set up config to remove the slashes, passed it fine and it looks very good for me:

[ReneWerner87]

@luk3skyw4lker Try this:

// Find the start of the last segment
lastSlashIndex := strings.LastIndex(c.Path(), "/")
lastSegment := c.Path()[lastSlashIndex+1:] // +1 to skip the slash itself

// Direct string comparison for the last segment of the path
if lastSegment == cfg.ReadinessEndpoint {
    return isReadyHandler(c)
} else if lastSegment == cfg.LivenessEndpoint {
    return isLiveHandler(c)
}

Try with and without "+1"

same idea

func New(config ...Config) fiber.Handler {
	cfg := defaultConfig(config...)

	isLiveHandler := healthCheckerHandler(cfg.LivenessProbe)
	isReadyHandler := healthCheckerHandler(cfg.ReadinessProbe)

	return func(c *fiber.Ctx) error {
		// Don't execute middleware if Next returns true
		if cfg.Next != nil && cfg.Next(c) {
			return c.Next()
		}

		if c.Method() != fiber.MethodGet {
			return c.Next()
		}

		// Extract the last segment of the path
		slashPos := strings.LastIndex(c.Path(), "/")
		lastSegment := c.Path()[slashPos+1:]

		// Direct string comparison for the last segment of the path
		if lastSegment == utils.TrimLeft(cfg.ReadinessEndpoint, '/') {
			return isReadyHandler(c)
		} else if lastSegment == utils.TrimLeft(cfg.LivenessEndpoint, '/') {
			return isLiveHandler(c)
		}

		return c.Next()
	}
}

[gaby]

@gaby without the +1 looks better, because then we wouldn't have to trim the already set up config to remove the slashes, passed it fine and it looks very good for me:

Awesome, is that using the Switch ? + lastindex?

[ReneWerner87]

why is this test passing?

[luk3skyw4lker]

@gaby using if and no +lastIndex

[luk3skyw4lker]

Let me commit so you guys can check it out

[luk3skyw4lker]

Using switch in @gaby solution makes it go back to around 190 ns/op

[gaby]

Using switch in @gaby solution makes it go back to around 190 ns/op

Wow!

[luk3skyw4lker]

@ReneWerner87 I think I wrote it badly, let me check it again

[ReneWerner87]

@ReneWerner87 I think I wrote it badly, let me check it again

yes but this is a good test, we should leave something like this in, because this path should not match

[luk3skyw4lker]

@ReneWerner87 I have just added a test for livez with a final slash (/livez/) and it is indeed not matching. I'll commit it right now so you can check it out

[gaby]

@luk3skyw4lker We definitely need a test that checks both /livez and /livez/.

[luk3skyw4lker]

@gaby I have just added this case, checking for /livez/ and /readyz/

[ReneWerner87]

we have to check the pre-route as well, otherwise every suffix would match the path

please also add negative tests

let me create some briefly

[luk3skyw4lker]

@ReneWerner87 that's something I have talked about in the issue, every route with /livez and /readyz will be matched in case of using the middleware globally.

Also, if a user pass a nested path as a endpoint, the actual structure would fail in running the middleware, since we're only checking the suffix

[luk3skyw4lker]

In this case the nested path would work actually, but only if the user placed the middleware into a route group

[ReneWerner87]

i added some more cases https://github.com/gofiber/fiber/pull/2863/commits/1b1bc46432d0907e44579ff87d42ee87c4c6ac96

so you can work test driven

[luk3skyw4lker]

Alright, I'll be checking out ways to improve the route matching globally, thanks for all the help! @ReneWerner87 and @gaby

I'll see if I can work something out by today, if not, I'll leave an update here