feat: make key version count configurable per landscape and provider by davidbolet · Pull Request #420 · openkcm/cmk

davidbolet · 2026-09-10T07:58:39Z

Implements configurable key version limits per landscape and provider

feat(config): add per-provider maxKeyVersions configuration
feat: add configurable per-landscape key version limits

Changes

Configuration: Added MaxKeyVersions map to Landscape struct with per-provider limits
Eviction Logic: Implemented automatic version eviction in KeyVersionManager.UpdateVersions()
Re-addition Support: Leverages existing upsert logic to handle previously evicted versions
Testing: Added 7 comprehensive unit tests covering all eviction scenarios

Configuration Example

landscape:
  name: dev
  maxKeyVersions:
    AWS: 10
    GCP: 5
    FORTANIX: -1  # unlimited

coderabbitai · 2026-09-10T07:58:45Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 11b6c0f3-5d86-4c00-a328-8d9da93a796e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

🟡 Changes recommended

The eviction implementation currently hard-caps reads to 10,000 versions, which can lead to incomplete enforcement and unnecessary load, and should be refactored to page/delete without a fixed cap.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds per-landscape, per-provider configuration for maximum retained key versions and enforces those limits during key version sync, with accompanying tests and documentation notes.

Changes:

Extend config.Landscape with maxKeyVersions (per-provider), plus defaults, unlimited sentinel, and validation.
Enforce version retention limits after KeyVersionManager.UpdateVersions() upserts versions.
Add unit tests for landscape config helpers/validation and key-version eviction scenarios; update example config.yaml and add implementation/analysis docs.

File summaries

File	Description
`PLUGIN-VERIFICATION-STATUS.md`	Documents plugin SDK `GetKeyVersions` interface compliance investigation.
`KMS20-6953-ANALYSIS-UPDATED.md`	Updated technical analysis reflecting recent architecture changes and recommended implementation approach.
`internal/manager/keyversion.go`	Inject landscape config into `KeyVersionManager` and enforce per-provider version eviction after upsert.
`internal/manager/keyversion_test.go`	Adds unit tests for eviction behavior across limits/providers/unlimited/nil-config scenarios.
`internal/manager/key.go`	Wires landscape config into `KeyVersionManager` from the loaded application config.
`internal/manager/base.go`	Updates manager construction to pass landscape config into `KeyVersionManager`.
`internal/config/landscape_test.go`	New unit tests for `Landscape.GetMaxVersionsForProvider()` and `Landscape.Validate()`.
`internal/config/config.go`	Adds `Landscape.MaxKeyVersions`, default/unlimited constants, getter, validation, and hooks validation into `Config.Validate()`.
`IMPLEMENTATION-PROGRESS.md`	Progress/decision log for KMS20-6953 implementation steps and open questions.
`config.yaml`	Adds an example `landscape.maxKeyVersions` configuration block.

Review details

Files reviewed: 10/10 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+	maxVersions := kvm.landscapeConfig.GetMaxVersionsForProvider(key.Provider)
+	if maxVersions == config.UnlimitedKeyVersions {
+		// Unlimited versions - no eviction
+		return nil
+	}


+	// 3. Get all current versions ordered by RotatedAt DESC (most recent first)
+	allVersions, _, err := kvm.GetKeyVersions(
+		ctx,
+		keyID,
+		repo.Pagination{Top: 10000, Skip: 0, Count: true}, // High limit to get all versions
+	)


+// This method logs warnings on failures but does not return errors to avoid
+// rolling back version upserts that have already succeeded.


