refactor: Remove nginx proxy and use Ingress directly #5
Conversation
- Remove nginx proxy sidecar container from deployment
- Update service to expose Odoo port (8069) directly
- Configure Ingress with proper Odoo annotations for proxy settings
- Remove nginx configmap (keep only maintenance page configs)
- Bump chart version to 0.3.0
This change simplifies the architecture by leveraging Kubernetes Ingress
controller capabilities instead of an additional nginx proxy layer.
Breaking change: Service port changed from 80 to 8069
Signed-off-by: Alexandre Nuttinck <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Odoo Helm chart architecture by removing the nginx proxy sidecar container and using Kubernetes Ingress capabilities directly. This reduces complexity and resource usage while maintaining the same functionality.
Key Changes:
- Removed nginx proxy sidecar container and related ConfigMap
- Modified service to expose Odoo directly on port 8069
- Enhanced Ingress annotations with Odoo-specific proxy settings
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| values.yaml | Updated service port from 80 to 8069 and added Odoo-specific Ingress annotations for proxy configuration |
| templates/service.yaml | Changed service name from {release}-nginx to {release} and updated port naming from nginx-http to odoo-http |
| templates/ingress.yaml | Updated service reference to use the new service name without -nginx suffix |
| templates/deployment.yaml | Removed nginx proxy container and associated volume mounts/volumes |
| templates/configmap.yaml | Removed nginx ConfigMap containing proxy configuration |
| Chart.yaml | Bumped chart version from 0.2.7 to 0.3.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| annotations: | ||
| cert-manager.io/cluster-issuer: letsencrypt | ||
| nginx.ingress.kubernetes.io/backend-protocol: HTTP | ||
| nginx.ingress.kubernetes.io/force-ssl-redirect: "true" |
There was a problem hiding this comment.
The proxy-body-size annotation is set to '0' which disables size limits. Consider documenting this in a comment, as it allows unlimited upload sizes which may have security or performance implications.
| nginx.ingress.kubernetes.io/force-ssl-redirect: "true" | |
| nginx.ingress.kubernetes.io/force-ssl-redirect: "true" | |
| # Setting proxy-body-size to "0" disables upload size limits. | |
| # This allows unlimited upload sizes, which may have security or performance implications. | |
| # Ensure this is intentional. To set a limit, use e.g. "50m" for 50 megabytes. |
| - name: {{ include "..fullname" . }}-nginx-conf | ||
| mountPath: /etc/nginx/conf.d/default.conf | ||
| subPath: nginx.conf | ||
| - name: {{ include "..fullname" . }}-service |
There was a problem hiding this comment.
The container name includes '-service' suffix which may be confusing now that the nginx proxy is removed. Consider renaming to just use the fullname template without the suffix for clarity.
| - name: {{ include "..fullname" . }}-service | |
| - name: {{ include "..fullname" . }} |
| - name: odoo-http | ||
| port: {{ .Values.service.port }} | ||
| targetPort: nginx-http | ||
| targetPort: odoo-http |
There was a problem hiding this comment.
The targetPort references 'odoo-http' but there is no corresponding containerPort with this name defined in the deployment. The Odoo container needs to define a port with name 'odoo-http' and containerPort 8069.
| {{- if $maintenanceEnabled }} | ||
| name: {{ $fullName }}-maintenance-page | ||
| port: | ||
| number: 80 |
There was a problem hiding this comment.
The hardcoded port number 80 is inconsistent with the service port change to 8069. This port reference should be updated to match the new service port.
| number: 80 | |
| number: {{ $svcPort }} |
| proxy_set_header X-Forwarded-Host $http_host; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| proxy_set_header X-Real-IP $remote_addr; |
There was a problem hiding this comment.
The configuration snippet doesn't include WebSocket upgrade headers that were present in the nginx ConfigMap. Consider documenting whether WebSocket support (previously handled via /websocket location in nginx) is now managed differently or if additional configuration is needed.
| proxy_set_header X-Real-IP $remote_addr; | |
| proxy_set_header X-Real-IP $remote_addr; | |
| proxy_set_header Upgrade $http_upgrade; | |
| proxy_set_header Connection "upgrade"; |
This PR refactors the Helm chart to remove the nginx proxy sidecar container and leverage Kubernetes Ingress controller capabilities directly.
Changes
Benefits
Breaking Changes
Testing Checklist