core: add warning when disabling activity #10485
Conversation
vault/logical_system_activity.go
Outdated
| @@ -186,8 +188,13 @@ func (b *SystemBackend) handleActivityConfigUpdate(ctx context.Context, req *log | |||
| { | |||
| // Parse the enabled setting | |||
| if enabledRaw, ok := d.GetOk("enabled"); ok { | |||
| if config.Enabled == "enable" && enabledRaw.(string) == "disable" { | |||
There was a problem hiding this comment.
enabledRaw could also be "default".
I think it would be better to have SetConfig return the warnings list, so we can handle the 3x3 matrix "enable", "disable", "default" correctly all in one place. The code there already interprets "default" correctly, so all you have to do is check the original and the final value of a.enabled.
There was a problem hiding this comment.
But then we have to change the method signature and all corresponding unit tests. Also, SetConfig(ctx, config) (warnings []string) seems like a strange API, no? What's wrong with just having these checks here?
There was a problem hiding this comment.
I would like to avoid duplicating the logic to parse the enabled state and apply it correctly. (On the other hand, the parsing does appear three times in the code already...)
It should be fine to check a.enabled before and after SetConfig instead, but we would probably then need a new IsEnabled() method that handles the mutex locking correctly to avoid setting off the race detector.
There was a problem hiding this comment.
Sorry, I'm a bit lost here. Is the following not correct?
if config.Enabled == "enable" && (enabledStr == "disable" || enabledStr == "default") {
warnings = append(warnings, "...")
}I'm not doing any additional parsing here. I'm just checking the new value against the current value.
There was a problem hiding this comment.
It's correct on OSS but not on enterprise. You would need to incorporate activityLogEnabledDefault, or move the check into a function that is similarly set up to have oss and ent versions. Something like:
if ( config.Enabled == "enable" || (config.Enabled == "default" && activityLogEnabledDefault)) &&
(enabledStr == "disable" || (enabledStr == "default" && !activityLogEnabledDefault)) {
...
}
|
@alexanderbez I've tentatively milestoned this for 1.6.1, but depending on where the quotas work is, it might go to 1.6.2. Can you update or confirm the milestone on Monday please? |
|
I've changed the milestone to 1.6.2 so we can try to move ahead with staging. |
vault/logical_system_activity.go
Outdated
| if config.Enabled == "enable" && enabledStr == "disable" || | ||
| !activityLogEnabledDefault && config.Enabled == "enable" && enabledStr == "default" { | ||
| warnings = append(warnings, "the current monthly segment will be deleted because the activity log was disabled") | ||
| } |
There was a problem hiding this comment.
There are three transitions where we need to warn
enabled => disabled // part before ||, OK
enabled => default && !activityLogEnabledDefault // part after ||, OK
default && activityLogEnabledDefault => disabled // I don't see how this gets handled
There was a problem hiding this comment.
default && activityLogEnabledDefault => disabled It's not handling this. I'll fix it.
* core: add warning when disabling activity (#10485) * Fix setting Activity Log enable flag through the API (#10594) Co-authored-by: swayne275 <[email protected]>
No description provided.