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

Various fixes to health check scripts #3002

Merged

Conversation

kkimurak
Copy link
Contributor

close #2992, partially implement #2766

Access to health check resources such as /-/liveness is restricted to IPs specified in gitlab.monitoring.ip_whitelist (GITLAB_MONITORING_IP_WHITELIST).
Currently healthcheck may report unhealthy because the name localhost is solved to IPv6 loopback address (::1) that is not listed in the whitelist by default.

This PR fix the issue by using 127.0.0.1 (IPv4 loopback address) instead of localhost for monitoring endpoint url.

Possible alternate designs:

/cc @Gaibhne I have create commit based on your suggestion in #2766 so I have set you an author of the commit. Let me know if you don't like it.

@kkimurak
Copy link
Contributor Author

kkimurak commented Sep 13, 2024

request for comment: Should this PR contain all suggestion in #2766?

@sachilles
Copy link
Collaborator

@kkimurak Yes, this would be nice. However, do you have an idea to adress all issues?

@kkimurak
Copy link
Contributor Author

@sachilles Ok, I'll try to implement it by the next release. As far as I checked, the suggestion contains everything to do.

@sachilles
Copy link
Collaborator

@kkimurak Thank you! But please bear in mind that the reverse proxy can also play an important role in the choice of protocol in the curl command (depending on whether the reverse proxy terminates the SSL connection or not).

Gaibhne and others added 4 commits September 20, 2024 23:14
When the healthcheck feature introduced, the script were generated on build time
and url was fixed to "http://localhost/-/liveness".
See pull request sameersbn#2102

nginx is configured to redirect all http traffic to https when `GITLAB_HTTPS` is enabled.
(see https://github.com/sameersbn/docker-gitlab/blob/ac9e1fe/assets/runtime/config/nginx/gitlab-ssl#L41-L54),
`--location` option is set to follow the redirection.
See pull request sameersbn#2165

Health check script generation has been ported to the runtime,
allowing us to dynamically generate health check URLs
while referencing configuration parameters.
See sameersbn#2338

If configured correctly, the redirect will not occur and the option can be removed.

Original removal suggestion by @Gaibhne , additional (historical) research by @kkimurak.
Co-authored-by: Kazunori Kimura <kkimura@ims.ac.jp>
On upstream, expected default value is `127.0.0.1/8`
and it is already listed in corresponding configuration.
`GITLAB_MONITORING_IP_WHITELIST` is used to allow monitoring from hosts other than loopback (localhost).

So just unset default value for it.
If the value is not set, the line specifying this "additional" IP range will be removed.
Access to health check resources such as /-/liveness
is restricted to IPs specified in gitlab.monitoring.ip_whitelist
(`GITLAB_MONITORING_IP_WHITELIST`).
The name `localhost` is solved to IPv6 loopback address (::1)
that is not listed in the whitelist by default.

Possible alternate designs:
- Add IPv6 loopback to whitelist
- Disable IPv6 for gitlab container by specifying `net.ipv6.conf.all.disable_ipv6=1`
  in docker-compose.yml for example
  See sameersbn#2766 (comment)
@kkimurak kkimurak force-pushed the issue/2766-2992_use-ipv4-for-healcheck-url branch from 9c7e016 to e5dc2dd Compare September 21, 2024 15:38
@kkimurak
Copy link
Contributor Author

kkimurak commented Sep 21, 2024

I've implemented all (4) changes and rebased on the current master (v17.3.3), but I haven't been able to test them yet as my local environment has become very slow. It's taking over 10 hours to build the image..

@kkimurak
Copy link
Contributor Author

CI failed due to timeout.

@kkimurak kkimurak changed the title healthcheck: Use IPv4 loopback address instead of localhost Various fixes to health check scripts Sep 23, 2024
@kkimurak
Copy link
Contributor Author

Now CI looks succeed. It seems like it was automatically restarted after it failed.

@sachilles
Copy link
Collaborator

@kkimurak I did the restart of the CI. However, in my setup the healthcheck is working. I'll backport the healthcheck into the 17.x branches and the 16.11.x branch as well.

@sachilles
Copy link
Collaborator

Backport is done by cherry-picking the commits.

@sachilles sachilles merged commit 9f025ea into sameersbn:master Sep 25, 2024
3 checks passed
@kkimurak kkimurak deleted the issue/2766-2992_use-ipv4-for-healcheck-url branch September 25, 2024 14:29
@kkimurak
Copy link
Contributor Author

@sachilles Thank you.

@sachilles
Copy link
Collaborator

You're welcome.

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.

healthcheck not works
3 participants