test: cross-SDK key-mapping split parity (scaffold)#541
Conversation
Add a key_mapping_resolution platform feature gate and a parity test asserting a go SDK that resolves key splits via GetKeyMappingsByFqns produces the same TDF key-access split structure as a previous released go SDK, with cross-decrypt. Covers value-level mapped keys; definition/namespace levels and any_of/hierarchy across distinct KASes are TODO (need key_assign_attribute/key_assign_namespace otdfctl helpers). Skips until the platform exposes the RPC and a new go SDK build is present alongside a released one. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
|
Important Review skippedDraft detected. 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the "key_mapping_resolution" feature flag gated on platform version 0.14.0 and adds a cross-SDK parity test to verify that the new GetKeyMappingsByFqns RPC produces the same key-access split structure as previous released SDKs. The feedback suggests defining the missing attribute_with_different_kids fixture to avoid a test failure, sorting the released SDK versions to ensure deterministic selection, and using a deep byte-by-byte comparison (shallow=False) in filecmp.cmp to prevent false positives.
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.
| def test_key_mapping_split_parity_value_level( | ||
| attribute_with_different_kids: Attribute, | ||
| pt_file: Path, | ||
| tmp_dir: Path, | ||
| ): |
There was a problem hiding this comment.
The test function test_key_mapping_split_parity_value_level requests a fixture named attribute_with_different_kids, but this fixture is not defined anywhere in the codebase (including xtest/fixtures/attributes.py or other loaded fixture plugins). Running this test will result in a FixtureLookupError from pytest. Please define this fixture in xtest/fixtures/attributes.py or ensure it is imported correctly.
| def _go_sdk_pair() -> tuple[tdfs.SDK, tdfs.SDK]: | ||
| go = tdfs.all_versions_of("go") | ||
| new = next((s for s in go if not s.is_released()), None) | ||
| prev = next((s for s in go if s.is_released()), None) | ||
| if new is None or prev is None: | ||
| pytest.skip("requires both a new (main/sha) and a previous released go SDK build") | ||
| return new, prev |
There was a problem hiding this comment.
In _go_sdk_pair(), next((s for s in go if s.is_released()), None) is used to retrieve a released version of the Go SDK. Since os.listdir (used by tdfs.all_versions_of) returns directory entries in an arbitrary order, this will non-deterministically select any released version present in the directory. To ensure deterministic and correct test behavior, the released versions should be sorted (e.g., by semantic version) to consistently select the latest released version.
def _go_sdk_pair() -> tuple[tdfs.SDK, tdfs.SDK]:
go = tdfs.all_versions_of("go")
new = next((s for s in go if not s.is_released()), None)
released = [s for s in go if s.is_released()]
prev = None
if released:
released.sort(key=lambda s: tdfs._parse_semver(s.version) or (0, 0, 0), reverse=True)
prev = released[0]
if new is None or prev is None:
pytest.skip("requires both a new (main/sha) and a previous released go SDK build")
return new, prev| for ct, decrypt_sdk in ((new_ct, prev_sdk), (prev_ct, new_sdk)): | ||
| rt = tmp_dir / f"{ct.stem}.untdf" | ||
| decrypt_sdk.decrypt(ct, rt, "ztdf") | ||
| assert filecmp.cmp(pt_file, rt) |
There was a problem hiding this comment.
By default, filecmp.cmp uses a shallow comparison (shallow=True), which only compares the os.stat signature (size, mtime, etc.) of the files. In fast-running test environments where files are created and modified rapidly, this can lead to false positives due to identical stat metadata. It is safer to use shallow=False to force a byte-by-byte content comparison.
| for ct, decrypt_sdk in ((new_ct, prev_sdk), (prev_ct, new_sdk)): | |
| rt = tmp_dir / f"{ct.stem}.untdf" | |
| decrypt_sdk.decrypt(ct, rt, "ztdf") | |
| assert filecmp.cmp(pt_file, rt) | |
| for ct, decrypt_sdk in ((new_ct, prev_sdk), (prev_ct, new_sdk)): | |
| rt = tmp_dir / f"{ct.stem}.untdf" | |
| decrypt_sdk.decrypt(ct, rt, "ztdf") | |
| assert filecmp.cmp(pt_file, rt, shallow=False) |
The server now resolves legacy grants inside GetKeyMappingsByFqns (grants with a cached key are converted to keys), so the parity story covers grant-configured policy too. Broaden the parity test from a single-KAS multi-KID case to also cover two-KAS ANY_OF (share) and multi-value ALL_OF (split), which exercise the rule-driven combine logic through the new resolution path. Document that legacy grants auto-convert to mapped keys on key_management platforms, so a dedicated legacy-grant-only case remains a TODO. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
|
1 participant
What
Scaffolds the cross-SDK parity test requested for the new narrow attribute read APIs (opentdf/platform): proving a go SDK that resolves key splits via
GetKeyMappingsByFqnsproduces the same TDF key-access split structure as a previous released go SDK (which walks the full attribute set), and that the two containers cross-decrypt.Changes
tdfs.py: newkey_mapping_resolutionplatform feature (gates the test; TODO: pin the exact platform release that shipsGetKeyMappingsByFqns— currently guarded at>= 0.14.0conservatively).test_abac_key_mapping_parity.py:test_key_mapping_split_parity_value_level— encrypts the same plaintext with a new (main/sha) and a previous released go SDK over a value-level keyed attribute, then compares split structure (key-access objects grouped by split id → set of(kas_url, kid)) and cross-decrypts.Status — DRAFT, not yet validated end-to-end
This cannot pass until both land and a build is available, so it skips otherwise:
GetKeyMappingsByFqns— feat(policy): add narrow attribute read API protos and generated code platform#3634 (proto) + #3697 (service).sdk/go/distalongside a previous released go SDK.Validated here only at the unit level: syntax +
ruff. It has not been run against a live stack.Follow-ups
key_mapping_resolutionto the real release version once #3634 ships.key_assign_attribute/key_assign_namespacehelpers added tootdfctl.py(today onlykey_assign_valueexists) and multi-KAS keyed fixtures.