fiber

fiber copied to clipboard

Bug Description

Hi, I have a problem with running healthcheck middleware with a group:

package main

import (
	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/compress"
	"github.com/gofiber/fiber/v2/middleware/healthcheck"
	"github.com/gofiber/fiber/v2/middleware/recover"
)

func main() {
	app := fiber.New()

	app.Use(recover.New())
	app.Use(compress.New())

	group := app.Group("/app")
	group.Use(healthcheck.New())

	group.Get("/hello", func(c *fiber.Ctx) error {
		return c.SendString("Hello, World 👋!")
	})

	err := app.Listen(":5001")
	if err != nil {
		panic(err)
	}
}

• localhost:5001/app/livez doesn't work:

  curl localhost:5001/app/livez
  Cannot GET /app/livez
  

• localhost:5001/app/hello works

How to Reproduce

Steps to reproduce the behavior:

1. Run the main file above
2. Run curl localhost:5001/app/livez
3. It returns not found.

Expected Behavior

The endpoint should return HTTP 200

Fiber Version

v2.52.0

Code Snippet (optional)

No response

Checklist:

• [X] I agree to follow Fiber's Code of Conduct.
• [X] I have checked for existing issues that describe my problem prior to opening this one.
• [X] I understand that improperly formatted bug reports may be closed without explanation.

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ulasakdeniz What happens if you do this instead:

group.Use("/livez", healthcheck.New())

group.Use() is expecting multiple params, and the middleware only returns a Fiber.Handler()

https://github.com/gofiber/fiber/blob/v2/group.go#L62

https://github.com/gofiber/fiber/blob/v2/middleware/healthcheck/healthcheck.go#L13

@ReneWerner87 Is this a limitation, bug, or possible feature we need to add?

@efectn Forgot to tag you :-)

@ulasakdeniz What happens if you do this instead:

group.Use("/livez", healthcheck.New())

Hi @gaby thanks for your answer. That also doesn't work.

Edit: My Go version: go version go1.21.4 darwin/amd64

https://github.com/gofiber/fiber/blob/4e0f180fe3425b92d2c7b7e362182d17c21ee50b/middleware/healthcheck/healthcheck.go#L43 the problem is this check, where the current path should be reduced with the prefix of the current route

@luk3skyw4lker can you help with a fix ?

@ReneWerner87 sure! I can check it out today and try to send a PR with the fix

@ReneWerner87 by what I've read right now the problem is that c.Path returns the full path (like /group/livez) and the check fails, so I should be reducing the c.Path to check if the last route is a /livez route. Is this right?

right, that would be one solution, a second one is to check only the suffix of the route, but that would be a not quite perfect one

https://github.com/gofiber/fiber/blob/main/router.go#L62 https://docs.gofiber.io/api/ctx#route

we should reduce it with the information of the current route

@ReneWerner87 I could only think about splitting the Path and checking the suffix, but that would remove any possbility of having any /[n+]/(livez | readyz) routes. But I think this could be a problem in the future, right?

someting like this but better

		switch c.Path()[len(c.Route().Path):] {
		case cfg.ReadinessEndpoint:
			return isReadyHandler(c)
		case cfg.LivenessEndpoint:
			return isLiveHandler(c)
		}

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

	app := fiber.New()
	app.Group("/v1", New())
	v2 := app.Group("/v2/")
	customer := v2.Group("/customer/")
	customer.Use(New())

	req, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/v1/readyz", nil))
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, fiber.StatusOK, req.StatusCode)

	req, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/v1/livez", nil))
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, fiber.StatusOK, req.StatusCode)

	req, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/v2/customer/readyz", nil))
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, fiber.StatusOK, req.StatusCode)

	req, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/v2/customer/livez", nil))
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, fiber.StatusOK, req.StatusCode)
}

		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)
			}
		}

can you make these adjustments and check them again and also the benchmarks

perhaps we also need to test the strictRouting mode

and you find a better way to compare routes

@ReneWerner87 yep, I'll check them and write a test with StrictRouting mode too. Thanks for the help and heads up!

@ReneWerner87 I'll send a PR with the reports and results by Sunday, is that okay?

Sure thx