Skip to content

[SHOT-247] fix: Disable proxy for container healthchecks - #8170

Open
keithhubner wants to merge 2 commits into
mainfrom
shot-247-healthcheck-proxy
Open

[SHOT-247] fix: Disable proxy for container healthchecks#8170
keithhubner wants to merge 2 commits into
mainfrom
shot-247-healthcheck-proxy

Conversation

@keithhubner

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

https://bitwarden.atlassian.net/browse/SHOT-247

Regression from #7858 (SHOT-184), first shipped in 2026.7.x.

📔 Objective

Container healthchecks fail on self-hosted deployments behind a forward proxy.

The service images are Alpine-based, so wget resolves to BusyBox wget. BusyBox wget honours HTTP_PROXY/HTTPS_PROXY but does not implement NO_PROXY at all. When proxy variables are present in the container runtime environment (e.g. via the Docker client config.json proxies block), the healthcheck's loopback probe is routed through the proxy and fails, marking the container unhealthy. curl, used before #7858, honoured NO_PROXY, so this worked prior to 2026.7.x.

This adds -Y off to every wget healthcheck so the loopback probe always connects directly, keeping the image-size/CVE benefit of having dropped curl.

Reproduced and verified against both base images (BusyBox v1.37.0):

$ export http_proxy=http://10.255.255.1:3128 no_proxy=localhost NO_PROXY=localhost

$ wget --tries=1 -O /dev/null http://localhost:5000/alive
Connecting to 10.255.255.1:3128 (10.255.255.1:3128)     # NO_PROXY silently ignored

$ wget -Y off --tries=1 -O /dev/null http://localhost:5000/alive
Connecting to localhost:5000 ([::1]:5000)               # correct

Note that BusyBox rejects the GNU --no-proxy spelling (wget: unrecognized option: no-proxy), so -Y off is the portable form here.

Covers all 13 wget healthcheck invocations across 12 Dockerfiles:

  • /alive — Api, Admin, Billing, Events, EventsProcessor, Notifications, Scim, Sso, Attachments
  • Identity — /.well-known/openid-configuration
  • Icons — /google.com/icon.png
  • Nginx — both legs, the https://localhost:8443 primary and the http://localhost:8080 fallback

Out of scope: util/SeederApi is Debian-based with real curl, and util/MsSql uses sqlcmd — neither is affected. Helm/k8s deployments are also unaffected, since kubelet performs the probing rather than the container.

BusyBox wget honours HTTP_PROXY/HTTPS_PROXY but does not implement
NO_PROXY. Since #7858 switched the healthchecks from curl to wget on
these Alpine images, deployments behind a forward proxy have their
loopback probe routed through the proxy, marking containers unhealthy.

Pass -Y off so the healthcheck always connects directly. BusyBox does
not accept the GNU --no-proxy spelling, so -Y off is the portable form
here.
@keithhubner keithhubner added the ai-review Request a Claude code review label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds -Y off to every BusyBox wget healthcheck probe across 12 Dockerfiles so loopback probes bypass HTTP_PROXY/HTTPS_PROXY values injected into the container environment on self-hosted deployments behind a forward proxy. I confirmed all 13 wget healthcheck invocations in the repo are covered (including both legs of the Nginx probe), and that every affected runtime stage is Alpine-based (mcr.microsoft.com/dotnet/aspnet:10.0-alpine3.23, nginx:1.31-alpine3.24) with no apk add wget shadowing the BusyBox applet — so -Y on/off is the correct portable spelling and NO_PROXY is genuinely unimplemented there. The two untouched healthchecks are correctly out of scope: util/SeederApi is Debian-based with real curl, and util/MsSql uses sqlcmd.

I also checked for healthcheck overrides that would need a matching update and found none — no docker-compose/.yml healthcheck definitions, nothing in util/Setup/Templates/DockerCompose.hbs, and no wget calls in entrypoint or helper scripts. The change is behavior-preserving when no proxy variables are set, since the probes already targeted loopback.

Code Review Details

No blocking or non-blocking findings.

The PR description documents the regression source, the BusyBox vs. GNU wget behavior difference, why --no-proxy is not portable here, and a before/after reproduction against both base images.

@keithhubner keithhubner added the t:bugfix Change Type - Bugfix label Aug 7, 2026
@keithhubner
keithhubner marked this pull request as ready for review August 7, 2026 14:52
@keithhubner
keithhubner requested review from a team as code owners August 7, 2026 14:52
@keithhubner
keithhubner requested a review from mimartin12 August 7, 2026 14:52
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.04%. Comparing base (4f8c0f0) to head (3ebbeaa).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8170   +/-   ##
=======================================
  Coverage   63.04%   63.04%           
=======================================
  Files        2315     2315           
  Lines      100510   100510           
  Branches     9043     9043           
=======================================
+ Hits        63364    63365    +1     
+ Misses      34957    34956    -1     
  Partials     2189     2189           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants