bmz-UID2-4612 add endpoint to serve operator config#200
Conversation
config endpoint uses vertx FileSystem rather than ConfigRetriever
More detailed error message in handleGetConfig method
BehnamMozafari
requested review from
abuabraham-ttd,
cody-constine-ttd,
jon8787,
mcollins-ttd and
sunnywu
| "refresh_token_expires_after_seconds": 86400, | ||
| "refresh_identity_token_after_seconds": 900, | ||
| "sharing_token_expiry_seconds": 2592000 | ||
| } No newline at end of file |
There was a problem hiding this comment.
suggest using an object ;
OperatorRunTimeConfig (or similar) and using that instead of JsonObject
It will improve readability as we know what exactly we need/have to set in config.
Also in followup MRs; Please make sure we remove the unwanted config from operators
There was a problem hiding this comment.
@BehnamMozafari Could you give more context why we need to use an additional config file for this? The current config hierarchy - default, local, integ, prod config already covers config for each env. I am not sure where operator-config.config is fit into this hierarchy.
If we want to serve the run time config (config might be changed at start time, as the config can also be taken from/overwrited by env vars), we should read them from the run time system and send in api.
There was a problem hiding this comment.
(I will explain, please correct if I am wrong @BehnamMozafari )
this is the config, core servers to operators. We are adding this new operator-config in core that servers runtime config core sets for operators.
Since it is not core config, we added a new file?
There was a problem hiding this comment.
Thanks Abu, yep that's correct, the operator-config is going to be served to operator and will be mounted to core using a configmap.
There was a problem hiding this comment.
suggest using an object ;
OperatorRunTimeConfig (or similar) and using that instead of JsonObject
I agree, this would help in validating any updates to operator-config. I'm considering implementing this in a future MR. However, a concern is that it would require us to redeploy core whenever we want to add a new operator config value to be consumed at run time.
There was a problem hiding this comment.
How would we add a new operator config value with the JsonObject implementation?
There was a problem hiding this comment.
Stronger typing and validation on config updates can reduce risks.
Adding new config_values without an E2E test offers no guarantee it won't break. If it passes E2E tests, it's safe for release, otherwise, prioritizing and addressing those issues would be essential.
There was a problem hiding this comment.
How would we add a new operator config value with the JsonObject implementation?
With the current implementation, we would need to add the new value to the configmap in the deployment repo, we would also have to make the changes in operator to apply the new config value.
| "refresh_token_expires_after_seconds": 86400, | ||
| "refresh_identity_token_after_seconds": 900, | ||
| "sharing_token_expiry_seconds": 2592000 | ||
| } No newline at end of file |
There was a problem hiding this comment.
How would we add a new operator config value with the JsonObject implementation?
- Update tests to use testContext.succeeding to handle failures and exceptions thrown by success case - Error handling to make sure response is sent when an exception is thrown in handleGetConfig. - Inject vertx filesystem into CoreVerticle - Add unit test to cover when operator-config content is invalid JSON
|
|
||
| @Test | ||
| void getConfigInvalidJson(Vertx vertx, VertxTestContext testContext) { | ||
| operatorConfig = "invalid config"; |
There was a problem hiding this comment.
I think it would be better if we check json config on startup to make sure it is valid. That will be easier to debug then having it only checked when operator call.
There was a problem hiding this comment.
Since the config will be updated at runtime through a configmap, it might not be sufficient to only check it at startup, but checking it whenever it has changed seems like a good idea. I'll implement this in a future mr. Thanks for the feedback!
7 participants
/operator/configconf/operator-config.jsonas a JsonObjectoperator-config.jsoncurrently stores config values"identity_token_expires_after_seconds", "refresh_token_expires_after_seconds", "refresh_identity_token_after_seconds" and "sharing_token_expiry_seconds"fromlocal-config.jsonin operator