Configure backbeat from the environment, without rewriting the config file - #2809
Open
francoisferrand wants to merge 7 commits into
Open
Configure backbeat from the environment, without rewriting the config file#2809francoisferrand wants to merge 7 commits into
francoisferrand wants to merge 7 commits into
Conversation
Contributor
Hello francoisferrand,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
francoisferrand
changed the base branch from
development/9.0
to
development/9.5
August 18, 2026 16:24
Contributor
DarkIsDude
requested changes
Aug 19, 2026
DarkIsDude
left a comment
DarkIsDude
left a comment
There was a problem hiding this comment.
Sorry for the review. I thought it'll be a fast one but finally not really.
With what we have, if we have for example SERVER_PORT as an env var and a extension.Replication.Server.Port, the env var can have an impact. Those very short env var name and generic, can be very dangerous. Setting a short env var car have a wide impact ? What do you think ? Maybe I'm not very clear, let me know by slack if it's the case.
|
Should land to the next release ? |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.6 #2809 +/- ##
===================================================
+ Coverage 75.77% 75.95% +0.18%
===================================================
Files 200 203 +3
Lines 13922 14007 +85
===================================================
+ Hits 10549 10639 +90
+ Misses 3363 3358 -5
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
francoisferrand
changed the base branch from
development/9.5
to
development/9.6
August 20, 2026 17:39
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
francoisferrand
force-pushed
the
improvement/BB-808
branch
from
August 21, 2026 22:39
06dcc11 to
58986df
Compare
The log configuration only defaulted as a whole object: a section setting just one level left the other undefined, and werelogs was then configured with an undefined dump level. The per-extension log section turned that gap into a hard failure by requiring both levels, which leaves no way to set only one — and an override from the environment always produces such a partial section, since a variable names a single field. Each level now carries its own default, shared by the global and the per-extension schemas, so setting one leaves the other at its default instead of unset or rejected. Issue: BB-808
docker-entrypoint.sh applied every environment variable by rewriting conf/config.json in place with jq. That fails outright when the file is mounted read-only, as zenko does: setting any mapped variable crashed the container at startup. The mapping was also a hand-maintained shell table duplicated from the joi schema, and drifting from it. Overrides are now applied in memory before validation, and their names are derived from the schema itself, so a new configuration field becomes settable from the environment as soon as it is declared. The names that do not follow from the config path are annotated next to the field they name, and the few variables setting several fields at once are kept in an explicit table. Values go through the schema like any other configuration: an invalid one fails the startup instead of being silently ignored. Every variable the entrypoint used to apply keeps working, pinned by a test: Zenko and Federation set them, and Federation forwards arbitrary ones from the field. Two exceptions, both already already broken: * the `EXTENSIONS_LIFECYCLE_RULES_*` and `REDIS_LOCALCACHE_*` variables wrote config sections the schema no longer knows, so setting one made validation reject the whole file and the process fail to start - they are now simply ignored; * `MONGODB_HOSTS` no longer selects the "mongo" log source -not supported anymore- and only sets the replica set hosts of the shared MongoDB connection. Issue: BB-808
Setting a per-extension log level silently reset the dump level to a constant, dropping whatever a deployment had configured globally. An extension now inherits the levels it does not configure from the global log config, so it overrides only what it names — which is also what an override from the environment produces, since a variable names a single field. Extension schemas reach the global configuration through the validation context. The extension validators were already given it, but as the configuration object still being built, so nothing could be read from it. The extension configurations are kept out of it: they are validated one after the other, so referencing one would resolve differently depending on the order of the configuration file, and an extension must not depend on another one. Issue: BB-808
extension validators re-derive the envVarMapping it on every call, memoizing to avoid the redundant cost (~30ms each time). Not critical since it happens only once on startup, but adds up in tests. Issue: BB-808
The complexity was artificial, change mapping to simplify. Issue: BB-808
Move tests of each source file into its own spec, and ensure each module is tested at its own level. Issue: BB-808
francoisferrand
force-pushed
the
improvement/BB-808
branch
from
August 21, 2026 22:40
58986df to
e5ae10f
Compare
DarkIsDude
reviewed
Aug 24, 2026
DarkIsDude
left a comment
DarkIsDude
left a comment
There was a problem hiding this comment.
Check also the CI even if I guess it's related to flakyness
|
|
||
| - `TIME_PROGRESSION_FACTOR`: decreases the weight of a day, to expedite the | ||
| lifecycle of objects. | ||
| - `EXPIRE_ONE_DAY_EARLIER`, `TRANSITION_ONE_DAY_EARLIER`: deprecated in favor of |
There was a problem hiding this comment.
@francoisferrand do we have a follow-up for that ?
Comment on lines
+91
to
+92
| // redis is reached either through its sentinels or directly: the two modes | ||
| // are exclusive, and each has the settings of its own |
There was a problem hiding this comment.
Suggested change
| // redis is reached either through its sentinels or directly: the two modes | |
| // are exclusive, and each has the settings of its own |
It's just what happens one line after 🙏
| port: joi.number().required(), | ||
| })), | ||
| ]).required(), | ||
| // group name of the master the sentinels watch |
There was a problem hiding this comment.
Suggested change
| // group name of the master the sentinels watch |
Comment on lines
+46
to
+47
| // the levels default individually, so that setting one of them, from the | ||
| // configuration or from the environment, leaves the other one alone |
There was a problem hiding this comment.
Suggested change
| // the levels default individually, so that setting one of them, from the | |
| // configuration or from the environment, leaves the other one alone |
| const BOOTSTRAPLIST_MORE = 'EXTENSIONS_REPLICATION_DEST_BOOTSTRAPLIST_MORE'; | ||
|
|
||
| /** | ||
| * Under its historic name, the bootstrap list holds the servers of the single |
There was a problem hiding this comment.
Even if I get the point, I have the feeling that this comment is a more a context that you want to output from your brain than a real useful comment. The comment don't describe what happens for the value provided ?
|
|
||
| const { getField, setField } = require('./fields'); | ||
|
|
||
| // A container exposes a single probe endpoint, shared by all the probe servers |
| */ | ||
| function applyEnvOverrides(config, schema, prefix = [], env = process.env) { | ||
| if (config === null || typeof config !== 'object') { | ||
| // leave the invalid configuration for joi to report |
There was a problem hiding this comment.
If you really wants to keep it, move it under the function description then or the return value ?
DarkIsDude
reviewed
Aug 24, 2026
| zookeeperPath: joi.string().required(), | ||
| bucketTasksTopic: joi.string().required(), | ||
| objectTasksTopic: joi.string().required(), | ||
| bucketTasksTopic: joi.string().required().meta({ env: 'BUCKET_TASK_TOPIC' }), |
There was a problem hiding this comment.
One last thing that came out the review. Should we add tests for that ? I'm mitigated as this is pure config joi but if not caught can be a pain (not sure we have tests for all them in Zenko...).
Contributor
Author
There was a problem hiding this comment.
by "this" do you mean the meta/env annotation? in that case, we actually have tests: these variables are the "legacy" variables, and there are test validating that they indeed are still working as expected.
Contributor
Author
this is the (known) issue about librdkafka revert breaking the newest tests, shall be fixed by rebasing on the fix once merged, just waiting confirmation for which way to go. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The docker entrypoint applied env-var configuration by rewriting conf/config.json in place with jq. That breaks when the config is mounted read-only, which is what zenko-operator does: setting any mapped variable made the container exit at startup. The mapping was also a hand-maintained shell table duplicated from the joi schema, and had drifted from it — LOG_LEVEL rejected the warn and error levels the schema accepts, and a few variables whose config section had since been removed were written back in, so setting one stopped the process from starting.
Overrides are now applied in memory before validation, with their names derived from the schemas themselves: a new configuration field is settable from the environment as soon as it is declared. Names that don't follow from the config path are annotated next to the field they name, which is how the historic ones keep working, and the handful of variables setting several fields at once stay in an explicit table. Values go through the schema like any other configuration, so an invalid one fails the startup instead of being silently ignored. docs/configuration.md documents the whole thing.
Every variable the entrypoint applied is pinned by a test, since zenko-operator and Federation set them and Federation forwards arbitrary ones from the field. Two deliberate exceptions, both already dead: MONGODB_HOSTS no longer selects the mongo log source dropped in BB-836, and the lifecycle rules and local cache variables are ignored rather than fatal.
Removed variables
The following variables are not supported anymore, and will now be ignored. They used to be worse than useless: the entrypoint wrote them into configuration sections the schema no longer knows, and validation then rejected the whole file, so setting one of them stopped the process from starting.
EXTENSIONS_LIFECYCLE_RULES_EXPIRATION_ENABLED,EXTENSIONS_LIFECYCLE_RULES_NC_VERSION_EXPIRATION_ENABLED,EXTENSIONS_LIFECYCLE_RULES_TRANSITIONS_ENABLED,EXTENSIONS_LIFECYCLE_RULES_ABORT_INCOMPLETE_MPU_ENABLED: the rules supported by the lifecycle extension are configured withEXTENSIONS_LIFECYCLE_SUPPORTED_LIFECYCLE_RULES.REDIS_LOCALCACHE_HOST,REDIS_LOCALCACHE_PORT: the local cache is not part of the configuration.MONGODB_HOSTSkeeps settingqueuePopulator.mongo.replicaSetHosts, but no longer selects themongolog source, which the queue populator dropped: a deployment relying on it has to setqueuePopulator.logSourceitself.Issue: BB-808