fix OpenResty crash (#7)

* chmod the built .so (not sure if it matters)
* avoid use of ngx_core_conf_t

Author: dgoffredo

.circleci/config.yml

@@ -41,6 +41,14 @@ jobs:
         destination: nginx-tag
 
   test:
+    parameters:
+      # I'm hacking in testing support for OpenResty.
+      # If this parameter is not empty, then the runner (below) will override
+      # the docker image used inside of the tests, so it's an OpenResty image
+      # instead of an nginx image.
+      openresty-image:
+        type: string
+        default: ""
     docker:
     # "cimg" means "convenience image."  Helpful images provided by CircleCI.
     # This one contains Python, docker-compose, and a bunch of other things.
@@ -67,7 +75,13 @@ jobs:
     # TODO: This sucks, though.  Find a better way.  The output of a failed test
     # runner should be red in the CI interface.
     - run: |
-        if test/bin/run --verbose; then
+        openresty_image=<< parameters.openresty-image >>
+        if [ "$openresty_image" != '' ]; then
+          export NGINX_MODULES_DIRECTORY=/usr/local/openresty/nginx/modules
+          export NGINX_IMAGE=$openresty_image
+          export NGINX_CONF_PATH=/usr/local/openresty/nginx/conf/nginx.conf
+        fi 
+        if test/bin/run --verbose --failfast; then
           touch .tests-succeeded
         fi
     - store_artifacts:
@@ -83,6 +97,29 @@ jobs:
 workflows:
   build-and-test-all:
     jobs:
+    # In addition to the jobs generated by `../bin/generate_jobs_yaml.sh`,
+    # I hard-code the latest OpenResty equivalent, just for testing.
+    - test:
+        name: "test 1.21.4-alpine OpenResty"
+        openresty-image: "openresty/openresty:1.21.4.1-alpine"
+        requires:
+        - "build 1.21.4-alpine"
+        filters:
+          tags:
+            only: /^v[0-9]+\.[0-9]+\.[0-9]+/
+    # I'm commenting out the Debian-based version of OpenResty because, as of
+    # this writing, their nginx is built without --with-compat, which means
+    # that our module fails the module signature check.
+    # - test:
+    #     name: "test 1.21.4 OpenResty"
+    #     openresty-image: "openresty/openresty:1.21.4.1-bullseye"
+    #     requires:
+    #     - "build 1.21.4"
+    #     filters:
+    #       tags:
+    #         only: /^v[0-9]+\.[0-9]+\.[0-9]+/
+
+    # The following are generated by `../bin/generate_jobs_yaml.sh`.
     - build:
         name: "build 1.23.2-alpine"
         build-image: "datadog/docker-library:nginx-datadog-build-1.23.2-alpine"

.dockerignore

@@ -0,0 +1,2 @@
+.build/
+.docker-build/

Makefile

@@ -9,6 +9,7 @@ MAKE_JOB_COUNT ?= $(shell nproc)
 .PHONY: build
 build: build-deps nginx/objs/Makefile sources
 	mkdir -p $(BUILD_DIR) && cd $(BUILD_DIR) && cmake -DBUILD_TESTING=OFF .. && make -j $(MAKE_JOB_COUNT) VERBOSE=1
+	chmod 755 $(BUILD_DIR)/libngx_http_datadog_module.so
 	@echo 'build successful 👍'
 
 .PHONY: sources

src/datadog_conf.h

@@ -13,6 +13,7 @@ extern "C" {
 #include <opentracing/span.h>
 
 #include <string>
+#include <vector>
 
 namespace datadog {
 namespace nginx {
@@ -22,12 +23,17 @@ struct datadog_tag_t {
   NgxScript value_script;
 };
 
-struct conf_directive_source_location {
+struct conf_directive_source_location_t {
   ngx_str_t file_name;       // e.g. "nginx.conf"
   ngx_uint_t line;           // line number within the file `file_name`
   ngx_str_t directive_name;  // e.g. "proxy_pass"
 };
 
+struct environment_variable_t {
+  std::string name;
+  std::string value;
+};
+
 struct datadog_main_conf_t {
   ngx_array_t *tags;
   // `is_tracer_configured` is whether the tracer has been configured, either
@@ -48,7 +54,7 @@ struct datadog_main_conf_t {
   // > Configuration already loaded to default configuration by
   // > [[source location]].  Explicit configuration must appear before
   // > the first [[directive name]].
-  conf_directive_source_location tracer_conf_source_location;
+  conf_directive_source_location_t tracer_conf_source_location;
   // `are_log_formats_defined` is whether we have already injected `log_format`
   // directives into the configuration.  The directives define Datadog-specific
   // access log formats; one of which will override nginx's default.
@@ -58,6 +64,13 @@ struct datadog_main_conf_t {
   // directive).
   bool are_log_formats_defined;
   ngx_array_t *span_context_keys;
+  // This module automates the forwarding of the environment variables in
+  // `TracingLibrary::environment_variable_names()`. Rather than injecting
+  // `env` directives into the configuration, or mucking around with the core
+  // module configuration, instead we grab the values from the environment
+  // of the master process and apply them later in the worker processes after
+  // `fork()`.
+  std::vector<environment_variable_t> environment_variables;
 };
 
 struct datadog_loc_conf_t {

src/datadog_directive.cpp

@@ -76,9 +76,9 @@ char *set_tracer(const ngx_command_t *command, ngx_conf_t *conf, string_view tra
   main_conf->is_tracer_configured = true;
   main_conf->tracer_conf = to_ngx_str(conf->pool, tracer_conf);
   main_conf->tracer_conf_source_location =
-      conf_directive_source_location{.file_name = conf->conf_file->file.name,
-                                     .line = conf->conf_file->line,
-                                     .directive_name = command->name};
+      conf_directive_source_location_t{.file_name = conf->conf_file->file.name,
+                                       .line = conf->conf_file->line,
+                                       .directive_name = command->name};
 
   // In order for span context propagation to work, the names of the HTTP
   // headers added to requests need to be known ahead of time.

src/ngx_http_datadog_module.cpp

@@ -5,6 +5,7 @@
 #include <cstdlib>
 #include <exception>
 #include <iterator>
+#include <string>
 #include <utility>
 
 #include "datadog_conf.h"
@@ -25,6 +26,7 @@ extern "C" {
 #include <ngx_config.h>
 #include <ngx_core.h>
 #include <ngx_http.h>
+#include <stdlib.h>  // ::setenv
 }
 
 // clang-format off
@@ -68,7 +70,6 @@ static ngx_uint_t anywhere =
   | NGX_HTTP_MAIN_CONF;  // the toplevel configuration, e.g. where modules are loaded
 
 static ngx_command_t datadog_commands[] = {
-
     { ngx_string("opentracing"),
       anywhere | NGX_CONF_TAKE1,
       toggle_opentracing,
@@ -213,7 +214,7 @@ static ngx_command_t datadog_commands[] = {
       NGX_HTTP_LOC_CONF_OFFSET,
       0,
       nullptr},
-
+  
     ngx_null_command
 };
 
@@ -247,52 +248,7 @@ ngx_module_t ngx_http_datadog_module = {
 };
 // clang-format on
 
-// Configure nginx to set the environment variable as indicated by the
-// specified `entry` in the context of the specified `cycle`.  `entry` is a
-// string in one of the following forms:
-//
-// 1. "FOO"
-// 2. "FOO=value"
-//
-// The environment variable name in this example is "FOO".  In the case of the
-// first form, the value of the environment variable will be inherited from the
-// parent process.  In the case of the second form, the value of the
-// environment variable will be as specified after the equal sign.
-//
-// Note that `ngx_set_env` is adapted from the function of the same name in
-// `nginx.c` within the nginx source code.
-static void *ngx_set_env(string_view entry, ngx_cycle_t *cycle) {
-  ngx_core_conf_t *ccf = (ngx_core_conf_t *)ngx_get_conf(cycle->conf_ctx, ngx_core_module);
-
-  ngx_str_t *value, *var;
-  ngx_uint_t i;
-
-  var = (ngx_str_t *)ngx_array_push(&ccf->env);
-  if (var == NULL) {
-    return NGX_CONF_ERROR;
-  }
-
-  const ngx_str_t entry_str = to_ngx_str(entry);
-  *var = entry_str;
-
-  for (i = 0; i < var->len; i++) {
-    if (var->data[i] == '=') {
-      var->len = i;
-      return NGX_CONF_OK;
-    }
-  }
-
-  return NGX_CONF_OK;
-}
-
 static ngx_int_t datadog_master_process_post_config(ngx_cycle_t *cycle) noexcept {
-  // Forward tracer-specific environment variables to worker processes.
-  for (const auto &env_var_name : TracingLibrary::environment_variable_names()) {
-    if (const void *const error = ngx_set_env(env_var_name, cycle)) {
-      return ngx_int_t(error);
-    }
-  }
-
   // If tracing has not so far been configured, then give it a default
   // configuration.  This means that the nginx configuration did not use the
   // `datadog` directive, and did not use any overridden directives, such as
@@ -304,6 +260,16 @@ static ngx_int_t datadog_master_process_post_config(ngx_cycle_t *cycle) noexcept
     main_conf->tracer_conf = ngx_string("");  // default config
   }
 
+  // Forward tracer-specific environment variables to worker processes.
+  std::string name;
+  for (const auto &env_var_name : TracingLibrary::environment_variable_names()) {
+    name = env_var_name;
+    if (const char *value = std::getenv(name.c_str())) {
+      main_conf->environment_variables.push_back(
+          environment_variable_t{.name = name, .value = value});
+    }
+  }
+
   return NGX_OK;
 }
 
@@ -343,6 +309,11 @@ static ngx_int_t datadog_init_worker(ngx_cycle_t *cycle) noexcept try {
     return NGX_OK;
   }
 
+  for (const auto &entry : main_conf->environment_variables) {
+    const bool overwrite = false;
+    ::setenv(entry.name.c_str(), entry.value.c_str(), overwrite);
+  }
+
   std::shared_ptr<ot::Tracer> tracer = load_tracer(cycle->log, str(main_conf->tracer_conf));
   if (!tracer) {
     return NGX_ERROR;

test/bin/run

@@ -4,7 +4,8 @@ cd "$(dirname "$0")/../"
 
 set -e
 
-export NGINX_IMAGE="nginx:$(cat ../nginx-tag)"
+export NGINX_IMAGE=${NGINX_IMAGE:-nginx:$(cat ../nginx-tag)}
+export NGINX_MODULES_DIRECTORY=${NGINX_MODULES_DIRECTORY:-/usr/lib/nginx/modules}
 
 docker-compose build --parallel
 python3 -m unittest "$@"

test/bin/run_parallel

@@ -4,7 +4,8 @@ cd "$(dirname "$0")/../"
 
 set -e
 
-export NGINX_IMAGE="nginx:$(cat ../nginx-tag)"
+export NGINX_IMAGE=${NGINX_IMAGE:-nginx:$(cat ../nginx-tag)}
+export NGINX_MODULES_DIRECTORY=${NGINX_MODULES_DIRECTORY:-/usr/lib/nginx/modules}
 docker-compose build --parallel
 
 scratch=$(mktemp -d)

test/cases/environment_variables/conf/nginx.conf

@@ -2,6 +2,10 @@ error_log stderr;
 
 load_module modules/ngx_http_datadog_module.so;
 
+# Make sure that if the user overrides the value of an environment variable,
+# that the override is honored.
+env DD_VERSION=overridden;
+
 events {
     worker_connections  1024;
 }

test/cases/environment_variables/test_environment_variables.py

@@ -73,6 +73,10 @@ def test_environment_variables(self):
                     # Env variables that are not on the allow list return "-".
                     self.assertEqual('-', worker_env[variable_name],
                                      error_context)
+                elif variable_name == 'DD_VERSION':
+                    # conf/nginx.conf has an "env" directive that overrides DD_VERSION.
+                    self.assertEqual('overridden', worker_env[variable_name],
+                                     error_context)
                 else:
                     self.assertEqual(value, worker_env[variable_name],
                                      error_context)

test/cases/orchestration.py

@@ -30,6 +30,11 @@
 docker_compose_flags = []
 docker_flags = []
 
+# Depending on which Docker image nginx is running in, the nginx config might be
+# in different locations.  Ideally we could deduce the path to the config file
+# by parsing `nginx -V`, but that doesn't always work (e.g. OpenResty).
+nginx_conf_path = os.environ.get('NGINX_CONF_PATH', '/etc/nginx/nginx.conf')
+
 
 def docker_compose_command(*args):
     return [docker_compose_command_path, *docker_compose_flags, *args]
@@ -56,13 +61,9 @@ def child_env(parent_env=None):
         parent_env = os.environ
 
     result = {}
-    if 'NGINX_IMAGE' in parent_env:
-        result['NGINX_IMAGE'] = parent_env['NGINX_IMAGE']
-    else:
-        # [repo]/test/cases/orchestration.py  ->  [repo]/nginx-tag
-        version = (Path(__file__).resolve().parent.parent.parent /
-                   'nginx-tag').read_text()
-        result['NGINX_IMAGE'] = f'nginx:{version}'
+    for name in ('NGINX_IMAGE', 'NGINX_MODULES_DIRECTORY'):
+        if name in parent_env:
+            result[name] = parent_env[name]
 
     # Forward DOCKER_HOST, DOCKER_TLS_VERIFY, etc.
     for name, value in parent_env.items():
@@ -258,7 +259,9 @@ def print_duration(of_what, output):
     before = time.monotonic()
     yield
     after = time.monotonic()
-    print(f'{of_what} took {after - before} seconds.', file=output, flush=True)
+    print(f'{of_what} took {after - before:.5} seconds.',
+          file=output,
+          flush=True)
 
 
 def docker_compose_services():
@@ -561,7 +564,7 @@ def nginx_replace_config(self, nginx_conf_text, file_name):
             return status, log_lines
 
         script = f"""
->/etc/nginx/nginx.conf cat <<'END_CONF'
+>{nginx_conf_path} cat <<'END_CONF'
 {nginx_conf_text}
 END_CONF
 """

test/docker-compose.yml

@@ -10,6 +10,7 @@ services:
       dockerfile: ./Dockerfile
       args:
         NGINX_IMAGE: ${NGINX_IMAGE}
+        NGINX_MODULES_DIRECTORY: ${NGINX_MODULES_DIRECTORY}
     environment:
       - DD_ENV=prod
       - DD_AGENT_HOST=agent

test/services/nginx/Dockerfile

@@ -2,13 +2,21 @@
 ARG NGINX_IMAGE
 FROM ${NGINX_IMAGE}
 
+# If NGINX_IMAGE is not a core nginx image but instead OpenResty, the file system layout
+# will differ from that expected in the tests' conf files.
+# Put nginx's default HTML in a known location to smooth things over.
+RUN mkdir -p /usr/share/nginx/html
+COPY index.html /usr/share/nginx/html
+
 # Install the `kill` command (`procps` package on Debian), so that one-off
 # nginx instances that are started via `docker-compose exec` can be signaled to
 # gracefully shutdown.
 COPY ./install_tools.sh /tmp/
 RUN /tmp/install_tools.sh
 
-COPY ngx_http_datadog_module.so /usr/lib/nginx/modules
+ARG NGINX_MODULES_DIRECTORY
+RUN mkdir -p ${NGINX_MODULES_DIRECTORY}
+COPY ngx_http_datadog_module.so ${NGINX_MODULES_DIRECTORY}
 
 ENTRYPOINT ["nginx"]
 CMD ["-g", "daemon off;"]

test/services/nginx/index.html

@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Welcome to nginx!</title>
+<style>
+    body {
+        width: 35em;
+        margin: 0 auto;
+        font-family: Tahoma, Verdana, Arial, sans-serif;
+    }
+</style>
+</head>
+<body>
+<h1>Welcome to nginx!</h1>
+<p>If you see this page, the nginx web server is successfully installed and
+working. Further configuration is required.</p>
+
+<p>For online documentation and support please refer to
+<a href="http://nginx.org/">nginx.org</a>.<br/>
+Commercial support is available at
+<a href="http://nginx.com/">nginx.com</a>.</p>
+
+<p><em>Thank you for using nginx.</em></p>
+</body>
+</html>