From b832687ce61b491034d5b8977f08b81dc9ab8145 Mon Sep 17 00:00:00 2001 From: Chris Reed Date: Mon, 13 Apr 2026 16:57:07 -0500 Subject: [PATCH 1/4] feat(cli): Commit actions, executor shape. --- ...mespacedPolicy.go => namespaced_policy.go} | 21 +- .../migrations/namespacedpolicy/execute.go | 263 +++++++++++++ .../namespacedpolicy/execute_test.go | 352 ++++++++++++++++++ otdfctl/migrations/namespacedpolicy/plan.go | 16 +- 4 files changed, 644 insertions(+), 8 deletions(-) rename otdfctl/cmd/migrate/{namespacedPolicy.go => namespaced_policy.go} (78%) create mode 100644 otdfctl/migrations/namespacedpolicy/execute.go create mode 100644 otdfctl/migrations/namespacedpolicy/execute_test.go diff --git a/otdfctl/cmd/migrate/namespacedPolicy.go b/otdfctl/cmd/migrate/namespaced_policy.go similarity index 78% rename from otdfctl/cmd/migrate/namespacedPolicy.go rename to otdfctl/cmd/migrate/namespaced_policy.go index 87052e3b72..635adb4561 100644 --- a/otdfctl/cmd/migrate/namespacedPolicy.go +++ b/otdfctl/cmd/migrate/namespaced_policy.go @@ -2,7 +2,6 @@ package migrate import ( "encoding/json" - "errors" "os" "path/filepath" @@ -42,25 +41,33 @@ func migrateNamespacedPolicy(cmd *cobra.Command, args []string) { if err != nil { cli.ExitWithError("could not read --commit flag", err) } - if commit { - cli.ExitWithError("commit is not implemented for namespaced-policy", errors.New("--commit is not supported yet")) - } h := otdfctl.NewHandler(c) defer h.Close() planner, err := namespacedpolicy.NewPlanner(&h, scopeCSV) if err != nil { - cli.ExitWithError("could not create namespacedpolicy planner", err) + cli.ExitWithError("could not create namespaced-policy planner", err) } plan, err := planner.Plan(cmd.Context()) if err != nil { - cli.ExitWithError("could not build namespacedpolicy plan", err) + cli.ExitWithError("could not build namespaced-policy plan", err) + } + + if commit { + executor, err := namespacedpolicy.NewExecutor(h) + if err != nil { + cli.ExitWithError("could not create namespaced-policy executor", err) + } + + if err := executor.Execute(cmd.Context(), plan); err != nil { + cli.ExitWithError("could not execute namespaced-policy commit", err) + } } if err := writeNamespacedPolicyPlan(outputPath, plan); err != nil { - cli.ExitWithError("could not write namespacedpolicy plan", err) + cli.ExitWithError("could not write namespaced-policy plan", err) } } diff --git a/otdfctl/migrations/namespacedpolicy/execute.go b/otdfctl/migrations/namespacedpolicy/execute.go new file mode 100644 index 0000000000..23760a00b3 --- /dev/null +++ b/otdfctl/migrations/namespacedpolicy/execute.go @@ -0,0 +1,263 @@ +package namespacedpolicy + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/google/uuid" + "github.com/opentdf/platform/protocol/go/common" + "github.com/opentdf/platform/protocol/go/policy" +) + +var ( + ErrNilExecutorHandler = errors.New("executor handler is required") + ErrNilExecutionPlan = errors.New("execution plan is required") + ErrPlanNotExecutable = errors.New("plan is not executable") + ErrExecutionPhaseNotImplemented = errors.New("execution phase is not implemented") + ErrActionMissingExistingTarget = errors.New("action target is missing an existing standard target id") + ErrActionMissingMigratedTarget = errors.New("action target is missing an already-migrated target id") + ErrActionMissingTargetNamespace = errors.New("action target namespace is not set") + ErrActionMissingCreatedTargetID = errors.New("create action returned no target id") + ErrActionUnsupportedStatus = errors.New("action target has unsupported status") +) + +const ( + migrationLabelMigratedFrom = "migrated_from" + migrationLabelRun = "migration_run" +) + +type ExecutorHandler interface { + CreateAction(ctx context.Context, name string, namespace string, metadata *common.MetadataMutable) (*policy.Action, error) +} + +type Executor struct { + handler ExecutorHandler + runID string + actionIDs map[string]map[string]string +} + +func NewExecutor(handler ExecutorHandler) (*Executor, error) { + if handler == nil { + return nil, ErrNilExecutorHandler + } + + return &Executor{ + handler: handler, + runID: uuid.NewString(), + actionIDs: make(map[string]map[string]string), + }, nil +} + +func (e *Executor) Execute(ctx context.Context, plan *Plan) error { + if err := e.validatePlan(plan); err != nil { + return err + } + + if err := e.executeActions(ctx, plan.Actions); err != nil { + return err + } + if err := e.executeSubjectConditionSets(ctx, plan); err != nil { + return err + } + if err := e.executeSubjectMappings(ctx, plan); err != nil { + return err + } + if err := e.executeRegisteredResources(ctx, plan); err != nil { + return err + } + if err := e.executeObligationTriggers(ctx, plan); err != nil { + return err + } + + return nil +} + +func (e *Executor) validatePlan(plan *Plan) error { + if e == nil || e.handler == nil { + return ErrNilExecutorHandler + } + if plan == nil { + return ErrNilExecutionPlan + } + if plan.Unresolved != nil && hasUnresolved(*plan.Unresolved) { + return fmt.Errorf("%w: finalized plan contains unresolved entries", ErrPlanNotExecutable) + } + + return nil +} + +func metadataForCreate(sourceID string, sourceLabels map[string]string, runID string) *common.MetadataMutable { + labels := map[string]string{} + for key, value := range sourceLabels { + labels[key] = value + } + + labels[migrationLabelMigratedFrom] = sourceID + labels[migrationLabelRun] = runID + + return &common.MetadataMutable{ + Labels: labels, + } +} + +func namespaceIdentifier(namespace *policy.Namespace) string { + if namespace == nil { + return "" + } + if id := strings.TrimSpace(namespace.GetId()); id != "" { + return id + } + return strings.TrimSpace(namespace.GetFqn()) +} + +func namespaceLabel(namespace *policy.Namespace) string { + if namespace == nil { + return "" + } + if id := strings.TrimSpace(namespace.GetId()); id != "" { + return id + } + if fqn := strings.TrimSpace(namespace.GetFqn()); fqn != "" { + return fqn + } + return "" +} + +func (e *Executor) setActionTargetID(sourceID string, namespace *policy.Namespace, targetID string) { + if e == nil || sourceID == "" || targetID == "" { + return + } + + namespaceKey := namespaceRefKey(namespace) + if namespaceKey == "" { + return + } + + if e.actionIDs == nil { + e.actionIDs = make(map[string]map[string]string) + } + if e.actionIDs[sourceID] == nil { + e.actionIDs[sourceID] = make(map[string]string) + } + + e.actionIDs[sourceID][namespaceKey] = targetID +} + +func (e *Executor) actionTargetID(sourceID string, namespace *policy.Namespace) string { + if e == nil || sourceID == "" { + return "" + } + + namespaceKey := namespaceRefKey(namespace) + if namespaceKey == "" { + return "" + } + + return e.actionIDs[sourceID][namespaceKey] +} + +func (e *Executor) executeActions(ctx context.Context, actionPlans []*ActionPlan) error { + for _, actionPlan := range actionPlans { + if actionPlan == nil || actionPlan.Source == nil { + continue + } + + for _, target := range actionPlan.Targets { + if target == nil { + continue + } + + switch target.Status { + case TargetStatusExistingStandard: + if target.TargetID() == "" { + return fmt.Errorf("%w: action %q target %q", ErrActionMissingExistingTarget, actionPlan.Source.GetId(), namespaceLabel(target.Namespace)) + } + e.setActionTargetID(actionPlan.Source.GetId(), target.Namespace, target.TargetID()) + case TargetStatusAlreadyMigrated: + if target.TargetID() == "" { + return fmt.Errorf("%w: action %q target %q", ErrActionMissingMigratedTarget, actionPlan.Source.GetId(), namespaceLabel(target.Namespace)) + } + e.setActionTargetID(actionPlan.Source.GetId(), target.Namespace, target.TargetID()) + continue + case TargetStatusCreate: + namespace := namespaceIdentifier(target.Namespace) + if namespace == "" { + return fmt.Errorf("%w: action %q", ErrActionMissingTargetNamespace, actionPlan.Source.GetId()) + } + + created, err := e.handler.CreateAction( + ctx, + actionPlan.Source.GetName(), + namespace, + metadataForCreate( + actionPlan.Source.GetId(), + actionPlan.Source.GetMetadata().GetLabels(), + e.runID, + ), + ) + if err != nil { + target.Execution = &ExecutionResult{ + RunID: e.runID, + Failure: err.Error(), + } + return fmt.Errorf("create action %q in namespace %q: %w", actionPlan.Source.GetId(), namespaceLabel(target.Namespace), err) + } + if created == nil || created.GetId() == "" { + target.Execution = &ExecutionResult{ + RunID: e.runID, + Failure: ErrActionMissingCreatedTargetID.Error(), + } + return fmt.Errorf("%w: action %q target %q", ErrActionMissingCreatedTargetID, actionPlan.Source.GetId(), namespaceLabel(target.Namespace)) + } + + target.Execution = &ExecutionResult{ + RunID: e.runID, + Applied: true, + CreatedTargetID: created.GetId(), + } + e.setActionTargetID(actionPlan.Source.GetId(), target.Namespace, created.GetId()) + case TargetStatusUnresolved: + // ! Note: This should never really happen, as the validatePlan should be run before this; good defensive check though. + return fmt.Errorf("%w: action %q target %q is unresolved: %s", ErrPlanNotExecutable, actionPlan.Source.GetId(), namespaceLabel(target.Namespace), target.Reason) + default: + return fmt.Errorf("%w: action %q target %q has unsupported status %q", ErrActionUnsupportedStatus, actionPlan.Source.GetId(), namespaceLabel(target.Namespace), target.Status) + } + } + } + + return nil +} + +func (e *Executor) executeSubjectConditionSets(_ context.Context, plan *Plan) error { + if plan == nil || len(plan.SubjectConditionSets) == 0 { + return nil + } + + return fmt.Errorf("%w: %s", ErrExecutionPhaseNotImplemented, ScopeSubjectConditionSets) +} + +func (e *Executor) executeSubjectMappings(_ context.Context, plan *Plan) error { + if plan == nil || len(plan.SubjectMappings) == 0 { + return nil + } + + return fmt.Errorf("%w: %s", ErrExecutionPhaseNotImplemented, ScopeSubjectMappings) +} + +func (e *Executor) executeRegisteredResources(_ context.Context, plan *Plan) error { + if plan == nil || len(plan.RegisteredResources) == 0 { + return nil + } + + return fmt.Errorf("%w: %s", ErrExecutionPhaseNotImplemented, ScopeRegisteredResources) +} + +func (e *Executor) executeObligationTriggers(_ context.Context, plan *Plan) error { + if plan == nil || len(plan.ObligationTriggers) == 0 { + return nil + } + + return fmt.Errorf("%w: %s", ErrExecutionPhaseNotImplemented, ScopeObligationTriggers) +} diff --git a/otdfctl/migrations/namespacedpolicy/execute_test.go b/otdfctl/migrations/namespacedpolicy/execute_test.go new file mode 100644 index 0000000000..46cea91385 --- /dev/null +++ b/otdfctl/migrations/namespacedpolicy/execute_test.go @@ -0,0 +1,352 @@ +package namespacedpolicy + +import ( + "context" + "errors" + "testing" + + "github.com/opentdf/platform/protocol/go/common" + "github.com/opentdf/platform/protocol/go/policy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var errMissingMockActionResult = errors.New("missing mock action result") + +type mockExecutorHandler struct { + created map[string]map[string]*createdActionCall + results map[string]map[string]*policy.Action + errs map[string]map[string]error +} + +type createdActionCall struct { + Name string + Namespace string + Metadata *common.MetadataMutable +} + +func (m *mockExecutorHandler) CreateAction(_ context.Context, name string, namespace string, metadata *common.MetadataMutable) (*policy.Action, error) { + if m.created == nil { + m.created = make(map[string]map[string]*createdActionCall) + } + if m.created[name] == nil { + m.created[name] = make(map[string]*createdActionCall) + } + + m.created[name][namespace] = &createdActionCall{ + Name: name, + Namespace: namespace, + Metadata: metadata, + } + + if m.errs != nil && m.errs[name] != nil { + if err := m.errs[name][namespace]; err != nil { + return nil, err + } + } + if m.results != nil && m.results[name] != nil { + if result := m.results[name][namespace]; result != nil { + return result, nil + } + } + + return nil, errMissingMockActionResult +} + +func TestExecutorExecute(t *testing.T) { + t.Parallel() + + namespace1 := &policy.Namespace{Id: "ns-1", Fqn: "https://example.com"} + namespace2 := &policy.Namespace{Id: "ns-2", Fqn: "https://example.net"} + namespace3 := &policy.Namespace{Id: "ns-3", Fqn: "https://example.org"} + + tests := []struct { + name string + plan *Plan + handler *mockExecutorHandler + runID string + wantErrIs error + wantErr string + assert func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, plan *Plan) + }{ + { + name: "handles created, existing, and already migrated action targets", + plan: &Plan{ + Scopes: []Scope{ScopeActions}, + Actions: []*ActionPlan{ + { + Source: &policy.Action{ + Id: "action-1", + Name: "decrypt", + Metadata: &common.Metadata{ + Labels: map[string]string{ + "owner": "policy-team", + "env": "dev", + }, + }, + }, + Targets: []*ActionTargetPlan{ + { + Namespace: namespace1, + Status: TargetStatusCreate, + }, + { + Namespace: namespace2, + Status: TargetStatusExistingStandard, + Existing: &policy.Action{Id: "standard-action"}, + }, + { + Namespace: namespace3, + Status: TargetStatusAlreadyMigrated, + Existing: &policy.Action{Id: "migrated-action"}, + }, + }, + }, + }, + }, + handler: &mockExecutorHandler{ + results: map[string]map[string]*policy.Action{ + "decrypt": { + "ns-1": {Id: "created-action-1", Name: "decrypt"}, + }, + }, + }, + runID: "run-123", + assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, plan *Plan) { + t.Helper() + + require.NoError(t, err) + require.Contains(t, handler.created, "decrypt") + require.Contains(t, handler.created["decrypt"], "ns-1") + assert.Len(t, handler.created["decrypt"], 1) + assert.Equal(t, "decrypt", handler.created["decrypt"]["ns-1"].Name) + assert.Equal(t, "ns-1", handler.created["decrypt"]["ns-1"].Namespace) + assert.Equal(t, map[string]string{ + "owner": "policy-team", + "env": "dev", + migrationLabelMigratedFrom: "action-1", + migrationLabelRun: "run-123", + }, handler.created["decrypt"]["ns-1"].Metadata.GetLabels()) + + createdTarget := plan.Actions[0].Targets[0] + assert.Equal(t, TargetStatusCreate, createdTarget.Status) + assert.Nil(t, createdTarget.Existing) + require.NotNil(t, createdTarget.Execution) + assert.True(t, createdTarget.Execution.Applied) + assert.Equal(t, "created-action-1", createdTarget.Execution.CreatedTargetID) + assert.Equal(t, "run-123", createdTarget.Execution.RunID) + assert.Equal(t, "created-action-1", createdTarget.TargetID()) + + existingTarget := plan.Actions[0].Targets[1] + assert.Equal(t, "standard-action", existingTarget.TargetID()) + + migratedTarget := plan.Actions[0].Targets[2] + assert.Equal(t, "migrated-action", migratedTarget.TargetID()) + + assert.Equal(t, "created-action-1", executor.actionTargetID("action-1", namespace1)) + assert.Equal(t, "standard-action", executor.actionTargetID("action-1", namespace2)) + assert.Equal(t, "migrated-action", executor.actionTargetID("action-1", namespace3)) + assert.Empty(t, executor.actionTargetID("action-2", namespace1)) + }, + }, + { + name: "returns not executable for unresolved target status", + plan: &Plan{ + Scopes: []Scope{ScopeActions}, + Actions: []*ActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + { + Namespace: namespace1, + Status: TargetStatusUnresolved, + Reason: "missing target namespace mapping", + }, + }, + }, + }, + }, + handler: &mockExecutorHandler{}, + wantErrIs: ErrPlanNotExecutable, + wantErr: `action "action-1" target "ns-1" is unresolved: missing target namespace mapping`, + assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { + t.Helper() + + require.Error(t, err) + assert.Empty(t, handler.created) + assert.Empty(t, executor.actionTargetID("action-1", namespace1)) + }, + }, + { + name: "returns error for missing existing standard target id", + plan: &Plan{ + Scopes: []Scope{ScopeActions}, + Actions: []*ActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + { + Namespace: namespace1, + Status: TargetStatusExistingStandard, + }, + }, + }, + }, + }, + handler: &mockExecutorHandler{}, + wantErrIs: ErrActionMissingExistingTarget, + wantErr: `action "action-1" target "ns-1"`, + assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { + t.Helper() + + require.Error(t, err) + assert.Empty(t, handler.created) + assert.Empty(t, executor.actionTargetID("action-1", namespace1)) + }, + }, + { + name: "returns error for missing already migrated target id", + plan: &Plan{ + Scopes: []Scope{ScopeActions}, + Actions: []*ActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + { + Namespace: namespace1, + Status: TargetStatusAlreadyMigrated, + }, + }, + }, + }, + }, + handler: &mockExecutorHandler{}, + wantErrIs: ErrActionMissingMigratedTarget, + wantErr: `action "action-1" target "ns-1"`, + assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { + t.Helper() + + require.Error(t, err) + assert.Empty(t, handler.created) + assert.Empty(t, executor.actionTargetID("action-1", namespace1)) + }, + }, + { + name: "returns error for missing target namespace", + plan: &Plan{ + Scopes: []Scope{ScopeActions}, + Actions: []*ActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + { + Status: TargetStatusCreate, + }, + }, + }, + }, + }, + handler: &mockExecutorHandler{}, + wantErrIs: ErrActionMissingTargetNamespace, + wantErr: `action "action-1"`, + assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { + t.Helper() + + require.Error(t, err) + assert.Empty(t, handler.created) + assert.Empty(t, executor.actionTargetID("action-1", nil)) + }, + }, + { + name: "returns error for missing created target id", + plan: &Plan{ + Scopes: []Scope{ScopeActions}, + Actions: []*ActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + { + Namespace: namespace1, + Status: TargetStatusCreate, + }, + }, + }, + }, + }, + handler: &mockExecutorHandler{ + results: map[string]map[string]*policy.Action{ + "decrypt": { + "ns-1": {}, + }, + }, + }, + wantErrIs: ErrActionMissingCreatedTargetID, + wantErr: `action "action-1" target "ns-1"`, + assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, plan *Plan) { + t.Helper() + + require.Error(t, err) + require.Contains(t, handler.created, "decrypt") + require.NotNil(t, plan.Actions[0].Targets[0].Execution) + assert.Equal(t, ErrActionMissingCreatedTargetID.Error(), plan.Actions[0].Targets[0].Execution.Failure) + assert.Empty(t, executor.actionTargetID("action-1", namespace1)) + }, + }, + { + name: "returns error for unsupported target status", + plan: &Plan{ + Scopes: []Scope{ScopeActions}, + Actions: []*ActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + { + Namespace: namespace1, + Status: TargetStatus("bogus"), + }, + }, + }, + }, + }, + handler: &mockExecutorHandler{}, + wantErrIs: ErrActionUnsupportedStatus, + wantErr: `action "action-1" target "ns-1" has unsupported status "bogus"`, + assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { + t.Helper() + + require.Error(t, err) + assert.Empty(t, handler.created) + assert.Empty(t, executor.actionTargetID("action-1", namespace1)) + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + executor, err := NewExecutor(tt.handler) + require.NoError(t, err) + if tt.runID != "" { + executor.runID = tt.runID + } + + err = executor.Execute(t.Context(), tt.plan) + switch { + case tt.wantErrIs != nil: + require.Error(t, err) + require.ErrorIs(t, err, tt.wantErrIs) + case tt.wantErr != "": + require.Error(t, err) + default: + require.NoError(t, err) + } + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + } + + tt.assert(t, err, executor, tt.handler, tt.plan) + }) + } +} diff --git a/otdfctl/migrations/namespacedpolicy/plan.go b/otdfctl/migrations/namespacedpolicy/plan.go index e653d66391..430ba56a35 100644 --- a/otdfctl/migrations/namespacedpolicy/plan.go +++ b/otdfctl/migrations/namespacedpolicy/plan.go @@ -48,6 +48,13 @@ const ( TargetStatusUnresolved TargetStatus = "unresolved" ) +type ExecutionResult struct { + RunID string `json:"run_id,omitempty"` + Applied bool `json:"applied,omitempty"` + CreatedTargetID string `json:"created_target_id,omitempty"` + Failure string `json:"failure,omitempty"` +} + type ActionPlan struct { Source *policy.Action `json:"source"` // TODO: Add analogous reference metadata for other policy object plan types @@ -76,6 +83,7 @@ type ActionTargetPlan struct { Namespace *policy.Namespace `json:"namespace"` Status TargetStatus `json:"status"` Existing *policy.Action `json:"existing,omitempty"` + Execution *ExecutionResult `json:"execution,omitempty"` Reason string `json:"reason,omitempty"` } @@ -300,7 +308,13 @@ func sameNamespace(left, right *policy.Namespace) bool { } func (t *ActionTargetPlan) TargetID() string { - if t == nil || t.Existing == nil { + if t == nil { + return "" + } + if t.Execution != nil && t.Execution.CreatedTargetID != "" { + return t.Execution.CreatedTargetID + } + if t.Existing == nil { return "" } return t.Existing.GetId() From f9b0a15a250b3ddd7af3f8e9e1bb24b1ce051883 Mon Sep 17 00:00:00 2001 From: Chris Reed Date: Tue, 14 Apr 2026 08:54:55 -0500 Subject: [PATCH 2/4] gemini comment. --- otdfctl/migrations/namespacedpolicy/execute.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/otdfctl/migrations/namespacedpolicy/execute.go b/otdfctl/migrations/namespacedpolicy/execute.go index 23760a00b3..75a0d520d8 100644 --- a/otdfctl/migrations/namespacedpolicy/execute.go +++ b/otdfctl/migrations/namespacedpolicy/execute.go @@ -170,17 +170,15 @@ func (e *Executor) executeActions(ctx context.Context, actionPlans []*ActionPlan } switch target.Status { - case TargetStatusExistingStandard: + case TargetStatusExistingStandard, TargetStatusAlreadyMigrated: if target.TargetID() == "" { - return fmt.Errorf("%w: action %q target %q", ErrActionMissingExistingTarget, actionPlan.Source.GetId(), namespaceLabel(target.Namespace)) - } - e.setActionTargetID(actionPlan.Source.GetId(), target.Namespace, target.TargetID()) - case TargetStatusAlreadyMigrated: - if target.TargetID() == "" { - return fmt.Errorf("%w: action %q target %q", ErrActionMissingMigratedTarget, actionPlan.Source.GetId(), namespaceLabel(target.Namespace)) + errKind := ErrActionMissingExistingTarget + if target.Status == TargetStatusAlreadyMigrated { + errKind = ErrActionMissingMigratedTarget + } + return fmt.Errorf("%w: action %q target %q", errKind, actionPlan.Source.GetId(), namespaceLabel(target.Namespace)) } e.setActionTargetID(actionPlan.Source.GetId(), target.Namespace, target.TargetID()) - continue case TargetStatusCreate: namespace := namespaceIdentifier(target.Namespace) if namespace == "" { @@ -204,7 +202,7 @@ func (e *Executor) executeActions(ctx context.Context, actionPlans []*ActionPlan } return fmt.Errorf("create action %q in namespace %q: %w", actionPlan.Source.GetId(), namespaceLabel(target.Namespace), err) } - if created == nil || created.GetId() == "" { + if created.GetId() == "" { target.Execution = &ExecutionResult{ RunID: e.runID, Failure: ErrActionMissingCreatedTargetID.Error(), From 78324f4d9e56247c6281ae9a066f27fa5c1c3959 Mon Sep 17 00:00:00 2001 From: Chris Reed Date: Tue, 14 Apr 2026 09:15:33 -0500 Subject: [PATCH 3/4] clean up error code. --- .../namespacedpolicy/execute_test.go | 76 +++++++++++-------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/otdfctl/migrations/namespacedpolicy/execute_test.go b/otdfctl/migrations/namespacedpolicy/execute_test.go index 46cea91385..e836469f73 100644 --- a/otdfctl/migrations/namespacedpolicy/execute_test.go +++ b/otdfctl/migrations/namespacedpolicy/execute_test.go @@ -3,6 +3,7 @@ package namespacedpolicy import ( "context" "errors" + "fmt" "testing" "github.com/opentdf/platform/protocol/go/common" @@ -13,6 +14,18 @@ import ( var errMissingMockActionResult = errors.New("missing mock action result") +type expectedError struct { + is error + message string +} + +func wantError(is error, format string, args ...any) *expectedError { + return &expectedError{ + is: is, + message: fmt.Sprintf("%s: %s", is, fmt.Sprintf(format, args...)), + } +} + type mockExecutorHandler struct { created map[string]map[string]*createdActionCall results map[string]map[string]*policy.Action @@ -61,13 +74,12 @@ func TestExecutorExecute(t *testing.T) { namespace3 := &policy.Namespace{Id: "ns-3", Fqn: "https://example.org"} tests := []struct { - name string - plan *Plan - handler *mockExecutorHandler - runID string - wantErrIs error - wantErr string - assert func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, plan *Plan) + name string + plan *Plan + handler *mockExecutorHandler + runID string + wantErr *expectedError + assert func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, plan *Plan) }{ { name: "handles created, existing, and already migrated action targets", @@ -166,9 +178,14 @@ func TestExecutorExecute(t *testing.T) { }, }, }, - handler: &mockExecutorHandler{}, - wantErrIs: ErrPlanNotExecutable, - wantErr: `action "action-1" target "ns-1" is unresolved: missing target namespace mapping`, + handler: &mockExecutorHandler{}, + wantErr: wantError( + ErrPlanNotExecutable, + `action %q target %q is unresolved: %s`, + "action-1", + "ns-1", + "missing target namespace mapping", + ), assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { t.Helper() @@ -193,9 +210,8 @@ func TestExecutorExecute(t *testing.T) { }, }, }, - handler: &mockExecutorHandler{}, - wantErrIs: ErrActionMissingExistingTarget, - wantErr: `action "action-1" target "ns-1"`, + handler: &mockExecutorHandler{}, + wantErr: wantError(ErrActionMissingExistingTarget, `action %q target %q`, "action-1", "ns-1"), assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { t.Helper() @@ -220,9 +236,8 @@ func TestExecutorExecute(t *testing.T) { }, }, }, - handler: &mockExecutorHandler{}, - wantErrIs: ErrActionMissingMigratedTarget, - wantErr: `action "action-1" target "ns-1"`, + handler: &mockExecutorHandler{}, + wantErr: wantError(ErrActionMissingMigratedTarget, `action %q target %q`, "action-1", "ns-1"), assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { t.Helper() @@ -246,9 +261,8 @@ func TestExecutorExecute(t *testing.T) { }, }, }, - handler: &mockExecutorHandler{}, - wantErrIs: ErrActionMissingTargetNamespace, - wantErr: `action "action-1"`, + handler: &mockExecutorHandler{}, + wantErr: wantError(ErrActionMissingTargetNamespace, `action %q`, "action-1"), assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { t.Helper() @@ -280,8 +294,7 @@ func TestExecutorExecute(t *testing.T) { }, }, }, - wantErrIs: ErrActionMissingCreatedTargetID, - wantErr: `action "action-1" target "ns-1"`, + wantErr: wantError(ErrActionMissingCreatedTargetID, `action %q target %q`, "action-1", "ns-1"), assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, plan *Plan) { t.Helper() @@ -308,9 +321,14 @@ func TestExecutorExecute(t *testing.T) { }, }, }, - handler: &mockExecutorHandler{}, - wantErrIs: ErrActionUnsupportedStatus, - wantErr: `action "action-1" target "ns-1" has unsupported status "bogus"`, + handler: &mockExecutorHandler{}, + wantErr: wantError( + ErrActionUnsupportedStatus, + `action %q target %q has unsupported status %q`, + "action-1", + "ns-1", + TargetStatus("bogus"), + ), assert: func(t *testing.T, err error, executor *Executor, handler *mockExecutorHandler, _ *Plan) { t.Helper() @@ -334,17 +352,13 @@ func TestExecutorExecute(t *testing.T) { err = executor.Execute(t.Context(), tt.plan) switch { - case tt.wantErrIs != nil: - require.Error(t, err) - require.ErrorIs(t, err, tt.wantErrIs) - case tt.wantErr != "": + case tt.wantErr != nil: require.Error(t, err) + require.ErrorIs(t, err, tt.wantErr.is) + require.EqualError(t, err, tt.wantErr.message) default: require.NoError(t, err) } - if tt.wantErr != "" { - require.ErrorContains(t, err, tt.wantErr) - } tt.assert(t, err, executor, tt.handler, tt.plan) }) From 6ba76481a0cb20f577ad2d48d287bb45c3f8b2b8 Mon Sep 17 00:00:00 2001 From: Chris Reed Date: Tue, 14 Apr 2026 09:16:18 -0500 Subject: [PATCH 4/4] execute test. --- otdfctl/migrations/namespacedpolicy/execute_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/otdfctl/migrations/namespacedpolicy/execute_test.go b/otdfctl/migrations/namespacedpolicy/execute_test.go index e836469f73..e436ff9a35 100644 --- a/otdfctl/migrations/namespacedpolicy/execute_test.go +++ b/otdfctl/migrations/namespacedpolicy/execute_test.go @@ -66,7 +66,7 @@ func (m *mockExecutorHandler) CreateAction(_ context.Context, name string, names return nil, errMissingMockActionResult } -func TestExecutorExecute(t *testing.T) { +func ExecuteActions(t *testing.T) { t.Parallel() namespace1 := &policy.Namespace{Id: "ns-1", Fqn: "https://example.com"}