feat: upgrade nginx#6
Open
razbroc wants to merge 10 commits into
Open
Conversation
…ents and remove pool timeout Co-authored-by: Copilot <copilot@github.com>
This was referenced Jul 5, 2026
Closed
- drop leftover nginx.conf/default.conf configmap keys that referenced deleted files and rendered as empty strings (empty /etc/nginx/nginx.conf crashes nginx with 'no "events" section in configuration') - mount extension files individually via subPath instead of mounting the whole configmap as the extensions directory - disable the chart's built-in backend proxy_pass, which conflicted with uwsgi_pass injected by mapproxy-location.conf Matches the merged nginx upgrades in pycsw, file-download-server and geoserver-polygon-parts (MAPCO-10119). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The default extraVolumes referenced {{ .Release.Name }}-nginx-configmap,
but the configmap is named by nginx-configmap.fullname as
<release>-mapproxy-nginx-configmap; a standalone install with default
values left the nginx pod stuck in ContainerCreating.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The common nginx chart hardcodes server-level Access-Control add_headers; MapProxy already emits its own CORS headers (CORS_ENABLED), so responses carried Access-Control-Allow-Origin twice and browsers rejected them. An add_header in nginx.mapproxy.conf (included by every proxy location) stops inheritance of the server-level set, keeping MapProxy the single CORS owner as it was before the chart upgrade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Helm nginx dependency and refactors the MapProxy NGINX configuration from a single monolithic config into multiple modular fragments, while also adjusting default tracing and Redis TLS settings.
Changes:
- Upgraded the
nginxHelm dependency to2.1.5and switched to the sharedcommonchart repository. - Split NGINX configuration into modular files (
mapproxy-http.conf,mapproxy-server.conf,mapproxy-location.conf) and updated the NGINX ConfigMap + mounts accordingly. - Adjusted observability-related defaults (NGINX OpenTelemetry/prometheus exporter values; MapProxy telemetry toggles; trace hook attribute cleanup in
src/app.py) and changed Redis TLS cert verification defaults.
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/app.py |
Removes selected OpenTelemetry span attributes from Redis/Boto hooks to reduce trace attribute noise. |
helm/values.yaml |
Updates nginx image/dependency settings, config mounting strategy, observability knobs, and Redis/telemetry defaults. |
helm/templates/nginx/nginx-configmap.yaml |
Refactors the NGINX ConfigMap data keys to ship the new modular NGINX config fragments. |
helm/templates/mapproxy/mapproxy-configmap.yaml |
Removes REDIS_POOL_TIMEOUT env wiring from the MapProxy ConfigMap. |
helm/config/nginx.mapproxy.conf |
Adds a location-level header to avoid duplicate CORS headers from the new common nginx chart behavior. |
helm/config/nginx.conf |
Removes the legacy top-level nginx.conf (now expected to be provided by the nginx chart/image). |
helm/config/mapproxy-http.conf |
Adds http-context fragments (upstreams + optional uwsgi_cache_path). |
helm/config/mapproxy-server.conf |
Converts server config into an extension fragment (no longer defines the whole server {} block). |
helm/config/mapproxy-location.conf |
Adds a location-context fragment intended to back the root location behavior. |
helm/config/log_format.conf |
Renames log format and switches trace/span variables to new names. |
helm/Chart.yaml |
Upgrades nginx dependency version and changes dependency repository path. |
helm/Chart.lock |
Updates locked dependency metadata for the upgraded nginx chart. |
Dockerfile |
Removes REDIS_POOL_TIMEOUT and changes default SSL_CERT_REQS. |
Comments suppressed due to low confidence (1)
helm/config/mapproxy-server.conf:122
auth_request /_validate_jwt;is still used when Opala/JWT auth is enabled, but this chart no longer defines the corresponding internallocation = /_validate_jwt(nor the supportingjs_content/OPA proxy locations) anywhere in the rendered ConfigMap fragments. A repo-wide search shows only theseauth_requestcall sites remain, so enabling.Values.nginx.authorization.enabled+.Values.global.opalaEnabledwill likely break auth (subrequest to an undefined URI). Either reintroduce the internal auth locations in an NGINX config fragment that is guaranteed to be included in the same server, or remove/guard theseauth_requestdirectives if the new common/nginx chart now supplies the auth endpoint.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+9
to
+14
| # Timeout settings valid in location context | ||
| proxy_connect_timeout 600; | ||
| proxy_send_timeout 600; | ||
| send_timeout 600; | ||
| client_body_timeout 600; | ||
| fastcgi_read_timeout 300; |
| poolTimeout: 0.1 | ||
| healthCheckInterval: 0 | ||
| sslCertReqs: "required" | ||
| sslCertReqs: "optional" # 'required'/'optional'/'none' |
| REDIS_TLS="false" \ | ||
| REDIS_HEALTH_CHECK_INTERVAL="0" \ | ||
| SSL_CERT_REQS="required" | ||
| SSL_CERT_REQS="optional" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to the Helm chart and configuration for the NGINX-based MapProxy deployment. The main themes are modernization of dependencies, a major refactor of NGINX configuration management, and enhancements to observability and maintainability. The changes include updating the NGINX chart dependency, reorganizing NGINX configuration files for better modularity, adding new configuration options for observability, and cleaning up application-level tracing hooks.
Helm Chart and NGINX Configuration Refactor:
nginxHelm dependency to version 2.1.5 and changed its repository to use the sharedcommonchart, ensuring better maintainability and compatibility. [1] [2]default.confinto modular files (mapproxy-server.conf,mapproxy-http.conf,mapproxy-location.conf), and updated the ConfigMap and volume mounts to reflect this structure. This improves clarity, reusability, and future extensibility. [1] [2] [3] [4] [5] [6] [7] [8]nginx.confin favor of a more modular and Helm-driven configuration approach.Observability and Instrumentation:
values.yaml, enabling improved tracing and metrics collection for NGINX.Application Tracing Cleanup:
src/app.py, reducing unnecessary data in traces. [1] [2]