Move RabbitMQ env-var credential override into a dedicated RabbitMQConfig class - #65
Move RabbitMQ env-var credential override into a dedicated RabbitMQConfig class#65bladeroot wants to merge 5 commits into
Conversation
…nfig class
connection_parameters() in RabbitMQ.py isn't the right place to decide
whether credentials come from an env var or the config file - it's a
config-dict -> pika.ConnectionParameters builder, not a config-resolution
layer. It's also invoked from two different entry points (Daemon.consume()
for heppyd, bin/heppyc directly) that were each separately assembling the
RabbitMQ sub-config already (config.get('RabbitMQ', {}) + queue default),
so the override logic had no single natural owner.
RabbitMQConfig now owns resolving the final RabbitMQ.* dict: config file
values, the default 'heppy-<name>' queue name, and RABBITMQ_USERNAME/
RABBITMQ_PASSWORD env-var overrides - used identically by Daemon.py and
bin/heppyc. RabbitMQ.py's connection_parameters() goes back to a plain
config-dict reader, same as before the env-var override existed.
Also fixes a correctness bug from that override: it used `or`, so a
present-but-empty env var (e.g. a secretKeyRef resolving to "") silently
fell back to the config file's value instead of overriding - reintroducing
the exact kind of drift the override was meant to prevent. RabbitMQConfig
checks presence (`in os.environ`) instead.
|
Warning Review limit reached
Next review available in: 3 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRabbitMQ configuration resolution now uses ChangesRabbitMQ configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR centralizes RabbitMQ credential and queue configuration resolution while preserving the connection-parameter builder behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Daemon.consume() shouldn't decide when/how to resolve its own transport config either - same reasoning as RabbitMQ.py earlier in this branch. bin/heppyd and bin/reppyd both construct Daemon and already assemble Config there; they now resolve RabbitMQConfig too and hand Daemon the finished dict via a required constructor argument, matching how bin/heppyc already hands RPCClient an already-resolved rabbit_config. Daemon no longer imports RabbitMQConfig or touches os.environ, even indirectly. reppyd resolves it too, even though its SocketServer path never uses it today - consume()'s branch is decided by config content, not by which bin/ script launched it, so both entrypoints need to hand Daemon a real rabbit_config rather than relying on a silent None default.
Same _FILE convention the official rabbitmq Docker image already uses for RABBITMQ_DEFAULT_PASS_FILE, so a deployment can point both the broker and every worker at the same mounted secret file instead of (or in addition to) plain env vars, without inventing a new convention. Setting both a var and its _FILE counterpart for the same key raises rather than silently picking one - consistent with the "presence, not truthiness" rule already applied to the plain env vars.
…TIALS_FILE
The previous _FILE-per-key design was justified as "the same convention
the official rabbitmq Docker image uses" - that's factually wrong. The
official image's docker-entrypoint.sh actively rejects
RABBITMQ_DEFAULT_USER_FILE/RABBITMQ_DEFAULT_PASS_FILE with a hard error
("set but deprecated") since 3.9, pointing users at a real config file
instead - it doesn't support that pattern, this version included.
Replaced with a single RABBITMQ_CREDENTIALS_FILE pointing at one JSON file
shaped {"username": ..., "password": ...} - matching how the credential
pair is already kept together as one Kubernetes Secret
(ahnames-epp-rabbitmq) rather than two, so that Secret can be mounted and
read as one unit. Setting it together with either plain env var is still
rejected outright, same "presence, not truthiness" and "no silent
precedence" rules as before.
dict(self.config.get('RabbitMQ', {})) leaned on .get()'s default
parameter to express "this section or an empty dict" - correct, but the
branch was implicit. _config_section() spells out the same two outcomes
as an explicit if/return instead.
1 participant
Summary
Follow-up to #64 (HQD-355).
connection_parameters()inRabbitMQ.pyisn't the right place to decide whether credentials come from an env var or the config file — it's a config-dict →pika.ConnectionParametersbuilder, not a config-resolution layer. It's also called from two independent entry points (Daemon.consume()forheppyd,bin/heppycdirectly) that were each separately assembling theRabbitMQsub-config already, so the override logic had no single natural owner.RabbitMQConfigclass owns resolving the finalRabbitMQ.*dict: config file values, the defaultheppy-<name>queue name, andRABBITMQ_USERNAME/RABBITMQ_PASSWORDenv-var overrides — used identically byDaemon.pyandbin/heppyc.RabbitMQ.py'sconnection_parameters()goes back to a plain config-dict reader, same as before the env-var override existed in HQD-355: Stop duplicating the RabbitMQ broker password into every EPP worker's config #64.or, so a present-but-empty env var (e.g. asecretKeyRefresolving to"") silently fell back to the config file's value instead of overriding — reintroducing the exact kind of credential drift the override was meant to prevent.RabbitMQConfigchecks presence (in os.environ) instead of truthiness.Test plan
python3 -m unittest discover -s tests— 137 passed (moved/extended the env-var tests fromtest_RabbitMQ.pyinto a newtest_RabbitMQConfig.py, added a test for the empty-env-var case)Summary by CodeRabbit
New Features
Bug Fixes
Tests