Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RabbitMQ config issue in Docker #2739

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Fix RabbitMQ config issue in Docker #2739

merged 1 commit into from
Mar 14, 2024

Conversation

Ulincsys
Copy link
Contributor

Description

  • Change RabbitMQ configuration scheme
    • Add definitions.json to contain default user, password, and vhost definitions
    • Add update_config.py to convert Docker args to RabbitMQ config items
      • NOTE: RabbitMQ ignores all envvars if any config file is present
    • I'm not sure if the original augur.conf is doing anything anymore, but there's no reason to get rid of it as of right now
  • Add update_config.py
    • Pull Docker args and check that they exist
    • Compute password hash with rabbitmqctl
      • AFAICT RabbitMQ does not support plain-text passwords in a config file
    • load default definitions.json, update with new values and write changes
  • Provide default advanced.config
    • Previously we were creating this file with RUN echo ...
    • Extra entries are required in this file to enable loading of definitions.json
  • In docker-compose.yml
    • Move environment section to args section, update formatting
    • Make default vhost configurable (defaults to same, augur_vhost)
  • In rabbitmq/Dockerfile
    • Import default args
    • Import new config files described above
    • Add Python to container, and run the config updater at build time

Signed commits

  • Yes, I signed my commits.

@Ulincsys Ulincsys requested a review from sgoggins as a code owner March 13, 2024 22:05
@@ -0,0 +1,41 @@
from os import environ as env

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
C0114: Missing module docstring (missing-module-docstring)

@@ -0,0 +1,41 @@
from os import environ as env
import json, subprocess

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
C0410: Multiple imports on one line (json, subprocess) (multiple-imports)


config_file = Path("/etc/rabbitmq/definitions.json")

with config_file.open() as file:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
W1514: Using open without explicitly specifying an encoding (unspecified-encoding)

with config_file.open() as file:
config = json.load(file)

hash_processor = subprocess.run(f"rabbitmqctl hash_password {rabbit_pass}".split(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
W1510: 'subprocess.run' used without explicitly defining the value for 'check'. (subprocess-run-check)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably set check to True here so it doesn't fail silently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config["permissions"][0]["user"] = rabbit_user
config["permissions"][0]["vhost"] = rabbit_vhost

with config_file.open("w") as file:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
W1514: Using open without explicitly specifying an encoding (unspecified-encoding)

Copy link
Contributor

@IsaacMilarky IsaacMilarky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one subprocess.run call in the build that can fail silently as well as some other minor linting suggestions. Otherwise looks great and is greatly needed.

with config_file.open() as file:
config = json.load(file)

hash_processor = subprocess.run(f"rabbitmqctl hash_password {rabbit_pass}".split(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably set check to True here so it doesn't fail silently.

@IsaacMilarky
Copy link
Contributor

Oh I just saw that it checks the return code never mind.

Copy link
Member

@sgoggins sgoggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sgoggins sgoggins merged commit 99f2db1 into dev Mar 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants