enable nginx configuration auto-reload for the nginx deployment in artifactory's helm chart

[bramaq]

Is this a request for help?:

---

Is this a BUG REPORT or FEATURE REQUEST? (choose one): FEATURE REQUEST

Version of Helm and Kubernetes: Kubernetes 1.21 Helm 3.x

Which chart: jfrog/artifactory 107.38.10 app ver: 7.38.10

What happened:

When either a configmap or the secret providing the ssl certificates for nginx changes, the pods have to be deleted to reload the config.

What you expected to happen:

The nginx pods should auto-reload the nginx configuration upon changes to the mounted configmaps and ssl secrets.

The nginx container already has installed inotifyd, which could be pre-configured to watch the following mount points:

• /etc/nginx/nginx.conf
• /var/opt/jfrog/nginx/conf.d/
• /var/opt/jfrog/nginx/ssl/

Upon changes of any of these, inotifyd can trigger 'nginx -s reload'. A similar technique has been described in several blogs; ie: https://cyral.com/blog/how-to-auto-reload-nginx/

How to reproduce it (as minimally and precisely as possible):

• Deploy artifactory with nginx
• Watch kubectl nginx logs
• Edit the x--nginx-artifactory-conf configmap in another terminal

The nginx logs should indicate reloading the config.

Anything else we need to know:

[chukka]

Thanks for this feature request, we will evaluate this internally and come back .

If possible can you provide a PR for this request .

[natw]

I've gotten this working, with the only change to the artifactory chart being adding shareProcessNamespace: true to the nginx pods.

Then I have the following in my values.yaml:

nginx:
    customSidecarContainers: |
      - name: nginx-reloader
        image: dockerpinata/inotify-tools:latest
        command:
          - /bin/sh
          - -cxe
          - |
            while true; do
              inotifywait -e delete_self /certs/tls.crt && killall -SIGHUP nginx
            done
        volumeMounts:
          - name: ssl-certificates
            mountPath: /certs

dockerpinata/inotify-tools is just the least sketchy looking inotify-tools image I saw on dockerhub, but anything with a shell and inotify-tools installed should work.

Adding an option to the chart to enable shareProcessNamespace on the nginx pods seems like a pretty simple change, no? The sidecar container approach seems more idiomatic than trying to shoehorn something into the nginx image itself.

[bramaq]

Natw, this is awesome; but if it still requires altering the chart anyway, would it not be better to rather implement this directly in the nginx container and save having to run an extra sidecar with all the extra implications?

[natw]

eh, I get that argument, but I think the sidecar method is more explicit, more flexible, and doesn't require maintaining another set of patches on top of the nginx image. I like the notion of having containers just do one thing.

Plus, adding a single, backwards compatible option is a lot more likely to get merged upstream. :)

[bramaq]

Explicit having to use an optional customSidecarContainer with a chunk of user edited yaml? Reloading on ssl & config changes by default is just best user experience tbh. I cannot imagine anyone who object to having that, so don't get much the need for being explicit here. The chart should ideally just do it by default.

Forgive me, but I think using customSidecarContainers to enable this is more of a workaround than a proper implementation.

We could also set nginx.enabled=false and inject the entire nginx-related objects via additionalResources but this is just hacky. It is great to have this great flexibility around in the chart (customX, additionalResources), but I hope is use is kept for minimal, less general-use things (ie cert-manager's Certificate objects or CRDs alike that not everyone may need) ?

[bramaq]

ok, I have got this working just changing the nginx container command. Sorting out the legal clearance to push a pr

[chukka]

Fixed via #1682