Apply the configured Tornado settings, with debug mode off when deployed, and trim the deployment config to its own settings

README.md

@@ -29,6 +29,53 @@ The documentation is maintained at the [biothings_explorer](https://github.com/b
 as each knowledgebase API will be integrated into the [BioThings Explorer application](https://explorer.biothings.io) become
 a Translator's standard KP (Knowledge Provider) API.
 
+## Configuration
+
+Settings are read from `src/nodenorm/config/config.default.json`, which is set up for local
+development (Elasticsearch on localhost, Tornado debug mode on, telemetry off). A file passed with
+`--conf` is laid over it section by section, so it only needs the keys it changes. The Docker image
+passes `docker/configuration/config.json`, which holds the deployment's own settings: the in-cluster
+Elasticsearch host, telemetry on, and debug mode off (in debug mode Tornado puts tracebacks in its
+error pages). It also repeats `ES_INDEX` and `ES_ALIAS`, though they match the default today: they
+name the index in the deployment's cluster, so pinning them means a change to the default for local
+development can't silently repoint production. The cost is that a rename has to be made in both
+files. Other settings the deployment leaves out follow the default, so a change there reaches
+production on the next build. `tests/test_configuration.py` checks that the deployment file names no
+setting the default lacks. Apart from `deployment_environment`, which `NODENORM_DEPLOYMENT_ENV`
+overrides (see [below](#reporting-the-deployment-environment)), the app reads none of these settings
+from environment variables, so that is the only one the Helm chart sets. Letting the environment
+override the others (the chart used to pass `ES_HOST`, `OPENTELEMETRY_ENABLED` and `PORT`, which the
+app never read) was considered and left out for now: no deployment has needed a different
+Elasticsearch host; `PORT` can't be honoured, since 8000 is Caddy's port and the app runs on 9001
+behind it; and once honoured, an unsubstituted `ES_HOST_VALUE` placeholder or a bare hostname from a
+values file would go live and stop the pod starting. It is a small change in `load_configuration()`
+if the need arises; the question is open in
+[issue #60](https://github.com/biothings/NodeNormalizationAPI/issues/60).
+
+### Checking the deployment image locally
+
+The image clones its code and configuration from `NODENORM_BRANCH`, so push the branch first.
+Build it as Jenkins does, with `docker/` as the context, and run it against an Elasticsearch
+container that answers to the in-cluster hostname and holds one index with Babel metadata behind
+the `annotator-nodenorm` alias (discovery and `/status` read `mappings._meta.src.nodenorm`):
+
+```bash
+docker build -f docker/Dockerfile --build-arg NODENORM_REPO=<your fork> --build-arg NODENORM_BRANCH=<branch> -t nodenorm:check docker/
+docker network create nodenorm-check
+docker run -d --name nodenorm-check-es --network nodenorm-check \
+  --network-alias elasticsearch.es-core-components.svc.cluster.local \
+  -e discovery.type=single-node -e xpack.security.enabled=false docker.elastic.co/elasticsearch/elasticsearch:8.19.3
+# once it answers:
+docker exec nodenorm-check-es curl -s -X PUT localhost:9200/nodenorm_check -H 'Content-Type: application/json' \
+  -d '{"mappings":{"_meta":{"src":{"nodenorm":{"version":"2025sep1"}}}},"aliases":{"annotator-nodenorm":{}}}'
+docker run -d --name nodenorm-check-app --network nodenorm-check -p 127.0.0.1:18000:8000 nodenorm:check
+```
+
+`/status`, `/`, `/version` and `/webapp/openapi.json` should answer 200 and `/openapi.json` redirect
+to the webapp copy. Stopping the Elasticsearch container and requesting
+`/get_normalized_nodes?curie=MESH:D014867` should give a 500 whose body is the bare error page: a
+traceback there means debug mode is on.
+
 ## Running Node Normalization locally
 
 With the project installed in `.venv`, start the server from the repository root:

deploy/templates/deployment.yaml

@@ -26,17 +26,15 @@ spec:
         - name: {{ .Values.containers.name }}
           image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
           imagePullPolicy: {{ .Values.image.pullPolicy }}
+          # The Elasticsearch host, port and telemetry settings come from
+          # docker/configuration/config.json, baked into the image; the app reads no env vars for them.
+          # Env overrides were considered and deferred: see the README's Configuration section and
+          # https://github.com/biothings/NodeNormalizationAPI/issues/60.
           env:
             - name: NODENORM_DEPLOYMENT_ENV
               valueFrom:
                 fieldRef:
                   fieldPath: metadata.labels['gov.nih.ncats.appenv']
-            - name: ES_HOST
-              value: {{ .Values.containers.es_host }}
-            - name: OPENTELEMETRY_ENABLED
-              value: "{{ .Values.env.OPENTELEMETRY_ENABLED_VALUE }}"  
-            - name: PORT
-              value: "{{ .Values.containers.port }}"      
           ports:
             - name: http
               containerPort: {{ .Values.containers.port }}

deploy/values.yaml

@@ -24,12 +24,8 @@ image:
 
 containers:
   name: node-normalization
-  es_host: ES_HOST_VALUE
   port: 8000
 
-env:
-  OPENTELEMETRY_ENABLED_VALUE: True
-
 service:
   type: ClusterIP
   port: 80

docker-compose.yml

@@ -5,8 +5,8 @@ services:
       context: .
       dockerfile: ./docker/Dockerfile
       args:
-        - ARG NODENORM_REPO=https://github.com/biothings/NodeNormalizationAPI.git
-        - ARG NODENORM_BRANCH=main
+        - NODENORM_REPO=https://github.com/biothings/NodeNormalizationAPI.git
+        - NODENORM_BRANCH=main
     container_name: nodenorm
     ports:
-      - 9000:9000
+      - 9000:8000  # Caddy listens on 8000 inside the container

docker/Dockerfile

@@ -24,16 +24,17 @@ COPY --from=builder --chown=nodenorm:nodenorm /build/wheels /home/nodenorm/wheel
 RUN /home/nodenorm/venv/bin/pip install /home/nodenorm/wheels/*.whl && rm -rf /home/nodenorm/wheels
 
 WORKDIR /home/nodenorm/configuration
-COPY --from=builder --chown=nodenorm:nodenorm /build/nodenorm/src/nodenorm/config/config.default.json /home/nodenorm/configuration/default.json
 COPY --from=builder --chown=nodenorm:nodenorm /build/nodenorm/version.txt /home/nodenorm/configuration/version.txt
 
-# Override with corrected configuration
-COPY --chown=nodenorm:nodenorm configuration/config.json /home/nodenorm/configuration/config.json
+# The configuration comes from the same clone as the code, not from the build context, so the
+# two always match (config.json relies on the code's section-by-section merge) and the image
+# builds the same from any context. It is laid over the packaged config.default.json.
+COPY --from=builder --chown=nodenorm:nodenorm /build/nodenorm/docker/configuration/config.json /home/nodenorm/configuration/config.json
 
 # Switch back to root for system-level operations
 USER root
-COPY configuration/supervisord.conf /etc/supervisor/supervisord.conf
-COPY configuration/Caddyfile /etc/caddy/Caddyfile
+COPY --from=builder /build/nodenorm/docker/configuration/supervisord.conf /etc/supervisor/supervisord.conf
+COPY --from=builder /build/nodenorm/docker/configuration/Caddyfile /etc/caddy/Caddyfile
 COPY --from=caddy_builder /usr/bin/caddy /usr/bin/caddy
 # Switch back to nodenorm user for runtime
 USER nodenorm

docker/configuration/config.json

@@ -1,33 +1,17 @@
 {
     "webserver": {
-        "HOST": "localhost",
-        "PORT": 8000,
-        "ENABLE_CURL_CLIENT": true,
         "SETTINGS": {
             "debug": false,
             "autoreload": false
         }
     },
-    "api": {
-        "API_PREFIX": "nodenorm",
-        "API_VERSION": ""
-    },
     "elasticsearch": {
         "ES_HOST": "http://elasticsearch.es-core-components.svc.cluster.local:9200",
         "ES_INDEX": "annotator-nodenorm",
-        "ES_ALIAS": "annotator-nodenorm", 
-        "ES_DOC_TYPE": "node",
-        "ES_INDICES": {},
-        "ES_ARGS": {
-            "sniff": false,
-            "request_timeout": 60
-        }
+        "ES_ALIAS": "annotator-nodenorm"
     },
     "telemetry": {
         "OPENTELEMETRY_ENABLED": true,
-        "OPENTELEMETRY_SERVICE_NAME": "NodeNorm",
-        "OPENTELEMETRY_JAEGER_HOST": "http://jaeger-otel-collector.sri",
-        "OPENTELEMETRY_JAEGER_PORT": 4318,
-        "OPENTELEMETRY_EXCLUDED_URLS": ["^/$", "^/status$", "^/version$", "^/webapp", "^/openapi\\.json$", "^/favicon\\.ico$"]
+        "OPENTELEMETRY_JAEGER_HOST": "http://jaeger-otel-collector.sri"
     }
 }

src/nodenorm/application.py

@@ -25,6 +25,8 @@ def get_app(cls, namespace: NodeNormalizationAPINamespace):
         handlers = build_handlers()
         namespace.populate_handlers(handlers)
         settings = namespace.config.webserver["SETTINGS"]
+        # As keywords: Application's second positional parameter is default_host, where these
+        # settings used to land unused, so debug and autoreload never took effect.
         app = cls(handlers.values(), **settings)
         app.biothings = namespace
         return app

src/nodenorm/namespace.py

@@ -26,6 +26,21 @@
 BABEL_REFRESH_TIMEOUT = 10
 
 
+def merge_configuration(base: dict, override: dict) -> dict:
+    """Overlay one configuration on another, recursing into nested sections.
+
+    An override file then only needs the keys it changes: {"elasticsearch": {"ES_HOST": ...}}
+    replaces ES_HOST and keeps the rest of the section. Lists and other values are replaced whole.
+    """
+    merged = dict(base)
+    for key, value in override.items():
+        if isinstance(value, dict) and isinstance(merged.get(key), dict):
+            merged[key] = merge_configuration(merged[key], value)
+        else:
+            merged[key] = value
+    return merged
+
+
 class NodeNormalizationAPINamespace:
     """Simplied namespace instance for our NodeNormalization API."""
 
@@ -273,31 +288,30 @@ def load_configuration(self, option_configuration: tornado.options.OptionParser)
         Loads the default configuration file found in ~/src/nodenorm/config/config.default.json
         to create a base structure. It then checks if an optional configuration file was provided
         via the command line options. We override the default configuration file with this option
-        based file. We check any other command line options and then return our configuration
-        data structure. Static frontend routes are defined explicitly by build_handlers.
+        based file, merging nested sections key by key (see merge_configuration). We then check
+        any other command line options and return our configuration data structure. Static
+        frontend routes are defined explicitly by build_handlers.
         """
-        configuration = {}
-
         # note we have to use this format as relative paths were only supported for
         # importlib.resources.read_text in python3.13
         default_configuration_path = importlib.resources.files(nodenorm) / "config" / self.default_configuration
         with default_configuration_path.open("r", encoding="utf-8") as handle:
-            default_configuration = json.load(handle)
-
-        configuration.update(default_configuration)
+            configuration = json.load(handle)
 
         optional_configuration_file = getattr(option_configuration, "conf", None)
         if optional_configuration_file is not None:
             optional_configuration = pathlib.Path(optional_configuration_file).absolute().resolve()
             if optional_configuration.exists():
                 with open(optional_configuration, "r", encoding="utf-8") as handle:
-                    configuration.update(json.load(handle))
+                    configuration = merge_configuration(configuration, json.load(handle))
 
         deployment_environment = os.getenv("NODENORM_DEPLOYMENT_ENV") or configuration["deployment_environment"]
         if not isinstance(deployment_environment, str):
             raise ValueError("deployment_environment must be a string")
         configuration["deployment_environment"] = deployment_environment.strip() or "unknown"
 
+        # build_handlers() serves / and /webapp/ itself. Don't set Tornado's static_path here: with
+        # a static_url_prefix of "/", Tornado adds a catch-all route ahead of every API handler.
         configuration_namespace = types.SimpleNamespace(**configuration)
 
         # override options

tests/test_configuration.py

@@ -0,0 +1,92 @@
+"""
+The deployment configuration (docker/configuration/config.json, passed with --conf) is laid
+over the packaged default (src/nodenorm/config/config.default.json), so it holds only the
+settings the deployment changes.
+"""
+
+import json
+import pathlib
+from types import SimpleNamespace
+
+import tornado.web
+from tornado.testing import AsyncHTTPTestCase
+
+from nodenorm.application import NodeNormalizationAPI
+from nodenorm.namespace import NodeNormalizationAPINamespace, merge_configuration
+
+DEPLOYMENT_CONFIGURATION = pathlib.Path(__file__).parent.parent / "docker" / "configuration" / "config.json"
+
+
+def namespace(conf=None) -> NodeNormalizationAPINamespace:
+    """A namespace configured by the real loader, without the Elasticsearch connection the constructor makes."""
+    namespace = NodeNormalizationAPINamespace.__new__(NodeNormalizationAPINamespace)
+    namespace.handlers = {}
+    namespace.default_configuration = "config.default.json"
+    namespace.config = namespace.load_configuration(SimpleNamespace(conf=conf, host=None, port=None))
+    return namespace
+
+
+def load(conf=None) -> SimpleNamespace:
+    return namespace(conf).config
+
+
+def keys(configuration: dict, prefix: str = "") -> set[str]:
+    """Every key path in a configuration, such as "elasticsearch.ES_ARGS.sniff"."""
+    paths = set()
+    for key, value in configuration.items():
+        paths.add(prefix + key)
+        if isinstance(value, dict):
+            paths |= keys(value, prefix + key + ".")
+    return paths
+
+
+def test_merge_keeps_the_keys_an_override_leaves_out():
+    base = {"elasticsearch": {"ES_HOST": "http://localhost:9200", "ES_ARGS": {"sniff": False, "request_timeout": 60}}}
+    override = {"elasticsearch": {"ES_HOST": "http://es:9200", "ES_ARGS": {"request_timeout": 5}}}
+
+    assert merge_configuration(base, override) == {
+        "elasticsearch": {"ES_HOST": "http://es:9200", "ES_ARGS": {"sniff": False, "request_timeout": 5}}
+    }
+    assert base["elasticsearch"]["ES_HOST"] == "http://localhost:9200", "the base must not be modified"
+
+
+def test_merge_replaces_lists_whole():
+    merged = merge_configuration({"URLS": ["^/$", "^/status$"]}, {"URLS": ["^/webapp"]})
+    assert merged == {"URLS": ["^/webapp"]}
+
+
+def test_deployment_configuration_only_names_settings_the_default_has():
+    # A misspelt key in the override would otherwise be added silently and change nothing.
+    default = vars(load())
+    deployment = json.loads(DEPLOYMENT_CONFIGURATION.read_text(encoding="utf-8"))
+    assert keys(deployment) <= keys(default)
+
+
+def test_deployment_configuration_is_complete_and_not_in_debug_mode():
+    default = vars(load())
+    deployed = vars(load(str(DEPLOYMENT_CONFIGURATION)))
+
+    assert keys(deployed) == keys(default)
+    # Tornado's debug mode serves tracebacks in error pages and reloads on file changes.
+    assert deployed["webserver"]["SETTINGS"]["debug"] is False
+    assert deployed["webserver"]["SETTINGS"]["autoreload"] is False
+    assert deployed["elasticsearch"]["ES_HOST"] != default["elasticsearch"]["ES_HOST"]
+    assert deployed["telemetry"]["OPENTELEMETRY_ENABLED"] is True
+
+
+class TestDeployedApplication(AsyncHTTPTestCase):
+    """The application get_app() builds from the deployment configuration."""
+
+    def get_app(self) -> tornado.web.Application:
+        return NodeNormalizationAPI.get_app(namespace(str(DEPLOYMENT_CONFIGURATION)))
+
+    def test_settings_reach_tornado(self):
+        # Regression: get_app() passed the settings positionally, into default_host, so none applied.
+        assert self._app.settings["debug"] is False
+        assert self._app.settings["autoreload"] is False
+        assert "static_path" not in self._app.settings, "a static_path adds a catch-all route ahead of the API"
+
+    def test_routes_still_reach_their_handlers(self):
+        assert self.fetch("/version").code == 200
+        assert self.fetch("/webapp/openapi.json").code == 200
+        assert self.fetch("/").code == 200