feat(nextcloud): add notify_push support

[wrenix]

Pull Request

Description of the change

Not yet tested

Benefits

• support notify_push
• improve ServiceMonitor (to collect data from nextcloud-exporter and notify_push)

Possible drawbacks

Applicable issues

• fixes [Feature] High Performance File Backend #109
• closes Feature: client push #551

Additional information

Checklist

• DCO has been signed off on the commit.
• Chart version bumped in Chart.yaml according to semver.
• (optional) Variables are documented in the README.md

TODO

• redis password from existing secret (or URL)
• put redis password from env to secrets
• solve bootstrap problem (nextcloud and notify_push needs to be online)
• option to set urlEnv on notifyPush (for external redis)

[AndreKoepke]

Can we ensure that the notify_push-plugin is installed? Maybe something like this?

 lifecycle:
      postStart:
        exec:
          command: ["occ",  "app:install notify_push"]

And we have to active that plugin by running sudo -u www-data ./occ notify_push:setup https://NEXTCLOUD_HOST/push. Maybe we should add a little script like this (pseudocode):

if (!notify_push installed)
  install notify_push
/occ notify_push:setup https://NEXTCLOUD_HOST/push

[provokateurin]

I think it makes sense to give this option so the installation and setup is completely automatic, but I'd rather put it behind a second config flag:

notify_push:
	enabled: true
	automatic_setup: true

Maybe some people don't want to have this done automatically, so it's nice to give them the option.

[wrenix]

