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

Ensure health check always returns 0 or 1 #502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

garlic-hub
Copy link

Dockerfile reference says that health checks should always return 0 or 1. Any other status code is reserved. Adding an "|| exit 1" will return a 1 anytime drill returns non-zero status

Dockerfile reference says that health checks should always return 0 or 1. Any other status code is reserved. Adding an "|| exit 1" Will return a 1 anytime drill returns non-zero status
@garlic-hub
Copy link
Author

@@ -29,7 +29,7 @@ services:
default:
ipv4_address: 172.28.0.2
healthcheck:
test: ["CMD", "drill", "@127.0.0.1", "dnssec.works"]
test: ["CMD", "drill", "@127.0.0.1", "dnssec.works", "||", "exit", "1"]
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately the || operator only exists in shell commands (e.g. sh, bash, etc) and this image has none of those. Will need to consider a different approach to solve this.

In any non-distroless container you could do something like the following:

test: ["CMD", "sh", "-c", "drill @127.0.0.1 dnssec.works || exit 1"]

But not here I'm afraid

@garlic-hub
Copy link
Author

I reviewed the source code for drill and it does seem capable of returning status codes other than 0 and 1. I see a Quiet flag, but it seems to only alter what is printed to stdout. I was hoping it would be like the quiet flag in grep. Without any scripting capabilities or programs with a runtime (e.g. python) it seems we need a compiled binary. Would be it crazy to write a simple drill-health wrapper that forks the subprocess and clamps the exit code to 0 or 1? Or maybe contribute a new flag upstream?

@klutchell
Copy link
Owner

We actually did something very similar on another repo I maintain, to solve a similar issue. Not sure it's worth it here though or if we should take another approach.

klutchell/dnscrypt-proxy-docker#117

@garlic-hub
Copy link
Author

garlic-hub commented Oct 2, 2024

I don't know Go, but if you're open to Rust or C we can use one of these. I haven't tested them, but you can get the general idea.

use std::process::{Command, ExitCode};

fn main() -> ExitCode {
    let mut child = match Command::new("/opt/usr/bin/drill")
        .args(std::env::args().skip(1))
        .spawn() {
        Ok(child) => child,
        Err(e) => {
            eprintln!("Unable to launch drill process: {e:?}");
            return ExitCode::from(1);
        }
    };

    match child.wait() {
        // Return 0 on 0 exit code
        Ok(status) if status.success() => ExitCode::from(0),
        // Return 1 on any other exit code
        Ok(_) => ExitCode::from(1),
        Err(e) => {
            eprintln!("Error waiting for drill process to finish: {e:?}");
            ExitCode::from(1)
        }
    }
}
#include <stdio.h>
#include <sys/wait.h>
#include <unistd.h>


int main(int argc, char **argv)
{
    char* DRILL_ARGS[] = {"/opt/usr/bin/drill", NULL};
    if (argc == 0) {
        argv = DRILL_ARGS;
    }
    else {
        argv[0] = DRILL_ARGS[0];
    }

    switch (fork())
    {
    case -1:
        perror("Error calling fork()");
        return 1;
    
    case 0:
        execv(argv[0], argv);
        perror("Error calling execv() with drill");
        return 1;

    default:
        int status;
        if (wait(&status) == -1) {
            perror("Error calling wait()");
            return 1;
        }
        if (WIFEXITED(status)) {
            if (WEXITSTATUS(status) == 0) {
                return 0;
            }
            else {
                return 1;
            }

        } else {
            fprintf(stderr, "drill process was terminated for unknown reason\n");
            return 1;
        }
    }
}

@klutchell
Copy link
Owner

@garlic-hub I like the looks of the rust binary wrapper. We would need a Docker build stage for rust with the correct toolchains available. Is this something you have experience with and could try another PR?

@klutchell
Copy link
Owner

Also, I just remembered that dig is present in the image. Does that help with the correct error codes?

@klutchell
Copy link
Owner

Here is a draft PR to add your example Rust wrapper to the image, let me know if we still need it or if dig is suitable.
#510

@garlic-hub
Copy link
Author

dig won't work. I see no flags to change status codes and man page shows various exit codes. I'll take a look at the Rust PR

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.

2 participants