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]

#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?

[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]

@ReneWerner87

We have two options for this:

1. Improve the route matching and support nested routes in the config of the middleware
2. Add in the middleware docs instructions saying that nested routes are not allowed, only one level routes

I think that improving the route matching part will be quite hard to do, but maybe if we go to another PR to do this it could be possible.

[luk3skyw4lker]

This should be the test case for this:

func Test_HealthCheck_Custom_Nested(t *testing.T) {
	t.Parallel()

	app := fiber.New()

	c1 := make(chan struct{}, 1)
	go func() {
		time.Sleep(1 * time.Second)
		c1 <- struct{}{}
	}()

	app.Use(New(Config{
		LivenessProbe: func(c *fiber.Ctx) bool {
			return true
		},
		LivenessEndpoint: "/probe/live",
		ReadinessProbe: func(c *fiber.Ctx) bool {
			select {
			case <-c1:
				return true
			default:
				return false
			}
		},
		ReadinessEndpoint: "/probe/ready",
	}))

	shouldWork(t, app, "/probe/livez")
	shouldWork(t, app, "/probe/readyz")
	shouldNotWork(t, app, "/probe/livez/")
	shouldNotWork(t, app, "/probe/readyz/")
	shouldNotWork(t, app, "/livez")
	shouldNotWork(t, app, "/readyz")
	shouldNotWork(t, app, "/readyz/")
	shouldNotWork(t, app, "/livez/")
}

[efectn]

do you( @efectn @gaby @luk3skyw4lker ) have any comments or other test cases you would like to add?

Will check the code tomorrow

[ReneWerner87]

have added some cases
unfortunately i realized that this kind of matching is not perfect, because normally we should allow all router possibilities
f.e.

app.Use("*", New())
app.Use("/bla/:param/", New())
...

I would solve this in version 3

[ReneWerner87]

in v3 i will do something like this

	app.Use("/livez", NewLivenessCheck(Config{
		Next: func(c *fiber.Ctx) bool {
			return true // somthing
		},
	}))
	app.Use(DefaultLivenessEndpoint, NewLivenessCheck(Config{
		Next: func(c *fiber.Ctx) bool {
			return true // somthing
		},
	}))

routing stuff should be done by the router and not the middleware itself

[luk3skyw4lker]

@ReneWerner87 that would be perfect! I thought about doing that too inside the middleware, but analyzing the framework structure made me realize that it would be complicated.

If you need any help in v3 I could chip in too, it wouldn't be too much. I have more free time now that I have solved some work related problems.

[ReneWerner87]

Can you pls benchmark the last changes? I got totally different numbers

[luk3skyw4lker]

@ReneWerner87 I runned it twice and it looks fine:

[luk3skyw4lker]

@ReneWerner87 those helpers could be moved to a helpers package after, they could be used by tests all around the framework. Maybe this could be an improvement to v3 too, for a better developer experience while contributing.

I think doing this here would be outside of the scope of the issue/PR, but it's a suggestion

[gaby]

Shouldnt these be "Not found"?

[ReneWerner87]

@gaby can you check my last comments and resolve the conversations

[gaby]

@gaby can you check my last comments and resolve the conversations

Looks good 💪

[ReneWerner87]

in v3 i will do something like this

	app.Use("/livez", NewLivenessCheck(Config{
		Next: func(c *fiber.Ctx) bool {
			return true // somthing
		},
	}))
	app.Use(DefaultLivenessEndpoint, NewLivenessCheck(Config{
		Next: func(c *fiber.Ctx) bool {
			return true // somthing
		},
	}))

routing stuff should be done by the router and not the middleware itself

@luk3skyw4lker if you want, you can support with a PR for this change
Target is "v2ToV3Merge" branch (merge is not completely done, but the base is the pr code #2864 )

[luk3skyw4lker]

@ReneWerner87 Will do! I'll be checking it out this week and see if I can open a PR already. Thanks for the suggestion and heads up!