+		assert.Equal(t, primaryVersion.ID, allVersions[0].ID, "Primary should match latest")
+	})
+
+	t.Run("Should restore previously evicted version on re-addition", func(t *testing.T) {


Copilot

🟡 Changes recommended

There is a test compilation issue (missing context import) and the eviction enforcement currently relies on a fixed page size and len() rather than total count, which can fail to enforce limits for large version histories.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 10/10 changed files
Comments generated: 4
Review effort level: Lite

 import (
+	"fmt"
 	"testing"
 	"time"



+	// 3. Get all current versions ordered by RotatedAt DESC (most recent first)
+	allVersions, _, err := kvm.GetKeyVersions(
+		ctx,
+		keyID,
+		repo.Pagination{Top: 10000, Skip: 0, Count: true}, // High limit to get all versions
+	)
+	if err != nil {
+		return fmt.Errorf("failed to get versions for limit enforcement: %w", err)
+	}
+
+	// 4. Check if eviction is needed
+	if len(allVersions) <= maxVersions {
+		// Within limit - nothing to do
+		return nil
+	}
+
+	// 5. Delete oldest versions (beyond limit)
+	return kvm.deleteExcessVersions(ctx, keyID, key.Provider, allVersions, maxVersions)
+}


 	return &Manager{
 		Keys:          keyManager,
-		KeyVersions:   NewKeyVersionManager(repo, svcRegistry, tenantConfigManager, certManager, cmkAuditor),
+		KeyVersions: NewKeyVersionManager(
+		repo, svcRegistry, tenantConfigManager, certManager, cmkAuditor, &config.Landscape,
+	),
 		TenantConfigs: tenantConfigManager,


+// This method logs warnings on failures but does not return errors to avoid
+// rolling back version upserts that have already succeeded.


- Add MaxKeyVersions map to Landscape struct for provider-scoped version limits - Implement GetMaxVersionsForProvider() helper with default of 5 versions - Add Landscape.Validate() to ensure limits are -1 (unlimited) or >= 1 - Add comprehensive unit tests for config loading and validation - Update example config.yaml with maxKeyVersions configuration Part of KMS20-6953: Make managed key versions count configurable per landscape Default behavior: - 5 versions per key if not configured - -1 for unlimited (no eviction) - Independent limits per provider (AWS, GCP, FORTANIX) Test coverage: - GetMaxVersionsForProvider with various scenarios - Validate with valid and invalid configurations - All tests passing (0.713s) Related: - KMS20-6484 (GCP HYOK provider - in progress) Signed-off-by: David <davidbolet@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>

- Add PLUGIN-VERIFICATION-STATUS.md with plugin compatibility analysis - Add IMPLEMENTATION-PROGRESS.md with detailed progress summary - Add KMS20-6953-ANALYSIS-UPDATED.md with updated technical analysis Documentation includes: - Plugin GetKeyVersions interface verification - Implementation steps 1 & 2 completion status - Test results and coverage metrics - Open questions for review - Next steps and blockers Signed-off-by: David <davidbolet@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>

Signed-off-by: David <davidbolet@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>

Signed-off-by: David <davidbolet@gmail.com>

Copilot

🟡 Changes recommended

The new eviction/enforcement logic has correctness bugs (missing-key handling can return nil error and eviction relies on a capped list) that can silently skip enforcement or fail to enforce limits.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 10/10 changed files
Comments generated: 4
Review effort level: Lite

+	key := &model.Key{ID: keyID}
+	found, err := kvm.repo.First(ctx, key, *repo.NewQuery())
+	if err != nil || !found {
+		return fmt.Errorf("failed to get key for version limit enforcement: %w", err)
+	}


+	// 3. Get all current versions ordered by RotatedAt DESC (most recent first)
+	allVersions, _, err := kvm.GetKeyVersions(
+		ctx,
+		keyID,
+		repo.Pagination{Top: 10000, Skip: 0, Count: true}, // High limit to get all versions
+	)
+	if err != nil {
+		return fmt.Errorf("failed to get versions for limit enforcement: %w", err)
+	}
+
+	// 4. Check if eviction is needed
+	if len(allVersions) <= maxVersions {
+		// Within limit - nothing to do
+		return nil
+	}
+
+	// 5. Delete oldest versions (beyond limit)
+	return kvm.deleteExcessVersions(ctx, keyID, key.Provider, allVersions, maxVersions)


+// This method logs warnings on failures but does not return errors to avoid
+// rolling back version upserts that have already succeeded.


+		assert.Equal(t, primaryVersion.ID, allVersions[0].ID, "Primary should match latest")
+	})
+
+	t.Run("Should restore previously evicted version on re-addition", func(t *testing.T) {


Signed-off-by: David <davidbolet@gmail.com>

Copilot

🔵 Needs a closer look

The eviction enforcement has a correctness bug when the key is not found and should use a safer deletion strategy (and tests should cover the “evicted version becomes primary” re-addition case).

Review details

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

internal/manager/keyversion.go:234

repo.First returns (found=false, err=nil) when the key doesn't exist; the current check if err != nil || !found then wraps err with %w, which will return nil when err is nil. That makes missing keys silently skip enforcement, and defeats the warning log in UpdateVersions. Handle the !found case explicitly so it becomes a real error.
internal/manager/keyversion.go:268
Eviction currently loads up to 10k versions into memory and assumes that contains all versions; if a key ever has >10k versions, this will leave older versions undeleted (still exceeding the limit). A safer approach is to delete versions beyond the limit in batches using pagination with Skip=maxVersions, so you don't need to fetch the entire history.
internal/manager/keyversion_test.go:796
This test intends to cover the acceptance criterion that a previously evicted version is restored if it becomes the primary/most-recent version. As written, the re-added v1 keeps the old timestamp, so it correctly gets evicted again and the primary-restoration path isn't exercised. Adjust the re-add input so v1 becomes most recent.
internal/manager/keyversion.go:224
The doc comment says this method "does not return errors", but it does return an error to the caller (which then logs and continues). Updating the comment avoids confusion for future maintainers.
internal/manager/keyversion_test.go:818
With the re-addition scenario making v1 the most recent version, the assertions should expect v1 to be present and ordered first (primary), with the remaining most-recent versions filling the rest of the limit.

Files reviewed: 10/10 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟡 Changes recommended

There are correctness issues in the eviction/enforcement implementation (including nil-config handling and incomplete/unsafe eviction behavior under larger datasets) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

internal/manager/keyversion.go:195

When k.CreationTime is nil, this path uses repo.Create without any conflict handling. If the provider repeatedly omits CreationTime for the same version ID, the next sync will hit the (key_id, native_id) unique constraint and fail; using the same upsert strategy as the non-nil path keeps UpdateVersions idempotent.
internal/manager/keyversion.go:255
Eviction currently fetches at most 10,000 versions and then deletes allVersions[maxVersions:]. If a key has >10,000 versions, this will not evict enough rows to reach maxVersions (older versions beyond the first 10k are never considered/deleted). Consider implementing a DB-side delete that removes all rows older than the Nth most recent version (e.g., DELETE WHERE key_id=? AND id NOT IN (SELECT id ... ORDER BY rotated_at DESC, created_at DESC LIMIT ?)), or iterating deletion in a way that can’t leave more than maxVersions behind.
internal/manager/keyversion_test.go:712
This subtest name says it "restores" an evicted version, but it only re-sends the same (oldest) timestamps and then asserts the version is evicted again. This doesn’t cover the acceptance criterion that an evicted version should be re-inserted and retained if it becomes primary/most-recent (e.g., same nativeID returning with a newer CreationTime).

Files reviewed: 10/10 changed files
Comments generated: 1
Review effort level: Lite

+		KeyVersions: NewKeyVersionManager(
+			repo, svcRegistry, tenantConfigManager, certManager, cmkAuditor, &config.Landscape,
+		),


Signed-off-by: David <davidbolet@gmail.com>

jmpTeixeira02 · 2026-09-15T09:09:42Z

Please remove these .md files. Not only is this slop that shouldn't be committed as it also exposes internal stuff... Please be careful and ALWAYS review AI output

jmpTeixeira02 · 2026-09-15T09:13:33Z

config_test.go

jmpTeixeira02 · 2026-09-15T09:16:11Z

+			now := time.Now().UTC()
+			k.CreationTime = &now


Why are we making this more verbose when the new() exists in current go version?

jmpTeixeira02 · 2026-09-15T09:17:44Z

+	if kvm.landscapeConfig == nil {
+		// No config available - skip enforcement
+		log.Debug(ctx, "Landscape config not available, skipping version limit enforcement",
+			slog.String("keyId", keyID.String()))
+		return nil
+	}


This is wrong, it should use the default

jmpTeixeira02 · 2026-09-15T09:18:17Z

+	allVersions, _, err := kvm.GetKeyVersions(
+		ctx,
+		keyID,
+		repo.Pagination{Top: 10000, Skip: 0, Count: true}, // High limit to get all versions


Top:1000 ???

This should also use the Count method as you only want the number of versions

jmpTeixeira02 · 2026-09-15T09:22:16Z

As mentioned before in multiple occasions, please disable your copilot reviews! If you want automated PR reviews use the AI agent we have on openkcm. coderabbit

Copilot AI lite review requested due to automatic review settings September 10, 2026 07:58

push-tags-from-workflow Bot added the tests label Sep 10, 2026

Copilot started reviewing on behalf of davidbolet September 10, 2026 07:59 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 10, 2026 13:15

davidbolet force-pushed the task/make_key_versions_count_configurable branch from b574c4d to 7cc7d44 Compare September 10, 2026 13:15

Copilot started reviewing on behalf of davidbolet September 10, 2026 13:16 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

davidbolet and others added 4 commits September 10, 2026 15:29

feat: add configurable per-landscape key version limits

10eb17f

Signed-off-by: David <davidbolet@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>

linter errors

1a4413f

Signed-off-by: David <davidbolet@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>

davidbolet force-pushed the task/make_key_versions_count_configurable branch from 7cc7d44 to 1a4413f Compare September 10, 2026 13:31

vet error

d1d299c

Signed-off-by: David <davidbolet@gmail.com>

Copilot AI review requested due to automatic review settings September 10, 2026 13:32

Copilot started reviewing on behalf of davidbolet September 10, 2026 13:33 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

format error and gci

a049aff

Signed-off-by: David <davidbolet@gmail.com>

Copilot AI review requested due to automatic review settings September 11, 2026 09:50

Copilot started reviewing on behalf of davidbolet September 11, 2026 09:50 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 11, 2026 10:32

Copilot started reviewing on behalf of davidbolet September 11, 2026 10:32 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread internal/manager/base.go

Comment on lines +86 to +88

KeyVersions: NewKeyVersionManager(

repo, svcRegistry, tenantConfigManager, certManager, cmkAuditor, &config.Landscape,

),

fix: nullpointer on config.Landscape

eda48b6

Signed-off-by: David <davidbolet@gmail.com>

davidbolet force-pushed the task/make_key_versions_count_configurable branch from de21332 to eda48b6 Compare September 11, 2026 10:53

jmpTeixeira02 requested changes Sep 15, 2026

View reviewed changes

Navigation Menu

feat: make key version count configurable per landscape and provider - #420

feat: make key version count configurable per landscape and provider#420
davidbolet wants to merge 7 commits into
mainfrom
task/make_key_versions_count_configurable

davidbolet commented Sep 10, 2026

Uh oh!

coderabbitai Bot commented Sep 10, 2026 •

edited

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

jmpTeixeira02 Sep 15, 2026

Uh oh!

jmpTeixeira02 Sep 15, 2026

Uh oh!

jmpTeixeira02 Sep 15, 2026

Uh oh!

jmpTeixeira02 Sep 15, 2026

Uh oh!

jmpTeixeira02 Sep 15, 2026

Uh oh!

jmpTeixeira02 Sep 15, 2026

Uh oh!

jmpTeixeira02 commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// This method logs warnings on failures but does not return errors to avoid
		// rolling back version upserts that have already succeeded.

Conversation

davidbolet commented Sep 10, 2026

Changes

Configuration Example

Uh oh!

coderabbitai Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (5)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

jmpTeixeira02 Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

jmpTeixeira02 Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

jmpTeixeira02 Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

jmpTeixeira02 Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

jmpTeixeira02 Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

jmpTeixeira02 Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

jmpTeixeira02 commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Sep 10, 2026 •

edited

Loading