Unify labels and tags

[davidbolet]

Summary

• Introduce unified resource_labels model/table for labels and key-configuration tags.
• Migrate existing key labels and tags into resource_labels with new tenant migrations.
• Update managers, authz repo resource types/policies, and affected tests to use the new storage.

Test plan

• go test ./... (integration tests may require local container/runtime setup)

[coderabbitai]

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: Pro Plus

Run ID: ec582ca2-5e6b-445a-8a29-cd5a5247f9fa

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[Copilot]

Pull request overview

This PR introduces a unified resource_labels storage model/table to represent both key labels and key-configuration tags, migrates existing data into the new schema, and updates managers/authz/tests to use the new backing store while keeping the existing Tag/Label manager APIs.

Changes:

• Added resource_labels table + uniqueness rules to support label semantics and multi-value system.tag semantics.
• Added tenant migrations to move existing key_labels and tags data into resource_labels.
• Implemented ResourceLabelManager and refactored Tag/Label managers and tests to use the new unified storage.

Reviewed changes

Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/integration/tenant-manager/provisioning_test.go	Wires ResourceLabelManager into TagManager construction for integration tests.
migrations/tenant/schema/00016_add_resource_labels_table.sql	Creates resource_labels table and supporting indexes/uniqueness constraints.
migrations/tenant/schema/00017_migrate_labels_and_tags_data.sql	Migrates existing key_labels and tags data into resource_labels.
internal/repo/query.go	Adds query fields needed to filter resource_labels (resource_type, value).
internal/operator/operator_test.go	Updates test manager wiring to use ResourceLabelManager-backed TagManager.
internal/model/resource_label.go	Introduces the ResourceLabel model + resource type constants and reserved system.tag key.
internal/manager/workflow_test.go	Updates workflow manager test setup to use new tag wiring.
internal/manager/tenant_test.go	Updates tenant manager test setup to use new tag wiring.
internal/manager/tag.go	Refactors TagManager into an adapter delegating to ResourceLabelManager.
internal/manager/tag_test.go	Updates tag tests to validate behavior via resource_labels.
internal/manager/system_test.go	Updates system manager test setup to use new tag wiring.
internal/manager/resource_label.go	Adds ResourceLabelManager implementing unified label/tag operations.
internal/manager/resource_label_test.go	Adds comprehensive tests for ResourceLabelManager label/tag behavior.
internal/manager/keyversion_test.go	Updates key version test setup to use new tag wiring.
internal/manager/keyconfiguration_test.go	Updates key configuration tests to use resource_labels for tags and new query fields.
internal/manager/key_test.go	Updates key tests to use new tag wiring.
internal/manager/key_label.go	Refactors LabelManager into an adapter delegating to ResourceLabelManager.
internal/manager/key_label_test.go	Updates label manager tests to validate behavior via resource_labels.
internal/manager/errors.go	Adds/adjusts tag-related error(s) for the new implementation.
internal/manager/base.go	Updates manager factory wiring to share a ResourceLabelManager across Tags/Labels.
internal/controllers/cmk/keyconfiguration_tags_controller_test.go	Updates controller tests to seed/verify tags via resource_labels.
internal/controllers/cmk/key_labels_controller_test.go	Updates controller tests to seed/verify labels via resource_labels.
internal/constants/table-names.go	Adds ResourceLabelTable constant.
internal/authz/repo_business_policies.go	Grants repo actions for the new resource_labels resource type.
internal/authz/policy_tests/workflow_autoassign_test.go	Updates policy tests to use new tag wiring.
internal/authz/policy_tests/keystore_pool_test.go	Updates policy tests to use new tag wiring.
internal/authz/policy_tests/hyok_sync_test.go	Updates policy tests to use new tag wiring.
internal/authz/policies.go	Registers RepoResourceTypeResourceLabel and its supported actions.
internal/async/tasks/tenant/workflow_expiry_test.go	Updates task test setup to use new tag wiring.
cmd/tenant-manager/main.go	Updates tenant-manager wiring to use ResourceLabelManager-backed TagManager.
cmd/tenant-manager/main_test.go	Avoids port collisions by binding status server to an ephemeral port in tests.
cmd/tenant-manager-cli/commands/commands.go	Updates CLI wiring to use ResourceLabelManager-backed TagManager.
cmd/tenant-manager-cli/cli_test.go	Updates CLI tests to use new tag wiring.
cmd/task-worker/main.go	Updates task-worker wiring to use ResourceLabelManager-backed TagManager.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

