Repository navigation
CLDSRV-1031: Report the health of every configured KMS - #6347
Conversation
Hello dvasilas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.6 #6347 +/- ##
===================================================
+ Coverage 90.74% 90.78% +0.04%
===================================================
Files 207 207
Lines 38954 38995 +41
===================================================
+ Hits 35348 35402 +54
+ Misses 3606 3593 -13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
My Claude review surfaced that we could have conflicts in the keys used by the healthchecks if we ever chose a location name that matches a kms provider. It's somewhat new since they weren't both freely chosen fields (kms had only a couple of valid names). I think we can go multiple ways, all of which are fine by me:
- namespace the kms fields e.g.
kms:thales/kms:scality(not sure if we can just go ahead in this PR, it could have side effects on s3-frontend and monitoring which I haven't looked at, but I think we are making breaking changes anyway so it might be the right time to do it) - create a ticket and handle later
- ignore it
At first, I thought that conflicts would be very unlikely but I could see a deployment where one would use city names for data location and for KMSs because they would have multiple regions. Still, I don't know how reasonable that is, hence why ignoring is still a valid option IMO.
The effect would be that two conflicting keys would overwrite each other, and one successful healthcheck could hide an unsuccessful one. We might also report the health status from the other one in the monitoring.
|
|
||
| const shouldRefreshCache = healthCache.shouldRefresh(); | ||
|
|
||
| if (shouldRefreshCache) { |
There was a problem hiding this comment.
Suggesting readability nit:
- Call function inline
- Process the negation first so we have fewer indent when processing the "should refresh" part
i.e.
| if (shouldRefreshCache) { | |
| if (!healthCache.shouldRefresh()) { | |
| // Use cached healthcheck result if within a 1-hour window | |
| if (!healthCache.shouldRefresh()) { | |
| logger.debug('using cached KMS health check', { provider, cachedResult }); | |
| return cb(null, cachedResult); | |
| } |
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/improvement/CLDSRV-1031-report-health-of-every-kms && \
git merge origin/development/9.6Resolve merge conflicts and commit git push origin HEAD:improvement/CLDSRV-1031-report-health-of-every-kms |
|
One more thing: I might be confused with which PR is ongoing or where they're pointed but make sure that the regex in Federation pre-checks and the one validating cloudserver config for KMS names match. I think it's currently |
Good catch, I have created S3C-11505 for this. |
My claude review also flagged it, and I first thought a conflict is very unlikely, but your comment changed my mind. I also considered nesting (something like |
8973404 to
917dc94
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The deep health check has one entry per configured KMS, keyed by kms: and its providerName, each with its own cache; KMS failures stay informational.
917dc94 to
9edd6fe
Compare
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue CLDSRV-1031. Goodbye dvasilas. The following options are set: approve |
Context (S3C-11459): Allow a deployment to configure multiple KMSs.
The deep health check currently reports only the default KMS, keyed by its client type (
kmip,aws,fileKms, …). The other configured KMSs are never checked.This PR runs the existing KMS health check for every configured KMS and reports one entry per KMS, keyed by
kms:and itsproviderName. The prefix keeps a KMS from taking the key of another entry, such as a data location with the same name.Example:
KMS health stays informational: a failing KMS fails neither start-up nor readiness, even when every KMS fails. A KMS whose client has no health check reports OK without being contacted; today only the KMIP client has one.