refactor: remove isPrimary from key and change primary on keyconfig patch#208
Open
jmpTeixeira02 wants to merge 3 commits intomainfrom
Open
refactor: remove isPrimary from key and change primary on keyconfig patch#208jmpTeixeira02 wants to merge 3 commits intomainfrom
jmpTeixeira02 wants to merge 3 commits intomainfrom
Conversation
jmpTeixeira02
requested review from
MarkMcAleese,
bkieling,
doanhluongsap and
minh-nghia
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
from
4bf66d0 to
fe23c38
Compare
|
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: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPrimary-key boolean was removed from Key records; primary identity moved to KeyConfiguration.PrimaryKeyID. KeyConfiguration updates now support transactional primary changes that validate key enabled state, rewrite related retry/system events, and emit system-switch events via an injected EventFactory. API, repo, manager, controllers, migrations, and tests updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as KeyConfigController
participant Workflow as WorkflowService
participant Manager as KeyConfigManager
participant Repo as Repository
participant EventFactory as EventFactory
Client->>Controller: PATCH /keyConfigurations/{id} { primaryKeyID }
Controller->>Workflow: IsWorkflowRequired(ctx)
alt workflow required
Workflow-->>Controller: required==true
Controller-->>Client: 403 ErrActionRequireWorkflow
else workflow not required
Workflow-->>Controller: required==false
Controller->>Manager: UpdateKeyConfigurationByID(id, patch)
Manager->>Repo: Begin Transaction
Manager->>Repo: First(key) to validate enabled state
alt key disabled
Repo-->>Manager: key.disabled
Manager-->>Controller: ErrKeyIsNotEnabled
Controller-->>Client: 4xx error
else key enabled
Manager->>Repo: Query retry/system events for old primary
Repo-->>Manager: events
Manager->>EventFactory: Update event payloads (replace KeyIDTo)
EventFactory-->>Manager: events updated
Manager->>EventFactory: Emit SystemSwitchNewPrimaryKey events
EventFactory-->>Manager: events created
Manager->>Repo: Patch KeyConfiguration.PrimaryKeyID
Repo-->>Manager: patched config
Manager->>Repo: Commit
Manager-->>Controller: updated KeyConfiguration
Controller-->>Client: 200 OK
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
from
fe23c38 to
aeeaad9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/manager/key.go (1)
264-302:⚠️ Potential issue | 🟠 MajorClear or replace
PrimaryKeyIDwhen deleting the primary key.After this delete succeeds, the owning
KeyConfigurationstill points at the removed key because nothing patchesPrimaryKeyID. Sincerepo.IsPrimaryKeynow resolves through that field, later primary checks and editable-region logic will read dangling state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key.go` around lines 264 - 302, The deletion path currently removes the provider key and DB records (km.deleteProviderKey, km.repo.Delete of model.KeyVersion and model.Key) but does not clear or replace the KeyConfiguration.PrimaryKeyID, leaving a dangling reference; inside the same transaction (km.repo.Transaction) after deleting the key/version rows, load the owning KeyConfiguration (using key.KeyConfigurationID), check if its PrimaryKeyID equals keyID, and if so either set PrimaryKeyID to null/empty (or pick another existing key ID if business logic requires) and persist the update via km.repo.Update (or equivalent) so repo.IsPrimaryKey and subsequent checks no longer reference the deleted key. Ensure this update is performed inside the same transaction and wrap any DB errors with ErrDeleteKeyDB as done for the other deletes.
🧹 Nitpick comments (1)
test/db-migration/migration_test.go (1)
340-350: Assert the v5 schema change itself.These cases only verify the goose version. A broken
tenant/00005_delete_key_primary.sqlcan still pass without proving thatkeys.is_primarydisappears on upgrade and comes back on rollback. Add anassertMigrationfor column presence/absence here; while you’re there, rename the second case to “down” so the subtest names stay distinct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/db-migration/migration_test.go` around lines 340 - 350, Rename the second test case's name field from "Should up tenant/00005_delete_key_primary.sql" to "Should down tenant/00005_delete_key_primary.sql" so subtest names are unique, and add assertMigration checks around the v5 migration cases: after running the upgrade case (version: 5, downgrade: false, target: db.TenantTarget) call assertMigration to confirm the keys.is_primary column is absent, and after running the rollback case (downgrade: true) call assertMigration to confirm keys.is_primary is present again; locate these hooks near the existing test table entries and use the assertMigration helper and the migration identifier tenant/00005_delete_key_primary.sql to validate column presence/absence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controllers/cmk/keyconfiguration_controller_test.go`:
- Around line 593-600: The test case named "Should 403 when update primary key
and workflow is required" has invalid JSON and lacks an authenticated context so
it can return 403 for the wrong reason; fix inputJSON by removing the trailing
comma (e.g. `"primaryKeyID": "id"`) and supply an authenticated request context
(the same auth context used by other tests, e.g. set authContext or include the
auth field on the test case) so the request is authenticated but still fails due
to workflow requirement; update the test case fields (name, keyConfigID,
inputJSON, expectedStatus) accordingly in keyconfiguration_controller_test.go so
the test validates the workflow-forbidden rule.
In `@internal/controllers/cmk/keyconfiguration_controller.go`:
- Around line 127-136: The current check gates on the mere presence of
request.Body.PrimaryKeyID and wrongly requires a workflow even if the value is
unchanged; update the logic in the handler around request.Body.PrimaryKeyID to
first load the current key configuration (the existing primary key id on the
resource), compare the requested PrimaryKeyID with the current primary key
(handling nil vs non-nil and string/ID equality), and only call
c.Manager.Workflow.IsWorkflowRequired(ctx) and return
apierrors.ErrActionRequireWorkflow when the primary key is actually changing.
Use the same request.Body.PrimaryKeyID symbol and the controller's existing
access to the current key configuration (via the controller/manager methods that
retrieve the resource) to perform the comparison before invoking
IsWorkflowRequired.
In `@internal/manager/key.go`:
- Around line 697-705: The current guard only rejects DISABLED and may
auto-promote keys in other unusable states; change the check around the
promotion (the block using key, key.State, exist, and setting
KeyConfiguration.PrimaryKeyID then calling km.repo.Patch) to require key.State
equals string(cmkapi.KeyStateENABLED) and return ErrKeyIsNotEnabled for any
other state, so only keys with state ENABLED are made primary; update the
conditional and error path accordingly where KeyConfiguration{PrimaryKeyID:
&key.ID} is assembled before km.repo.Patch.
In `@internal/manager/keyconfiguration_test.go`:
- Around line 935-937: The test creates an independent context with
testutils.CreateCtxWithTenant("orbital") which breaks the parent context chain;
replace that call with cmkcontext.CreateTenantContext(ctx, "orbital") so the
Orbital tenant context is derived from the existing parent ctx (preserving
cancellation and context values). Update the variable usage where orbitalCtx is
assigned to call cmkcontext.CreateTenantContext(ctx, "orbital") and ensure the
rest of the test (e.g., the r.First call using jobFromDB) continues to use
orbitalCtx.
- Line 875: The test currently compares only the 32-bit DCE identifier by
calling key.ID.ID() and keyConfig.PrimaryKeyID.ID(); replace that assertion to
compare the full UUID values instead: directly compare key.ID with
keyConfig.PrimaryKeyID (dereference keyConfig.PrimaryKeyID if it is a pointer)
so the test verifies the complete UUID, not the truncated DCE ID.
In `@internal/manager/keyconfiguration.go`:
- Around line 456-463: The lookup currently only verifies the key exists but
doesn't ensure it belongs to the same configuration; after retrieving model.Key
(primaryKeyID) via m.r.First, add a check that key.KeyConfigurationID equals the
current keyConfig.ID (or return a new/appropriate error like
ErrKeyNotOwnedByConfiguration) before proceeding to the state check; keep the
existing disabled-state check (key.State == string(cmkapi.KeyStateDISABLED)) but
ensure ownership is validated first to prevent cross-configuration primary key
assignment.
- Around line 507-534: The current updatePrimaryKeySystemEvents function
rewrites any event with data->>'keyIDTo'==oldPkey; restrict it to only
SYSTEM_SWITCH events with retryable statuses by adding type and status
predicates to the repo query (e.g., include
repo.Field(repo.TypeField).Equals(model.EventTypeSystemSwitch) and
repo.Field(repo.StatusField).In(<retryable status constants>)) and/or add a
guard inside the batch callback to skip events whose e.Type is not
model.EventTypeSystemSwitch or whose e.Status is not one of the intended
retryable statuses before unmarshalling/modifying systemJobData and calling
m.r.Patch.
- Around line 464-492: The batch loop builds a SystemSwitch event using
keyConfig.PrimaryKeyID.String(), which will panic when PrimaryKeyID is nil;
compute an oldPrimary string safely (e.g., using the same
ptr.GetSafeDeref(keyConfig.PrimaryKeyID).String() used earlier) before the
repo.ProcessInBatch call or at the top of the closure and pass that safe
oldPrimary into m.eventFactory.SystemSwitchNewPrimaryKey instead of
dereferencing keyConfig.PrimaryKeyID; updatePrimaryKeySystemEvents and the
closure that calls m.eventFactory.SystemSwitchNewPrimaryKey should use this safe
value to avoid nil deref.
In `@internal/manager/workflow_test.go`:
- Around line 283-286: The fixture key isn't linked to its configuration: after
creating keyConfig with testutils.NewKeyConfig and before persisting the key
created with testutils.NewKey, set the key's KeyConfigurationID
(key.KeyConfigurationID = keyConfig.ID) so the key truly belongs to that
configuration, and remove the duplicated PrimaryKeyID assignment (leave only
kc.PrimaryKeyID = ptr.PointTo(key.ID) on the keyConfig setup); apply the same
change to the other occurrence referenced (lines ~313-317) to ensure the tests
assert a key that actually belongs to the configuration.
In `@internal/repo/repository.go`:
- Around line 365-384: The IsPrimaryKey helper relies on Repo.First inspecting
keyConfig.ID via *NewQuery(), which the Repo contract doesn't guarantee; instead
construct an explicit query predicate setting IDField = key.KeyConfigurationID
(e.g., q := NewQuery(); q.IDField = key.KeyConfigurationID or
q.WhereID(key.KeyConfigurationID) depending on query API) and pass that query to
r.First so the lookup is scoped to the correct KeyConfiguration ID; update the
call in IsPrimaryKey to use this explicit query before invoking r.First with
keyConfig.
In `@migrations/tenant/schema/00005_delete_key_primary.sql`:
- Line 5: The Down section in 00005_delete_key_primary.sql is empty; open that
file, inspect the Up statements (what primary key, constraint or index was
dropped and on which table), and add the inverse SQL under the line "-- +goose
Down" to make the migration reversible (e.g., ALTER TABLE <table> ADD CONSTRAINT
<constraint_name> PRIMARY KEY (<cols>); or recreate any dropped indexes/columns
with their original definitions). Ensure the SQL names match the ones removed in
Up (use the original constraint/index names) and test the Down path to confirm
the database returns to the pre-migration state.
---
Outside diff comments:
In `@internal/manager/key.go`:
- Around line 264-302: The deletion path currently removes the provider key and
DB records (km.deleteProviderKey, km.repo.Delete of model.KeyVersion and
model.Key) but does not clear or replace the KeyConfiguration.PrimaryKeyID,
leaving a dangling reference; inside the same transaction (km.repo.Transaction)
after deleting the key/version rows, load the owning KeyConfiguration (using
key.KeyConfigurationID), check if its PrimaryKeyID equals keyID, and if so
either set PrimaryKeyID to null/empty (or pick another existing key ID if
business logic requires) and persist the update via km.repo.Update (or
equivalent) so repo.IsPrimaryKey and subsequent checks no longer reference the
deleted key. Ensure this update is performed inside the same transaction and
wrap any DB errors with ErrDeleteKeyDB as done for the other deletes.
---
Nitpick comments:
In `@test/db-migration/migration_test.go`:
- Around line 340-350: Rename the second test case's name field from "Should up
tenant/00005_delete_key_primary.sql" to "Should down
tenant/00005_delete_key_primary.sql" so subtest names are unique, and add
assertMigration checks around the v5 migration cases: after running the upgrade
case (version: 5, downgrade: false, target: db.TenantTarget) call
assertMigration to confirm the keys.is_primary column is absent, and after
running the rollback case (downgrade: true) call assertMigration to confirm
keys.is_primary is present again; locate these hooks near the existing test
table entries and use the assertMigration helper and the migration identifier
tenant/00005_delete_key_primary.sql to validate column presence/absence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 571cd0cb-a1a1-427b-9cb3-57a3271b1112
📒 Files selected for processing (34)
apis/cmk/cmk-ui.yamlcmd/task-worker/main.gocmd/tenant-manager-cli/cli_test.gocmd/tenant-manager-cli/commands/commands.gocmd/tenant-manager/main.gointernal/api/cmkapi/cmkapi.gointernal/api/transform/key/key.gointernal/api/transform/key/key_test.gointernal/apierrors/key.gointernal/controllers/cmk/key_controller.gointernal/controllers/cmk/key_controller_test.gointernal/controllers/cmk/keyconfiguration_controller.gointernal/controllers/cmk/keyconfiguration_controller_test.gointernal/manager/base.gointernal/manager/errors.gointernal/manager/key.gointernal/manager/key_test.gointernal/manager/keyconfiguration.gointernal/manager/keyconfiguration_test.gointernal/manager/keyversion_test.gointernal/manager/system_test.gointernal/manager/tenant_test.gointernal/manager/workflow_test.gointernal/model/key.gointernal/operator/operator_test.gointernal/repo/repository.gointernal/workflow/key_configuration_actions.gointernal/workflow/key_configuration_actions_test.gomigrations/tenant/schema/00005_delete_key_primary.sqltest/db-migration/migration_test.gotest/integration/async_test/common_test.gotest/integration/async_test/workflowassign_test.gotest/integration/tenant-manager/provisioning_test.gotest/security/xss/key_controller_test.go
💤 Files with no reviewable changes (6)
- test/security/xss/key_controller_test.go
- internal/apierrors/key.go
- internal/api/transform/key/key.go
- internal/api/transform/key/key_test.go
- internal/controllers/cmk/key_controller.go
- internal/manager/errors.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/manager/key.go (1)
259-302:⚠️ Potential issue | 🟠 MajorClear the configuration primary when deleting the current primary key.
After moving primary ownership to
KeyConfiguration.PrimaryKeyID, this path deletes the key but never updates the owning configuration. IfisPrimaryis true, the configuration is left pointing at a non-existent key ID, which can break later reads and workflow checks. Clear or reassign the primary in the same transaction before removing the key rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key.go` around lines 259 - 302, When deleting a key that IsPrimaryKey returned true for, you must in the same repo.Transaction clear or reassign the owning KeyConfiguration.PrimaryKeyID before removing the key rows; modify the transaction passed to km.repo.Transaction (the block that currently creates ck via repo.NewCompositeKey and deletes KeyVersion and Key) to first call the repo update for the KeyConfiguration owning this key (use key.KeyConfigurationID) to set PrimaryKeyID to nil (or a new value) and wrap that update in the same transaction so the configuration no longer points at the soon-to-be-deleted key; keep the existing deleteProviderKey call and existing delete steps but ensure the configuration update happens before the Delete calls.
♻️ Duplicate comments (5)
internal/manager/workflow_test.go (1)
283-292:⚠️ Potential issue | 🟡 MinorLink each fixture key back to its key configuration.
Both subtests set
PrimaryKeyID, but the newmodel.Keystill keeps the zeroKeyConfigurationID, so the fixture does not prove that the chosen key actually belongs to that configuration. Also,PrimaryKeyIDis assigned twice in each block.🧪 Suggested test cleanup
key := testutils.NewKey(func(k *model.Key) {}) keyConfig := testutils.NewKeyConfig(func(kc *model.KeyConfiguration) { - kc.PrimaryKeyID = &key.ID kc.PrimaryKeyID = ptr.PointTo(key.ID) }) +key.KeyConfigurationID = keyConfig.IDApply the same cleanup to the second subtest as well.
Also applies to: 313-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/workflow_test.go` around lines 283 - 292, The fixtures are assigning PrimaryKeyID on KeyConfiguration but not linking the created model.Key back via its KeyConfigurationID and they erroneously set PrimaryKeyID twice; update the test fixture builders (calls to testutils.NewKey and testutils.NewKeyConfig) so the created key's KeyConfigurationID is set to the keyConfig.ID (e.g., in NewKey’s init func set k.KeyConfigurationID = &keyConfig.ID) and remove the duplicate PrimaryKeyID assignment in NewKeyConfig (keep only one assignment to kc.PrimaryKeyID = ptr.PointTo(key.ID)); apply the same fix to both subtests so the key truly belongs to the key configuration.internal/manager/keyconfiguration_test.go (1)
867-875:⚠️ Potential issue | 🟡 MinorCompare full UUIDs instead of
UUID.ID().
uuid.UUID.ID()only returns the DCE local identifier, so this assertion can pass for different UUIDs that share the same first 32 bits. Comparekey.IDwith*keyConfig.PrimaryKeyIDdirectly.🧪 Suggested fix
- assert.Equal(t, key.ID.ID(), keyConfig.PrimaryKeyID.ID()) + assert.Equal(t, key.ID, *keyConfig.PrimaryKeyID)What does github.com/google/uuid UUID.ID() return, and does it represent the full UUID value or only the DCE Security local identifier?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/keyconfiguration_test.go` around lines 867 - 875, The test currently compares only the DCE local identifier via uuid.UUID.ID(), which can yield false positives; update the assertion after calling UpdateKeyConfigurationByID to compare the full UUID values directly—use key.ID and the returned pointer value in keyConfig.PrimaryKeyID (dereference as needed) instead of calling .ID() so the test verifies the entire UUID matches (refer to UpdateKeyConfigurationByID, keyConfig, key.ID, and PrimaryKeyID to locate the change).internal/manager/key.go (1)
696-705:⚠️ Potential issue | 🟠 MajorRequire
ENABLEDbefore auto-promoting the first key.This guard only rejects
DISABLED, so a first key inPENDING_IMPORT,UNKNOWN, orFORBIDDENcan still become the configuration primary. Check forENABLEDexplicitly instead.🛠️ Suggested fix
- if key.State == string(cmkapi.KeyStateDISABLED) { + if key.State != string(cmkapi.KeyStateENABLED) { return ErrKeyIsNotEnabled }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key.go` around lines 696 - 705, The code currently only rejects DISABLED states before auto-promoting the first key, allowing PENDING_IMPORT/UNKNOWN/FORBIDDEN to be promoted; update the guard in the block that sets KeyConfiguration.PrimaryKeyID (the branch using exist, keyConfig, km.repo.Patch) to require explicit ENABLED state by checking key.State == string(cmkapi.KeyStateENABLED) and returning ErrKeyIsNotEnabled otherwise, referencing the symbols key.State, cmkapi.KeyStateENABLED, and ErrKeyIsNotEnabled.internal/controllers/cmk/keyconfiguration_controller_test.go (1)
593-600:⚠️ Potential issue | 🟠 MajorMake the new 403 case reach the workflow gate.
This payload is malformed and uses an invalid UUID, so the request can fail before the controller evaluates
ErrActionRequireWorkflow. The case also omits auth context, which makes the 403 source ambiguous.🧪 Suggested test fix
{ name: "Should 403 when update primary key and workflow is required", keyConfigID: keyConfig.ID.String(), inputJSON: `{ - "primaryKeyID": "id", - }`, + "primaryKeyID": "12345678-90ab-cdef-1234-567890abcdef" + }`, expectedStatus: http.StatusForbidden, + expectedBody: "error", + additionalContext: authClient.GetClientMap(), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/cmk/keyconfiguration_controller_test.go` around lines 593 - 600, The test case "Should 403 when update primary key and workflow is required" sends a malformed payload and lacks auth, so it never reaches the workflow gate; update the test to send a valid UUID for "primaryKeyID" (e.g. use an existing primary key ID from the fixture like keyConfig.PrimaryKey.ID.String() or another valid UUID) and include the same auth context used by other 403 workflow tests so the request is authenticated; this ensures the controller hits the ErrActionRequireWorkflow branch (refer to the test name and ErrActionRequireWorkflow handling in keyconfiguration_controller_test.go and the keyConfig fixture).internal/controllers/cmk/keyconfiguration_controller.go (1)
127-136:⚠️ Potential issue | 🟠 MajorGate this on an actual primary-key transition.
Any PATCH that merely includes
primaryKeyIDnow gets blocked when workflows are required, even if the client is just re-sending the existing value while updating another field. Load the current key configuration first and only require a workflow when the requestedPrimaryKeyIDdiffers from the stored one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/cmk/keyconfiguration_controller.go` around lines 127 - 136, The current check gates any PATCH that includes request.Body.PrimaryKeyID regardless of whether it actually changes the stored value; modify the logic to first load the existing key configuration (retrieve the stored PrimaryKeyID for this resource) and only call c.Manager.Workflow.IsWorkflowRequired(ctx) and return apierrors.ErrActionRequireWorkflow when request.Body.PrimaryKeyID is non-nil AND differs from the stored PrimaryKeyID; update the block around request.Body.PrimaryKeyID to fetch the current configuration, compare the two IDs, and only enforce the workflow requirement on a true transition.
🧹 Nitpick comments (3)
internal/manager/system_test.go (1)
85-88: Prefer wiring a realeventFactoryintoNewKeyConfigManagerin shared setup.Passing
nilhere makes the shared fixture diverge from production wiring and can hide nil-dependent paths in future test additions.♻️ Suggested refactor
- keyConfigManager := manager.NewKeyConfigManager(dbRepository, certManager, userManager, tagManager, nil, nil, &cfg) - eventFactory, err := eventprocessor.NewEventFactory(t.Context(), &cfg, dbRepository) require.NoError(t, err) + keyConfigManager := manager.NewKeyConfigManager( + dbRepository, + certManager, + userManager, + tagManager, + nil, + eventFactory, + &cfg, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/system_test.go` around lines 85 - 88, The test creates keyConfigManager with a nil eventFactory which diverges from production wiring; create the real eventFactory via eventprocessor.NewEventFactory(t.Context(), &cfg, dbRepository) before calling manager.NewKeyConfigManager, check require.NoError on its creation, and pass that eventFactory instance (instead of nil) into NewKeyConfigManager so the shared fixture uses the real event factory path.internal/workflow/key_configuration_actions_test.go (1)
52-67: Use a different target key in the update-primary case.
keyConfig.PrimaryKeyIDandwf.Parametersboth resolve to the samekeyID, so this path only confirms a no-op and can still pass without proving the lifecycle switched the configuration primary. Create a current primary key and a second candidate key, then assert the storedKeyConfiguration.PrimaryKeyIDchanged afterApplyTransition.Also applies to: 76-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/workflow/key_configuration_actions_test.go` around lines 52 - 67, The test uses the same key for KeyConfiguration.PrimaryKeyID and wf.Parameters so the update-primary transition is a no-op; modify the test to create two distinct keys (e.g., currentPrimary and newPrimary), set kc.PrimaryKeyID to currentPrimary.ID and wf.Parameters to newPrimary.ID (instead of k.ID), run ApplyTransition, then assert that the persisted model.KeyConfiguration.PrimaryKeyID changed from currentPrimary.ID to newPrimary.ID; update both the test case at the shown block and the similar cases around lines 76-110 to use distinct keys and assert the PrimaryKeyID was updated after ApplyTransition.internal/manager/key_test.go (1)
434-454: This now tests “no primary configured,” not “non-primary key.”
kc.PrimaryKeyIDis left nil here, so this case can still pass even ifEditableRegionsbreaks specifically when another key is primary. SeedPrimaryKeyIDwith a different key ID so the test exercises the real non-primary branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key_test.go` around lines 434 - 454, The test is asserting the "non-primary key" path but leaves kc.PrimaryKeyID nil so it actually tests "no primary configured"; set kc.PrimaryKeyID to a different key's ID before creating entities so the code exercises the non-primary branch. Locate the KeyConfiguration instance created by testutils.NewKeyConfig (kc) and assign kc.PrimaryKeyID to a UUID (or the ID of a separate test key) that is different from the key created by testutils.NewKey, ensuring the PrimaryKeyID is non-nil and not equal to key.ID before calling testutils.CreateTestEntities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/manager/key_test.go`:
- Around line 929-939: The system fixture created with testutils.NewSystem(...)
lacks a Status, so change the initializer for sys to mark it as connected (e.g.,
in the anonymous func passed to testutils.NewSystem, set s.Status =
model.SystemStatusConnected or the string "connected" used by your model) so the
test actually exercises the “primary-with-connected-systems” guard; keep the
existing assignments to KeyConfigurationID but add the Status assignment in the
same initializer.
In `@test/db-migration/migration_test.go`:
- Around line 346-351: The test case entry with fields (name, downgrade, target,
version) has a copy-paste error: the name says "Should up
tenant/00005_delete_key_primary.sql" while downgrade is set to true; update the
name string in that test case to "Should down
tenant/00005_delete_key_primary.sql" so it accurately reflects the
downgrade=true setting and aligns with other downgrade cases (locate the struct
literal where name: "Should up tenant/00005_delete_key_primary.sql" is declared
and change only the name value).
---
Outside diff comments:
In `@internal/manager/key.go`:
- Around line 259-302: When deleting a key that IsPrimaryKey returned true for,
you must in the same repo.Transaction clear or reassign the owning
KeyConfiguration.PrimaryKeyID before removing the key rows; modify the
transaction passed to km.repo.Transaction (the block that currently creates ck
via repo.NewCompositeKey and deletes KeyVersion and Key) to first call the repo
update for the KeyConfiguration owning this key (use key.KeyConfigurationID) to
set PrimaryKeyID to nil (or a new value) and wrap that update in the same
transaction so the configuration no longer points at the soon-to-be-deleted key;
keep the existing deleteProviderKey call and existing delete steps but ensure
the configuration update happens before the Delete calls.
---
Duplicate comments:
In `@internal/controllers/cmk/keyconfiguration_controller_test.go`:
- Around line 593-600: The test case "Should 403 when update primary key and
workflow is required" sends a malformed payload and lacks auth, so it never
reaches the workflow gate; update the test to send a valid UUID for
"primaryKeyID" (e.g. use an existing primary key ID from the fixture like
keyConfig.PrimaryKey.ID.String() or another valid UUID) and include the same
auth context used by other 403 workflow tests so the request is authenticated;
this ensures the controller hits the ErrActionRequireWorkflow branch (refer to
the test name and ErrActionRequireWorkflow handling in
keyconfiguration_controller_test.go and the keyConfig fixture).
In `@internal/controllers/cmk/keyconfiguration_controller.go`:
- Around line 127-136: The current check gates any PATCH that includes
request.Body.PrimaryKeyID regardless of whether it actually changes the stored
value; modify the logic to first load the existing key configuration (retrieve
the stored PrimaryKeyID for this resource) and only call
c.Manager.Workflow.IsWorkflowRequired(ctx) and return
apierrors.ErrActionRequireWorkflow when request.Body.PrimaryKeyID is non-nil AND
differs from the stored PrimaryKeyID; update the block around
request.Body.PrimaryKeyID to fetch the current configuration, compare the two
IDs, and only enforce the workflow requirement on a true transition.
In `@internal/manager/key.go`:
- Around line 696-705: The code currently only rejects DISABLED states before
auto-promoting the first key, allowing PENDING_IMPORT/UNKNOWN/FORBIDDEN to be
promoted; update the guard in the block that sets KeyConfiguration.PrimaryKeyID
(the branch using exist, keyConfig, km.repo.Patch) to require explicit ENABLED
state by checking key.State == string(cmkapi.KeyStateENABLED) and returning
ErrKeyIsNotEnabled otherwise, referencing the symbols key.State,
cmkapi.KeyStateENABLED, and ErrKeyIsNotEnabled.
In `@internal/manager/keyconfiguration_test.go`:
- Around line 867-875: The test currently compares only the DCE local identifier
via uuid.UUID.ID(), which can yield false positives; update the assertion after
calling UpdateKeyConfigurationByID to compare the full UUID values directly—use
key.ID and the returned pointer value in keyConfig.PrimaryKeyID (dereference as
needed) instead of calling .ID() so the test verifies the entire UUID matches
(refer to UpdateKeyConfigurationByID, keyConfig, key.ID, and PrimaryKeyID to
locate the change).
In `@internal/manager/workflow_test.go`:
- Around line 283-292: The fixtures are assigning PrimaryKeyID on
KeyConfiguration but not linking the created model.Key back via its
KeyConfigurationID and they erroneously set PrimaryKeyID twice; update the test
fixture builders (calls to testutils.NewKey and testutils.NewKeyConfig) so the
created key's KeyConfigurationID is set to the keyConfig.ID (e.g., in NewKey’s
init func set k.KeyConfigurationID = &keyConfig.ID) and remove the duplicate
PrimaryKeyID assignment in NewKeyConfig (keep only one assignment to
kc.PrimaryKeyID = ptr.PointTo(key.ID)); apply the same fix to both subtests so
the key truly belongs to the key configuration.
---
Nitpick comments:
In `@internal/manager/key_test.go`:
- Around line 434-454: The test is asserting the "non-primary key" path but
leaves kc.PrimaryKeyID nil so it actually tests "no primary configured"; set
kc.PrimaryKeyID to a different key's ID before creating entities so the code
exercises the non-primary branch. Locate the KeyConfiguration instance created
by testutils.NewKeyConfig (kc) and assign kc.PrimaryKeyID to a UUID (or the ID
of a separate test key) that is different from the key created by
testutils.NewKey, ensuring the PrimaryKeyID is non-nil and not equal to key.ID
before calling testutils.CreateTestEntities.
In `@internal/manager/system_test.go`:
- Around line 85-88: The test creates keyConfigManager with a nil eventFactory
which diverges from production wiring; create the real eventFactory via
eventprocessor.NewEventFactory(t.Context(), &cfg, dbRepository) before calling
manager.NewKeyConfigManager, check require.NoError on its creation, and pass
that eventFactory instance (instead of nil) into NewKeyConfigManager so the
shared fixture uses the real event factory path.
In `@internal/workflow/key_configuration_actions_test.go`:
- Around line 52-67: The test uses the same key for
KeyConfiguration.PrimaryKeyID and wf.Parameters so the update-primary transition
is a no-op; modify the test to create two distinct keys (e.g., currentPrimary
and newPrimary), set kc.PrimaryKeyID to currentPrimary.ID and wf.Parameters to
newPrimary.ID (instead of k.ID), run ApplyTransition, then assert that the
persisted model.KeyConfiguration.PrimaryKeyID changed from currentPrimary.ID to
newPrimary.ID; update both the test case at the shown block and the similar
cases around lines 76-110 to use distinct keys and assert the PrimaryKeyID was
updated after ApplyTransition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49299c46-2966-4b3f-aecc-eaf9a3ee66e4
📒 Files selected for processing (34)
apis/cmk/cmk-ui.yamlcmd/task-worker/main.gocmd/tenant-manager-cli/cli_test.gocmd/tenant-manager-cli/commands/commands.gocmd/tenant-manager/main.gointernal/api/cmkapi/cmkapi.gointernal/api/transform/key/key.gointernal/api/transform/key/key_test.gointernal/apierrors/key.gointernal/controllers/cmk/key_controller.gointernal/controllers/cmk/key_controller_test.gointernal/controllers/cmk/keyconfiguration_controller.gointernal/controllers/cmk/keyconfiguration_controller_test.gointernal/manager/base.gointernal/manager/errors.gointernal/manager/key.gointernal/manager/key_test.gointernal/manager/keyconfiguration.gointernal/manager/keyconfiguration_test.gointernal/manager/keyversion_test.gointernal/manager/system_test.gointernal/manager/tenant_test.gointernal/manager/workflow_test.gointernal/model/key.gointernal/operator/operator_test.gointernal/repo/repository.gointernal/workflow/key_configuration_actions.gointernal/workflow/key_configuration_actions_test.gomigrations/tenant/schema/00005_delete_key_primary.sqltest/db-migration/migration_test.gotest/integration/async_test/common_test.gotest/integration/async_test/workflowassign_test.gotest/integration/tenant-manager/provisioning_test.gotest/security/xss/key_controller_test.go
💤 Files with no reviewable changes (6)
- internal/api/transform/key/key.go
- test/security/xss/key_controller_test.go
- internal/controllers/cmk/key_controller.go
- internal/apierrors/key.go
- internal/manager/errors.go
- internal/api/transform/key/key_test.go
✅ Files skipped from review due to trivial changes (2)
- test/integration/async_test/workflowassign_test.go
- cmd/task-worker/main.go
🚧 Files skipped from review as they are similar to previous changes (14)
- migrations/tenant/schema/00005_delete_key_primary.sql
- test/integration/tenant-manager/provisioning_test.go
- internal/manager/keyversion_test.go
- cmd/tenant-manager/main.go
- cmd/tenant-manager-cli/commands/commands.go
- test/integration/async_test/common_test.go
- internal/manager/tenant_test.go
- cmd/tenant-manager-cli/cli_test.go
- internal/repo/repository.go
- internal/manager/base.go
- internal/model/key.go
- internal/workflow/key_configuration_actions.go
- internal/api/cmkapi/cmkapi.go
- internal/manager/keyconfiguration.go
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
3 times, most recently
from
957c8fc to
3633eac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/workflow/key_configuration_actions_test.go (1)
51-67:⚠️ Potential issue | 🟡 MinorThe "Update primary key" case is currently a no-op.
This fixture seeds
keyConfig.PrimaryKeyIDwith the samekeyIDthat the workflow later patches back in viawf.Parameters, so the test still passes even ifUpdateKeyConfigurationByIDnever moves the primary. Use a second key as the promotion target and assert thatPrimaryKeyIDchanged after the transition.Also applies to: 76-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/workflow/key_configuration_actions_test.go` around lines 51 - 67, The "Update primary key" test is a no-op because kc.PrimaryKeyID is seeded with the same key ID used in wf.Parameters; change the fixture to create a second model.Key (e.g., newKey) and set wf.Parameters to newKey.ID.String(), keep kc.PrimaryKeyID set to the original key's ID, then assert after invoking the workflow transition (the code path that calls UpdateKeyConfigurationByID / the workflow state transition) that kc.PrimaryKeyID was updated to newKey.ID; apply the same fix to the second similar test case around lines 76-96 so both tests actually exercise promotion of a different key.internal/manager/key.go (1)
264-301:⚠️ Potential issue | 🟠 MajorClear or reassign
PrimaryKeyIDwhen deleting the primary key.If this path deletes the current primary,
model.KeyConfiguration.PrimaryKeyIDis left untouched, so the configuration still points at a key that no longer exists. That leaves a dangling primary reference for later reads andrepo.IsPrimaryKeychecks. At minimum, patch it tonilin the same transaction when no replacement is chosen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key.go` around lines 264 - 301, The code deletes a primary key (isPrimary) but never clears model.KeyConfiguration.PrimaryKeyID, leaving a dangling reference; inside the same km.repo.Transaction (the func passed to km.repo.Transaction that currently removes KeyVersion and Key), add a step to load or construct the KeyConfiguration for key.KeyConfigurationID and set its PrimaryKeyID to nil (or assign a replacement if chosen), then persist that change via the repo update call (use the same transaction context). Ensure this update occurs before committing the deletion of the Key so repo.IsPrimaryKey and later reads don't point at a deleted key; reference symbols: isPrimary, key.KeyConfigurationID, keyID, km.repo.Transaction, model.KeyConfiguration.PrimaryKeyID, km.deleteProviderKey.
♻️ Duplicate comments (5)
internal/manager/key.go (1)
696-700:⚠️ Potential issue | 🟠 MajorOnly promote the first key once it is actually
ENABLED.This still assigns
PrimaryKeyIDfor non-usable states because it only rejectsDISABLED. A first BYOK key inPENDING_IMPORTcan still become the configuration primary here, which leaves the config pointing at a key that cannot be used yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key.go` around lines 696 - 700, The code currently allows setting PrimaryKeyID for any non-existing key as long as it's not DISABLED; change the guard to require the key be in the ENABLED state before promoting it. In the block that checks !exist, replace the current state check (which only rejects cmkapi.KeyStateDISABLED) with an explicit equality check for cmkapi.KeyStateENABLED (using key.State == string(cmkapi.KeyStateENABLED)) and return ErrKeyIsNotEnabled for any other state so PrimaryKeyID is only assigned when the key is actually usable; update any related comments and tests referencing PrimaryKeyID, key.State, cmkapi.KeyStateENABLED, cmkapi.KeyStateDISABLED, ErrKeyIsNotEnabled and exist accordingly.internal/manager/workflow_test.go (1)
316-323:⚠️ Potential issue | 🟡 MinorLink the fixture key back to
keyConfig.
PrimaryKeyIDpoints atkey.ID, but the key itself is never reassigned tokeyConfig.IDhere. This subtest can still pass with a key that belongs to some other configuration, and the duplicatedPrimaryKeyIDassignment just adds noise.Suggested fix
key := testutils.NewKey(func(k *model.Key) {}) keyConfig := testutils.NewKeyConfig(func(kc *model.KeyConfiguration) { - kc.PrimaryKeyID = &key.ID kc.PrimaryKeyID = ptr.PointTo(key.ID) }) + key.KeyConfigurationID = keyConfig.ID testutils.CreateTestEntities(ctxSys, t, repo, key, keyConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/workflow_test.go` around lines 316 - 323, The key fixture is not linked back to the keyConfig fixture: after creating key and keyConfig via testutils.NewKey and testutils.NewKeyConfig you should set the key's configuration foreign key to the keyConfig's ID (e.g., key.KeyConfigurationID = keyConfig.ID or key.ConfigurationID = keyConfig.ID depending on the model) and remove the duplicated PrimaryKeyID assignment on keyConfig (leave a single kc.PrimaryKeyID = ptr.PointTo(key.ID)). Ensure you then pass these updated fixtures to testutils.CreateTestEntities so the key truly belongs to keyConfig.internal/controllers/cmk/keyconfiguration_controller_test.go (1)
593-600:⚠️ Potential issue | 🟡 MinorUse an authenticated request and a valid UUID in this 403 case.
As written, this can fail before the workflow-required branch:
primaryKeyIDis not a valid UUID and the request has no auth context. That means the test can still pass without proving the workflow guard is what produced the 403.Suggested fix
{ name: "Should 403 when update primary key and workflow is required", keyConfigID: keyConfig.ID.String(), - inputJSON: `{ - "primaryKeyID": "id" - }`, + inputJSON: fmt.Sprintf(`{"primaryKeyID":"%s"}`, uuid.NewString()), expectedStatus: http.StatusForbidden, + expectedBody: "error", + additionalContext: authClient.GetClientMap(), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/cmk/keyconfiguration_controller_test.go` around lines 593 - 600, The test case "Should 403 when update primary key and workflow is required" currently sends an unauthenticated request with primaryKeyID set to "id"; update it to use a valid UUID (e.g. primaryKeyID: keyConfig.ID.String() or another generated UUID) and ensure the request is made with an authenticated context (use the same auth helper or test fixture used by other authenticated cases) so the request reaches the workflow-required guard and the handler returns 403 as expected while keeping expectedStatus http.StatusForbidden.internal/manager/key_test.go (1)
929-939:⚠️ Potential issue | 🟠 MajorMake the system fixture explicitly connected for this delete-guard test.
Line 954 expects the “connected systems” protection, but the fixture at Line 933-Line 935 does not set a connected status. This can let the test pass without covering the intended guard.
🧪 Minimal fix
sys := testutils.NewSystem(func(s *model.System) { s.KeyConfigurationID = ptr.PointTo(keyConfigWSystems.ID) + s.Status = cmkapi.SystemStatusCONNECTED })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key_test.go` around lines 929 - 939, The system fixture created with testutils.NewSystem for the delete-guard test is not marked as connected so the "connected systems" protection isn't actually exercised; update the system fixture (the model.System created in the test via testutils.NewSystem) to be explicitly connected (set the appropriate field on model.System — e.g., s.Connected = true or s.Status = "connected" depending on the struct) while keeping s.KeyConfigurationID = ptr.PointTo(keyConfigWSystems.ID) so the delete guard for connected systems is exercised by the test.internal/manager/keyconfiguration.go (1)
518-545:⚠️ Potential issue | 🟠 MajorScope event payload rewrites to the intended switch/retry events only.
Current filtering by
data->>'keyIDTo'alone can rewrite unrelated or historical events sharing that field. Restrict this path to system-switch events (and retryable statuses) before patching.🔒 Minimal guard inside batch loop
for _, e := range events { + if e.Type != model.EventTypeSystemSwitch { + continue + } + // Optional: also gate by your retryable statuses before mutating payload. + systemJobData, err := eventprocessor.GetSystemJobData(e) if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/keyconfiguration.go` around lines 518 - 545, The current updatePrimaryKeySystemEvents(...) broad query only filters on data->>'keyIDTo' and may rewrite unrelated events; narrow it to target only system-switch events and retryable statuses by extending the repo query (the repo.NewQuery()/repo.NewCompositeKey()/repo.JSONBField conditions) to also require the system-switch event type and that data->>'status' is one of the retryable statuses, and add a defensive runtime guard inside the repo.ProcessInBatch loop before patching (in updatePrimaryKeySystemEvents's inner loop) that checks e.Type equals the system-switch type and that the parsed systemJobData.Status is in the allowed retry set (using eventprocessor.GetSystemJobData and systemJobData.Status) to avoid touching unrelated/historical events.
🧹 Nitpick comments (1)
internal/workflow/key_configuration_actions.go (1)
16-20: Drop the unused*model.KeyConfigurationreturn from this workflow-facing interface.
Lifecycle.updatePrimaryKeyignores the value, so this signature only adds coupling frominternal/workflowtointernal/model. Returning justerrorkeeps the boundary narrower and avoids the extra dependency in this package.Also applies to: 38-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/workflow/key_configuration_actions.go` around lines 16 - 20, Change the workflow interface method UpdateKeyConfigurationByID to return only error (UpdateKeyConfigurationByID(ctx context.Context, keyConfigID uuid.UUID, patchKeyConfig cmkapi.KeyConfigurationPatch) error), update its implementations and all callers (including Lifecycle.updatePrimaryKey) to stop expecting a *model.KeyConfiguration value, and remove any resulting dependency/import on internal/model from the internal/workflow package; apply the same change to the other workflow-facing method with the same unused *model.KeyConfiguration return (the method around the 38-40 region) so the interface only returns error everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/manager/keyconfiguration.go`:
- Line 455: Replace the incorrect nolint directive `// nolint:nestif` with the
correct `//nolint:nestif` (remove the space) so golangci-lint recognizes it;
locate the directive in internal/manager/keyconfiguration.go (the existing `//
nolint:nestif` comment) and update it to the no-space form.
---
Outside diff comments:
In `@internal/manager/key.go`:
- Around line 264-301: The code deletes a primary key (isPrimary) but never
clears model.KeyConfiguration.PrimaryKeyID, leaving a dangling reference; inside
the same km.repo.Transaction (the func passed to km.repo.Transaction that
currently removes KeyVersion and Key), add a step to load or construct the
KeyConfiguration for key.KeyConfigurationID and set its PrimaryKeyID to nil (or
assign a replacement if chosen), then persist that change via the repo update
call (use the same transaction context). Ensure this update occurs before
committing the deletion of the Key so repo.IsPrimaryKey and later reads don't
point at a deleted key; reference symbols: isPrimary, key.KeyConfigurationID,
keyID, km.repo.Transaction, model.KeyConfiguration.PrimaryKeyID,
km.deleteProviderKey.
In `@internal/workflow/key_configuration_actions_test.go`:
- Around line 51-67: The "Update primary key" test is a no-op because
kc.PrimaryKeyID is seeded with the same key ID used in wf.Parameters; change the
fixture to create a second model.Key (e.g., newKey) and set wf.Parameters to
newKey.ID.String(), keep kc.PrimaryKeyID set to the original key's ID, then
assert after invoking the workflow transition (the code path that calls
UpdateKeyConfigurationByID / the workflow state transition) that kc.PrimaryKeyID
was updated to newKey.ID; apply the same fix to the second similar test case
around lines 76-96 so both tests actually exercise promotion of a different key.
---
Duplicate comments:
In `@internal/controllers/cmk/keyconfiguration_controller_test.go`:
- Around line 593-600: The test case "Should 403 when update primary key and
workflow is required" currently sends an unauthenticated request with
primaryKeyID set to "id"; update it to use a valid UUID (e.g. primaryKeyID:
keyConfig.ID.String() or another generated UUID) and ensure the request is made
with an authenticated context (use the same auth helper or test fixture used by
other authenticated cases) so the request reaches the workflow-required guard
and the handler returns 403 as expected while keeping expectedStatus
http.StatusForbidden.
In `@internal/manager/key_test.go`:
- Around line 929-939: The system fixture created with testutils.NewSystem for
the delete-guard test is not marked as connected so the "connected systems"
protection isn't actually exercised; update the system fixture (the model.System
created in the test via testutils.NewSystem) to be explicitly connected (set the
appropriate field on model.System — e.g., s.Connected = true or s.Status =
"connected" depending on the struct) while keeping s.KeyConfigurationID =
ptr.PointTo(keyConfigWSystems.ID) so the delete guard for connected systems is
exercised by the test.
In `@internal/manager/key.go`:
- Around line 696-700: The code currently allows setting PrimaryKeyID for any
non-existing key as long as it's not DISABLED; change the guard to require the
key be in the ENABLED state before promoting it. In the block that checks
!exist, replace the current state check (which only rejects
cmkapi.KeyStateDISABLED) with an explicit equality check for
cmkapi.KeyStateENABLED (using key.State == string(cmkapi.KeyStateENABLED)) and
return ErrKeyIsNotEnabled for any other state so PrimaryKeyID is only assigned
when the key is actually usable; update any related comments and tests
referencing PrimaryKeyID, key.State, cmkapi.KeyStateENABLED,
cmkapi.KeyStateDISABLED, ErrKeyIsNotEnabled and exist accordingly.
In `@internal/manager/keyconfiguration.go`:
- Around line 518-545: The current updatePrimaryKeySystemEvents(...) broad query
only filters on data->>'keyIDTo' and may rewrite unrelated events; narrow it to
target only system-switch events and retryable statuses by extending the repo
query (the repo.NewQuery()/repo.NewCompositeKey()/repo.JSONBField conditions) to
also require the system-switch event type and that data->>'status' is one of the
retryable statuses, and add a defensive runtime guard inside the
repo.ProcessInBatch loop before patching (in updatePrimaryKeySystemEvents's
inner loop) that checks e.Type equals the system-switch type and that the parsed
systemJobData.Status is in the allowed retry set (using
eventprocessor.GetSystemJobData and systemJobData.Status) to avoid touching
unrelated/historical events.
In `@internal/manager/workflow_test.go`:
- Around line 316-323: The key fixture is not linked back to the keyConfig
fixture: after creating key and keyConfig via testutils.NewKey and
testutils.NewKeyConfig you should set the key's configuration foreign key to the
keyConfig's ID (e.g., key.KeyConfigurationID = keyConfig.ID or
key.ConfigurationID = keyConfig.ID depending on the model) and remove the
duplicated PrimaryKeyID assignment on keyConfig (leave a single kc.PrimaryKeyID
= ptr.PointTo(key.ID)). Ensure you then pass these updated fixtures to
testutils.CreateTestEntities so the key truly belongs to keyConfig.
---
Nitpick comments:
In `@internal/workflow/key_configuration_actions.go`:
- Around line 16-20: Change the workflow interface method
UpdateKeyConfigurationByID to return only error (UpdateKeyConfigurationByID(ctx
context.Context, keyConfigID uuid.UUID, patchKeyConfig
cmkapi.KeyConfigurationPatch) error), update its implementations and all callers
(including Lifecycle.updatePrimaryKey) to stop expecting a
*model.KeyConfiguration value, and remove any resulting dependency/import on
internal/model from the internal/workflow package; apply the same change to the
other workflow-facing method with the same unused *model.KeyConfiguration return
(the method around the 38-40 region) so the interface only returns error
everywhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24fd8401-b206-4c34-ab23-9b161715a3cf
📒 Files selected for processing (34)
apis/cmk/cmk-ui.yamlcmd/task-worker/main.gocmd/tenant-manager-cli/cli_test.gocmd/tenant-manager-cli/commands/commands.gocmd/tenant-manager/main.gointernal/api/cmkapi/cmkapi.gointernal/api/transform/key/key.gointernal/api/transform/key/key_test.gointernal/apierrors/key.gointernal/controllers/cmk/key_controller.gointernal/controllers/cmk/key_controller_test.gointernal/controllers/cmk/keyconfiguration_controller.gointernal/controllers/cmk/keyconfiguration_controller_test.gointernal/manager/base.gointernal/manager/errors.gointernal/manager/key.gointernal/manager/key_test.gointernal/manager/keyconfiguration.gointernal/manager/keyconfiguration_test.gointernal/manager/keyversion_test.gointernal/manager/system_test.gointernal/manager/tenant_test.gointernal/manager/workflow_test.gointernal/model/key.gointernal/operator/operator_test.gointernal/repo/repository.gointernal/workflow/key_configuration_actions.gointernal/workflow/key_configuration_actions_test.gomigrations/tenant/schema/00005_delete_key_primary.sqltest/db-migration/migration_test.gotest/integration/async_test/common_test.gotest/integration/async_test/workflowassign_test.gotest/integration/tenant-manager/provisioning_test.gotest/security/xss/key_controller_test.go
💤 Files with no reviewable changes (6)
- internal/apierrors/key.go
- internal/api/transform/key/key.go
- test/security/xss/key_controller_test.go
- internal/controllers/cmk/key_controller.go
- internal/manager/errors.go
- internal/api/transform/key/key_test.go
✅ Files skipped from review due to trivial changes (4)
- migrations/tenant/schema/00005_delete_key_primary.sql
- cmd/tenant-manager-cli/cli_test.go
- internal/manager/base.go
- internal/manager/keyconfiguration_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- cmd/task-worker/main.go
- internal/manager/keyversion_test.go
- test/integration/tenant-manager/provisioning_test.go
- test/db-migration/migration_test.go
- cmd/tenant-manager/main.go
- cmd/tenant-manager-cli/commands/commands.go
- internal/controllers/cmk/keyconfiguration_controller.go
- internal/manager/tenant_test.go
- test/integration/async_test/common_test.go
- internal/repo/repository.go
- internal/model/key.go
- internal/api/cmkapi/cmkapi.go
- apis/cmk/cmk-ui.yaml
minh-nghia
reviewed
Mar 19, 2026
| type: string | ||
| maxLength: 4096 | ||
| example: This Key is used for DB key store encryption. | ||
| isPrimary: |
Contributor
There was a problem hiding this comment.
The DB column may be redundant but I don't think we should remove it in the API response at all. In the UI keys view we still have to annotate which one is primary key
Contributor
Author
There was a problem hiding this comment.
We can keep it even though it would result in extra calls, but it's something we can maintain. This could also be a match in UI to show element key.ID == keyconfig.ID
Contributor
There was a problem hiding this comment.
I still believe we should not remove such useful information and make the life of this API's consumer harder 😃
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
2 times, most recently
from
c858b22 to
0a63363
Compare
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
from
0a63363 to
981b970
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/controllers/cmk/keyconfiguration_controller_test.go (1)
593-600:⚠️ Potential issue | 🟠 MajorAuthenticate this case and use a valid UUID.
As written, this request can still return 403 because it is unauthenticated, and Line 597 uses a non-UUID
primaryKeyID. WithexpectedBodyleft empty, the shared body assertion is vacuous as well. That means the test can pass without ever exercising the workflow-required branch.🧪 Suggested test fix
{ name: "Should 403 when update primary key and workflow is required", keyConfigID: keyConfig.ID.String(), inputJSON: `{ - "primaryKeyID": "id" + "primaryKeyID": "12345678-90ab-cdef-1234-567890abcdef" }`, - expectedStatus: http.StatusForbidden, + expectedStatus: http.StatusForbidden, + expectedBody: "error", + additionalContext: authClient.GetClientMap(), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/cmk/keyconfiguration_controller_test.go` around lines 593 - 600, The test case "Should 403 when update primary key and workflow is required" is unauthenticated and uses a non-UUID primaryKeyID so it can pass without exercising the workflow-required branch; update the test to authenticate the request (use the same auth helper the other cases use or attach a valid auth token/session used elsewhere in these tests) and replace the inputJSON primaryKeyID value with a real UUID (e.g. the existing primaryKey.ID.String() or uuid generation used in other tests) and populate expectedBody to assert the specific 403 response payload so the test actually validates the workflow-required denial path instead of passing vacuously.internal/manager/key.go (1)
696-705:⚠️ Potential issue | 🟠 MajorOnly auto-promote keys that are actually
ENABLED.Line 698 only rejects
DISABLED, so the first key can still becomePrimaryKeyIDwhile it is in another unusable state, e.g. a BYOK key inPENDING_IMPORT. That makes a fresh configuration advertise a primary key that cannot be used.✅ Minimal fix
if !exist { - if key.State == string(cmkapi.KeyStateDISABLED) { + if key.State != string(cmkapi.KeyStateENABLED) { return ErrKeyIsNotEnabled }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key.go` around lines 696 - 705, The code currently only rejects DISABLED keys before auto-promoting (checking key.State == string(cmkapi.KeyStateDISABLED)), which allows other non-usable states to be promoted; change the check to require the key be explicitly ENABLED by validating key.State == string(cmkapi.KeyStateENABLED) (or equivalently return ErrKeyIsNotEnabled when key.State != string(cmkapi.KeyStateENABLED)) before constructing model.KeyConfiguration and calling km.repo.Patch so only genuinely enabled keys become PrimaryKeyID.internal/manager/workflow_test.go (1)
316-320:⚠️ Potential issue | 🟡 MinorLink the fixture key back to
keyConfig.This case still leaves the candidate key with a zero
KeyConfigurationID, so it can pass by matchingPrimaryKeyIDalone without proving the selected key belongs to the configuration. The duplicatePrimaryKeyIDassignment is redundant too.🧪 Suggested test fix
key := testutils.NewKey(func(k *model.Key) {}) keyConfig := testutils.NewKeyConfig(func(kc *model.KeyConfiguration) { - kc.PrimaryKeyID = &key.ID kc.PrimaryKeyID = ptr.PointTo(key.ID) }) + key.KeyConfigurationID = keyConfig.ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/workflow_test.go` around lines 316 - 320, The test creates a key and a keyConfig but doesn't link the key back to the configuration and redundantly assigns PrimaryKeyID twice; update the fixture so the key's KeyConfigurationID points to the created keyConfig.ID (e.g., set key.KeyConfigurationID = ptr.PointTo(keyConfig.ID) or the appropriate field on model.Key) and remove the duplicate PrimaryKeyID assignment on the keyConfig (only set kc.PrimaryKeyID once) so the candidate key both references and belongs to the KeyConfiguration in testutils.NewKey / testutils.NewKeyConfig setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controllers/cmk/keyconfiguration_controller.go`:
- Around line 140-142: The code dereferences kc.PrimaryKeyID without checking
for nil which can panic; update the comparison in the handler that uses
kc.PrimaryKeyID and request.Body.PrimaryKeyID so you first check for nil (e.g.,
if kc.PrimaryKeyID == nil || request.Body.PrimaryKeyID == nil) and only
dereference when both are non-nil, and maintain the existing
ErrActionRequireWorkflow return only when both IDs exist and are unequal (use
the symbols kc.PrimaryKeyID and request.Body.PrimaryKeyID to locate the change).
In `@internal/manager/key.go`:
- Around line 259-264: The delete path currently checks primary status via
repo.IsPrimaryKey(ctx, km.repo, key) and, if allowed, deletes the key without
updating model.KeyConfiguration.PrimaryKeyID; you must update the owning key
configuration inside the same DB transaction that deletes the key: when
IsPrimaryKey returns true, within the transaction that performs the delete (the
code handling the key delete for `key` in the same function), either clear
PrimaryKeyID or set it to a different valid key on the associated
KeyConfiguration and persist that change (update the KeyConfiguration's
PrimaryKeyID) before committing; locate uses of `repo.IsPrimaryKey`, `km.repo`,
`key`, and `model.KeyConfiguration.PrimaryKeyID` to add the configuration update
in that transaction.
---
Duplicate comments:
In `@internal/controllers/cmk/keyconfiguration_controller_test.go`:
- Around line 593-600: The test case "Should 403 when update primary key and
workflow is required" is unauthenticated and uses a non-UUID primaryKeyID so it
can pass without exercising the workflow-required branch; update the test to
authenticate the request (use the same auth helper the other cases use or attach
a valid auth token/session used elsewhere in these tests) and replace the
inputJSON primaryKeyID value with a real UUID (e.g. the existing
primaryKey.ID.String() or uuid generation used in other tests) and populate
expectedBody to assert the specific 403 response payload so the test actually
validates the workflow-required denial path instead of passing vacuously.
In `@internal/manager/key.go`:
- Around line 696-705: The code currently only rejects DISABLED keys before
auto-promoting (checking key.State == string(cmkapi.KeyStateDISABLED)), which
allows other non-usable states to be promoted; change the check to require the
key be explicitly ENABLED by validating key.State ==
string(cmkapi.KeyStateENABLED) (or equivalently return ErrKeyIsNotEnabled when
key.State != string(cmkapi.KeyStateENABLED)) before constructing
model.KeyConfiguration and calling km.repo.Patch so only genuinely enabled keys
become PrimaryKeyID.
In `@internal/manager/workflow_test.go`:
- Around line 316-320: The test creates a key and a keyConfig but doesn't link
the key back to the configuration and redundantly assigns PrimaryKeyID twice;
update the fixture so the key's KeyConfigurationID points to the created
keyConfig.ID (e.g., set key.KeyConfigurationID = ptr.PointTo(keyConfig.ID) or
the appropriate field on model.Key) and remove the duplicate PrimaryKeyID
assignment on the keyConfig (only set kc.PrimaryKeyID once) so the candidate key
both references and belongs to the KeyConfiguration in testutils.NewKey /
testutils.NewKeyConfig setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45b6ea6e-414d-49fa-bd03-735bbaf759b5
📒 Files selected for processing (34)
apis/cmk/cmk-ui.yamlcmd/task-worker/main.gocmd/tenant-manager-cli/cli_test.gocmd/tenant-manager-cli/commands/commands.gocmd/tenant-manager/main.gointernal/api/cmkapi/cmkapi.gointernal/api/transform/key/key.gointernal/api/transform/key/key_test.gointernal/apierrors/key.gointernal/controllers/cmk/key_controller.gointernal/controllers/cmk/key_controller_test.gointernal/controllers/cmk/keyconfiguration_controller.gointernal/controllers/cmk/keyconfiguration_controller_test.gointernal/manager/base.gointernal/manager/errors.gointernal/manager/key.gointernal/manager/key_test.gointernal/manager/keyconfiguration.gointernal/manager/keyconfiguration_test.gointernal/manager/keyversion_test.gointernal/manager/system_test.gointernal/manager/tenant_test.gointernal/manager/workflow_test.gointernal/model/key.gointernal/operator/operator_test.gointernal/repo/repository.gointernal/workflow/key_configuration_actions.gointernal/workflow/key_configuration_actions_test.gomigrations/tenant/schema/00005_delete_key_primary.sqltest/db-migration/migration_test.gotest/integration/async_test/common_test.gotest/integration/async_test/workflowassign_test.gotest/integration/tenant-manager/provisioning_test.gotest/security/xss/key_controller_test.go
💤 Files with no reviewable changes (6)
- internal/apierrors/key.go
- internal/api/transform/key/key.go
- internal/manager/errors.go
- test/security/xss/key_controller_test.go
- internal/controllers/cmk/key_controller.go
- internal/api/transform/key/key_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/repo/repository.go
- migrations/tenant/schema/00005_delete_key_primary.sql
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/manager/tenant_test.go
- test/integration/async_test/workflowassign_test.go
- test/integration/tenant-manager/provisioning_test.go
- internal/manager/base.go
- cmd/tenant-manager-cli/commands/commands.go
- cmd/tenant-manager/main.go
- cmd/task-worker/main.go
- test/db-migration/migration_test.go
- internal/workflow/key_configuration_actions_test.go
- internal/model/key.go
- apis/cmk/cmk-ui.yaml
- internal/api/cmkapi/cmkapi.go
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
from
981b970 to
08b957c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
internal/manager/workflow_test.go (1)
316-323:⚠️ Potential issue | 🟡 MinorLink the fixture key to
keyConfigin this remaining case.This test still leaves
key.KeyConfigurationIDat the zero UUID, so it can hitErrAlreadyPrimaryKeywithout proving the selected key actually belongs to the configuration. Also, the firstPrimaryKeyIDassignment is dead code because the second one overwrites it.Suggested fixture fix
key := testutils.NewKey(func(k *model.Key) {}) keyConfig := testutils.NewKeyConfig(func(kc *model.KeyConfiguration) { - kc.PrimaryKeyID = &key.ID kc.PrimaryKeyID = ptr.PointTo(key.ID) }) + key.KeyConfigurationID = keyConfig.ID testutils.CreateTestEntities(ctxSys, t, repo, key, keyConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/workflow_test.go` around lines 316 - 323, The fixture isn't linking the key to the configuration and the first PrimaryKeyID assignment is overwritten; update the fixtures so keyConfig.PrimaryKeyID is set once to key.ID and also set the created key's KeyConfigurationID to the keyConfig.ID (so the key truly belongs to that configuration) before calling testutils.CreateTestEntities; reference the test fixtures created via testutils.NewKey (key), testutils.NewKeyConfig (keyConfig), and the fields Key.KeyConfigurationID and model.KeyConfiguration.PrimaryKeyID.internal/controllers/cmk/keyconfiguration_controller_test.go (1)
593-600:⚠️ Potential issue | 🟠 MajorUse a valid, authorized PATCH to exercise the workflow guard.
"primaryKeyID": "id"will not decode intoKeyConfigurationPatch.PrimaryKeyID, and this case also omits the admin context used by the other PATCH tests. The 4xx can therefore come from request/auth handling instead of the new workflow check. Use a valid UUID that differs from the current primary key, passauthClient.GetClientMap(), and make sure the fixture explicitly enables workflow-required behavior before asserting 403.Suggested test adjustment
{ name: "Should 403 when update primary key and workflow is required", keyConfigID: keyConfig.ID.String(), inputJSON: `{ - "primaryKeyID": "id" + "primaryKeyID": "12345678-90ab-cdef-1234-567890abcdef" }`, expectedStatus: http.StatusForbidden, + expectedBody: "error", + additionalContext: authClient.GetClientMap(), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/cmk/keyconfiguration_controller_test.go` around lines 593 - 600, The test case "Should 403 when update primary key and workflow is required" is using an invalid primaryKeyID ("id") and lacks the admin auth context so the failure may come from decoding/auth rather than the workflow guard; change inputJSON to set "primaryKeyID" to a valid UUID string that differs from the existing primary key (so it decodes into KeyConfigurationPatch.PrimaryKeyID), ensure the test uses the admin auth map (pass authClient.GetClientMap()) like the other PATCH tests, and configure the fixture to explicitly enable workflow-required behavior before asserting http.StatusForbidden so the 403 is produced by the workflow check.internal/manager/keyconfiguration.go (2)
469-502:⚠️ Potential issue | 🟠 MajorStill fan out switch jobs on the first primary assignment.
This early return skips Lines 483-509 entirely. If a configuration already has systems but no current
PrimaryKeyID, the patch updates the row and exits without creating anySystemSwitchNewPrimaryKeyjobs for those systems.💡 Suggested fix
- // Key is valid. If keyconfig has no existing key no need for further validations - if keyConfig.PrimaryKeyID == nil { - return nil - } - - err = m.updatePrimaryKeySystemEvents( - ctx, - ptr.GetSafeDeref(keyConfig.PrimaryKeyID).String(), - primaryKeyID.String(), - ) - if err != nil { - return err - } + var oldPrimary string + if keyConfig.PrimaryKeyID != nil { + oldPrimary = keyConfig.PrimaryKeyID.String() + if err := m.updatePrimaryKeySystemEvents(ctx, oldPrimary, primaryKeyID.String()); err != nil { + return err + } + } // Send system switches for systems in keyconfig query := repo.NewQuery().Where( @@ _, err := m.eventFactory.SystemSwitchNewPrimaryKey( ctx, s, primaryKeyID.String(), - keyConfig.PrimaryKeyID.String(), + oldPrimary, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/keyconfiguration.go` around lines 469 - 502, The early return on keyConfig.PrimaryKeyID == nil prevents creating SystemSwitchNewPrimaryKey jobs when assigning the first primary; remove that return and ensure the fan-out still runs in that case. Specifically, in the method referencing keyConfig.PrimaryKeyID and primaryKeyID, do not return when keyConfig.PrimaryKeyID == nil; instead proceed to call m.updatePrimaryKeySystemEvents(...) and then run the repo.ProcessInBatch(...) block that uses query and m.eventFactory.SystemSwitchNewPrimaryKey to enqueue switches for systems associated with the key config so first-time primary assignments get fan-out jobs.
517-544:⚠️ Potential issue | 🟠 MajorScope event rewrites to retryable
SYSTEM_SWITCHrows.The current query matches any event whose JSON payload happens to contain
keyIDTo, then decodes it as switch data. That can rewrite completed history or fail the whole update on an unrelated event type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/keyconfiguration.go` around lines 517 - 544, The updatePrimaryKeySystemEvents function currently matches any event whose JSON contains keyIDTo; restrict this to only retryable SYSTEM_SWITCH system events by adding an explicit filter on the event type and retryable flag (rather than relying on JSON content). Modify the repo query built in updatePrimaryKeySystemEvents to include conditions for the event type being "SYSTEM_SWITCH" and retryable=true (e.g., add repo.FieldEquals(repo.TypeField, "SYSTEM_SWITCH") and repo.FieldEquals(repo.RetryableField, true) or the equivalent fields/keys used in your repo/model), and/or defensively check inside the batch loop that e.Type == "SYSTEM_SWITCH" and e.Retryable == true before calling eventprocessor.GetSystemJobData and rewriting e.Data. Ensure only those identified events are patched back via m.r.Patch.internal/manager/key_test.go (1)
933-935:⚠️ Potential issue | 🟠 MajorMark this system fixture as connected.
This case is supposed to cover deleting a primary key while connected systems exist, but
sysnever gets a connected status here. As written, the test can pass without exercising the guard named in the subtest.💡 Minimal fix
sys := testutils.NewSystem(func(s *model.System) { s.KeyConfigurationID = ptr.PointTo(keyConfigWSystems.ID) + s.Status = cmkapi.SystemStatusCONNECTED })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/manager/key_test.go` around lines 933 - 935, The system fixture `sys` created via testutils.NewSystem currently only sets `s.KeyConfigurationID` and never marks the system as connected; update the initializer closure passed to testutils.NewSystem (the func(s *model.System) { ... } that sets `s.KeyConfigurationID = ptr.PointTo(keyConfigWSystems.ID)`) to also mark the system as connected (for example set `s.Connected = true` or the equivalent connected status field on model.System) so the subtest actually exercises the delete-primary-key guard when connected systems exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controllers/cmk/keyconfiguration_controller.go`:
- Around line 129-145: The patch currently detects when the requested
PrimaryKeyID equals the existing one but still passes that non-nil value into
KeyConfig.UpdateKeyConfigurationByID, which triggers handleUpdatePrimaryKey and
unwanted switch/job work; before calling KeyConfig.UpdateKeyConfigurationByID,
compare kc.PrimaryKeyID with request.Body.PrimaryKeyID (using the existing
lookup via c.Manager.KeyConfig.GetKeyConfigurationByID and the
IsWorkflowRequired guard) and if they are equal set request.Body.PrimaryKeyID =
nil so the update becomes a true no-op for primary key changes; keep all
existing error checks and only null out the field when unchanged.
In `@internal/manager/keyconfiguration.go`:
- Around line 465-467: The current check only rejects cmkapi.KeyStateDISABLED so
keys in any other non-usable state could be promoted; update the condition to
accept only ENABLED keys by replacing the check on key.State (and the constant
cmkapi.KeyStateDISABLED) with an explicit allow: if key.State !=
string(cmkapi.KeyStateENABLED) return ErrKeyIsNotEnabled, referencing the
key.State field and the ErrKeyIsNotEnabled error to enforce the single allowed
primary state.
- Around line 242-247: Currently you always call handleUpdatePrimaryKey and
enqueue switch jobs even when patchKeyConfig.PrimaryKeyID equals the existing
keyConfig.PrimaryKeyID; change the branch that handles
patchKeyConfig.PrimaryKeyID so it first compares the current value and only
proceeds if they differ: if patchKeyConfig.PrimaryKeyID != nil and
(keyConfig.PrimaryKeyID == nil or *patchKeyConfig.PrimaryKeyID !=
*keyConfig.PrimaryKeyID) then call m.handleUpdatePrimaryKey and update
keyConfig.PrimaryKeyID, otherwise skip the update to avoid rewriting pending
payloads and enqueuing redundant switch jobs.
---
Duplicate comments:
In `@internal/controllers/cmk/keyconfiguration_controller_test.go`:
- Around line 593-600: The test case "Should 403 when update primary key and
workflow is required" is using an invalid primaryKeyID ("id") and lacks the
admin auth context so the failure may come from decoding/auth rather than the
workflow guard; change inputJSON to set "primaryKeyID" to a valid UUID string
that differs from the existing primary key (so it decodes into
KeyConfigurationPatch.PrimaryKeyID), ensure the test uses the admin auth map
(pass authClient.GetClientMap()) like the other PATCH tests, and configure the
fixture to explicitly enable workflow-required behavior before asserting
http.StatusForbidden so the 403 is produced by the workflow check.
In `@internal/manager/key_test.go`:
- Around line 933-935: The system fixture `sys` created via testutils.NewSystem
currently only sets `s.KeyConfigurationID` and never marks the system as
connected; update the initializer closure passed to testutils.NewSystem (the
func(s *model.System) { ... } that sets `s.KeyConfigurationID =
ptr.PointTo(keyConfigWSystems.ID)`) to also mark the system as connected (for
example set `s.Connected = true` or the equivalent connected status field on
model.System) so the subtest actually exercises the delete-primary-key guard
when connected systems exist.
In `@internal/manager/keyconfiguration.go`:
- Around line 469-502: The early return on keyConfig.PrimaryKeyID == nil
prevents creating SystemSwitchNewPrimaryKey jobs when assigning the first
primary; remove that return and ensure the fan-out still runs in that case.
Specifically, in the method referencing keyConfig.PrimaryKeyID and primaryKeyID,
do not return when keyConfig.PrimaryKeyID == nil; instead proceed to call
m.updatePrimaryKeySystemEvents(...) and then run the repo.ProcessInBatch(...)
block that uses query and m.eventFactory.SystemSwitchNewPrimaryKey to enqueue
switches for systems associated with the key config so first-time primary
assignments get fan-out jobs.
- Around line 517-544: The updatePrimaryKeySystemEvents function currently
matches any event whose JSON contains keyIDTo; restrict this to only retryable
SYSTEM_SWITCH system events by adding an explicit filter on the event type and
retryable flag (rather than relying on JSON content). Modify the repo query
built in updatePrimaryKeySystemEvents to include conditions for the event type
being "SYSTEM_SWITCH" and retryable=true (e.g., add
repo.FieldEquals(repo.TypeField, "SYSTEM_SWITCH") and
repo.FieldEquals(repo.RetryableField, true) or the equivalent fields/keys used
in your repo/model), and/or defensively check inside the batch loop that e.Type
== "SYSTEM_SWITCH" and e.Retryable == true before calling
eventprocessor.GetSystemJobData and rewriting e.Data. Ensure only those
identified events are patched back via m.r.Patch.
In `@internal/manager/workflow_test.go`:
- Around line 316-323: The fixture isn't linking the key to the configuration
and the first PrimaryKeyID assignment is overwritten; update the fixtures so
keyConfig.PrimaryKeyID is set once to key.ID and also set the created key's
KeyConfigurationID to the keyConfig.ID (so the key truly belongs to that
configuration) before calling testutils.CreateTestEntities; reference the test
fixtures created via testutils.NewKey (key), testutils.NewKeyConfig (keyConfig),
and the fields Key.KeyConfigurationID and model.KeyConfiguration.PrimaryKeyID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb52aed6-dd02-4faf-84a9-ff9a872c88b8
📒 Files selected for processing (34)
apis/cmk/cmk-ui.yamlcmd/task-worker/main.gocmd/tenant-manager-cli/cli_test.gocmd/tenant-manager-cli/commands/commands.gocmd/tenant-manager/main.gointernal/api/cmkapi/cmkapi.gointernal/api/transform/key/key.gointernal/api/transform/key/key_test.gointernal/apierrors/key.gointernal/controllers/cmk/key_controller.gointernal/controllers/cmk/key_controller_test.gointernal/controllers/cmk/keyconfiguration_controller.gointernal/controllers/cmk/keyconfiguration_controller_test.gointernal/manager/base.gointernal/manager/errors.gointernal/manager/key.gointernal/manager/key_test.gointernal/manager/keyconfiguration.gointernal/manager/keyconfiguration_test.gointernal/manager/keyversion_test.gointernal/manager/system_test.gointernal/manager/tenant_test.gointernal/manager/workflow_test.gointernal/model/key.gointernal/operator/operator_test.gointernal/repo/repository.gointernal/workflow/key_configuration_actions.gointernal/workflow/key_configuration_actions_test.gomigrations/tenant/schema/00005_delete_key_primary.sqltest/db-migration/migration_test.gotest/integration/async_test/common_test.gotest/integration/async_test/workflowassign_test.gotest/integration/tenant-manager/provisioning_test.gotest/security/xss/key_controller_test.go
💤 Files with no reviewable changes (6)
- internal/apierrors/key.go
- internal/api/transform/key/key.go
- internal/controllers/cmk/key_controller.go
- test/security/xss/key_controller_test.go
- internal/manager/errors.go
- internal/api/transform/key/key_test.go
✅ Files skipped from review due to trivial changes (5)
- migrations/tenant/schema/00005_delete_key_primary.sql
- cmd/tenant-manager-cli/cli_test.go
- internal/operator/operator_test.go
- cmd/tenant-manager/main.go
- cmd/task-worker/main.go
🚧 Files skipped from review as they are similar to previous changes (11)
- test/integration/async_test/workflowassign_test.go
- test/integration/async_test/common_test.go
- test/integration/tenant-manager/provisioning_test.go
- internal/manager/keyversion_test.go
- test/db-migration/migration_test.go
- internal/manager/base.go
- internal/repo/repository.go
- internal/model/key.go
- apis/cmk/cmk-ui.yaml
- internal/api/cmkapi/cmkapi.go
- internal/manager/key.go
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
4 times, most recently
from
4a2c53f to
63e6652
Compare
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
3 times, most recently
from
d6ed062 to
f9bafb0
Compare
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
from
f9bafb0 to
45ec4b7
Compare
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
2 times, most recently
from
22e8069 to
bbc8361
Compare
jmpTeixeira02
force-pushed
the
task/key-keyconfig-refactor
branch
from
bbc8361 to
ed9efe0
Compare
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
3 participants
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 by CodeRabbit
New Features
Breaking Changes
Database