inveniosoftware / helm-invenio

Helm charts for deploying an Invenio instance
https://helm-invenio.readthedocs.io
8 stars 21 forks source link

Support for custom AMQP protocol and external INVENIO_SEARCH_HOSTS #125

Closed fradeve closed 5 days ago

fradeve commented 3 months ago

Description

This PR solves 3 issues with the Helm chart

RabbitMQ protocol can't be customized

The default protocol for RabbitMQ is amqp and can't be edited -- this is probably fine when running RabbitMQ locally, but most hosted RMQ instances will only accept connections on amqps; this PR adds support for a custom protocol when using .Values.rabbitmqExternal; in any other case, amqp is used as a default.

Files affected:

Impossible to define custom Search username, password or port

The easiest way to use an external {Open/Elastic}Search instance is to pass INVENIO_SEARCH_HOSTS in the environment; however, the way INVENIO_SEARCH_HOSTS is set in the Configmap:

INVENIO_SEARCH_HOSTS: {{ printf "[{'host': '%s'}]" (include "invenio.opensearch.hostname" .) | quote }}

means that only the hostname can be configured, without port, user or password; this is of course unworkable when using an hosted {Elastic/Open}Search that needs proper authentication to be passed to the application.

A possible way around this is to add the INVENIO_SEARCH_HOSTS string into Values.invenio.extra_config; this will however force Helm to have 2 INVENIO_SEARCH_HOSTS in the Configmap -- which won't work either.

In this PR, if INVENIO_SEARCH_HOSTS is set in extra_config, it is removed from the Configmap so we don't get this defined twice, avoiding clashes and allowing the app to connect to an externally hosted search engine.

Files affected:

Web service type cannot be changed

service.type defaults to ClusterIP, however when using some cloud reverse proxies like Traefik, this should be set to NodePort and currently there is no way to change it. I have added Values.web.service.type; if not specified, it defaults to ClusterIP -- which is the current default anyway.

Files affected

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.
Samk13 commented 1 week ago

Thank you for your contribution. However, there's a PR introducing changes where the application constructs connection strings from environment variables. If merged, this might make the modifications in this PR redundant or require adjustments to align with the new approach. @lindhe could you please take a look at this?

lindhe commented 1 week ago

You are right that our planned changes to connection strings would change things relating to this PR. But I don't think it's worth waiting for that.

If we merge this first, it's just a little more work for our future improvement. But if we wait for that first, than it risks delaying these improvements unnecessarily.

So I think we should not block this PR, but it's worth a heads-up that related changes are coming. And that may or may not be breaking changes in the Helm chart, depending on how easy it is for me to be backwards compatible with the change.

egabancho commented 1 week ago

🤔 We are running these changes already in our test instance with no issues. I'll take a look and see if I can reproduce it locally.

lindhe commented 1 week ago

I think I found the issues, I posted suggestions. Currently untested, I'll get back later.

lindhe commented 1 week ago

I finally got around to testing my changes. Having bare strings will break things if rabbitmq.enabled=true. But if we wrap them in template strings according to my suggestions, it works. If you fix that, I'll approve. :)