[Bug][Config-UI] Health endpoint is not accessible for livenessProbe/readinessProbe when basicAuth is enabled

[sayeedhussain]

Search before asking

• [X] I had searched in the issues and found no similar issues.

What happened

We have deployed devlake using helm chart in AWS EKS cluster.

We noticed that when basicAuth is enabled in config-ui, the /health endpoint which seems to be added as part of this PR https://github.com/apache/incubator-devlake/pull/5745 is not accessible without auth. Hitting /health shows up the authentication popup. Refer screenshot.

So we are not able to configure /health for our liveness/readiness probe. With basicAuth enabled, the existing probes also fail due to authencation error and the config-ui pod doesn't come up.

To overcome this, we had to disable the probes using kubectl patch deployment command.

What do you expect to happen

/health endpoint should be accessible without auth.

How to reproduce

1. Deploy devlake through helm chart with basicAuth enabled.
2. Hit the /health endpoint. Basic auth pop-up will come up.

Anything else

Always reproducible.

Version

0.21.0-beta1

Are you willing to submit PR?

• [X] Yes I am willing to submit a PR!

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[ankurkejriwal]

The end point specified in the default.conf.tp is /health/ maybe try using that instead of /health

[sayeedhussain]

@ankurkejriwal Doesn't work. Already tried.

[klesh]

@sayeedhussain Weird, it is working perfectly on my local machine: /health requires authorization while /health/ does not.

Could you try it again and share screenshots if it didn't work.

[sayeedhussain]

@klesh Tried again and found it works when we give a trailiing slash after health/. thanks!

It will be better to remove the trailing slash from nginx as trailing slashes is not a good idea and source of confusion when we don't need it as is the case here.

[klesh]

Yeah, I'm OK with it as long as it won't break the existing users.

[sayeedhussain]

@klesh It's hard to tell if it will be break existing consumers or not as that depends on how their respective libraries handle a trailing slash (strictly or loosely).

Personally I won't hesitate to break consumers and deal with the backlash if it fixes a source of confusion or mistake.

Feel free to close the issue whichever way you decide.

[klesh]

@sayeedhussain Maybe supporting both? That way we can be sure it will work for both and future users.

[sayeedhussain]

@klesh Sure.

Simplest and clearest would be adding another block for /health like here https://github.com/apache/incubator-devlake/pull/5745/files#diff-0bade717fcb1c175b0b0a027ab7edbe026b60684ea2c5a5115e2742d1de07449R40.

Better than adding regex to the existing path which increases complexity for reader.

If this approach works, I can raise a PR here and in the helm-charts repo with modified liveness/readiness probes here https://github.com/apache/incubator-devlake-helm-chart/blob/3a6338008d8617f6d937f91c0d34da97a6e59f42/charts/devlake/values.yaml#L260

[klesh]

@klesh Sure.

Simplest and clearest would be adding another block for /health like here https://github.com/apache/incubator-devlake/pull/5745/files#diff-0bade717fcb1c175b0b0a027ab7edbe026b60684ea2c5a5115e2742d1de07449R40.

Better than adding regex to the existing path which increases complexity for reader.

If this approach works, I can raise a PR here and in the helm-charts repo with modified liveness/readiness probes here https://github.com/apache/incubator-devlake-helm-chart/blob/3a6338008d8617f6d937f91c0d34da97a6e59f42/charts/devlake/values.yaml#L260

I agree. Please go ahead and raise the PR, thanks in advanced.

[sayeedhussain]

@klesh https://github.com/apache/incubator-devlake/pull/7009.

Once this gets incorporated in a release and the helm chart repo accommodates that release, will raise the PR there.

[klesh]

You may need to cherry-pick it back to v0.21(beta phase) or it will not be available until v1.0(development phase)

[sayeedhussain]

@klesh Do you mean cherry-pick this commit to release-v0.21 branch?

[klesh]

Yes. 👍

[sayeedhussain]

@klesh, @Startrekzky PR to release branch https://github.com/apache/incubator-devlake/pull/7038

[sayeedhussain]

Closing this defect now as the changes are merged to main and release branch cc: @klesh , @Startrekzky.

[sayeedhussain]

@klesh was planning to add this change in the helm repo but looks like someone did it already 2 weeks back. All good.

https://github.com/apache/incubator-devlake-helm-chart/blob/61cd2eea31e9daacd43ca29766fe70dbd003d3d4/charts/devlake/values.yaml#L273