Merge ~athos-ribeiro/ubuntu-docker-images/+git/bind9:foreground-flag into ~ubuntu-docker-images/ubuntu-docker-images/+git/bind9:9.18-22.10

[Athos Ribeiro]

LP: #1991719

This change allows users to reduce the logging enforcement level, allowing them to specify alternative log files in their deployments by starting their containers by passing "-f" to the FOREGROUND_FLAG value.

Once this is merged, we need to update the image docs to inform users about the new methods.

[Sergio Durigan Junior (sergiodj)]

Thanks, Athos.

I'm thinking about the new flag, and while I do agree with the overall idea I believe that we could implement it in a "nicer" way for the user. I'd like to take redis' "start-redi.sh" entrypoint script as an example. In it, you will find the $ALLOW_EMPTY_PASSWORD variable, which controls whether the redis server can be started without a password or not. What I like about that variable is that it abstracts the logic behind this operation, so that the user doesn't have have to bother about setting up a password{,less} redis.

WDYT about following the same approach here? We could have a $FOREGROUND flag that takes a "yes/no" value and decides the actual CLI parameters that need to be passed to named.

review: Needs Information

[Athos Ribeiro (athos-ribeiro)]

Hi Sergio,

We could do that, however, the idea here is to pass either -g or -f to named, and both mean "foreground". Do you have any different name suggestions for the variable name?

[Sergio Durigan Junior (sergiodj)]

Ah, right. What about $LOG_TO_STDERR or $STDERR_ONLY_LOG ?

[Sergio Durigan Junior (sergiodj)]

Or $STDERR_LOG_ONLY, maybe?

[Athos Ribeiro (athos-ribeiro)]

Thanks, Sergio!

On a second thought, I believe we should avoid trying to predict different use cases for our images (by bloating entrypoint scripts).

Instead, users should overwrite entrypoints or commands to accommodate for specific use cases. This way, we could focus on having lean entrypoints that would start services with, if not the default, the most common use case for a component (when possible). This seems to be coherent with the distro practices on how systemd services are crafted and configured.

Still, I agree this specific use case is common enough and the bug reporter request is indeed fair, and simple enough for us to include the requested option.

Upstream's bin/named/server.c contains the following function:

 if (named_g_logstderr) {
  const cfg_obj_t *logobj = NULL;

  isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
         NAMED_LOGMODULE_SERVER, ISC_LOG_INFO,
         "not using config file logging "
         "statement for logging due to "
         "-g option");

Moreover, by reading bin/named/logconf.c (channel_fromconf) we can also infer that, in the config file, users must pick one (and only one) of "a file", "stderr", "syslog", or "null" (meaning they can still chose to log to stderr from there).

Therefore, a good name for the variable could be USE_CONFIG_FILE_LOGGING. This variable would have the opposite semantics of what was suggested, so it will be unset, and if it contains any value (test -n), then we will use the "-f" option to read user set logging configuration (just like the redis example you mentioned).

For instance, running `podman run -e USE_CONFIG_FILE_LOGGING=1 -v $CUSTOM_CONF:/etc/named.conf docker.io/ubuntu/bind9:latest` would force named to read the logging params from $CUSTOM_CONF, if any.

Thoughts?

[Sergio Durigan Junior (sergiodj)]

On Friday, October 07 2022, Athos Ribeiro wrote:

> On a second thought, I believe we should avoid trying to predict different use cases for our images (by bloating entrypoint scripts).

That's another way to look at this problem, indeed :-). When we started
the project, we were experimenting with different ways to tackle this
problem and I liked how the redis image turned out, but you certainly
have more hands-on experience with what's expected from OCI images in
general.

> Instead, users should overwrite entrypoints or commands to accommodate for specific use cases. This way, we could focus on having lean entrypoints that would start services with, if not the default, the most common use case for a component (when possible). This seems to be coherent with the distro practices on how systemd services are crafted and configured.
>
> Still, I agree this specific use case is common enough and the bug reporter request is indeed fair, and simple enough for us to include the requested option.
>
> Upstream's bin/named/server.c contains the following function:
>
> if (named_g_logstderr) {
> const cfg_obj_t *logobj = NULL;
>
> isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
> NAMED_LOGMODULE_SERVER, ISC_LOG_INFO,
> "not using config file logging "
> "statement for logging due to "
> "-g option");
>
> Moreover, by reading bin/named/logconf.c (channel_fromconf) we can also infer that, in the config file, users must pick one (and only one) of "a file", "stderr", "syslog", or "null" (meaning they can still chose to log to stderr from there).
>
> Therefore, a good name for the variable could be USE_CONFIG_FILE_LOGGING. This variable would have the opposite semantics of what was suggested, so it will be unset, and if it contains any value (test -n), then we will use the "-f" option to read user set logging configuration (just like the redis example you mentioned).
>
> For instance, running `podman run -e USE_CONFIG_FILE_LOGGING=1 -v $CUSTOM_CONF:/etc/named.conf docker.io/ubuntu/bind9:latest` would force named to read the logging params from $CUSTOM_CONF, if any.
>
> Thoughts?

I like it. Thanks for doing a deeper analysis of the problem. +1 from
my side.

Cheers,

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Sergio Durigan Junior (sergiodj)]

LGTM, thanks.

review: Approve

diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh
index d04bfee..acfb0a1 100755
--- a/docker-entrypoint.sh
+++ b/docker-entrypoint.sh
@@ -1,6 +1,15 @@
 #!/bin/bash
 set -e
 
+# Allow users to pick the foreground flag to be used. This is useful if the
+# user wants to log to a file. In this case, set USE_CONFIG_FILE_LOGGING to any
+# value. Otherwise, any logging configuration will be ignored and logs will be
+# written to STDERR.
+FOREGROUND_FLAG="-g"
+if [[ -n "${USE_CONFIG_FILE_LOGGING}" ]]; then
+  FOREGROUND_FLAG="-f"
+fi
+
 # allow arguments to be passed to named
 if [[ "${1:0:1}" == '-' ]]; then
     EXTRA_ARGS="${*}"
@@ -17,8 +26,8 @@ BIND9_USER="${BIND9_USER:-bind}"
 # default behaviour is to launch named
 if [[ -z "${1}" ]]; then
     echo "Starting named..."
-    echo "exec $(which named) -u \"${BIND9_USER}\" -g \"${EXTRA_ARGS}\""
-    exec $(command -v named) -u "${BIND9_USER}" -g ${EXTRA_ARGS}
+    echo "exec $(which named) -u \"${BIND9_USER}\" \"${FOREGROUND_FLAG}\" \"${EXTRA_ARGS}\""
+    exec $(command -v named) -u "${BIND9_USER}" "${FOREGROUND_FLAG}" ${EXTRA_ARGS}
 else
     exec "${@}"
 fi