feat(licensing): quota enforcement, client rejection and trial license limits#71
feat(licensing): quota enforcement, client rejection and trial license limits#71philippfalk wants to merge 6 commits into
Conversation
Before this commit, the quota retrieval and distribution of exceeded IDs would be disabled on mgmtd startup if the feature wasn't licensed. This lead to a couple of issues with enforcement consistency: * When a license expired while mgmtd was running, enforcement would continue to work but only until mgmtd restarts * When mgmtd restarted, collection and distribution of exceeded quotas would be disabled, leading to other nodes enforcing based on an old state * When other nodes restarted, they would still be able to initially fetch outdated quota states and enforce based on them. This commit fixes all of the above by not allowing intial downloads of quota state if the feature is not licensed and not entirely disabling the quota distribution mechanism. If the quota feature is not licensed nodes quota collection from the nodes will be disabled for efficiency reasons, but nodes will still receive exceeded quota updates to allow for online changes in license state. These updates will not contain any information about exceeded IDs and will simply clear out the state on the nodes so quotas will no longer be enforced.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Drop the somewhat misleading "Internal" prefix for errors during license verification. These errors are usually caused by invalid or non-existing license files, which isn't an "internal" problem and requires user action.
rustybee42
left a comment
rustybee42
left a comment
There was a problem hiding this comment.
Found another couple of tiny and a slightly bigger structural issue
|
|
||
| // Config entries that should not be changed after initially set. Note that this only controls the | ||
| // functions below, the database entries could still be changed by manual query | ||
| const IMMUTABLE: &[Config] = &[Config::FsUuid, Config::FsInitDateSecs]; |
There was a problem hiding this comment.
issue: Should probably add the new field here to prevent it from being set more than once. Even if the application logic prevents that, just for completeness.
| d.r#type() == CertType::Trial | ||
| && prev_trial_serial.is_some_and(|s| serial != s) | ||
| }) { | ||
| library.init_cert_store(); |
There was a problem hiding this comment.
question: Why is this called again here?
There was a problem hiding this comment.
To clear the cert store. The license we loaded might be valid, so we have to actively clear it if we reject applying it.
| "Initializing licensing library failed. \ | ||
| Licensed features will be unavailable: {err}" |
There was a problem hiding this comment.
nitpick: Potentially incorrect message.
Should be rather like "Loading and verifying license failed"
There was a problem hiding this comment.
Yeah, I thought about that. But it also doesn't really matter to the user. I intended "Initializing library" to mean the entire process of initialization including loading and verifying a license file. But I have changed it. There is another log before on warning level when the license library can not be loaded.
| /// Fetches quota information for all storage targets and updates the quota usage database | ||
| pub(crate) async fn fetch_and_update(app: &impl App) -> Result<()> { | ||
| if app.verify_licensed_feature(LicensedFeature::Quota).is_err() { | ||
| log::info!("Quota is enabled but feature not licensed. Skipping quota collection"); |
There was a problem hiding this comment.
suggestion: This and the other log below should probably rather be warnings.
This is usually unexpected and the user should probably know about this with the default log level (warn).
| log::debug!("Running quota update"); | ||
|
|
||
| match update_and_distribute(&app).await { | ||
| match fetch_and_update(&app).await { |
There was a problem hiding this comment.
suggestion: Replace both with
if let Err(err) = fetch_and_update(&app).await {
log::error!("Updating quota failed: {err:#}")
}
if let Err(err) = distribute_exceeded(&app).await {
log::error!("Distributing exceeded quota failed: {err:#}")
}
| fn authenticate_connection(&mut self); | ||
| fn addr(&self) -> SocketAddr; | ||
| fn msg_id(&self) -> MsgId; | ||
| fn msg_compat_feature_flags(&self) -> u8; |
There was a problem hiding this comment.
issue: Instead of adding access to all the header flags separately, we should provide a borrow to the whole header at this point, e.g. replacing msg_id(&self) -> u8 with header(&self) -> &Header that can then be accessed as a whole (including the compat feature flag) instead.
The impl structs already contain the borrowed header which can just be returned, the test impl can just instantiate a default dummy header which it borrows from.
3 participants
This PR does three separate things related to licensing:
Tagging @iamjoemccormick for reviewing licensing related semantics and @rustybee42 for reviewing the code flow related changes around quotas and licensing.