migrations/tenant/schema/00017_migrate_labels_and_tags_data.sql:60

• The Down migration deletes all KEY_CONFIG system.tag rows in resource_labels, which can remove tags created after this migration (i.e., not actually migrated from the legacy tags table). This makes the migration effectively non-reversible and potentially destructive in rollback scenarios.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

internal/manager/resource_label.go:149

• GetTags builds a query without an ORDER BY, so the returned tag slice can be nondeterministic across runs/DB plans (and differs from the previous JSON-array storage which preserved ordering). Add an explicit order to make API behavior stable (e.g., by value).

	query := repo.NewQuery().Where(repo.NewCompositeKeyGroup(ck))

	labels := []*model.ResourceLabel{}
	err := m.r.List(ctx, &model.ResourceLabel{}, &labels, *query)

[Copilot]

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 12 comments.

Comments suppressed due to low confidence (1)

internal/manager/key_test.go:96

• NewKeyManager now requires a labels manager argument (Label) before eventFactory. This call still uses the old parameter list, which will not compile and also leaves KeyManager.labels nil.

	tagManager := manager.NewTagManager(resourceLabelManager)
	keyConfigManager := manager.NewKeyConfigManager(r, certManager, userManager, tagManager, cmkAuditor, eventFactory, cfg)

	km := manager.NewKeyManager(
		r, svcRegistry, tenantConfigManager, keyConfigManager, userManager, certManager, eventFactory, cmkAuditor,

[Copilot]

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

internal/async/tasks/tenant/workflow_expiry_test.go:127

• KeyManager construction wasn’t updated for the new NewKeyManager signature (now requires a Label manager and eventFactory). This currently passes nil where a Label manager is expected and shifts cmkAuditor into the eventFactory slot, which won’t compile and would panic if it did.

	resourceLabelManager := manager.NewResourceLabelManager(r)
	tagManager := manager.NewTagManager(resourceLabelManager)
	keyConfigManager := manager.NewKeyConfigManager(r, certManager, userManager, tagManager, cmkAuditor, eventFactory, cfg)
	groupManager := manager.NewGroupManager(r, svcRegistry, userManager)

internal/operator/operator_test.go:121

• KeyManager construction in this test still uses the old NewKeyManager argument list. With the new signature, the call is missing a Label manager parameter, so this won’t compile (and the labels dependency would be nil if forced).
  test/integration/tenant-manager/provisioning_test.go:70
• KeyManager construction later in this setup still uses the pre-labels NewKeyManager signature. Since KeyManager.Delete now calls into a Label manager, this setup needs to create a LabelManager (backed by the same ResourceLabelManager) and pass it into NewKeyManager, otherwise this won’t compile / would be nil.
  internal/manager/resource_label.go:178
• GetTags returns rows without an ORDER BY, so the tag list order becomes non-deterministic. Previously tags were stored as a JSON array and GetTags preserved that array order; returning tags in a stable order helps avoid subtle client/test regressions.

	query := repo.NewQuery().Where(repo.NewCompositeKeyGroup(ck))

	labels := []*model.ResourceLabel{}
	err := m.r.List(ctx, &model.ResourceLabel{}, &labels, *query)
	if err != nil {

internal/manager/resource_label.go:220

• SetTags deduplicates via a map and then iterates that map, which randomizes insertion order. Combined with DB ordering, this makes tag ordering unpredictable and diverges from the previous ‘order as provided’ behavior.

		// Deduplicate and filter empty tags
		uniqueTags := make(map[string]struct{})
		for _, tag := range tags {
			if tag != "" {
				uniqueTags[tag] = struct{}{}

[Copilot]

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/manager/resource_label.go:282

• upsertLabel tries to detect a uniqueness conflict via errors.As(err, *repo.UniqueConstraintError), but the SQL repo wraps duplicate-key errors as errs.Wrap(repo.ErrUniqueConstraint, err) and never returns *repo.UniqueConstraintError. As a result, updating an existing label will fail instead of falling back to the update path.

	// Check if it's a unique constraint violation
	var uniqueErr *repo.UniqueConstraintError
	if !errors.As(err, &uniqueErr) {
		// Some other error occurred
		return errs.Wrap(ErrInsertLabel, err)
	}

internal/manager/base.go:93

• labelManager is constructed above and passed into keyManager, but Manager.Labels is initialized with a second, newly constructed LabelManager. This duplication makes the wiring harder to follow and risks divergence if LabelManager ever gains state.

		Tags:          tagManager,
		Labels:        NewLabelManager(repo, resourceLabelManager),

[Copilot]

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

internal/manager/errors.go:129

• ErrQueryLabelList’s message currently says "failed to query system list", which is misleading now that it is used for label/resource_label listing errors.

	ErrQueryLabelList    = errors.New("failed to query system list")
	ErrFetchLabel        = errors.New("failed to fetch label")
	ErrUpdateLabelDB     = errors.New("failed to update label")
	ErrInsertLabel       = errors.New("failed to insert label")
	ErrDeleteLabelDB     = errors.New("failed to delete label")
	ErrReservedLabelKey  = errors.New("label key 'system.tag' is reserved for tags, use SetTags/GetTags instead")

internal/manager/resource_label.go:178

• GetTags returns tags in DB iteration order (no ORDER BY), which can be nondeterministic and may change the API response order compared to the previous JSON-array storage. Consider ordering the query to keep results stable (e.g., by Value).

	// Query all labels with key="system.tag"
	ck := repo.NewCompositeKey().
		Where(repo.ResourceTypeField, resourceType).
		Where(repo.ResourceIDField, resourceID).
		Where(repo.KeyField, model.SystemTagKey)

	query := repo.NewQuery().Where(repo.NewCompositeKeyGroup(ck))

	labels := []*model.ResourceLabel{}
	err := m.r.List(ctx, &model.ResourceLabel{}, &labels, *query)
	if err != nil {

internal/manager/base.go:94

• New() already constructs labelManager (used by KeyManager), but the returned Manager struct creates a second LabelManager instance for Manager.Labels. Reuse the existing labelManager to avoid duplicate construction and keep a single shared implementation.

		System:        systemManager,
		KeyConfig:     keyConfigManager,
		Tags:          tagManager,
		Labels:        NewLabelManager(repo, resourceLabelManager),
		Workflow: NewWorkflowManager(

[Copilot]

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/manager/errors.go:129

• In this error block, ErrQueryLabelList (a few lines above) currently says "failed to query system list". That message is misleading for label/listing failures (ResourceLabelManager.GetLabels wraps ErrQueryLabelList).

	ErrReservedLabelKey  = errors.New("label key 'system.tag' is reserved for tags, use SetTags/GetTags instead")

internal/manager/resource_label.go:298

• upsertLabel retries unique-constraint races via unbounded recursion. If the unique-constraint condition persists unexpectedly (e.g., error mapping issues or repeated contention), this can recurse indefinitely and risk stack growth. Prefer a bounded retry loop and then fall back to re-fetch+update.

		// Check if it's a unique constraint violation (race condition)
		if errors.Is(err, repo.ErrUniqueConstraint) {
			// Another transaction created it between our check and insert
			// Retry the whole operation
			return m.upsertLabel(ctx, label)
		}