Therefore we has that hook of the container script (see #525), i write a ConfigMap for it.

PS: the same way, then in #480 (@provokateurin you wanted to take a look there ...)

PSS: Does somebody test this/my code?

[AndreKoepke]

andre@server:~/k8s/nextcloud$ helm upgrade -n nextcloud akops-nextcloud -f values.yml ./helm/charts/nextcloud/
Error: UPGRADE FAILED: template: nextcloud/templates/notify_push/deployment.yaml:41:31: executing "nextcloud/templates/notify_push/deployment.yaml" at <$.Values.global.image.registry>: nil pointer evaluating interface {}.image

[AndreKoepke]

When I remove the global-references, then I got this error:

andre@server:~/k8s/nextcloud$ helm upgrade -n nextcloud akops-nextcloud -f values.yml ./helm/charts/nextcloud/
Error: UPGRADE FAILED: YAML parse error on nextcloud/templates/notify_push/deployment.yaml: error converting YAML to JSON: yaml: line 41: did not find expected key

[wrenix]

Oh sorry, that was a copy-paste error.
normally i has on my helm-charts a global.image part to overwrite for registry and so on.

[AndreKoepke]

I was able to try an install. The result was this:

Configuring Redis as session handler
=> Searching for scripts (*.sh) to run, located in the folder: /docker-entrypoint-hooks.d/before-starting
==> Running the script (cwd: /var/www/html): "/docker-entrypoint-hooks.d/before-starting/notify_push.sh"
notify_push already installed
✓ redis is configured
🗴 push server is not receiving redis messages (received 272721789, got 0)
==> Failed at executing "/docker-entrypoint-hooks.d/before-starting/notify_push.sh". Exit code: 1

---

Edit

I have a password for redis and it was not set. Can add this like this?

            - name: REDIS_URL
              value: "redis://:<PASSWORD>@{{ template "nextcloud.redis.fullname" . }}-master:{{ .Values.redis.master.service.ports.redis }}"

When I did this locally, then we have the chicken-egg-problem ...
The notify_push application needs a running nextcloud-instance to fully start. And the nextcloud-instance need a running notify_push.

Maybe there is a better hook after nextcloud has started?

[AndreKoepke]

With the fixed port, I still unable to run it.

Logs from notify_push pod:

[2024-06-19 06:44:36.746890 +00:00] ERROR [notify_push] src/main.rs:78: Self test failed: Error while communicating with nextcloud instance: error sending request for url (http://akops-nextcloud.nextcloud.svc.cluster.local:8080/index.php/apps/notify_push/test/version): error trying to connect: tcp connect error: Connection refused (os error 111)

[jessebot]

@wrenix I'm updated your push file to be .tpl instead of .gotmpl to match the rest of the tpl files. I may also pop in here and write a quick test so this gets tested in CI?

[wrenix]

I move the env handling back to the helper (and split it).
So we have nearly the same logic for database(_url) and redis_url for nextcloud and notify-push.

So it should be review ready (and merge ready).

I create an CHANGELOG.md for the Breaking Changes.

[provokateurin]

This looks mostly good to me, but I have some reservations.

You should also add some documentation to the readme and update the values tables.

@jessebot can you also take a look?

[provokateurin]

Hardcoded to https? This seems wrong to me.
Also accessing it through the external host shouldn't be necessary and wouldn't work if the port is different.

[provokateurin]

Or alternatively the hostname in the container should be bent to localhost similar to https://github.com/NixOS/nixpkgs/blob/a23b0bdc68f56700d03aec9598eeeae6975f0ade/nixos/modules/services/web-apps/nextcloud-notify_push.nix#L138-L141

[wrenix]

I am not sure, but is this url not used by the nextcloud client? so a bent to localhost looks strange for me.

(i use the ingress.tls parameter to check if it is set ...)

[provokateurin]

This doesn't feel good to me, anything that is not at least semi-official (like the nextcloud docker image) is a risky dependency to add.

[wrenix]

I understand you, and i would prefer to change that.
For the current state:

• Tagged docker images & Docker Hub repo notify_push#106 on project it-self

i find that current solution okay (not all images are from nextcloud company, e.g. nextcloud-exporter for prometheus).

Maybe somebody from nextcloud could internal request for a official container ala nextcloud/notify_push#106

Or you could try the aio container: https://hub.docker.com/r/nextcloud/aio-notify-push/tags (which use a other container then notify-push itself )

[provokateurin]

Could you try the AIO container? I'd be more comfortable with that, but if it doesn't work we can use this image for now.

[wrenix]

this image has an strange behavour:
https://github.com/nextcloud/all-in-one/blob/main/Containers/notify-push/Dockerfile + https://github.com/nextcloud/all-in-one/blob/main/Containers/notify-push/start.sh

It runs on /nextcloud data from another container with path:

/nextcloud/custom_apps/notify_push/bin/"$CPU_ARCH"/notify_push

It does not contain the notify_push binary (just shell scripts) ...
and a own / independent environment variable management.

[provokateurin]

Suggested change

	/var/www/html/occ app:install notify_push
	/var/www/html/occ app:enable notify_push

This will both install and enable the app. I'd expect that the app is automatically enabled, if it was disabled but installed previously.

[provokateurin]

Why is this commented out?
Does the command not work due to startup order creating a circular dependency?
Either way I don't think a commented out command is helpful, so please remove one of them.

[wrenix]

Yes, it is a startup circular dependency.
Should I write a comment instatt of removing it?
(So that other person would not change that could to use notify_push:setup)

[provokateurin]

Yes please

[provokateurin]

Why is the port changed? This could be an unintended and unnecessary breaking change for some setups.

[wrenix]

Why?:
The service port is independent of the pod port. I make it equal to the new notify_push port.
I choose 9100 to be equal with the node-exporter the most famous exporter. So more or less the default port of exporters ...

Breaking Change:
I do not know if it is really one. Prometheus access it on name and technical aspect like networkpolicy just look for pod-ports ... But yes, if any unknown service outside of that helm-chart use that service it is one.

For me, it does not matter which port I use here, so two option in announce Changelog or roll it back.

[provokateurin]

Ok fine, just add it to the changelog to avoid surprises.

[provokateurin]

This is a bit ugly, a generic option like we already have for the server deployment would be nicer:

helm/charts/nextcloud/values.yaml

Lines 259 to 264 in 70403bc

	extraEnv:
	# - name: SOME_SECRET_ENV
	# valueFrom:
	# secretKeyRef:
	# name: nextcloud
	# key: secret_key

[wrenix]

Hmm okay, I need to change the logic and would drop the fail check if something is set for redis.

[provokateurin]

So we only test that the deployment succeeds, could we use the notify_push self-test check (or whatever it is called) to ensure it works correctly?
Or would that already be covered automatically because the deployments won't succeed otherwise?

[wrenix]

Hmm, I make some thoughts therefore.
(No there is no self-test at this moment)