Skip to content

fix(cli): Pull namespace for OTs from attr val - #3725

Merged
c-r33d merged 2 commits into
mainfrom
fix-obligation-trigger-bug
Jul 6, 2026
Merged

fix(cli): Pull namespace for OTs from attr val#3725
c-r33d merged 2 commits into
mainfrom
fix-obligation-trigger-bug

Conversation

@c-r33d

@c-r33d c-r33d commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes namespaced policy migration for obligation triggers so the target namespace is derived from the trigger’s attribute value, not the obligation
value.

Changes

  • Updated obligation trigger target derivation in otdfctl namespaced-policy migration.
  • Removed the now-unused obligation-value namespace helper.
  • Added/updated Go tests for cross-namespace obligation trigger derivation.
  • Updated BATS coverage for migration and prune scenarios where attribute value and obligation value are in different namespaces.

Summary by CodeRabbit

  • Bug Fixes

    • Updated namespaced-policy migration behavior so obligation triggers are placed in the correct namespace based on their attribute values.
    • Improved migration coverage for cross-namespace trigger cases, including legacy trigger preservation and idempotent re-runs.
    • Adjusted prune scenarios to better match mixed-namespace policy setups.
  • Tests

    • Expanded end-to-end and unit test coverage for obligation-trigger migration and namespace resolution.
    • Added a new test to verify trigger targeting uses attribute-based namespace data.

@c-r33d
c-r33d requested a review from a team as a code owner July 6, 2026 16:14
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the namespaced policy migration process for obligation triggers. By shifting the namespace derivation source from the obligation value to the trigger's attribute value, the system now correctly handles cases where these values reside in different namespaces. This change ensures more accurate policy migrations and includes updated test coverage to validate the new derivation logic.

Highlights

  • Obligation Trigger Namespace Derivation: Updated the namespaced-policy migration logic to derive the target namespace from the trigger's attribute value instead of the obligation value.
  • Code Cleanup: Removed the now-obsolete namespaceFromObligationValue helper function.
  • Testing and Coverage: Added new Go unit tests to verify cross-namespace obligation trigger derivation and updated BATS integration tests to cover migration and pruning scenarios.
New Features

🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.


The namespace was hard to define, / Derived from the wrong dotted line. / With attributes set, / The target is met, / And policies now will align.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions github-actions Bot added the size/s label Jul 6, 2026
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR changes namespace resolution for obligation triggers in the namespacedpolicy migration planner to derive the target namespace from the trigger's attribute value instead of its obligation value, removes the now-unused helper, and updates unit and E2E test fixtures/assertions accordingly.

Changes

Obligation-trigger namespace derivation

Layer / File(s) Summary
Namespace derivation logic change
otdfctl/migrations/namespacedpolicy/derived.go, otdfctl/migrations/namespacedpolicy/migration_plan.go
deriveObligationTrigger now resolves the target namespace via namespaceFromAttributeValue(trigger.GetAttributeValue()) instead of namespaceFromObligationValue(trigger.GetObligationValue()); the now-unused namespaceFromObligationValue helper is removed.
Unit test coverage for attribute-value-based namespace
otdfctl/migrations/namespacedpolicy/derived_test.go, otdfctl/migrations/namespacedpolicy/migration_planner_test.go
Adds a new test asserting the derived namespace comes from AttributeValue rather than Obligation, and updates existing ObligationTrigger fixtures across derivation and planner tests to include AttributeValue.
E2E migration and prune test updates
otdfctl/e2e/migrate-namespaced-policy.bats
Extends the obligation-trigger migration test with an additional legacy trigger migrated into NS_B, adds corresponding assertions and split idempotency tracking, and relocates prune-test fixtures (delete_b_obligation_id, not_migrated_source_obligation_id) to NS_B_ID.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • opentdf/platform#3432: Both PRs change obligation-trigger namespace derivation to come from the trigger's attribute value rather than the obligation value, updating related creation/planning logic and tests.
  • opentdf/platform#3297: Both PRs touch the namespacedpolicy dry-run planning pipeline's obligation-trigger derivation and namespace-resolution helpers.
  • opentdf/platform#3363: Both PRs modify the same migrate-namespaced-policy.bats obligation-trigger coverage and fixtures with updated migrated/legacy trigger assertions.

Suggested labels: comp:policy

Suggested reviewers: jakedoublev, elizabethhealy

A rabbit hops through namespace trees,
No longer bound by obligations' keys,
Attribute values now light the way,
Triggers migrate to where they should stay.
🐇✨ NS_B blooms anew,
Tests confirm it's all true! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: deriving obligation trigger namespaces from attribute values.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-obligation-trigger-bug

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

❤️ Share

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the namespaced policy migration logic to derive the namespace of an obligation trigger from its attribute value instead of its obligation value. The unused helper function namespaceFromObligationValue has been removed, and both unit and end-to-end tests have been updated to reflect and verify this change. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 197.277137ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 91.916495ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 436.599367ms
Throughput 229.04 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 44.618964259s
Average Latency 444.760092ms
Throughput 112.06 requests/second

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 216.021741ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 110.5873ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 422.451579ms
Throughput 236.71 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 46.290019554s
Average Latency 460.920145ms
Throughput 108.01 requests/second

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • examples
  • otdfctl
  • sdk
  • service
  • lib/fixtures
  • tests-bdd

See the workflow run for details.

@c-r33d c-r33d changed the title fix(cli): Namespace migration too restrictive for OTs. fix(cli): Pull namespace for OTs from attr val Jul 6, 2026
@c-r33d
c-r33d added this pull request to the merge queue Jul 6, 2026
Merged via the queue into main with commit 2146d0f Jul 6, 2026
47 checks passed
@c-r33d
c-r33d deleted the fix-obligation-trigger-bug branch July 6, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants