Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(healthcheck): use address configured if not empty #2938

Merged
merged 1 commit into from Jan 30, 2024

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Jan 16, 2024

Problem is I have FB_ADDRESS=10.0.0.28 (my internal bridged network), and the healthcheck always fail with localhost:

/ # curl -f http://localhost:80/health
curl: (7) Failed to connect to localhost port 80 after 0 ms: Couldn't connect to server

Changing localhost to the value of $FB_ADDRESS fixes it:

/ # curl -f http://10.0.0.28:80/health
{"status":"OK"}

The proposed changes pick the address using the following:

  1. Use $FB_ADDRESS content if not empty;
  2. Fallback on "address" value in /.filebrowser.json
  3. Fallback on localhost

1. Use `$FB_ADDRESS` content if not empty;
2. Fallback on "address" value in /.filebrowser.json
3. Fallback on `localhost`
@Guiorgy
Copy link

Guiorgy commented Apr 5, 2024

NGL, I'm not very versed in networking, but shouldn't localhost always loop back?

Copy link

@aseg187 aseg187 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change causes unhealthy status with error message = curl: (3) URL rejected: Bad hostname revert back to old file
#!/bin/sh
PORT=${FB_PORT:-$(jq .port /.filebrowser.json)}
curl -f http://localhost:$PORT/health || exit 1

@qdm12
Copy link
Contributor Author

qdm12 commented Apr 12, 2024

Indeed, my apologies. It looks like this fixes it:
#3085 (comment)

Should I open another PR or someone did already?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants