Unify labels and tags - #344
Open
davidbolet wants to merge 7 commits into
Open
davidbolet wants to merge 7 commits into
davidbolet wants to merge 7 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Contributor
There was a problem hiding this comment.
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_labelstable + uniqueness rules to support label semantics and multi-valuesystem.tagsemantics. - Added tenant migrations to move existing
key_labelsandtagsdata intoresource_labels. - Implemented
ResourceLabelManagerand 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.
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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)
davidbolet
force-pushed
the
task/resource-labels-pr
branch
from
July 24, 2026 09:20
aeab56d to
b745759
Compare
Contributor
There was a problem hiding this comment.
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,
Contributor
There was a problem hiding this comment.
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
nilwhere a Label manager is expected and shiftscmkAuditorinto 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{}{}
davidbolet
force-pushed
the
task/resource-labels-pr
branch
from
July 24, 2026 15:14
b17dc91 to
1e86e19
Compare
Contributor
There was a problem hiding this comment.
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
upsertLabeltries to detect a uniqueness conflict viaerrors.As(err, *repo.UniqueConstraintError), but the SQL repo wraps duplicate-key errors aserrs.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
labelManageris constructed above and passed intokeyManager, butManager.Labelsis initialized with a second, newly constructedLabelManager. This duplication makes the wiring harder to follow and risks divergence ifLabelManagerever gains state.
Tags: tagManager,
Labels: NewLabelManager(repo, resourceLabelManager),
Contributor
There was a problem hiding this comment.
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(
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated 6 comments.
Suppressed comments (1)
migrations/tenant/schema/00017_migrate_labels_and_tags_data.sql:5
- Creating the pgcrypto extension inside a tenant schema migration can fail in managed Postgres setups where the migration role lacks CREATE EXTENSION privileges. It’s safer to assert the extension exists (and fail with a clear error) rather than attempting to install it.
Comment on lines
+80
to
+84
| func (m *TagManager) GetTags(ctx context.Context, itemID uuid.UUID) ([]string, error) { | ||
| values := []string{} | ||
| tag := &model.Tag{ID: itemID} | ||
| _, err := m.r.First(ctx, tag, *repo.NewQuery()) | ||
|
|
||
| if errors.Is(err, repo.ErrNotFound) { | ||
| return values, nil | ||
| // Get all labels with key="system.tag" | ||
| systemTagFilter := &KeyFilter{Key: model.SystemTagKey, Exclude: false} | ||
| resourceLabels, _, err := m.resourceLabels.List( | ||
| ctx, |
Comment on lines
104
to
108
| func (m *TagManager) DeleteTags(ctx context.Context, itemID uuid.UUID) error { | ||
| // Delete all labels with key="system.tag" | ||
| systemTagFilter := &KeyFilter{Key: model.SystemTagKey, Exclude: false} | ||
| return m.resourceLabels.DeleteAll(ctx, model.ResourceTypeKeyConfig, itemID, systemTagFilter) | ||
| } |
Comment on lines
+113
to
+117
| // Check if label exists by business key (resource_type, resource_id, key) | ||
| existing, found, err := m.findByBusinessKey(ctx, label.ResourceType, label.ResourceID, label.Key) | ||
| if err != nil { | ||
| return err | ||
| } |
| ResourceType ResourceType `gorm:"type:varchar(50);not null"` | ||
| ResourceID uuid.UUID `gorm:"type:uuid;not null"` | ||
| Key string `gorm:"type:varchar(255);not null"` | ||
| Value string `gorm:"type:varchar(255);not null"` |
Comment on lines
+209
to
+213
| ln, err := net.Listen("tcp", "127.0.0.1:0") | ||
| require.NoError(t, err) | ||
| statusAddr := ln.Addr().String() | ||
| require.NoError(t, ln.Close()) | ||
|
|
Comment on lines
3
to
7
| import ( | ||
| "context" | ||
| "errors" | ||
| "net" | ||
| "os" |
davidbolet
added a commit
that referenced
this pull request
Aug 31, 2026
Fix two issues identified by GitHub Copilot: 1. GetTags pagination limit: Change from default limit (100) to high limit (10000) to avoid silently truncating tags when a key configuration has more than 100 tags. Realistically, no key config will have 10k tags, so this effectively retrieves all tags without hitting the limit. 2. Error wrapping: Wrap errors with ErrGetTags sentinel for consistency with other methods in the package and proper error handling. Addresses Copilot feedback on PR #344 Signed-off-by: David <davidbolet@gmail.com>
davidbolet
force-pushed
the
task/resource-labels-pr
branch
from
August 31, 2026 11:11
81317fc to
d7ff946
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
internal/manager/tag.go:110
DeleteTagsreturns rawResourceLabelManager.DeleteAllerrors, so callers won’t get the tag-specificErrDeletingTagswrapper (unlikeSetTags, which wraps delete errors). This makes error handling inconsistent and can leak lower-level DB error types to API layers.
// DeleteTags removes all tags for a key configuration
func (m *TagManager) DeleteTags(ctx context.Context, itemID uuid.UUID) error {
// Delete all labels with key="system.tag"
systemTagFilter := &KeyFilter{Key: model.SystemTagKey, Exclude: false}
return m.resourceLabels.DeleteAll(ctx, model.ResourceTypeKeyConfig, itemID, systemTagFilter)
}
internal/model/resource_label.go:36
ResourceLabel.Valueis declared asvarchar(255)in the GORM model, but the migration createsresource_labels.valueastext. This schema mismatch can cause drift if AutoMigrate is ever used and is confusing for future maintenance.
internal/manager/resource_label_test.go:232- The subtest name says it verifies transaction rollback, but the body never triggers an error or asserts that prior writes were rolled back. This makes the test misleading and reduces confidence in transactional behavior.
t.Run("Should rollback on error in transaction", func(t *testing.T) {
newResourceID := uuid.New()
// First create a valid label
validLabel := []*model.ResourceLabel{
{
ResourceType: model.ResourceTypeKey,
ResourceID: newResourceID,
Key: "test",
Value: "value",
},
}
err := m.CreateOrUpdateBatch(ctx, validLabel)
require.NoError(t, err)
// Verify it exists
excludeSystemTag := &manager.KeyFilter{Key: model.SystemTagKey, Exclude: true}
labels, _, err := m.List(ctx, model.ResourceTypeKey, newResourceID, excludeSystemTag, repo.Pagination{})
require.NoError(t, err)
assert.Len(t, labels, 1)
})
internal/manager/base.go:94
labelManageris created and used forkeyManager, butManager.Labelsis initialized with a differentLabelManagerinstance. Even if currently stateless, this duplication is easy to miss and can cause subtle inconsistencies if state/config is ever added.
return &Manager{
Keys: keyManager,
KeyVersions: NewKeyVersionManager(repo, svcRegistry, tenantConfigManager, certManager, cmkAuditor),
TenantConfigs: tenantConfigManager,
System: systemManager,
KeyConfig: keyConfigManager,
Tags: tagManager,
Labels: NewLabelManager(repo, resourceLabelManager),
Workflow: NewWorkflowManager(
| ErrDeleteLabelDB = errors.New("failed to delete label") | ||
| ErrGetKeyIDDB = errors.New("KeyID is required") | ||
| ErrEmptyInputLabelDB = errors.New("invalid input empty label name") | ||
| ErrQueryLabelList = errors.New("failed to query system list") |
davidbolet
added a commit
that referenced
this pull request
Sep 1, 2026
Fix two issues identified by GitHub Copilot: 1. GetTags pagination limit: Change from default limit (100) to high limit (10000) to avoid silently truncating tags when a key configuration has more than 100 tags. Realistically, no key config will have 10k tags, so this effectively retrieves all tags without hitting the limit. 2. Error wrapping: Wrap errors with ErrGetTags sentinel for consistency with other methods in the package and proper error handling. Addresses Copilot feedback on PR #344 Signed-off-by: David <davidbolet@gmail.com>
davidbolet
force-pushed
the
task/resource-labels-pr
branch
from
September 1, 2026 11:05
d7ff946 to
326a7b2
Compare
Signed-off-by: David <davidbolet@gmail.com>
davidbolet
force-pushed
the
task/resource-labels-pr
branch
from
September 2, 2026 08:06
326a7b2 to
6351585
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new resource_labels schema constraints and label sync logic can prevent multi-value tags and can leave resource_labels out-of-sync due to ignored unique-constraint failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+12
to
+20
| -- Unique constraint for labels (one value per key) | ||
| CONSTRAINT uq_resource_labels_type_id_key UNIQUE (resource_type, resource_id, key), | ||
|
|
||
| -- Unique constraint for tags (multiple values allowed, but each value unique) | ||
| CONSTRAINT uq_resource_labels_type_id_key_value UNIQUE (resource_type, resource_id, key, value) | ||
| ); | ||
|
|
||
| CREATE INDEX idx_resource_labels_resource ON resource_labels(resource_type, resource_id); | ||
| CREATE INDEX idx_resource_labels_key ON resource_labels(key); |
Comment on lines
+159
to
+170
| // syncCreateResourceLabel writes a new label to the resource_labels table | ||
| func (m *LabelManager) syncCreateResourceLabel(ctx context.Context, keyID uuid.UUID, label *model.KeyLabel) error { | ||
| resourceLabel := &model.ResourceLabel{ | ||
| ID: uuid.New(), | ||
| ResourceType: model.ResourceTypeKeyConfig, | ||
| ResourceID: keyID, | ||
| Key: label.Key, | ||
| Value: label.Value, | ||
| } | ||
| _ = m.repository.Create(ctx, resourceLabel) | ||
| return nil | ||
| } |
Comment on lines
+191
to
+193
| rl.Value = label.Value | ||
| _, _ = m.repository.Patch(ctx, rl, *repo.NewQuery().UpdateAll(true)) | ||
| return nil |
Signed-off-by: David <davidbolet@gmail.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current label double-write stores key labels under the key_configuration resource type (and the schema only allows that type), which breaks the intended unified model semantics and future reads/migration correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
migrations/tenant/schema/00019_create_resource_labels_table.sql:10
created_at/updated_atare defined asTIMESTAMP(no timezone), but existing tenant tables usetimestamptz. UsingTIMESTAMPhere can introduce timezone ambiguity and inconsistent behavior across environments; align this table with the rest of the schema by usingtimestamptz.
internal/manager/key_label.go:131
syncCreateResourceLabelcan fail with a unique-constraint error if a staleresource_labelsrow already exists for the same (resource_type, resource_id, key) (e.g. if a prior best-effort delete failed). In that case the new label value won’t be synced. Use the insert-or-update path here so the new table converges back to the primary labels table.
// Double-write: sync to resource_labels (best-effort)
_ = m.syncCreateResourceLabel(ctx, keyID, label)
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Lite
Comment on lines
+85
to
+88
| ck := repo.NewCompositeKey(). | ||
| Where(repo.ResourceTypeField, model.ResourceTypeKeyConfig). | ||
| Where(repo.ResourceIDField, keyID). | ||
| Where(repo.KeyField, labelName) |
Comment on lines
+102
to
+115
| // Insert new tags | ||
| for _, value := range values { | ||
| if value == "" { | ||
| continue | ||
| } | ||
| label := &model.ResourceLabel{ | ||
| ID: uuid.New(), | ||
| ResourceType: model.ResourceTypeKeyConfig, | ||
| ResourceID: itemID, | ||
| Key: model.SystemTagKey, | ||
| Value: value, | ||
| } | ||
| _ = m.r.Create(ctx, label) | ||
| } |
Comment on lines
+31
to
+32
| CreatedAt time.Time `gorm:"not null;default:now()"` | ||
| UpdatedAt time.Time `gorm:"not null;default:now()"` |
Comment on lines
+77
to
+78
| // Double-write: delete from resource_labels (best-effort) | ||
| _ = m.syncDeleteResourceLabel(ctx, keyID, labelName) |
Signed-off-by: David <davidbolet@gmail.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new double-write paths can break transactions (constraint violations + default delete limit) and the migration uses timestamp semantics inconsistent with existing timestamptz usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
migrations/tenant/schema/00019_create_resource_labels_table.sql:10
- The tenant schema migrations consistently use
timestamptzfor timestamps, but this table usesTIMESTAMP(no time zone). That can lead to inconsistent semantics and lost timezone information when reading/writingtime.Time. Aligncreated_at/updated_atwith the existingtimestamptzconvention.
internal/manager/tag.go:130
repo.Createcan return a unique-constraint error (e.g., duplicate tag values), and in Postgres that aborts the surrounding transaction. Because this is intentionally best-effort and errors are ignored, it should avoid generating constraint violations by using an upsert (Set+OnConflict) rather thanCreate.
label := &model.ResourceLabel{
ID: uuid.New(),
ResourceType: model.ResourceTypeKeyConfig,
ResourceID: itemID,
Key: model.SystemTagKey,
Value: value,
}
_ = m.r.Create(ctx, label)
}
internal/manager/tag.go:115
- This delete relies on
repo.Delete, which applies the default pagination limit (100) when no limit is set; that can leave old tag rows behind if there are more than 100. Set an explicit high limit for full cleanup deletes.
// Delete existing tags for this resource
_, _ = m.r.Delete(ctx, &model.ResourceLabel{
ResourceType: model.ResourceTypeKeyConfig,
ResourceID: itemID,
Key: model.SystemTagKey,
}, *repo.NewQuery().Where(
repo.NewCompositeKeyGroup(
repo.NewCompositeKey().
Where(repo.ResourceTypeField, model.ResourceTypeKeyConfig).
Where(repo.ResourceIDField, itemID).
Where(repo.KeyField, model.SystemTagKey),
),
))
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Lite
Comment on lines
+179
to
+188
| func (m *LabelManager) syncCreateResourceLabel(ctx context.Context, keyID uuid.UUID, label *model.KeyLabel) { | ||
| resourceLabel := &model.ResourceLabel{ | ||
| ID: uuid.New(), | ||
| ResourceType: model.ResourceTypeKeyConfig, | ||
| ResourceID: keyID, | ||
| Key: label.Key, | ||
| Value: label.Value, | ||
| } | ||
| _ = m.repository.Create(ctx, resourceLabel) | ||
| } |
Comment on lines
49
to
53
| func (m *TagManager) SetTags(ctx context.Context, itemID uuid.UUID, values []string) error { | ||
| if len(values) == 1 && values[0] == "" { | ||
| return m.DeleteTags(ctx, itemID) | ||
| bytes, err := json.Marshal(values) | ||
| if err != nil { | ||
| return err | ||
| } |
| return nil | ||
| } | ||
|
|
||
| func (m *TagManager) GetTags(ctx context.Context, itemID uuid.UUID) ([]string, error) { |
Comment on lines
+90
to
+97
| _, _ = m.r.Delete(ctx, &model.ResourceLabel{}, *repo.NewQuery().Where( | ||
| repo.NewCompositeKeyGroup( | ||
| repo.NewCompositeKey(). | ||
| Where(repo.ResourceTypeField, model.ResourceTypeKeyConfig). | ||
| Where(repo.ResourceIDField, itemID). | ||
| Where(repo.KeyField, model.SystemTagKey), | ||
| ), | ||
| )) |
Comment on lines
113
to
+135
| @@ -121,6 +130,9 @@ func (m *LabelManager) CreateOrUpdateLabel( | |||
| if err != nil { | |||
| return errs.Wrap(ErrUpdateLabelDB, err) | |||
| } | |||
|
|
|||
| // Double-write: sync to resource_labels (best-effort) | |||
| m.syncUpdateResourceLabel(ctx, keyID, label) | |||
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Key label double-write currently uses an incorrect resource_type namespace (and the migration constrains/structures resource_type), which risks writing inconsistent/ambiguous data into resource_labels.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
internal/manager/key_label.go:173
- These sync queries write
resource_type = key_configurationwhilekeyIDis amodel.KeyID (the existingkey_labels.resource_idFK points tokeys(id)). This will mix key labels into the key-configuration namespace inresource_labels, making future reads ambiguous and potentially preventing proper separation once more resource types are added.
migrations/tenant/schema/00019_create_resource_labels_table.sql:11 - The new table’s schema diverges from existing tenant tables: most use
timestamptzforcreated_at/updated_at(e.g.key_labelsin00001_init_tenant.sql), and label double-write needsresource_typeto support more than justkey_configuration(labels are keyed bymodel.KeyIDs).
This issue also appears on line 14 of the same file.
migrations/tenant/schema/00019_create_resource_labels_table.sql:18
- If the column is defined as quoted identifier
"key"(as in other migrations), the indexes and partial-index predicates should also reference"key"; otherwise this migration can fail on dialects wherekeyis reserved, and it’s inconsistent withkey_labelsusing a quoted key column.
CREATE UNIQUE INDEX uq_resource_labels_type_id_key
ON resource_labels(resource_type, resource_id, key)
WHERE key != 'system.tag';
-- Partial unique index for tags: each (resource_type, resource_id, key, value) must be unique when key = 'system.tag'
internal/manager/key_label.go:120
- LabelManager now double-writes into
resource_labels, but existing label manager tests only verify thekey_labelstable. Add tests similar toTestTagManagerDoubleWrite*to assert create/update/delete are reflected inresource_labelsas well (including reserved-key behavior if applicable).
// Double-write: sync to resource_labels (best-effort)
m.syncCreateResourceLabel(ctx, keyID, label)
internal/manager/tag.go:69
GetTagscurrently swallows non-ErrNotFounderrors fromm.r.Firstdue toerrors.Is(err, err)always being true, which can mask DB/authz failures during reads after these writes.
// Double-write: sync to resource_labels (best-effort)
m.syncToResourceLabels(ctx, itemID, values)
return nil
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new repo resource type is not fully wired into authz/resource-action registration (and there are a few schema/data-consistency issues) that can break or desync the migration path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
internal/manager/tag.go:126
syncToResourceLabelsinserts one row per input value, but it doesn't de-duplicate values. Becauseresource_labelsenforces uniqueness on(resource_type, resource_id, key, value)for tags, duplicate inputs will cause unique-constraint errors that are currently ignored, resulting in silent divergence betweentags(which stores duplicates) andresource_labels(which won’t).
// Insert new tags
for _, value := range values {
if value == "" {
continue
}
internal/manager/key_label.go:121
- LabelManager now performs a best-effort double-write into
resource_labels(viasyncCreateResourceLabel/syncUpdateResourceLabel), but existinginternal/manager/key_label_test.gotests only assert behavior inkey_labels. Adding assertions thatresource_labelsis created/updated alongside label operations would prevent silent regression during the migration.
// Primary write: create in labels table
err := m.repository.Create(ctx, label)
if err != nil {
return errs.Wrap(ErrInsertLabel, err)
}
// Double-write: sync to resource_labels (best-effort)
m.syncCreateResourceLabel(ctx, keyID, label)
} else {
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Lite
Signed-off-by: David <davidbolet@gmail.com>
Signed-off-by: David <davidbolet@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
resource_labelsmodel/table for labels and key-configuration tags.Test plan
go test ./...(integration tests may require local container/runtime setup)