From cb6683166372d3d80a3a7045cd2feec477dd362c Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Thu, 19 Sep 2024 13:18:50 -0400 Subject: [PATCH 1/9] Track the last PKI auto-tidy time ran for use across nodes - If the interval time for auto-tidy is longer then say a regularly scheduled restart of Vault, auto-tidy is never run. This is due to the time of the last run of tidy is only kept in memory and initialized on startup to the current time - Store the last run of any tidy, to maintain previous behavior, to a cluster local file, which is read in/initialized upon a mount initialization. --- builtin/logical/pki/backend.go | 50 +++++++++++++++++---- builtin/logical/pki/path_tidy.go | 58 ++++++++++++++++++++---- builtin/logical/pki/path_tidy_test.go | 64 +++++++++++++++++++++++++++ builtin/logical/pki/storage.go | 37 ++++++++++++++++ 4 files changed, 191 insertions(+), 18 deletions(-) diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index 4b88b27c2f64..755cc7b409e0 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -131,6 +131,7 @@ func Backend(conf *logical.BackendConfig) *backend { issuing.PathCerts, issuing.PathCertMetadata, acmePathPrefix, + autoTidyLastRunPath, }, Root: []string{ @@ -294,8 +295,8 @@ func Backend(conf *logical.BackendConfig) *backend { conf.System.ReplicationState().HasState(consts.ReplicationDRSecondary) b.crlBuilder = newCRLBuilder(!cannotRebuildCRLs) - // Delay the first tidy until after we've started up. - b.lastTidy = time.Now() + // Delay the first tidy until after we've started up, this will be reset within the initialize function + b.lastAutoTidy = time.Now() b.unifiedTransferStatus = newUnifiedTransferStatus() @@ -320,7 +321,9 @@ type backend struct { tidyStatusLock sync.RWMutex tidyStatus *tidyStatus - lastTidy time.Time + // lastAutoTidy should be accessed through the tidyStatusLock, + // use getAutoTidyLastRun and writeAutoTidyLastRun instead of direct access + lastAutoTidy time.Time unifiedTransferStatus *UnifiedTransferStatus @@ -448,9 +451,38 @@ func (b *backend) initialize(ctx context.Context, ir *logical.InitializationRequ b.GetCertificateCounter().SetError(err) } + // Initialize lastAutoTidy from disk + b.initializeLastTidyFromStorage(sc) + return b.initializeEnt(sc, ir) } +// initializeLastTidyFromStorage reads the time we last ran auto tidy from storage and initializes +// b.lastAutoTidy with the value. If no previous value existed, we persist time.Now() and initialize +// b.lastAutoTidy with that value. +func (b *backend) initializeLastTidyFromStorage(sc *storageContext) { + now := time.Now() + + lastTidyTime, err := sc.getAutoTidyLastRun() + if err != nil { + lastTidyTime = now + b.Logger().Error("failed loading previous tidy last run time, using now", "error", err.Error()) + } + if lastTidyTime.IsZero() { + // No previous time was set, persist now so we can track a starting point across Vault restarts + lastTidyTime = now + if err = b.updateLastAutoTidyTime(sc, now); err != nil { + b.Logger().Error("failed persisting tidy last run time", "error", err.Error()) + } + } + + // We bypass using updateLastAutoTidyTime here to avoid the storage write on init + // that normally isn't required + b.tidyStatusLock.Lock() + defer b.tidyStatusLock.Unlock() + b.lastAutoTidy = lastTidyTime +} + func (b *backend) cleanup(ctx context.Context) { sc := b.makeStorageContext(ctx, b.storage) @@ -708,9 +740,7 @@ func (b *backend) periodicFunc(ctx context.Context, request *logical.Request) er // Check if we should run another tidy... now := time.Now() - b.tidyStatusLock.RLock() - nextOp := b.lastTidy.Add(config.Interval) - b.tidyStatusLock.RUnlock() + nextOp := b.getLastAutoTidyTime().Add(config.Interval) if now.Before(nextOp) { return nil } @@ -724,9 +754,11 @@ func (b *backend) periodicFunc(ctx context.Context, request *logical.Request) er // Prevent ourselves from starting another tidy operation while // this one is still running. This operation runs in the background // and has a separate error reporting mechanism. - b.tidyStatusLock.Lock() - b.lastTidy = now - b.tidyStatusLock.Unlock() + err = b.updateLastAutoTidyTime(sc, now) + if err != nil { + // We don't really mind if this write fails, we'll re-run in the future + b.Logger().Warn("failed to persist auto tidy last run time", "error", err.Error()) + } // Because the request from the parent storage will be cleared at // some point (and potentially reused) -- due to tidy executing in diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index e18540cc334b..f1fa2506d668 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -896,16 +896,20 @@ func (b *backend) pathTidyWrite(ctx context.Context, req *logical.Request, d *fr Storage: req.Storage, } + resp := &logical.Response{} // Mark the last tidy operation as relatively recent, to ensure we don't // try to trigger the periodic function. - b.tidyStatusLock.Lock() - b.lastTidy = time.Now() - b.tidyStatusLock.Unlock() + // NOTE: not sure this is correct as we are updating the auto tidy time with this manual run. Ideally we + // could track when we ran each type of tidy was last run which would allow manual runs and auto + // runs to properly impact each other. + sc := b.makeStorageContext(ctx, req.Storage) + if err := b.updateLastAutoTidyTime(sc, time.Now()); err != nil { + resp.AddWarning(fmt.Sprintf("failed persisting tidy last run time: %v", err)) + } // Kick off the actual tidy. b.startTidyOperation(req, config) - resp := &logical.Response{} if !config.IsAnyTidyEnabled() { resp.AddWarning("Manual tidy requested but no tidy operations were set. Enable at least one tidy operation to be run (" + config.AnyTidyConfig() + ").") } else { @@ -1042,9 +1046,10 @@ func (b *backend) startTidyOperation(req *logical.Request, config *tidyConfig) { // Since the tidy operation finished without an error, we don't // really want to start another tidy right away (if the interval // is too short). So mark the last tidy as now. - b.tidyStatusLock.Lock() - b.lastTidy = time.Now() - b.tidyStatusLock.Unlock() + sc := b.makeStorageContext(ctx, req.Storage) + if err := b.updateLastAutoTidyTime(sc, time.Now()); err != nil { + logger.Error("error persisting last tidy run time", "error", err) + } } }() } @@ -1770,6 +1775,7 @@ func (b *backend) pathTidyStatusRead(_ context.Context, _ *logical.Request, _ *f "acme_account_safety_buffer": nil, "cert_metadata_deleted_count": nil, "cmpv2_nonce_deleted_count": nil, + "last_auto_tidy_finished": b.getLastAutoTidyTime(), }, } @@ -1814,7 +1820,6 @@ func (b *backend) pathTidyStatusRead(_ context.Context, _ *logical.Request, _ *f resp.Data["revocation_queue_deleted_count"] = b.tidyStatus.revQueueDeletedCount resp.Data["cross_revoked_cert_deleted_count"] = b.tidyStatus.crossRevokedDeletedCount resp.Data["revocation_queue_safety_buffer"] = b.tidyStatus.revQueueSafetyBuffer - resp.Data["last_auto_tidy_finished"] = b.lastTidy resp.Data["total_acme_account_count"] = b.tidyStatus.acmeAccountsCount resp.Data["acme_account_deleted_count"] = b.tidyStatus.acmeAccountsDeletedCount resp.Data["acme_account_revoked_count"] = b.tidyStatus.acmeAccountsRevokedCount @@ -1865,8 +1870,18 @@ func (b *backend) pathConfigAutoTidyWrite(ctx context.Context, req *logical.Requ return nil, err } + isAutoTidyBeingEnabled := false + if enabledRaw, ok := d.GetOk("enabled"); ok { - config.Enabled = enabledRaw.(bool) + enabled, err := parseutil.ParseBool(enabledRaw) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("failed to parse enabled flag as a boolean: %s", err.Error())), nil + } + if !config.Enabled && enabled { + // we are turning on auto-tidy reset our persisted time to now + isAutoTidyBeingEnabled = true + } + config.Enabled = enabled } if intervalRaw, ok := d.GetOk("interval_duration"); ok { @@ -1975,6 +1990,13 @@ func (b *backend) pathConfigAutoTidyWrite(ctx context.Context, req *logical.Requ return nil, err } + if isAutoTidyBeingEnabled { + if err := b.updateLastAutoTidyTime(sc, time.Now()); err != nil { + b.Logger().Warn("failed to update last auto tidy run time to now, the first auto-tidy "+ + "might run soon and not at the next delay provided", "error", err.Error()) + } + } + return &logical.Response{ Data: getTidyConfigData(*config), }, nil @@ -2114,6 +2136,24 @@ func (b *backend) tidyStatusIncCMPV2NonceDeletedCount() { b.tidyStatus.cmpv2NonceDeletedCount++ } +// updateLastAutoTidyTime should be used to update b.lastAutoTidy as the required locks +// are acquired and the auto tidy time is persisted to storage to work across restarts +func (b *backend) updateLastAutoTidyTime(sc *storageContext, lastRunTime time.Time) error { + b.tidyStatusLock.Lock() + defer b.tidyStatusLock.Unlock() + + b.lastAutoTidy = lastRunTime + return sc.writeAutoTidyLastRun(lastRunTime) +} + +// getLastAutoTidyTime should be used to read from b.lastAutoTidy as the required locks +// are acquired prior to reading +func (b *backend) getLastAutoTidyTime() time.Time { + b.tidyStatusLock.RLock() + defer b.tidyStatusLock.RUnlock() + return b.lastAutoTidy +} + const pathTidyHelpSyn = ` Tidy up the backend by removing expired certificates, revocation information, or both. diff --git a/builtin/logical/pki/path_tidy_test.go b/builtin/logical/pki/path_tidy_test.go index 46c9b5a89cd2..feca6732bb76 100644 --- a/builtin/logical/pki/path_tidy_test.go +++ b/builtin/logical/pki/path_tidy_test.go @@ -341,6 +341,67 @@ func TestAutoTidy(t *testing.T) { require.Nil(t, resp) } +// TestAutoTidyPersistsAcrossRestarts validates that on initial +// startup of a mount we persisted the current auto tidy time so that +// our counter that auto-tidy is based on isn't reset everytime Vault restarts +func TestAutoTidyPersistsAcrossRestarts(t *testing.T) { + t.Parallel() + + newPeriod := 1 * time.Second + + // This test requires the periodicFunc to trigger, which requires we stand + // up a full test cluster. + coreConfig := &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "pki": Factory, + }, + RollbackPeriod: newPeriod, + } + opts := &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + NumCores: 1, + } + cluster := vault.NewTestCluster(t, coreConfig, opts) + cluster.Start() + defer cluster.Cleanup() + + client := cluster.Cores[0].Client + + // Mount PKI + err := client.Sys().Mount("pki", &api.MountInput{ + Type: "pki", + }) + require.NoError(t, err, "failed mounting pki") + + // Run a tidy that should set us up + _, err = client.Logical().Write("pki/tidy", map[string]interface{}{ + "tidy_cert_store": "true", + }) + require.NoError(t, err, "failed running tidy") + + waitForTidyToFinish(t, client, "pki") + + resp, err := client.Logical().Read("pki/tidy-status") + require.NoError(t, err, "failed reading tidy status") + require.NotNil(t, resp, "response from tidy-status was nil") + lastAutoTidy, exists := resp.Data["last_auto_tidy_finished"] + require.True(t, exists, "did not find last_auto_tidy_finished") + + cluster.StopCore(t, 0) + cluster.StartCore(t, 0, opts) + cluster.UnsealCore(t, cluster.Cores[0]) + vault.TestWaitActive(t, cluster.Cores[0].Core) + + client = cluster.Cores[0].Client + resp, err = client.Logical().Read("pki/tidy-status") + require.NoError(t, err, "failed reading tidy status") + require.NotNil(t, resp, "response from tidy-status was nil") + postRestartLastAutoTidy, exists := resp.Data["last_auto_tidy_finished"] + require.True(t, exists, "did not find last_auto_tidy_finished") + + require.Equal(t, lastAutoTidy, postRestartLastAutoTidy, "values for last_auto_tidy_finished did not match on restart") +} + func TestTidyCancellation(t *testing.T) { t.Parallel() @@ -1302,6 +1363,9 @@ func waitForTidyToFinish(t *testing.T, client *api.Client, mount string) *api.Se if err != nil { return fmt.Errorf("failed reading path: %s: %w", tidyStatusPath, err) } + if statusResp == nil { + return fmt.Errorf("got nil, nil response from: %s", tidyStatusPath) + } if state, ok := statusResp.Data["state"]; !ok || state == "Running" { return fmt.Errorf("tidy status state is still running") } diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index 990d4af6868e..daa9bdc9e10f 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -50,6 +50,8 @@ const ( autoTidyConfigPath = "config/auto-tidy" clusterConfigPath = "config/cluster" + autoTidyLastRunPath = "config/auto-tidy-last-run" + maxRolesToScanOnIssuerChange = 100 maxRolesToFindOnIssuerChange = 10 ) @@ -769,6 +771,41 @@ func (sc *storageContext) writeClusterConfig(config *issuing.ClusterConfigEntry) return sc.Storage.Put(sc.Context, entry) } +// tidyLastRun Track the various pieces of information around tidy on a specific cluster +type tidyLastRun struct { + LastRunTime time.Time +} + +func (sc *storageContext) getAutoTidyLastRun() (time.Time, error) { + entry, err := sc.Storage.Get(sc.Context, autoTidyLastRunPath) + if err != nil { + return time.Time{}, fmt.Errorf("failed getting auto tidy last run: %w", err) + } + if entry == nil { + return time.Time{}, nil + } + + var result tidyLastRun + if err = entry.DecodeJSON(&result); err != nil { + return time.Time{}, fmt.Errorf("failed parsing auto tidy last run: %w", err) + } + return result.LastRunTime, nil +} + +func (sc *storageContext) writeAutoTidyLastRun(lastRunTime time.Time) error { + lastRun := tidyLastRun{LastRunTime: lastRunTime} + entry, err := logical.StorageEntryJSON(autoTidyLastRunPath, lastRun) + if err != nil { + return fmt.Errorf("failed generating json for auto tidy last run: %w", err) + } + + if err := sc.Storage.Put(sc.Context, entry); err != nil { + return fmt.Errorf("failed writing auto tidy last run: %w", err) + } + + return nil +} + func fetchRevocationInfo(sc pki_backend.StorageContext, serial string) (*revocation.RevocationInfo, error) { var revInfo *revocation.RevocationInfo revEntry, err := fetchCertBySerial(sc, revocation.RevokedPath, serial) From 3c19c954c4cb16e8c907ca681b09d7dd9122121f Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Fri, 20 Sep 2024 09:13:32 -0400 Subject: [PATCH 2/9] Add auto-tidy configuration fields for backing off at startup --- builtin/logical/pki/backend.go | 23 +- builtin/logical/pki/path_tidy.go | 347 ++++++++++++-------------- builtin/logical/pki/path_tidy_test.go | 16 +- builtin/logical/pki/storage.go | 8 + 4 files changed, 203 insertions(+), 191 deletions(-) diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index 755cc7b409e0..2b9044db4e16 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -296,7 +296,11 @@ func Backend(conf *logical.BackendConfig) *backend { b.crlBuilder = newCRLBuilder(!cannotRebuildCRLs) // Delay the first tidy until after we've started up, this will be reset within the initialize function - b.lastAutoTidy = time.Now() + now := time.Now() + b.lastAutoTidy = now + + // Keep track of when this mount was started up. + b.mountStartup = now b.unifiedTransferStatus = newUnifiedTransferStatus() @@ -325,6 +329,11 @@ type backend struct { // use getAutoTidyLastRun and writeAutoTidyLastRun instead of direct access lastAutoTidy time.Time + // autoTidyBackoff a random time in the future in which auto-tidy can't start + // for after the system starts up to avoid a thundering herd of tidy operations + // at startup. + autoTidyBackoff time.Time + unifiedTransferStatus *UnifiedTransferStatus certificateCounter *CertificateCounter @@ -338,6 +347,9 @@ type backend struct { // Context around ACME operations acmeState *acmeState acmeAccountLock sync.RWMutex // (Write) Locked on Tidy, (Read) Locked on Account Creation + + // Track when this mount was started. + mountStartup time.Time } // BackendOps a bridge/legacy interface until we can further @@ -745,6 +757,15 @@ func (b *backend) periodicFunc(ctx context.Context, request *logical.Request) er return nil } + if b.autoTidyBackoff.IsZero() { + b.autoTidyBackoff = config.CalculateStartupBackoff(b.mountStartup) + } + + if b.autoTidyBackoff.After(now) { + b.Logger().Info("Auto tidy will not run as we are still within the random backoff ending at", "backoff_until", b.autoTidyBackoff) + return nil + } + // Ensure a tidy isn't already running... If it is, we'll trigger // again when the running one finishes. if !atomic.CompareAndSwapUint32(b.tidyCASGuard, 0, 1) { diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index f1fa2506d668..b6c955337017 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -8,6 +8,7 @@ import ( "crypto/x509" "errors" "fmt" + "math/rand/v2" "net/http" "sync/atomic" "time" @@ -81,8 +82,10 @@ type tidyStatus struct { type tidyConfig struct { // AutoTidy config - Enabled bool `json:"enabled"` - Interval time.Duration `json:"interval_duration"` + Enabled bool `json:"enabled"` + Interval time.Duration `json:"interval_duration"` + MinStartupBackoff time.Duration `json:"min_startup_backoff_duration"` + MaxStartupBackoff time.Duration `json:"max_startup_backoff_duration"` // Tidy Operations CertStore bool `json:"tidy_cert_store"` @@ -116,9 +119,24 @@ func (tc *tidyConfig) AnyTidyConfig() string { return "tidy_cert_store / tidy_revoked_certs / tidy_revoked_cert_issuer_associations / tidy_expired_issuers / tidy_move_legacy_ca_bundle / tidy_revocation_queue / tidy_cross_cluster_revoked_certs / tidy_acme" } +func (tc *tidyConfig) CalculateStartupBackoff(mountStartup time.Time) time.Time { + minBackoff := int64(tc.MinStartupBackoff.Seconds()) + maxBackoff := int64(tc.MaxStartupBackoff.Seconds()) + + maxNumber := maxBackoff - minBackoff + if maxNumber <= 0 { + return mountStartup.Add(tc.MinStartupBackoff) + } + + backoffSecs := rand.Int64N(maxNumber) + minBackoff + return mountStartup.Add(time.Duration(backoffSecs) * time.Second) +} + var defaultTidyConfig = tidyConfig{ Enabled: false, Interval: 12 * time.Hour, + MinStartupBackoff: 5 * time.Minute, + MaxStartupBackoff: 15 * time.Minute, CertStore: false, RevokedCerts: false, IssuerAssocs: false, @@ -561,6 +579,108 @@ func pathTidyStatus(b *backend) *framework.Path { } func pathConfigAutoTidy(b *backend) *framework.Path { + autoTidyResponseFields := map[string]*framework.FieldSchema{ + "enabled": { + Type: framework.TypeBool, + Description: `Specifies whether automatic tidy is enabled or not`, + Required: true, + }, + "min_startup_backoff_duration": { + Type: framework.TypeInt, + Description: `The minimum amount of time auto-tidy will be delayed after startup in seconds`, + Required: true, + }, + "max_startup_backoff_duration": { + Type: framework.TypeInt, + Description: `The maximum amount of time auto-tidy will be delayed after startup in seconds`, + Required: true, + }, + "interval_duration": { + Type: framework.TypeInt, + Description: `Specifies the duration between automatic tidy operation`, + Required: true, + }, + "tidy_cert_store": { + Type: framework.TypeBool, + Description: `Specifies whether to tidy up the certificate store`, + Required: true, + }, + "tidy_revoked_certs": { + Type: framework.TypeBool, + Description: `Specifies whether to remove all invalid and expired certificates from storage`, + Required: true, + }, + "tidy_revoked_cert_issuer_associations": { + Type: framework.TypeBool, + Description: `Specifies whether to associate revoked certificates with their corresponding issuers`, + Required: true, + }, + "tidy_expired_issuers": { + Type: framework.TypeBool, + Description: `Specifies whether tidy expired issuers`, + Required: true, + }, + "tidy_acme": { + Type: framework.TypeBool, + Description: `Tidy Unused Acme Accounts, and Orders`, + Required: true, + }, + "tidy_cert_metadata": { + Type: framework.TypeBool, + Description: `Tidy cert metadata`, + Required: true, + }, + "tidy_cmpv2_nonce_store": { + Type: framework.TypeBool, + Description: `Tidy CMPv2 nonce store`, + Required: true, + }, + "safety_buffer": { + Type: framework.TypeInt, + Description: `Safety buffer time duration`, + Required: true, + }, + "issuer_safety_buffer": { + Type: framework.TypeInt, + Description: `Issuer safety buffer`, + Required: true, + }, + "acme_account_safety_buffer": { + Type: framework.TypeInt, + Description: `Safety buffer after creation after which accounts lacking orders are revoked`, + Required: true, + }, + "pause_duration": { + Type: framework.TypeString, + Description: `Duration to pause between tidying certificates`, + Required: true, + }, + "tidy_cross_cluster_revoked_certs": { + Type: framework.TypeBool, + Description: `Tidy the cross-cluster revoked certificate store`, + Required: true, + }, + "tidy_revocation_queue": { + Type: framework.TypeBool, + Required: true, + }, + "tidy_move_legacy_ca_bundle": { + Type: framework.TypeBool, + Required: true, + }, + "revocation_queue_safety_buffer": { + Type: framework.TypeInt, + Required: true, + }, + "publish_stored_certificate_count_metrics": { + Type: framework.TypeBool, + Required: true, + }, + "maintain_stored_certificate_counts": { + Type: framework.TypeBool, + Required: true, + }, + } return &framework.Path{ Pattern: "config/auto-tidy", DisplayAttrs: &framework.DisplayAttributes{ @@ -571,6 +691,16 @@ func pathConfigAutoTidy(b *backend) *framework.Path { Type: framework.TypeBool, Description: `Set to true to enable automatic tidy operations.`, }, + "min_startup_backoff_duration": { + Type: framework.TypeDurationSecond, + Description: `The minimum amount of time auto-tidy will be delayed after startup in seconds.`, + Default: int(defaultTidyConfig.MinStartupBackoff.Seconds()), + }, + "max_startup_backoff_duration": { + Type: framework.TypeDurationSecond, + Description: `The maximum amount of time auto-tidy will be delayed after startup in seconds.`, + Default: int(defaultTidyConfig.MaxStartupBackoff.Seconds()), + }, "interval_duration": { Type: framework.TypeDurationSecond, Description: `Interval at which to run an auto-tidy operation. This is the time between tidy invocations (after one finishes to the start of the next). Running a manual tidy will reset this duration.`, @@ -601,97 +731,7 @@ available on the tidy-status endpoint.`, Responses: map[int][]framework.Response{ http.StatusOK: {{ Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "enabled": { - Type: framework.TypeBool, - Description: `Specifies whether automatic tidy is enabled or not`, - Required: true, - }, - "interval_duration": { - Type: framework.TypeInt, - Description: `Specifies the duration between automatic tidy operation`, - Required: true, - }, - "tidy_cert_store": { - Type: framework.TypeBool, - Description: `Specifies whether to tidy up the certificate store`, - Required: true, - }, - "tidy_revoked_certs": { - Type: framework.TypeBool, - Description: `Specifies whether to remove all invalid and expired certificates from storage`, - Required: true, - }, - "tidy_revoked_cert_issuer_associations": { - Type: framework.TypeBool, - Description: `Specifies whether to associate revoked certificates with their corresponding issuers`, - Required: true, - }, - "tidy_expired_issuers": { - Type: framework.TypeBool, - Description: `Specifies whether tidy expired issuers`, - Required: true, - }, - "tidy_acme": { - Type: framework.TypeBool, - Description: `Tidy Unused Acme Accounts, and Orders`, - Required: true, - }, - "tidy_cert_metadata": { - Type: framework.TypeBool, - Description: `Tidy cert metadata`, - Required: true, - }, - "tidy_cmpv2_nonce_store": { - Type: framework.TypeBool, - Description: `Tidy CMPv2 nonce store`, - Required: true, - }, - "safety_buffer": { - Type: framework.TypeInt, - Description: `Safety buffer time duration`, - Required: true, - }, - "issuer_safety_buffer": { - Type: framework.TypeInt, - Description: `Issuer safety buffer`, - Required: true, - }, - "acme_account_safety_buffer": { - Type: framework.TypeInt, - Description: `Safety buffer after creation after which accounts lacking orders are revoked`, - Required: false, - }, - "pause_duration": { - Type: framework.TypeString, - Description: `Duration to pause between tidying certificates`, - Required: true, - }, - "tidy_move_legacy_ca_bundle": { - Type: framework.TypeBool, - Required: true, - }, - "tidy_cross_cluster_revoked_certs": { - Type: framework.TypeBool, - Required: true, - }, - "tidy_revocation_queue": { - Type: framework.TypeBool, - Required: true, - }, - "revocation_queue_safety_buffer": { - Type: framework.TypeInt, - Required: true, - }, - "publish_stored_certificate_count_metrics": { - Type: framework.TypeBool, - Required: true, - }, - "maintain_stored_certificate_counts": { - Type: framework.TypeBool, - Required: true, - }, - }, + Fields: autoTidyResponseFields, }}, }, }, @@ -704,98 +744,7 @@ available on the tidy-status endpoint.`, Responses: map[int][]framework.Response{ http.StatusOK: {{ Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "enabled": { - Type: framework.TypeBool, - Description: `Specifies whether automatic tidy is enabled or not`, - Required: true, - }, - "interval_duration": { - Type: framework.TypeInt, - Description: `Specifies the duration between automatic tidy operation`, - Required: true, - }, - "tidy_cert_store": { - Type: framework.TypeBool, - Description: `Specifies whether to tidy up the certificate store`, - Required: true, - }, - "tidy_revoked_certs": { - Type: framework.TypeBool, - Description: `Specifies whether to remove all invalid and expired certificates from storage`, - Required: true, - }, - "tidy_revoked_cert_issuer_associations": { - Type: framework.TypeBool, - Description: `Specifies whether to associate revoked certificates with their corresponding issuers`, - Required: true, - }, - "tidy_expired_issuers": { - Type: framework.TypeBool, - Description: `Specifies whether tidy expired issuers`, - Required: true, - }, - "tidy_acme": { - Type: framework.TypeBool, - Description: `Tidy Unused Acme Accounts, and Orders`, - Required: true, - }, - "tidy_cert_metadata": { - Type: framework.TypeBool, - Description: `Tidy cert metadata`, - Required: true, - }, - "tidy_cmpv2_nonce_store": { - Type: framework.TypeBool, - Description: `Tidy CMPv2 nonce store`, - Required: true, - }, - "safety_buffer": { - Type: framework.TypeInt, - Description: `Safety buffer time duration`, - Required: true, - }, - "issuer_safety_buffer": { - Type: framework.TypeInt, - Description: `Issuer safety buffer`, - Required: true, - }, - "acme_account_safety_buffer": { - Type: framework.TypeInt, - Description: `Safety buffer after creation after which accounts lacking orders are revoked`, - Required: true, - }, - "pause_duration": { - Type: framework.TypeString, - Description: `Duration to pause between tidying certificates`, - Required: true, - }, - "tidy_cross_cluster_revoked_certs": { - Type: framework.TypeBool, - Description: `Tidy the cross-cluster revoked certificate store`, - Required: true, - }, - "tidy_revocation_queue": { - Type: framework.TypeBool, - Required: true, - }, - "tidy_move_legacy_ca_bundle": { - Type: framework.TypeBool, - Required: true, - }, - "revocation_queue_safety_buffer": { - Type: framework.TypeInt, - Required: true, - }, - "publish_stored_certificate_count_metrics": { - Type: framework.TypeBool, - Required: true, - }, - "maintain_stored_certificate_counts": { - Type: framework.TypeBool, - Required: true, - }, - }, + Fields: autoTidyResponseFields, }}, }, // Read more about why these flags are set in backend.go. @@ -1884,6 +1833,32 @@ func (b *backend) pathConfigAutoTidyWrite(ctx context.Context, req *logical.Requ config.Enabled = enabled } + if minStartupBackoffRaw, ok := d.GetOk("min_startup_backoff_duration"); ok { + minDuration, err := parseutil.ParseDurationSecond(minStartupBackoffRaw) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("failed to parse min_startup_backoff_duration flag as a duration: %s", err.Error())), nil + } + if minDuration.Seconds() < 1 { + return logical.ErrorResponse(fmt.Sprintf("min_startup_backoff_duration must be at least 1 second: parsed: %v", minDuration)), nil + } + config.MinStartupBackoff = minDuration + } + + if maxStartupBackoffRaw, ok := d.GetOk("max_startup_backoff_duration"); ok { + maxDuration, err := parseutil.ParseDurationSecond(maxStartupBackoffRaw) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("failed to parse max_startup_backoff_duration flag as a duration: %s", err.Error())), nil + } + if maxDuration.Seconds() < 1 { + return logical.ErrorResponse(fmt.Sprintf("max_startup_backoff_duration must be at least 1 second: parsed: %v", maxDuration)), nil + } + config.MaxStartupBackoff = maxDuration + } + + if config.MinStartupBackoff > config.MaxStartupBackoff { + return logical.ErrorResponse(fmt.Sprintf("max_startup_backoff_duration %v must be greater or equal to min_startup_backoff_duration %v", config.MaxStartupBackoff, config.MinStartupBackoff)), nil + } + if intervalRaw, ok := d.GetOk("interval_duration"); ok { config.Interval = time.Duration(intervalRaw.(int)) * time.Second if config.Interval < 0 { @@ -2250,6 +2225,8 @@ func getTidyConfigData(config tidyConfig) map[string]interface{} { // This map is in the same order as tidyConfig to ensure that all fields are accounted for "enabled": config.Enabled, "interval_duration": int(config.Interval / time.Second), + "min_startup_backoff_duration": int(config.MinStartupBackoff.Seconds()), + "max_startup_backoff_duration": int(config.MaxStartupBackoff.Seconds()), "tidy_cert_store": config.CertStore, "tidy_revoked_certs": config.RevokedCerts, "tidy_revoked_cert_issuer_associations": config.IssuerAssocs, diff --git a/builtin/logical/pki/path_tidy_test.go b/builtin/logical/pki/path_tidy_test.go index feca6732bb76..0b12e845b7d4 100644 --- a/builtin/logical/pki/path_tidy_test.go +++ b/builtin/logical/pki/path_tidy_test.go @@ -236,11 +236,13 @@ func TestAutoTidy(t *testing.T) { // Write the auto-tidy config. _, err = client.Logical().Write("pki/config/auto-tidy", map[string]interface{}{ - "enabled": true, - "interval_duration": "1s", - "tidy_cert_store": true, - "tidy_revoked_certs": true, - "safety_buffer": "1s", + "enabled": true, + "interval_duration": "1s", + "tidy_cert_store": true, + "tidy_revoked_certs": true, + "safety_buffer": "1s", + "min_startup_backoff_duration": "1s", + "max_startup_backoff_duration": "1s", }) require.NoError(t, err) @@ -614,6 +616,8 @@ func TestTidyIssuerConfig(t *testing.T) { defaultConfigMap["pause_duration"] = time.Duration(defaultConfigMap["pause_duration"].(float64)).String() defaultConfigMap["revocation_queue_safety_buffer"] = int(time.Duration(defaultConfigMap["revocation_queue_safety_buffer"].(float64)) / time.Second) defaultConfigMap["acme_account_safety_buffer"] = int(time.Duration(defaultConfigMap["acme_account_safety_buffer"].(float64)) / time.Second) + defaultConfigMap["min_startup_backoff_duration"] = int(time.Duration(defaultConfigMap["min_startup_backoff_duration"].(float64)) / time.Second) + defaultConfigMap["max_startup_backoff_duration"] = int(time.Duration(defaultConfigMap["max_startup_backoff_duration"].(float64)) / time.Second) require.Equal(t, defaultConfigMap, resp.Data) @@ -744,6 +748,8 @@ func TestCertStorageMetrics(t *testing.T) { "safety_buffer": "1s", "maintain_stored_certificate_counts": true, "publish_stored_certificate_count_metrics": false, + "min_startup_backoff_duration": "1s", + "max_startup_backoff_duration": "1s", }) require.NoError(t, err) diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index daa9bdc9e10f..43c4853ba34c 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -715,6 +715,14 @@ func (sc *storageContext) getAutoTidyConfig() (*tidyConfig, error) { result.IssuerSafetyBuffer = defaultTidyConfig.IssuerSafetyBuffer } + if result.MinStartupBackoff == 0 { + result.MinStartupBackoff = defaultTidyConfig.MinStartupBackoff + } + + if result.MaxStartupBackoff == 0 { + result.MaxStartupBackoff = defaultTidyConfig.MaxStartupBackoff + } + return &result, nil } From a010e7a81be7d7f637473d83817114e5d8a9ed80 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Fri, 20 Sep 2024 17:07:08 -0400 Subject: [PATCH 3/9] Add new auto-tidy fields to UI --- ui/app/models/pki/tidy.js | 31 ++++++++++++++++++- ui/app/serializers/pki/tidy.js | 2 ++ ui/lib/pki/addon/components/pki-tidy-form.hbs | 12 +++++++ ui/lib/pki/addon/components/pki-tidy-form.ts | 2 ++ .../helpers/openapi/expected-secret-attrs.js | 10 ++++++ .../components/pki/pki-tidy-form-test.js | 18 ++++++++--- ui/types/vault/models/pki/tidy.d.ts | 2 ++ 7 files changed, 72 insertions(+), 5 deletions(-) diff --git a/ui/app/models/pki/tidy.js b/ui/app/models/pki/tidy.js index ee4f335a1f82..cec0aaf7aa54 100644 --- a/ui/app/models/pki/tidy.js +++ b/ui/app/models/pki/tidy.js @@ -49,6 +49,24 @@ export default class PkiTidyModel extends Model { }) intervalDuration; // auto-tidy only + @attr('string', { + editType: 'ttl', + helperTextEnabled: + 'Sets the min_startup_backoff_duration field which forces the minimum delay after Vault startup auto-tidy can run', + hideToggle: true, + formatTtl: true, + }) + minStartupBackoffDuration; + + @attr('string', { + editType: 'ttl', + helperTextEnabled: + 'Sets the max_startup_backoff_duration field which forces the maximum delay after Vault startup auto-tidy can run', + hideToggle: true, + formatTtl: true, + }) + maxStartupBackoffDuration; + @attr('string', { editType: 'ttl', helperTextEnabled: @@ -131,10 +149,21 @@ export default class PkiTidyModel extends Model { tidyRevokedCerts; get allGroups() { - const groups = [{ autoTidy: ['enabled', 'intervalDuration'] }, ...this.sharedFields]; + const groups = [ + { autoTidy: ['enabled', 'intervalDuration', 'minStartupBackoffDuration', 'maxStartupBackoffDuration'] }, + ...this.sharedFields, + ]; return this._expandGroups(groups); } + // fields that are specific to auto-tidy, which should only show up when enabled. + get autoTidyEnabledFields() { + const enabledFields = [ + { 'Auto Tidy Startup Backoff': ['minStartupBackoffDuration', 'maxStartupBackoffDuration'] }, + ]; + return this._expandGroups(enabledFields); + } + // shared between auto and manual tidy operations get sharedFields() { const groups = [ diff --git a/ui/app/serializers/pki/tidy.js b/ui/app/serializers/pki/tidy.js index d56c887976ce..abfad560707e 100644 --- a/ui/app/serializers/pki/tidy.js +++ b/ui/app/serializers/pki/tidy.js @@ -11,6 +11,8 @@ export default class PkiTidySerializer extends ApplicationSerializer { if (tidyType === 'manual') { delete data?.enabled; delete data?.intervalDuration; + delete data?.minStartupBackoffDuration; + delete data?.maxStartupBackoffDuration; } return data; } diff --git a/ui/lib/pki/addon/components/pki-tidy-form.hbs b/ui/lib/pki/addon/components/pki-tidy-form.hbs index e45483b68710..42694f39e73e 100644 --- a/ui/lib/pki/addon/components/pki-tidy-form.hbs +++ b/ui/lib/pki/addon/components/pki-tidy-form.hbs @@ -27,6 +27,18 @@ /> {{/let}} {{/if}} + {{#each @tidy.autoTidyEnabledFields as |fieldGroup|}} + {{#each-in fieldGroup as |group fields|}} + {{#if (and (eq @tidyType "auto") @tidy.enabled)}} +

+ {{group}} +

+ {{#each fields as |attr|}} + + {{/each}} + {{/if}} + {{/each-in}} + {{/each}} {{#each @tidy.formFieldGroups as |fieldGroup|}} {{#each-in fieldGroup as |group fields|}} {{#if (or (eq @tidyType "manual") @tidy.enabled)}} diff --git a/ui/lib/pki/addon/components/pki-tidy-form.ts b/ui/lib/pki/addon/components/pki-tidy-form.ts index a5a290ad821e..f83e2d959c94 100644 --- a/ui/lib/pki/addon/components/pki-tidy-form.ts +++ b/ui/lib/pki/addon/components/pki-tidy-form.ts @@ -24,6 +24,8 @@ interface Args { interface PkiTidyTtls { intervalDuration: string; + minStartupBackoffDuration: string; + maxStartupBackoffDuration: string; acmeAccountSafetyBuffer: string; } interface PkiTidyBooleans { diff --git a/ui/tests/helpers/openapi/expected-secret-attrs.js b/ui/tests/helpers/openapi/expected-secret-attrs.js index 84c39c0188da..e123efa3a961 100644 --- a/ui/tests/helpers/openapi/expected-secret-attrs.js +++ b/ui/tests/helpers/openapi/expected-secret-attrs.js @@ -1406,6 +1406,16 @@ const pki = { 'Interval at which to run an auto-tidy operation. This is the time between tidy invocations (after one finishes to the start of the next). Running a manual tidy will reset this duration.', fieldGroup: 'default', }, + minStartupBackoffDuration: { + editType: 'ttl', + helpText: 'The minimum amount of time auto-tidy will be delayed after startup in seconds.', + fieldGroup: 'default', + }, + maxStartupBackoffDuration: { + editType: 'ttl', + helpText: 'The maximum amount of time auto-tidy will be delayed after startup in seconds.', + fieldGroup: 'default', + }, issuerSafetyBuffer: { editType: 'ttl', helpText: diff --git a/ui/tests/integration/components/pki/pki-tidy-form-test.js b/ui/tests/integration/components/pki/pki-tidy-form-test.js index d1d758cc28fc..19be4cf8f916 100644 --- a/ui/tests/integration/components/pki/pki-tidy-form-test.js +++ b/ui/tests/integration/components/pki/pki-tidy-form-test.js @@ -32,8 +32,9 @@ module('Integration | Component | pki tidy form', function (hooks) { }); test('it hides or shows fields depending on auto-tidy toggle', async function (assert) { - assert.expect(41); + assert.expect(47); const sectionHeaders = [ + 'Auto Tidy Startup Backoff', 'Universal operations', 'ACME operations', 'Issuer operations', @@ -82,9 +83,10 @@ module('Integration | Component | pki tidy form', function (hooks) { }); test('it renders all attribute fields, including enterprise', async function (assert) { - assert.expect(29); + assert.expect(33); this.autoTidy.enabled = true; const skipFields = ['enabled', 'tidyAcme', 'intervalDuration']; // combined with duration ttl or asserted separately + const fieldsNotPartOfManualTidy = ['minStartupBackoffDuration', 'maxStartupBackoffDuration']; // fields we shouldn't see on the manual tidy screen await render( hbs` { if (skipFields.includes(attr)) return; - assert.dom(PKI_TIDY_FORM.inputByAttr(attr)).exists(`renders ${attr} for manual tidyType`); + if (fieldsNotPartOfManualTidy.includes(attr)) { + assert + .dom(PKI_TIDY_FORM.inputByAttr(attr)) + .doesNotExist(`${attr} should not appear on manual tidyType`); + } else { + assert.dom(PKI_TIDY_FORM.inputByAttr(attr)).exists(`renders ${attr} for manual tidyType`); + } }); }); @@ -175,13 +183,15 @@ module('Integration | Component | pki tidy form', function (hooks) { }); test('it should change the attributes on the model', async function (assert) { - assert.expect(12); + assert.expect(11); this.server.post('/pki-auto-tidy/config/auto-tidy', (schema, req) => { assert.propEqual( JSON.parse(req.requestBody), { acme_account_safety_buffer: '72h', enabled: true, + min_startup_backoff_duration: '5m', + max_startup_backoff_duration: '15m', interval_duration: '10s', issuer_safety_buffer: '20s', pause_duration: '30s', diff --git a/ui/types/vault/models/pki/tidy.d.ts b/ui/types/vault/models/pki/tidy.d.ts index b12bebe01ad4..1dd13330ffd5 100644 --- a/ui/types/vault/models/pki/tidy.d.ts +++ b/ui/types/vault/models/pki/tidy.d.ts @@ -12,6 +12,8 @@ export default class PkiTidyModel extends Model { tidyAcme: boolean; enabled: boolean; intervalDuration: string; + minStartupBackoffDuration: string; + maxStartupBackoffDuration: string; issuerSafetyBuffer: string; pauseDuration: string; revocationQueueSafetyBuffer: string; From fd14c0195a4eb36b5e22504fd53e06d88dca5f06 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Tue, 24 Sep 2024 11:14:39 -0400 Subject: [PATCH 4/9] Update api docs for auto-tidy --- website/content/api-docs/secret/pki/index.mdx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index 1b8e6fa23705..da89644ddae7 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -4468,6 +4468,8 @@ $ curl \ "interval_duration": 43200, "issuer_safety_buffer": 31536000, "maintain_stored_certificate_counts": false, + "max_startup_backoff_duration": 1200, + "min_startup_backoff_duration": 900, "pause_duration": "0s", "publish_stored_certificate_count_metrics": false, "revocation_queue_safety_buffer": 172800, @@ -4521,6 +4523,12 @@ The below parameters are in addition to the regular parameters accepted by the this cluster. Instead, use audit logs and aggregate this data externally to Vault so as not to impact Vault performance. +- `min_startup_backoff_duration` `(string: "5m")` - The minimum amount of time auto-tidy + will be delayed after startup in seconds. Defaults to 5 minutes. + +- `max_startup_backoff_duration` `(string: "15m")` - The maximum amount of time auto-tidy + will be delayed after startup in seconds. Defaults to 15 minutes. + - `publish_stored_certificate_count_metrics` `(bool: false)` - When enabled, publishes the value computed by `maintain_stored_certificate_counts` to the mount's metrics. This requires the former to be enabled. @@ -4573,7 +4581,7 @@ The result includes the following fields: * `cross_revoked_cert_deleted_count`: the number of cross-cluster revoked certificate entries deleted * `revocation_queue_safety_buffer`: the value of this parameter when initiating the tidy operation * `pause_duration`: the value of this parameter when initiating the tidy operation -* `last_auto_tidy_finished`: the time when the last auto-tidy operation finished; may be different than `time_finished` especially if the last operation was a manually executed tidy operation. Set to current time at mount time to delay the initial auto-tidy operation; not persisted. +* `last_auto_tidy_finished`: the time when the last auto-tidy operation finished; may be different than `time_finished` especially if the last operation was a manually executed tidy operation. Set to current time at mount startup if no previous value was persisted. * `tidy_cert_metadata`: the value of this parameter when initiating the tidy operation * `cert_metadata_deleted_count`: the number of metadata entries deleted * `cmpv2_nonce_deleted_count`: the number of CMPv2 nonces deleted From f96e89e36f1f29f2f23e07eeed4604a8a62aa5fa Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Tue, 24 Sep 2024 12:25:09 -0400 Subject: [PATCH 5/9] Add cl --- changelog/28488.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/28488.txt diff --git a/changelog/28488.txt b/changelog/28488.txt new file mode 100644 index 000000000000..bc297fc2f20d --- /dev/null +++ b/changelog/28488.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Track the last time auto-tidy ran to address auto-tidy not running if the auto-tidy interval is longer than scheduled Vault restarts. +``` From e2c20749223cff4cc81f6c9b3e24d95295618fc1 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Wed, 25 Sep 2024 15:13:58 -0400 Subject: [PATCH 6/9] Update field description text --- builtin/logical/pki/path_tidy.go | 8 ++++---- ui/tests/helpers/openapi/expected-secret-attrs.js | 4 ++-- website/content/api-docs/secret/pki/index.mdx | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index b6c955337017..a8971832c2b1 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -587,12 +587,12 @@ func pathConfigAutoTidy(b *backend) *framework.Path { }, "min_startup_backoff_duration": { Type: framework.TypeInt, - Description: `The minimum amount of time auto-tidy will be delayed after startup in seconds`, + Description: `The minimum amount of time in seconds auto-tidy will be delayed after startup`, Required: true, }, "max_startup_backoff_duration": { Type: framework.TypeInt, - Description: `The maximum amount of time auto-tidy will be delayed after startup in seconds`, + Description: `The maximum amount of time in seconds auto-tidy will be delayed after startup`, Required: true, }, "interval_duration": { @@ -693,12 +693,12 @@ func pathConfigAutoTidy(b *backend) *framework.Path { }, "min_startup_backoff_duration": { Type: framework.TypeDurationSecond, - Description: `The minimum amount of time auto-tidy will be delayed after startup in seconds.`, + Description: `The minimum amount of time in seconds auto-tidy will be delayed after startup.`, Default: int(defaultTidyConfig.MinStartupBackoff.Seconds()), }, "max_startup_backoff_duration": { Type: framework.TypeDurationSecond, - Description: `The maximum amount of time auto-tidy will be delayed after startup in seconds.`, + Description: `The maximum amount of time in seconds auto-tidy will be delayed after startup.`, Default: int(defaultTidyConfig.MaxStartupBackoff.Seconds()), }, "interval_duration": { diff --git a/ui/tests/helpers/openapi/expected-secret-attrs.js b/ui/tests/helpers/openapi/expected-secret-attrs.js index e123efa3a961..1fb27c2eba76 100644 --- a/ui/tests/helpers/openapi/expected-secret-attrs.js +++ b/ui/tests/helpers/openapi/expected-secret-attrs.js @@ -1408,12 +1408,12 @@ const pki = { }, minStartupBackoffDuration: { editType: 'ttl', - helpText: 'The minimum amount of time auto-tidy will be delayed after startup in seconds.', + helpText: 'The minimum amount of time in seconds auto-tidy will be delayed after startup.', fieldGroup: 'default', }, maxStartupBackoffDuration: { editType: 'ttl', - helpText: 'The maximum amount of time auto-tidy will be delayed after startup in seconds.', + helpText: 'The maximum amount of time in seconds auto-tidy will be delayed after startup.', fieldGroup: 'default', }, issuerSafetyBuffer: { diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index da89644ddae7..5bf656e40784 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -4524,10 +4524,10 @@ The below parameters are in addition to the regular parameters accepted by the to Vault so as not to impact Vault performance. - `min_startup_backoff_duration` `(string: "5m")` - The minimum amount of time auto-tidy - will be delayed after startup in seconds. Defaults to 5 minutes. + will be delayed after startup. Defaults to 5 minutes. - `max_startup_backoff_duration` `(string: "15m")` - The maximum amount of time auto-tidy - will be delayed after startup in seconds. Defaults to 15 minutes. + will be delayed after startup. Defaults to 15 minutes. - `publish_stored_certificate_count_metrics` `(bool: false)` - When enabled, publishes the value computed by `maintain_stored_certificate_counts` to From 696b777d9692687ed143eb156e60a18cbf29d06a Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Wed, 25 Sep 2024 16:26:41 -0400 Subject: [PATCH 7/9] Apply Claire's suggestions from code review Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com> --- ui/app/models/pki/tidy.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ui/app/models/pki/tidy.js b/ui/app/models/pki/tidy.js index cec0aaf7aa54..0b5d8cf3275d 100644 --- a/ui/app/models/pki/tidy.js +++ b/ui/app/models/pki/tidy.js @@ -50,22 +50,26 @@ export default class PkiTidyModel extends Model { intervalDuration; // auto-tidy only @attr('string', { + label: 'Minimum startup backoff duration', + defaultValue: '5m', editType: 'ttl', helperTextEnabled: 'Sets the min_startup_backoff_duration field which forces the minimum delay after Vault startup auto-tidy can run', hideToggle: true, formatTtl: true, }) - minStartupBackoffDuration; + minStartupBackoffDuration; // auto-tidy only @attr('string', { + label: 'Maximum startup backoff duration', + defaultValue: '15m' editType: 'ttl', helperTextEnabled: 'Sets the max_startup_backoff_duration field which forces the maximum delay after Vault startup auto-tidy can run', hideToggle: true, formatTtl: true, }) - maxStartupBackoffDuration; + maxStartupBackoffDuration; // auto-tidy only @attr('string', { editType: 'ttl', From 642e5fce92a3058afe0a78127e8b7146b786c74b Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Wed, 25 Sep 2024 16:43:26 -0400 Subject: [PATCH 8/9] Implementing PR feedback from the UI team --- ui/app/models/pki/tidy.js | 17 +++++++--------- ui/lib/pki/addon/components/pki-tidy-form.hbs | 20 +++++++------------ ui/lib/pki/addon/components/pki-tidy-form.ts | 2 -- .../components/pki/pki-tidy-form-test.js | 5 +++-- 4 files changed, 17 insertions(+), 27 deletions(-) diff --git a/ui/app/models/pki/tidy.js b/ui/app/models/pki/tidy.js index 0b5d8cf3275d..f339448a4108 100644 --- a/ui/app/models/pki/tidy.js +++ b/ui/app/models/pki/tidy.js @@ -58,11 +58,11 @@ export default class PkiTidyModel extends Model { hideToggle: true, formatTtl: true, }) - minStartupBackoffDuration; // auto-tidy only + minStartupBackoffDuration; // auto-tidy only @attr('string', { - label: 'Maximum startup backoff duration', - defaultValue: '15m' + label: 'Maximum startup backoff duration', + defaultValue: '15m', editType: 'ttl', helperTextEnabled: 'Sets the max_startup_backoff_duration field which forces the maximum delay after Vault startup auto-tidy can run', @@ -154,18 +154,15 @@ export default class PkiTidyModel extends Model { get allGroups() { const groups = [ - { autoTidy: ['enabled', 'intervalDuration', 'minStartupBackoffDuration', 'maxStartupBackoffDuration'] }, + { autoTidy: ['enabled', 'intervalDuration', ...this.autoTidyConfigFields] }, ...this.sharedFields, ]; return this._expandGroups(groups); } - // fields that are specific to auto-tidy, which should only show up when enabled. - get autoTidyEnabledFields() { - const enabledFields = [ - { 'Auto Tidy Startup Backoff': ['minStartupBackoffDuration', 'maxStartupBackoffDuration'] }, - ]; - return this._expandGroups(enabledFields); + // fields that are specific to auto-tidy + get autoTidyConfigFields() { + return ['minStartupBackoffDuration', 'maxStartupBackoffDuration']; } // shared between auto and manual tidy operations diff --git a/ui/lib/pki/addon/components/pki-tidy-form.hbs b/ui/lib/pki/addon/components/pki-tidy-form.hbs index 42694f39e73e..5edc547eeb1e 100644 --- a/ui/lib/pki/addon/components/pki-tidy-form.hbs +++ b/ui/lib/pki/addon/components/pki-tidy-form.hbs @@ -13,7 +13,7 @@
- {{#if (and (eq @tidyType "auto") this.intervalDurationAttr)}} + {{#if (eq @tidyType "auto")}} {{#let this.intervalDurationAttr as |attr|}} {{/let}} + + {{#if @tidy.enabled}} + {{#each @tidy.autoTidyConfigFields as |field|}} + + {{/each}} + {{/if}} {{/if}} - {{#each @tidy.autoTidyEnabledFields as |fieldGroup|}} - {{#each-in fieldGroup as |group fields|}} - {{#if (and (eq @tidyType "auto") @tidy.enabled)}} -

- {{group}} -

- {{#each fields as |attr|}} - - {{/each}} - {{/if}} - {{/each-in}} - {{/each}} {{#each @tidy.formFieldGroups as |fieldGroup|}} {{#each-in fieldGroup as |group fields|}} {{#if (or (eq @tidyType "manual") @tidy.enabled)}} diff --git a/ui/lib/pki/addon/components/pki-tidy-form.ts b/ui/lib/pki/addon/components/pki-tidy-form.ts index f83e2d959c94..a5a290ad821e 100644 --- a/ui/lib/pki/addon/components/pki-tidy-form.ts +++ b/ui/lib/pki/addon/components/pki-tidy-form.ts @@ -24,8 +24,6 @@ interface Args { interface PkiTidyTtls { intervalDuration: string; - minStartupBackoffDuration: string; - maxStartupBackoffDuration: string; acmeAccountSafetyBuffer: string; } interface PkiTidyBooleans { diff --git a/ui/tests/integration/components/pki/pki-tidy-form-test.js b/ui/tests/integration/components/pki/pki-tidy-form-test.js index 19be4cf8f916..1861781a4e51 100644 --- a/ui/tests/integration/components/pki/pki-tidy-form-test.js +++ b/ui/tests/integration/components/pki/pki-tidy-form-test.js @@ -34,7 +34,6 @@ module('Integration | Component | pki tidy form', function (hooks) { test('it hides or shows fields depending on auto-tidy toggle', async function (assert) { assert.expect(47); const sectionHeaders = [ - 'Auto Tidy Startup Backoff', 'Universal operations', 'ACME operations', 'Issuer operations', @@ -183,7 +182,7 @@ module('Integration | Component | pki tidy form', function (hooks) { }); test('it should change the attributes on the model', async function (assert) { - assert.expect(11); + assert.expect(12); this.server.post('/pki-auto-tidy/config/auto-tidy', (schema, req) => { assert.propEqual( JSON.parse(req.requestBody), @@ -249,6 +248,8 @@ module('Integration | Component | pki tidy form', function (hooks) { pauseDuration: 30, revocationQueueSafetyBuffer: 40, safetyBuffer: 50, + minStartupBackoffDuration: 300, + maxStartupBackoffDuration: 900, }; this.autoTidy.eachAttribute(async (attr, { type }) => { const skipFields = ['enabled', 'tidyAcme', 'intervalDuration', 'acmeAccountSafetyBuffer']; // combined with duration ttl or asserted separately From 24332c27ef1eaa2ffa30d78208adf308069d870a Mon Sep 17 00:00:00 2001 From: claire bontempo Date: Wed, 25 Sep 2024 17:58:50 -0700 Subject: [PATCH 9/9] remove explicit defaults and types so we retrieve from backend, decouple enabling auto tidy from duration, move params to auto settings section --- ui/app/models/pki/tidy.js | 44 +++---- ui/lib/pki/addon/components/pki-tidy-form.hbs | 41 +++--- ui/lib/pki/addon/components/pki-tidy-form.ts | 6 - ui/tests/acceptance/pki/pki-tidy-test.js | 4 +- ui/tests/helpers/pki/pki-selectors.ts | 1 - .../pki/page/pki-tidy-auto-settings-test.js | 5 +- .../components/pki/pki-tidy-form-test.js | 117 +++++++++++------- 7 files changed, 121 insertions(+), 97 deletions(-) diff --git a/ui/app/models/pki/tidy.js b/ui/app/models/pki/tidy.js index f339448a4108..7b40ad1c7f58 100644 --- a/ui/app/models/pki/tidy.js +++ b/ui/app/models/pki/tidy.js @@ -16,7 +16,7 @@ export default class PkiTidyModel extends Model { label: 'Tidy ACME enabled', labelDisabled: 'Tidy ACME disabled', mapToBoolean: 'tidyAcme', - helperTextDisabled: 'Tidying of ACME accounts, orders and authorizations is disabled', + helperTextDisabled: 'Tidying of ACME accounts, orders and authorizations is disabled.', helperTextEnabled: 'The amount of time that must pass after creation that an account with no orders is marked revoked, and the amount of time after being marked revoked or deactivated.', detailsLabel: 'ACME account safety buffer', @@ -31,47 +31,45 @@ export default class PkiTidyModel extends Model { }) tidyAcme; + // * auto tidy only fields @attr('boolean', { label: 'Automatic tidy enabled', - defaultValue: false, + labelDisabled: 'Automatic tidy disabled', + helperTextDisabled: 'Automatic tidy operations will not run.', }) - enabled; // auto-tidy only + enabled; // renders outside FormField loop as a toggle, auto tidy fields only render if enabled @attr({ - label: 'Automatic tidy enabled', - labelDisabled: 'Automatic tidy disabled', - mapToBoolean: 'enabled', + editType: 'ttl', helperTextEnabled: 'Sets the interval_duration between automatic tidy operations; note that this is from the end of one operation to the start of the next.', - helperTextDisabled: 'Automatic tidy operations will not run.', - detailsLabel: 'Automatic tidy duration', + hideToggle: true, formatTtl: true, }) - intervalDuration; // auto-tidy only + intervalDuration; - @attr('string', { + @attr({ label: 'Minimum startup backoff duration', - defaultValue: '5m', editType: 'ttl', helperTextEnabled: - 'Sets the min_startup_backoff_duration field which forces the minimum delay after Vault startup auto-tidy can run', + 'Sets the min_startup_backoff_duration field which forces the minimum delay after Vault startup auto-tidy can run.', hideToggle: true, formatTtl: true, }) - minStartupBackoffDuration; // auto-tidy only + minStartupBackoffDuration; - @attr('string', { + @attr({ label: 'Maximum startup backoff duration', - defaultValue: '15m', editType: 'ttl', helperTextEnabled: - 'Sets the max_startup_backoff_duration field which forces the maximum delay after Vault startup auto-tidy can run', + 'Sets the max_startup_backoff_duration field which forces the maximum delay after Vault startup auto-tidy can run.', hideToggle: true, formatTtl: true, }) - maxStartupBackoffDuration; // auto-tidy only + maxStartupBackoffDuration; + // * end of auto-tidy only fields - @attr('string', { + @attr({ editType: 'ttl', helperTextEnabled: 'Specifies a duration that issuers should be kept for, past their NotAfter validity period. Defaults to 365 days (8760 hours).', @@ -98,7 +96,7 @@ export default class PkiTidyModel extends Model { }) revocationQueueSafetyBuffer; // enterprise only - @attr('string', { + @attr({ editType: 'ttl', helperTextEnabled: 'For a certificate to be expunged, the time must be after the expiration time of the certificate (according to the local clock) plus the safety buffer. Defaults to 72 hours.', @@ -153,16 +151,14 @@ export default class PkiTidyModel extends Model { tidyRevokedCerts; get allGroups() { - const groups = [ - { autoTidy: ['enabled', 'intervalDuration', ...this.autoTidyConfigFields] }, - ...this.sharedFields, - ]; + const groups = [{ autoTidy: ['enabled', ...this.autoTidyConfigFields] }, ...this.sharedFields]; return this._expandGroups(groups); } // fields that are specific to auto-tidy get autoTidyConfigFields() { - return ['minStartupBackoffDuration', 'maxStartupBackoffDuration']; + // 'enabled' is not included here because it is responsible for hiding/showing these params and renders separately in the form + return ['intervalDuration', 'minStartupBackoffDuration', 'maxStartupBackoffDuration']; } // shared between auto and manual tidy operations diff --git a/ui/lib/pki/addon/components/pki-tidy-form.hbs b/ui/lib/pki/addon/components/pki-tidy-form.hbs index 5edc547eeb1e..31d6b1f7cf15 100644 --- a/ui/lib/pki/addon/components/pki-tidy-form.hbs +++ b/ui/lib/pki/addon/components/pki-tidy-form.hbs @@ -14,28 +14,33 @@ {{#if (eq @tidyType "auto")}} - {{#let this.intervalDurationAttr as |attr|}} - + {{#let (get @tidy.allByKey "enabled") as |enabledAttr|}} +
+ + + + {{if @tidy.enabled enabledAttr.options.label enabledAttr.options.labelDisabled}} + + {{#unless @tidy.enabled}} +

{{enabledAttr.options.helperTextDisabled}}

+ {{/unless}} + + +
{{/let}} - {{#if @tidy.enabled}} +

+ Automatic tidy settings +

{{#each @tidy.autoTidyConfigFields as |field|}} {{/each}} {{/if}} {{/if}} - {{#each @tidy.formFieldGroups as |fieldGroup|}} - {{#each-in fieldGroup as |group fields|}} - {{#if (or (eq @tidyType "manual") @tidy.enabled)}} + + {{#if (or (eq @tidyType "manual") @tidy.enabled)}} + {{#each @tidy.formFieldGroups as |fieldGroup|}} + {{#each-in fieldGroup as |group fields|}}

{{group}}

@@ -58,9 +63,9 @@ {{/if}} {{/if}} {{/each}} - {{/if}} - {{/each-in}} - {{/each}} + {{/each-in}} + {{/each}} + {{/if}}
diff --git a/ui/lib/pki/addon/components/pki-tidy-form.ts b/ui/lib/pki/addon/components/pki-tidy-form.ts index a5a290ad821e..1b388b77eade 100644 --- a/ui/lib/pki/addon/components/pki-tidy-form.ts +++ b/ui/lib/pki/addon/components/pki-tidy-form.ts @@ -23,11 +23,9 @@ interface Args { } interface PkiTidyTtls { - intervalDuration: string; acmeAccountSafetyBuffer: string; } interface PkiTidyBooleans { - enabled: boolean; tidyAcme: boolean; } @@ -37,10 +35,6 @@ export default class PkiTidyForm extends Component { @tracked errorBanner = ''; @tracked invalidFormAlert = ''; - get intervalDurationAttr() { - return this.args.tidy?.allByKey.intervalDuration; - } - @task @waitFor *save(event: Event) { diff --git a/ui/tests/acceptance/pki/pki-tidy-test.js b/ui/tests/acceptance/pki/pki-tidy-test.js index 76b75aed7bf9..0c6d93605d72 100644 --- a/ui/tests/acceptance/pki/pki-tidy-test.js +++ b/ui/tests/acceptance/pki/pki-tidy-test.js @@ -129,7 +129,7 @@ module('Acceptance | pki tidy', function (hooks) { await click(PKI_TIDY.tidyEmptyStateConfigure); await click(PKI_TIDY.tidyConfigureModal.tidyModalAutoButton); assert.dom(PKI_TIDY_FORM.tidyFormName('auto')).exists('Auto tidy form exists'); - await click(PKI_TIDY_FORM.toggleLabel('Automatic tidy disabled')); + await click(GENERAL.ttl.toggle('enabled')); assert .dom(PKI_TIDY_FORM.tidySectionHeader('ACME operations')) .exists('Auto tidy form enabled shows ACME operations field'); @@ -192,7 +192,7 @@ module('Acceptance | pki tidy', function (hooks) { await click(PKI_TIDY.tidyConfigureModal.tidyOptionsModal); assert.dom(PKI_TIDY.tidyConfigureModal.configureTidyModal).exists('Configure tidy modal exists'); await click(PKI_TIDY.tidyConfigureModal.tidyModalAutoButton); - await click(PKI_TIDY_FORM.toggleLabel('Automatic tidy disabled')); + await click(GENERAL.ttl.toggle('enabled')); await click(PKI_TIDY_FORM.inputByAttr('tidyCertStore')); await click(PKI_TIDY_FORM.inputByAttr('tidyRevokedCerts')); await click(PKI_TIDY_FORM.tidySave); diff --git a/ui/tests/helpers/pki/pki-selectors.ts b/ui/tests/helpers/pki/pki-selectors.ts index 2bd7bd320e99..4b9839d21f40 100644 --- a/ui/tests/helpers/pki/pki-selectors.ts +++ b/ui/tests/helpers/pki/pki-selectors.ts @@ -195,7 +195,6 @@ export const PKI_TIDY_FORM = { tidyFormName: (attr: string) => `[data-test-tidy-form="${attr}"]`, inputByAttr: (attr: string) => `[data-test-input="${attr}"]`, toggleInput: (attr: string) => `[data-test-input="${attr}"] input`, - intervalDuration: '[data-test-ttl-value="Automatic tidy enabled"]', acmeAccountSafetyBuffer: '[data-test-ttl-value="Tidy ACME enabled"]', toggleLabel: (label: string) => `[data-test-toggle-label="${label}"]`, tidySectionHeader: (header: string) => `[data-test-tidy-header="${header}"]`, diff --git a/ui/tests/integration/components/pki/page/pki-tidy-auto-settings-test.js b/ui/tests/integration/components/pki/page/pki-tidy-auto-settings-test.js index 3968dd9ad24b..658a91573889 100644 --- a/ui/tests/integration/components/pki/page/pki-tidy-auto-settings-test.js +++ b/ui/tests/integration/components/pki/page/pki-tidy-auto-settings-test.js @@ -51,12 +51,14 @@ module('Integration | Component | page/pki-tidy-auto-settings', function (hooks) .hasText('Edit auto-tidy', 'toolbar edit link has correct text'); assert.dom('[data-test-row="enabled"] [data-test-label-div]').hasText('Automatic tidy enabled'); - assert.dom('[data-test-row="intervalDuration"] [data-test-label-div]').hasText('Automatic tidy duration'); + assert.dom('[data-test-value-div="Automatic tidy enabled"]').hasText('No'); + assert.dom('[data-test-value-div="Interval duration"]').hasText('2 days'); // Universal operations assert.dom('[data-test-group-title="Universal operations"]').hasText('Universal operations'); assert .dom('[data-test-value-div="Tidy the certificate store"]') .exists('Renders universal field when value exists'); + assert.dom('[data-test-value-div="Tidy the certificate store"]').hasText('No'); assert .dom('[data-test-value-div="Tidy revoked certificates"]') .doesNotExist('Does not render universal field when value null'); @@ -65,6 +67,7 @@ module('Integration | Component | page/pki-tidy-auto-settings', function (hooks) assert .dom('[data-test-value-div="Tidy expired issuers"]') .exists('Renders issuer op field when value exists'); + assert.dom('[data-test-value-div="Tidy expired issuers"]').hasText('Yes'); assert .dom('[data-test-value-div="Tidy legacy CA bundle"]') .doesNotExist('Does not render issuer op field when value null'); diff --git a/ui/tests/integration/components/pki/pki-tidy-form-test.js b/ui/tests/integration/components/pki/pki-tidy-form-test.js index 1861781a4e51..9f6c824eaaa1 100644 --- a/ui/tests/integration/components/pki/pki-tidy-form-test.js +++ b/ui/tests/integration/components/pki/pki-tidy-form-test.js @@ -10,6 +10,8 @@ import { hbs } from 'ember-cli-htmlbars'; import { setupEngine } from 'ember-engines/test-support'; import { setupMirage } from 'ember-cli-mirage/test-support'; import { PKI_TIDY_FORM } from 'vault/tests/helpers/pki/pki-selectors'; +import { GENERAL } from 'vault/tests/helpers/general-selectors'; +import { convertToSeconds } from 'core/utils/duration-utils'; module('Integration | Component | pki tidy form', function (hooks) { setupRenderingTest(hooks); @@ -24,21 +26,36 @@ module('Integration | Component | pki tidy form', function (hooks) { this.onSave = () => {}; this.onCancel = () => {}; this.manualTidy = this.store.createRecord('pki/tidy', { backend: 'pki-manual-tidy' }); + this.autoTidyServerDefaults = { + enabled: false, + interval_duration: '12h', + safety_buffer: '3d', + issuer_safety_buffer: '365d', + min_startup_backoff_duration: '5m', + max_startup_backoff_duration: '15m', + }; this.store.pushPayload('pki/tidy', { modelName: 'pki/tidy', id: 'pki-auto-tidy', + // setting defaults here to simulate how this form works in the app. + // on init, we retrieve these from the server and pre-populate form (instead of explicitly set on the model) + ...this.autoTidyServerDefaults, }); this.autoTidy = this.store.peekRecord('pki/tidy', 'pki-auto-tidy'); + this.numTidyAttrs = Object.keys(this.autoTidy.allByKey).length; }); test('it hides or shows fields depending on auto-tidy toggle', async function (assert) { - assert.expect(47); const sectionHeaders = [ + 'Automatic tidy settings', 'Universal operations', 'ACME operations', 'Issuer operations', 'Cross-cluster operations', ]; + const loopAssertCount = this.numTidyAttrs * 2 - 3; // loop skips 3 params + const headerAssertCount = sectionHeaders.length * 2; + assert.expect(loopAssertCount + headerAssertCount + 4); await render( hbs` @@ -51,11 +68,13 @@ module('Integration | Component | pki tidy form', function (hooks) { `, { owner: this.engine } ); - assert.dom(PKI_TIDY_FORM.toggleInput('intervalDuration')).isNotChecked('Automatic tidy is disabled'); - assert.dom(`[data-test-ttl-form-label="Automatic tidy disabled"]`).exists('renders disabled label text'); + assert.dom(GENERAL.toggleInput('enabled')).isNotChecked(); + assert + .dom(GENERAL.ttl.toggle('enabled')) + .hasText('Automatic tidy disabled Automatic tidy operations will not run.'); this.autoTidy.eachAttribute((attr) => { - if (attr === 'enabled' || attr === 'intervalDuration') return; + if (attr === 'enabled') return; assert .dom(PKI_TIDY_FORM.inputByAttr(attr)) .doesNotExist(`does not render ${attr} when auto tidy disabled`); @@ -66,12 +85,12 @@ module('Integration | Component | pki tidy form', function (hooks) { }); // ENABLE AUTO TIDY - await click(PKI_TIDY_FORM.toggleInput('intervalDuration')); - assert.dom(PKI_TIDY_FORM.toggleInput('intervalDuration')).isChecked('Automatic tidy is enabled'); - assert.dom(`[data-test-ttl-form-label="Automatic tidy enabled"]`).exists('renders enabled text'); + await click(GENERAL.toggleInput('enabled')); + assert.dom(GENERAL.toggleInput('enabled')).isChecked(); + assert.dom(GENERAL.ttl.toggle('enabled')).hasText('Automatic tidy enabled'); this.autoTidy.eachAttribute((attr) => { - const skipFields = ['enabled', 'tidyAcme', 'intervalDuration']; + const skipFields = ['enabled', 'tidyAcme']; if (skipFields.includes(attr)) return; // combined with duration ttl or asserted elsewhere assert.dom(PKI_TIDY_FORM.inputByAttr(attr)).exists(`renders ${attr} when auto tidy enabled`); }); @@ -82,10 +101,9 @@ module('Integration | Component | pki tidy form', function (hooks) { }); test('it renders all attribute fields, including enterprise', async function (assert) { - assert.expect(33); + assert.expect(35); this.autoTidy.enabled = true; - const skipFields = ['enabled', 'tidyAcme', 'intervalDuration']; // combined with duration ttl or asserted separately - const fieldsNotPartOfManualTidy = ['minStartupBackoffDuration', 'maxStartupBackoffDuration']; // fields we shouldn't see on the manual tidy screen + const skipFields = ['enabled', 'tidyAcme']; // combined with duration ttl or asserted separately await render( hbs` { if (skipFields.includes(attr)) return; - if (fieldsNotPartOfManualTidy.includes(attr)) { + // auto tidy fields we shouldn't see in the manual tidy form + if (this.manualTidy.autoTidyConfigFields.includes(attr)) { assert .dom(PKI_TIDY_FORM.inputByAttr(attr)) .doesNotExist(`${attr} should not appear on manual tidyType`); @@ -183,19 +203,35 @@ module('Integration | Component | pki tidy form', function (hooks) { test('it should change the attributes on the model', async function (assert) { assert.expect(12); + // ttl picker defaults to seconds, unless unit is set by default value (set in beforeEach hook) + // on submit, any user inputted values should be converted to seconds for the payload + const fillInValues = { + acmeAccountSafetyBuffer: { time: 680, unit: 'h' }, + intervalDuration: { time: 10, unit: 'h' }, + issuerSafetyBuffer: { time: 20, unit: 'd' }, + maxStartupBackoffDuration: { time: 30, unit: 'm' }, + minStartupBackoffDuration: { time: 10, unit: 'm' }, + pauseDuration: { time: 30, unit: 's' }, + revocationQueueSafetyBuffer: { time: 40, unit: 's' }, + safetyBuffer: { time: 50, unit: 'd' }, + }; + const calcValue = (param) => { + const { time, unit } = fillInValues[param]; + return `${convertToSeconds(time, unit)}s`; + }; this.server.post('/pki-auto-tidy/config/auto-tidy', (schema, req) => { assert.propEqual( JSON.parse(req.requestBody), { - acme_account_safety_buffer: '72h', + acme_account_safety_buffer: '48h', enabled: true, - min_startup_backoff_duration: '5m', - max_startup_backoff_duration: '15m', - interval_duration: '10s', - issuer_safety_buffer: '20s', - pause_duration: '30s', - revocation_queue_safety_buffer: '40s', - safety_buffer: '50s', + min_startup_backoff_duration: calcValue('minStartupBackoffDuration'), + max_startup_backoff_duration: calcValue('maxStartupBackoffDuration'), + interval_duration: calcValue('intervalDuration'), + issuer_safety_buffer: calcValue('issuerSafetyBuffer'), + pause_duration: calcValue('pauseDuration'), + revocation_queue_safety_buffer: calcValue('revocationQueueSafetyBuffer'), + safety_buffer: calcValue('safetyBuffer'), tidy_acme: true, tidy_cert_metadata: true, tidy_cert_store: true, @@ -222,16 +258,14 @@ module('Integration | Component | pki tidy form', function (hooks) { { owner: this.engine } ); - assert.dom(PKI_TIDY_FORM.toggleInput('intervalDuration')).isNotChecked('Automatic tidy is disabled'); - assert.dom(PKI_TIDY_FORM.toggleLabel('Automatic tidy disabled')).exists('auto tidy has disabled label'); + assert.dom(GENERAL.toggleInput('enabled')).isNotChecked(); + assert.dom(GENERAL.ttl.toggle('enabled')).hasTextContaining('Automatic tidy disabled'); assert.false(this.autoTidy.enabled, 'enabled is false on model'); // enable auto-tidy - await click(PKI_TIDY_FORM.toggleInput('intervalDuration')); - await fillIn(PKI_TIDY_FORM.intervalDuration, 10); - - assert.dom(PKI_TIDY_FORM.toggleInput('intervalDuration')).isChecked('toggle enabled auto tidy'); - assert.dom(PKI_TIDY_FORM.toggleLabel('Automatic tidy enabled')).exists('auto tidy has enabled label'); + await click(GENERAL.toggleInput('enabled')); + assert.dom(GENERAL.toggleInput('enabled')).isChecked(); + assert.dom(GENERAL.ttl.toggle('enabled')).hasText('Automatic tidy enabled'); assert.dom(PKI_TIDY_FORM.toggleInput('acmeAccountSafetyBuffer')).isNotChecked('ACME tidy is disabled'); assert @@ -240,30 +274,23 @@ module('Integration | Component | pki tidy form', function (hooks) { assert.false(this.autoTidy.tidyAcme, 'tidyAcme is false on model'); await click(PKI_TIDY_FORM.toggleInput('acmeAccountSafetyBuffer')); - await fillIn(PKI_TIDY_FORM.acmeAccountSafetyBuffer, 3); // units are days based on defaultValue + await fillIn(PKI_TIDY_FORM.acmeAccountSafetyBuffer, 2); // units are days based on defaultValue + assert.dom(PKI_TIDY_FORM.toggleInput('acmeAccountSafetyBuffer')).isChecked('ACME tidy is enabled'); + assert.dom(PKI_TIDY_FORM.toggleLabel('Tidy ACME enabled')).exists('ACME label has correct enabled text'); assert.true(this.autoTidy.tidyAcme, 'tidyAcme toggles to true'); - const fillInValues = { - issuerSafetyBuffer: 20, - pauseDuration: 30, - revocationQueueSafetyBuffer: 40, - safetyBuffer: 50, - minStartupBackoffDuration: 300, - maxStartupBackoffDuration: 900, - }; this.autoTidy.eachAttribute(async (attr, { type }) => { - const skipFields = ['enabled', 'tidyAcme', 'intervalDuration', 'acmeAccountSafetyBuffer']; // combined with duration ttl or asserted separately + const skipFields = ['enabled', 'tidyAcme', 'acmeAccountSafetyBuffer']; // combined with duration ttl or asserted separately if (skipFields.includes(attr)) return; + // all params right now are either a boolean or TTL, this if/else will need to be updated if that changes if (type === 'boolean') { await click(PKI_TIDY_FORM.inputByAttr(attr)); - } - if (type === 'string') { - await fillIn(PKI_TIDY_FORM.toggleInput(attr), `${fillInValues[attr]}`); + } else { + const { time } = fillInValues[attr]; + await fillIn(PKI_TIDY_FORM.toggleInput(attr), `${time}`); } }); - assert.dom(PKI_TIDY_FORM.toggleInput('acmeAccountSafetyBuffer')).isChecked('ACME tidy is enabled'); - assert.dom(PKI_TIDY_FORM.toggleLabel('Tidy ACME enabled')).exists('ACME label has correct enabled text'); await click(PKI_TIDY_FORM.tidySave); }); @@ -274,11 +301,11 @@ module('Integration | Component | pki tidy form', function (hooks) { assert.propEqual( JSON.parse(req.requestBody), { + ...this.autoTidyServerDefaults, acme_account_safety_buffer: '720h', - enabled: false, tidy_acme: false, }, - 'response contains auto-tidy params' + 'response contains default auto-tidy params' ); }); this.onSave = () => assert.ok(true, 'onSave callback fires on save success');