Skip to content

Conversation

@elizabethhealy
Copy link
Member

Proposed Changes

  • the auth tests have been failing intermittently since the addition of connect rpc due to what appears to be a race condition on the server close, separate the connect rpc and grpc tests for the auth interceptors

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

@github-actions github-actions bot added comp:sdk A software development kit, including library, for client applications and inter-service communicati size/xs labels Jun 2, 2025
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Hello @elizabethhealy, 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!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed.

This PR aims to address intermittent CI failures specifically within the authentication tests. According to the description, these failures seem to stem from a race condition that arose after the introduction of Connect RPC, potentially related to server teardown in the tests. To mitigate this, the changes involve separating the existing combined tests for the token adding interceptor into distinct tests for gRPC and Connect protocols. This refactoring should help isolate the test environments and prevent the race condition.

Highlights

  • CI Stability: Fixes intermittent failures observed in CI runs, specifically within the authentication test suite.
  • Race Condition Mitigation: Addresses a suspected race condition during test server shutdown by separating test execution paths.
  • Test Refactoring: Splits existing tests that covered both gRPC and Connect into dedicated tests for each protocol to improve reliability and clarity.
  • Test Helpers: Introduces helper functions to streamline test setup and assertion logic, making the tests more maintainable.

Changelog

  • sdk/auth/token_adding_interceptor_test.go
    • Renamed TestAddingTokensToOutgoingRequest to setupTokenAddingInterceptor and refactored it into a test setup helper function (lines 34-52).
    • Extracted the token and DPoP assertion logic into a new helper function checkAccessAndDpopTokens (lines 55-95).
    • Modified the original TestAddingTokensToOutgoingRequest to specifically test the gRPC path using the new helpers (lines 97-109).
    • Added a new test TestAddingTokensToOutgoingRequest_Connect to specifically test the Connect path using the new helpers (lines 111-122).
    • Modified Test_InvalidCredentials_DoesNotSendMessage to specifically test the gRPC path (lines 124-136).
    • Added a new test Test_InvalidCredentials_DoesNotSendMessage_Connect to specifically test the Connect path (lines 138-150).
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize 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 is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Tests fail now and then,
A race condition, oh when?
Split the protocols,
Fix the test goals,
CI green once again.

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.

Copy link
Contributor

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

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 effectively addresses the intermittent test failures by separating the gRPC and Connect RPC tests for the auth interceptors. The refactoring into helper functions setupTokenAddingInterceptor and checkAccessAndDpopTokens significantly improves the readability and maintainability of the test suite by reducing code duplication. Overall, the changes are well-implemented and directly target the described issue.

Summary of Findings

  • Minor inconsistencies in test helper function arguments and usage: The checkAccessAndDpopTokens function expects slice arguments (accessToken []string, dpopToken []string) but the fake Connect server implementation wraps a single string header value in a slice before passing it, while the fake gRPC server naturally returns a slice. While this works for the current test structure, it's a minor inconsistency. Additionally, the function immediately asserts the length of the dpopToken slice is 1, implying it always expects a single token. (Note: These points were identified but not included as review comments due to the configured severity filter.)
  • Ignored parsing errors in test helper: The jws.Parse and jwt.Parse calls within checkAccessAndDpopTokens ignore potential parsing errors using the blank identifier _. While acceptable in a test where inputs are controlled, it's generally good practice to handle or assert errors even in tests. (Note: This point was identified but not included as a review comment due to the configured severity filter.)

Merge Readiness

The code changes appear correct and effectively resolve the intermittent test failures by improving test isolation and structure. The refactoring enhances test maintainability. Based on my review, the pull request looks ready to be merged. Please ensure other required reviewers have approved before merging, as I am unable to approve the pull request directly.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 482.938311ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 311.967962ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 348.827495ms
Throughput 286.67 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 53.160060435s
Average Latency 528.197113ms
Throughput 94.06 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4972
Failed Requests 28
Concurrent Requests 50
Total Time 44.176043524s
Average Latency 438.991847ms
Throughput 112.55 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
28 occurrences

Standard Benchmark Metrics Skipped or Failed

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 465.743006ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 278.238754ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 352.252941ms
Throughput 283.89 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 52.04538888s
Average Latency 517.702253ms
Throughput 96.07 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4975
Failed Requests 25
Concurrent Requests 50
Total Time 44.074569124s
Average Latency 437.657006ms
Throughput 112.88 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
25 occurrences

Standard Benchmark Metrics Skipped or Failed

@elizabethhealy elizabethhealy marked this pull request as ready for review June 2, 2025 17:30
@elizabethhealy elizabethhealy requested review from a team as code owners June 2, 2025 17:31
@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Jun 3, 2025
Merged via the queue into main with commit 395988a Jun 3, 2025
28 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the fix-auth-interceptor-test-failures branch June 3, 2025 13:17
github-merge-queue bot pushed a commit that referenced this pull request Jun 23, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.5.0](sdk/v0.4.7...sdk/v0.5.0)
(2025-06-23)


