robjuz / helm-charts

https://robjuz.github.io/helm-charts/index.yaml
34 stars 30 forks source link

nominatim: Add opportunity to configure custom configmap for apache httpd.conf and external database secrets #38

Closed gishh2 closed 1 year ago

gishh2 commented 1 year ago

Hello,

thanks for maintaining this chart. I am setting this up with terraform currently and I am missing config options to:

There is a workaround for the 2nd problem, at least partially.

  1. Set up values.yaml and specify password in cleartext. Install the helm chart.
  2. Remove the helm release from the terraform state
  3. Comment out terraform resource
  4. Remove the cleartext password form the values.yaml by placeholder.
  5. Push to GIT.

However this is quite nasty. For any change you need to import state to terraform again, replace placeholder password by real password etc.

This does not avoid having the external Postgres password written in clear text in the K8s manifest though. Would appreciate such additional config options!

dbt-lucka commented 1 year ago

I need to find a solution for the 000-default.conf configuration myself, as I have a bit different requirements for the apache configuration. I could think of 2 ways to accomplish this:

  1. Add a parameter to configure one's own config map, optionally replacing inclusion of apache-configmap.yaml -> Reference custom config map in Deployment.yaml
  2. Add a parameter to optionally inject an own version of the apache config directly into apache-configmap.yaml -> Deployment.yaml stays untouched, apache-configmap.yaml injects "data" like for instance, the Bitnami Grafana chart does it with grafana.ini config options.

@robjuz Do you have a preference? I can provide a PR for that.

robjuz commented 1 year ago

@dbt-lucka Thanks for the intereset.

You are right, the current solution is not ideal. I personally like the solution bitnami is using in its charts.

someting like this:

apiVersion: v1
kind: ConfigMap
metadata:
  name: {{ include "nominatim.fullname" . }}-apache-configmap
  namespace: {{ include "common.names.namespace" . | quote }}
  labels: {{- include "common.labels.standard" . | nindent 4 }}
    {{- if .Values.commonLabels }}
    {{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 4 }}
    {{- end }}
  {{- if .Values.commonAnnotations }}
  annotations: {{- include "common.tplvalues.render" ( dict "value" .Values.commonAnnotations "context" $ ) | nindent 4 }}
  {{- end }}
data:
  000-default.conf: |-
    {{- include "common.tplvalues.render" ( dict "value" .Values.nominatimUi.apacheConfiguration "context" $ ) | nindent 4 }}

in values.yaml

nominatimUi:
  enabled: true
  version: 3.2.1
  apacheConfiguration: |-
    <VirtualHost *:80>
      DocumentRoot /nominatim/website
      CustomLog "|$/usr/bin/rotatelogs -n 7 /var/log/apache2/access.log 86400" combined
      ErrorLog  "|$/usr/bin/rotatelogs -n 7 /var/log/apache2/error.log 86400"
      LogLevel info

      <Directory "/nominatim/nominatim-ui/dist">
        DirectoryIndex search.html
        Require all granted
      </Directory>

      Alias /ui /nominatim/nominatim-ui/dist

      <Directory /nominatim/website>
        Options FollowSymLinks MultiViews
        DirectoryIndex search.php
        Require all granted

        RewriteEngine On

          # This must correspond to the URL where nominatim can be found.
          RewriteBase "/"

          # If no endpoint is given, then use search.
          RewriteRule ^(/|$)   "search.php"

          # If format-html is explicity requested, forward to the UI.
          RewriteCond %{QUERY_STRING} "format=html"
          RewriteRule ^([^/]+).php ui/$1.html [R,END]
          # Same but .php suffix is missing.
          RewriteCond %{QUERY_STRING} "format=html"
          RewriteRule ^([^/]+) ui/$1.html [R,END]

          # If no format parameter is there then forward anything
          # but /reverse and /lookup to the UI.
          RewriteCond %{QUERY_STRING} "!format="
          RewriteCond %{REQUEST_URI}  "!/lookup"
          RewriteCond %{REQUEST_URI}  "!/reverse"
          RewriteRule ^([^/]+).php ui/$1.html [R,END]
          # Same but .php suffix is missing.
          RewriteCond %{QUERY_STRING} "!format="
          RewriteCond %{REQUEST_URI}  "!/lookup"
          RewriteCond %{REQUEST_URI}  "!/reverse"
          RewriteRule ^([^/]+) ui/$1.html [R,END]
      </Directory>

      AddType text/html .php
    </VirtualHost>
  configuration: |-
    Nominatim_Config.Nominatim_API_Endpoint = '/';

with this approach we would keep the default behavior, (what is good I think) and give the possibility to easily swap the apache config.

What do you think about it?

dbt-lucka commented 1 year ago

Its a good idea. Will provide PR later today - but it looks as if you already finished it ;-) Do you do it or shall I provide a PR for that?

robjuz commented 1 year ago

It would be awesome if you could implement it, test it with default and your use case and provide a PR. Unfortunately I don't have the time right now.

Thx in advance!

dbt-lucka commented 1 year ago

I will do it as a branch from the other PR ok? This way we do not have to raise 2 steps the minor version, but can incorporate 2 features into 3.7.0.

robjuz commented 1 year ago

ok. I will close the other PR

robjuz commented 1 year ago

@dbt-lucka I can't see the new PR.

dbt-lucka commented 1 year ago

I haven't opened it yet. Currently testing my changes on GCP.

dbt-lucka commented 1 year ago

Opened it: #48