Skip to content

Commit

Permalink
[FIX] Ensure that health check output is available via HTTP gateway
Browse files Browse the repository at this point in the history
With #5866, we inadvertently broke the output of the HTTP gateway when
retrieving health check hook information. The overall health of a
service was still returned, but neither the standard output nor the
standard error were included.

Because the addition of `install` (and later, `unintsall`) hooks
introduced a situation in which hooks could be executed without having
a service group present (that is, outside of a running Supervisor
process), the common logic for finding the path to the log output of
hooks was changed to accept a `&str` rather than a
`&ServiceGroup`. The value that is ultimately used should have only
been the "service name" portion (e.g., "redis" and not
"redis.default").

Unfortunately, because `ServiceGroup` implements
`Deref<Target=String>`, the calls to `stdout_log_path` and
`stderr_log_path` in the `http_gateway` module could continue to
compile. Since `Deref` is automatically invoked, the compiler was able
to coerce the `&ServiceGroup` reference all the way to a `&str`, thus
satisfying the new code. However, this `&str` was the _full_ name of
the `ServiceGroup` (e.g., "redis.default"), which ended up silently
breaking this HTTP gateway functionality. Rather than looking for the
files at `/hab/svc/$SERVICE/logs`, we were instead looking at
`hab/svc/$SERVICE.$GROUP/logs`.

Now, we explicitly call `service_group.service()` at this callsite,
thus restoring the prior functionality.

A "proper" fix would involve more extensive refactoring to ensure that
we have stricter typing to ensure the right "kind" of string is making
its way down to `stdout_log_path` and `stderr_log_path`, and possibly
removing the `Deref<Target=String>` implementation for `ServiceGroup`,
since there is clearly more than one kind of string-like thing we
could conceivably want out of a `ServiceGroup`. Such an approach
appears to get us rather far afield into the workings and usage of our
`outputln!` macros, which is an entirely different can of worms that
is best left unopened here (see #6584 for more on that).

Signed-off-by: Christopher Maier <cmaier@chef.io>
  • Loading branch information
christophermaier committed Nov 17, 2020
1 parent 4d3a814 commit f968608
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 2 deletions.
31 changes: 31 additions & 0 deletions .expeditor/end_to_end.pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,37 @@ steps:
- BUILD_PKG_TARGET=x86_64-linux
- PIPELINE_HAB_AUTH_TOKEN

- label: "[:linux: test_health_check_output_of_http_gateway]"
command:
- bash .expeditor/scripts/end_to_end/run_e2e_test.sh dev test_health_check_output_of_http_gateway
artifact_paths:
- "*.log"
env:
BUILD_PKG_TARGET: x86_64-linux
expeditor:
executor:
docker:
privileged: true
environment:
- BUILD_PKG_TARGET=x86_64-linux
- PIPELINE_HAB_AUTH_TOKEN

- label: "[:windows: test_health_check_output_of_http_gateway]"
command:
- powershell .expeditor/scripts/end_to_end/run_e2e_test.ps1 dev test_health_check_output_of_http_gateway
artifact_paths:
- "*.log"
env:
BUILD_PKG_TARGET: x86_64-windows
expeditor:
executor:
docker:
host_os: windows
environment:
- BUILD_PKG_TARGET=x86_64-windows
- BUILDKITE_AGENT_ACCESS_TOKEN
- PIPELINE_HAB_AUTH_TOKEN

- wait

- label: "[:habicat: Promote to Acceptance]"
Expand Down
4 changes: 2 additions & 2 deletions components/sup/src/http_gateway.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,8 +398,8 @@ fn health_gsr(svc: String, group: String, org: Option<&str>, state: &AppState) -

if let Some(health_check) = state.gateway_state.lock_gsr().health_of(&service_group) {
let mut body = HealthCheckBody::default();
let stdout_path = hooks::stdout_log_path::<HealthCheckHook>(&service_group);
let stderr_path = hooks::stderr_log_path::<HealthCheckHook>(&service_group);
let stdout_path = hooks::stdout_log_path::<HealthCheckHook>(service_group.service());
let stderr_path = hooks::stderr_log_path::<HealthCheckHook>(service_group.service());
let http_status: StatusCode = health_check.into();

body.status = health_check.to_string();
Expand Down
28 changes: 28 additions & 0 deletions test/end-to-end/test_health_check_output_of_http_gateway.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
if ($IsWindows) {
$test_probe_release="habitat-testing/test-probe/0.1.0/20200716152719"
} else {
$test_probe_release="habitat-testing/test-probe/0.1.0/20200716143058"
}

Describe "HTTP gateway" {
AfterAll {
Stop-Supervisor
}

$supLog = New-SupervisorLogFile("test_health_check_output_of_http_gateway")
Start-Supervisor -LogFile $supLog -Timeout 45

Context "with a service with a health-check hook" {
Load-SupervisorService "habitat-testing/test-probe"
Wait-Release -Ident $test_probe_release

# test-probe has a long init hook, and we want
# to let the health-check hoo
Start-Sleep 20

It "returns output of the hook when queried" {
$stdout = (Invoke-WebRequest "http://localhost:9631/services/test-probe/default/health" | ConvertFrom-Json).stdout
$stdout | Should -MatchExactly "Running health_check hook: habitat-testing/test-probe"
}
}
}

0 comments on commit f968608

Please sign in to comment.