### Features

* add system metadata assertions to TDFConfig
([#2446](#2446))
([4eb9fff](4eb9fff))
* **core:** DSPX-608 - Deprecate public_client_id
([#2185](#2185))
([0f58efa](0f58efa))
* **sdk:** adds seeker interface to TDF Reader
([#2385](#2385))
([63ccd9a](63ccd9a))
* **sdk:** Allow key splits with same algo
([#2454](#2454))
([7422b15](7422b15))
* **sdk:** autoconfig kaos with kids
([#2438](#2438))
([c272016](c272016))
* **sdk:** Enable base key support.
([#2425](#2425))
([9ff3806](9ff3806))


### Bug Fixes

* **ci:** Fix intermittent failures from auth tests
([#2345](#2345))
([395988a](395988a))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to
0.4.0 in /sdk ([#2397](#2397))
([99e3aa4](99e3aa4))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to
0.5.0 in /sdk ([#2471](#2471))
([e8f97e0](e8f97e0))
* **deps:** bump the external group across 1 directory with 5 updates
([#2400](#2400))
([0b7ea79](0b7ea79))
* set consistent system metadata id and schema
([#2451](#2451))
([5db3cf2](5db3cf2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.7.0](sdk/v0.6.1...sdk/v0.7.0)
(2025-08-25)


### ⚠ BREAKING CHANGES

* **core:** Require go 1.23+
([#1979](#1979))

### Features

* add system metadata assertions to TDFConfig
([#2446](#2446))
([4eb9fff](4eb9fff))
* **authz:** authz v2 versioning implementation
([#2173](#2173))
([557fc21](557fc21))
* **core:** adds bulk rewrap to sdk and service
([#1835](#1835))
([11698ae](11698ae))
* **core:** Adds EC withSalt options
([#2126](#2126))
([67b6fb8](67b6fb8))
* **core:** Adds ErrInvalidPerSchema
([#1860](#1860))
([456639e](456639e))
* **core:** DSPX-608 - Deprecate public_client_id
([#2185](#2185))
([0f58efa](0f58efa))
* **core:** EXPERIMENTAL: EC-wrapped key support
([#1902](#1902))
([652266f](652266f))
* **core:** Expose version info
([#1841](#1841))
([92a9f5e](92a9f5e))
* **core:** Require go 1.23+
([#1979](#1979))
([164c922](164c922))
* **core:** v2 ERS with proto updates
([#2210](#2210))
([a161ef8](a161ef8))
* **policy:** actions service RPCs should actually hit storage layer
CRUD ([#2063](#2063))
([da4faf5](da4faf5))
* **policy:** Add list key mappings rpc.
([#2533](#2533))
([fbc2724](fbc2724))
* **policy:** adds new public keys table
([#1836](#1836))
([cad5048](cad5048))
* **policy:** Allow the deletion of a key.
([#2575](#2575))
([82b96f0](82b96f0))
* **policy:** Default Platform Keys
([#2254](#2254))
([d7447fe](d7447fe))
* **policy:** DSPX-902 NDR service crud implementation (2/2)
([#2066](#2066))
([030ad33](030ad33))
* **policy:** key management crud
([#2110](#2110))
([4c3d53d](4c3d53d))
* **sdk:** Add a KAS allowlist
([#2085](#2085))
([d7cfdf3](d7cfdf3))
* **sdk:** add nanotdf plaintext policy
([#2182](#2182))
([e5c56db](e5c56db))
* **sdk:** adds seeker interface to TDF Reader
([#2385](#2385))
([63ccd9a](63ccd9a))
* **sdk:** Allow key splits with same algo
([#2454](#2454))
([7422b15](7422b15))
* **sdk:** Allow schema validation during TDF decrypt
([#1870](#1870))
([b7e6fb2](b7e6fb2))
* **sdk:** autoconfig kaos with kids
([#2438](#2438))
([c272016](c272016))
* **sdk:** bump protocol/go v0.6.0
([#2536](#2536))
([23e4c2b](23e4c2b))
* **sdk:** CreateTDF option to run with specific target schema version
([#2045](#2045))
([0976b15](0976b15))
* **sdk:** Enable base key support.
([#2425](#2425))
([9ff3806](9ff3806))
* **sdk:** Expose connectrpc wrapper codegen for re-use
([#2322](#2322))
([8b29392](8b29392))
* **sdk:** MIC-1436: User can decrypt TDF files created with
FileWatcher2.0.8 and older.
([#1833](#1833))
([f77d110](f77d110))
* **sdk:** remove hex encoding for segment hash
([#1805](#1805))
([d7179c2](d7179c2))
* **sdk:** sdk.New should validate platform connectivity and provide
precise error ([#1937](#1937))
([aa3696d](aa3696d))
* **sdk:** Use ConnectRPC in the go client
([#2200](#2200))
([fc34ee6](fc34ee6))


### Bug Fixes

* Allow parsing IPs as hostnames
([#1999](#1999))
([d54b550](d54b550))
* **ci:** Fix intermittent failures from auth tests
([#2345](#2345))
([395988a](395988a))
* **ci:** Update expired ca and certs in oauth unit tests
([#2113](#2113))
([5440fcc](5440fcc))
* **core:** Autobump sdk
([#1863](#1863))
([855cb2b](855cb2b))
* **core:** Autobump sdk
([#1873](#1873))
([085ac7a](085ac7a))
* **core:** Autobump sdk
([#1894](#1894))
([201244e](201244e))
* **core:** Autobump sdk
([#1917](#1917))
([edeeb74](edeeb74))
* **core:** Autobump sdk
([#1941](#1941))
([0a5a948](0a5a948))
* **core:** Autobump sdk
([#1948](#1948))
([4dfb457](4dfb457))
* **core:** Autobump sdk
([#1968](#1968))
([7084061](7084061))
* **core:** Autobump sdk
([#1972](#1972))
([7258f5d](7258f5d))
* **core:** Autobump sdk
([#2102](#2102))
([0315635](0315635))
* **core:** Fixes protoJSON parse bug on ec rewrap
([#1943](#1943))
([9bebfd0](9bebfd0))
* **core:** Improves errors when under heavy load
([#2132](#2132))
([4490a14](4490a14))
* **core:** Update fixtures and flattening in sdk and service
([#1827](#1827))
([d6d6a7a](d6d6a7a))
* **core:** Updates ec-wrapped to newer salt
([#1961](#1961))
([0e17968](0e17968))
* **deps:** bump github.com/docker/docker from 28.2.2+incompatible to
28.3.3+incompatible in /sdk
([#2597](#2597))
([a68d00d](a68d00d))
* **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.2.0 to
0.3.0 in /sdk ([#2502](#2502))
([3ec8b35](3ec8b35))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to
0.4.0 in /sdk ([#2397](#2397))
([99e3aa4](99e3aa4))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to
0.5.0 in /sdk ([#2471](#2471))
([e8f97e0](e8f97e0))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.5.0 to
0.5.1 in /sdk ([#2505](#2505))
([4edab72](4edab72))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.6.0 to
0.6.2 in /sdk ([#2586](#2586))
([4ed9856](4ed9856))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.6.2 to
0.7.0 in /sdk ([#2627](#2627))
([e775e14](e775e14))
* **deps:** bump golang.org/x/oauth2 from 0.26.0 to 0.30.0 in /sdk
([#2252](#2252))
([9b775a2](9b775a2))
* **deps:** bump google.golang.org/grpc from 1.71.0 to 1.72.1 in /sdk
([#2244](#2244))
([49484e0](49484e0))
* **deps:** bump the external group across 1 directory with 5 updates
([#2400](#2400))
([0b7ea79](0b7ea79))
* **deps:** bump toolchain in /lib/fixtures and /examples to resolve CVE
GO-2025-3563 ([#2061](#2061))
([9c16843](9c16843))
* Improve http.Client usage for security and performance
([#1910](#1910))
([e6a53a3](e6a53a3))
* **sdk:** adds connection options to getPlatformConfiguration
([#2286](#2286))
([a3af31e](a3af31e))
* **sdk:** Allow reuse of session key
([#2016](#2016))
([d48c11e](d48c11e))
* **sdk:** bump lib/ocrypto to 0.1.8
([#1938](#1938))
([53fa8ab](53fa8ab))
* **sdk:** bump protocol/go module dependencies
([#2078](#2078))
([e027f43](e027f43))
* **sdk:** Display proper error on kas rewrap failure
([#2081](#2081))
([508cbcd](508cbcd))
* **sdk:** everything is `mixedSplits` now
([#1861](#1861))
([ba78f14](ba78f14))
* **sdk:** Fix compatibility between bulk and non-bulk rewrap
([#1914](#1914))
([74abbb6](74abbb6))
* **sdk:** Fixed token expiration time
([#1854](#1854))
([c3cda1b](c3cda1b))
* **sdk:** perfsprint lint issues
([#2208](#2208))
([d36a078](d36a078))
* **sdk:** Prefer KID and Algorithm selection from key maps
([#2475](#2475))
([98fd392](98fd392))
* **sdk:** Removes unnecessary down-cast of `int`
([#1869](#1869))
([66f0c14](66f0c14))
* **sdk:** Version config fix
([#1847](#1847))
([be5d817](be5d817))
* Service utilize `httputil.SafeHttpClient`
([#1926](#1926))
([af32700](af32700))
* set consistent system metadata id and schema
([#2451](#2451))
([5db3cf2](5db3cf2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:sdk A software development kit, including library, for client applications and inter-service communicati size/xs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants