Support for custom AMQP protocol and external INVENIO_SEARCH_HOSTS

[fradeve]

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:

• charts/invenio/templates/_helpers.tpl

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:

• charts/invenio/templates/invenio-configmap.yaml

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

• charts/invenio/templates/web-service.yaml
• charts/invenio/values.yaml

Checklist

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

• I'm aware of the code of conduct.
• I've created logical separate commits and followed the commit message format.
• I've added relevant test cases.
• I've added relevant documentation.
• I've marked translation strings.
• I've identified the copyright holder(s) and updated copyright headers for touched files (>15 lines contributions).
• I've NOT included third-party code (copy/pasted source code or new dependencies).
  • If you have added third-party code (copy/pasted or new dependencies), please reach out to an architect.

Frontend

• I've followed the CSS/JS and React guidelines.
• I've followed the web accessibility guidelines.
• I've followed the user interface guidelines.

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]

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]

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.

[lindhe]

I think this is looking very promising. I don't see the issue, but when I run helm template foo ./charts/invenio -f ~/tmp/invenio-foo-values.yaml it crashes. Here are the values I use:

invenio:
  hostname: foo
  secret_key: foo
  security_login_salt: foo
  csrf_secret_salt: foo
postgresql:
  auth:
    password: foo
rabbitmq:
  enabled: true
  auth:
    password: foo

I'll try to debug later, but I don't have time right now. Please see if you can find the bug before I do. 🙃

[egabancho]

🤔 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]

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

[lindhe]

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. :)

[lindhe]

I think it looks good now, thank you for your contribution!

Next time it would be great if you created one PR per logical change. Despite you doing exemplary work by creating three logically separate commits, GitHub's review system really only lets us review the aggregate changes. Also there's only a single PR title that people can search for, while your PR has three changes. So one PR per logical change would be even more helpful.

But that's for next time. This time: well done! 👍