fix(sdk): remove testcontainers from consumer dependency graph - #3129
Merged
Conversation
github-actions
Bot
added
comp:sdk
Contributor
Summary of ChangesHello, 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 refactors the project's dependency management by isolating integration tests that rely on containerization. By moving these tests to a separate Go module, the core SDK no longer inherits heavy test-only dependencies, significantly reducing its overall dependency footprint and improving the experience for consumers by avoiding unnecessary vulnerability scanner alerts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
Activity
Using Gemini Code AssistThe 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
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. 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. Dependencies, a heavy chain, Now lighter, free from strain. Tests find new home, serene, Code's purpose, sharp and keen. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request effectively removes testcontainers and its transitive dependencies from the SDK's go.mod, which is a great improvement for downstream consumers. Moving the integration tests to a new test/integration module is a clean solution.
I've reviewed the changes and the new test code. The overall structure is good. I have a few suggestions to improve the test code quality by handling errors more consistently and using more secure file permissions for test data. Please see my detailed comments.
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
X-Test Failure Reporttest-cases-mapping-report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Move keycloak integration tests from sdk/auth/oauth/ to a separate test/integration module with its own go.mod. This removes testcontainers-go and 60+ transitive container dependencies (docker, containerd, moby, etc.) from the SDK's published module graph. Downstream consumers will no longer inherit these test-only dependencies, eliminating false CVE alerts from vulnerability scanners. Resolves: DSPX-2582 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn@virtru.com>
…n from go.work Remove test/integration from go.work to fix Docker image build (Dockerfile does not copy test/ directory). The test module uses replace directives and works standalone with GOWORK=off. Also bumps golang.org/x/net to v0.51.0 to fix GO-2026-4559 (HTTP/2 panic vulnerability). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn@virtru.com>
Fixes GO-2026-4559 (HTTP/2 panic vulnerability) in service, tests-bdd, and examples modules. The SDK was already bumped in the previous commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn@virtru.com>
- Replace panic with s.Require().NoError in SetupSuite - Check tls.LoadX509KeyPair and x509.SystemCertPool errors immediately - Handle jwt.NewBuilder().Build() errors in mock server handlers - Use restrictive file permissions (0o644/0o600) for container certs/keys - Check keycloak.MappedPort errors instead of ignoring them Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn@virtru.com>
pflynn-virtru
force-pushed
the
fix/isolate-testcontainers-dep
branch
from
March 11, 2026 15:52
20a3d4c to
bf26798
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Member
Author
|
/gemini review |
Contributor
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restructures OAuth Keycloak integration testing so the SDK module no longer pulls testcontainers-go (and its large transitive container stack) into downstream consumers, reducing false-positive CVE noise while keeping SDK unit tests lightweight.
Changes:
- Moved Keycloak/testcontainers-based OAuth tests into a new standalone Go module under
test/integration/. - Removed
testcontainers-go(and related transitive deps) fromsdk/go.mod, updating SDK deps accordingly. - Updated supporting module dependencies (service/examples) due to
go mod tidyfallout and added integration test TLS/keystore fixtures.
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/oauth/testdata/sanX509su.conf | Adds SAN config fixture for integration test cert generation/usage. |
| test/integration/oauth/testdata/sanX509.conf | Adds SAN config fixture for integration test cert generation/usage. |
| test/integration/oauth/testdata/sampleuser.key | Adds client private key fixture for Keycloak mTLS/cert exchange integration tests. |
| test/integration/oauth/testdata/sampleuser.crt | Adds client certificate fixture for Keycloak mTLS/cert exchange integration tests. |
| test/integration/oauth/testdata/new-ca.key | Adds CA private key fixture used to build trust material for integration tests. |
| test/integration/oauth/testdata/new-ca.jks | Adds Java keystore fixture mounted into Keycloak container for TLS/truststore setup. |
| test/integration/oauth/testdata/new-ca.crt | Adds CA certificate fixture used by tests/client trust. |
| test/integration/oauth/testdata/localhost.key | Adds localhost server private key fixture for Keycloak HTTPS configuration. |
| test/integration/oauth/testdata/localhost.crt | Adds localhost server certificate fixture for Keycloak HTTPS configuration. |
| test/integration/oauth/oauth_test.go | New integration suite using testcontainers to run Keycloak and validate OAuth flows. |
| test/integration/go.sum | Locks integration-module dependency graph (includes testcontainers deps). |
| test/integration/go.mod | New standalone integration-test module with local replace directives back to repo modules. |
| service/go.sum | Updates sums due to dependency version changes. |
| service/go.mod | Bumps golang.org/x/* versions via tidy/update. |
| sdk/go.sum | Removes testcontainers-related sums and updates remaining dependency versions. |
| sdk/go.mod | Removes testcontainers-go/fixtures deps from SDK module and updates golang.org/x/* versions. |
| sdk/auth/oauth/oauth_test.go | Removes Keycloak/testcontainers (and related) tests from SDK; keeps unit coverage for token expiration. |
| examples/go.sum | Updates sums due to dependency version changes. |
| examples/go.mod | Bumps golang.org/x/* versions via tidy/update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The PEM keys and certs in sdk/auth/oauth/testdata/ were copied to test/integration/oauth/testdata/ but the originals were not removed. No SDK code references them anymore. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn@virtru.com>
jentfoo
approved these changes
Mar 11, 2026
dmihalcik-virtru
approved these changes
Mar 11, 2026
dmihalcik-virtru
left a comment
dmihalcik-virtru
left a comment
Member
There was a problem hiding this comment.
I'm assuming any suggestions from copilot/gemini are covering existing issues, which should be kept in place to keep this PR focused on its task
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
pflynn-virtru
enabled auto-merge
March 11, 2026 17:12
Contributor
This was referenced Mar 11, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Mar 11, 2026
🤖 I have created a release *beep* *boop* --- ## [0.14.0](sdk/v0.13.0...sdk/v0.14.0) (2026-03-11) ### Features * **sdk:** DSPX-2418 add attribute discovery methods ([#3082](#3082)) ([aeeaadd](aeeaadd)) ### Bug Fixes * **ci:** Upgrade toolchain version to 1.25.8 ([#3116](#3116)) ([e1b7882](e1b7882)) * **policy:** order List* results by created_at ([#3088](#3088)) ([ea90ac2](ea90ac2)) * **sdk:** remove testcontainers from consumer dependency graph ([#3129](#3129)) ([f17dcdd](f17dcdd)) --- 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>
JBCongdon
pushed a commit
to JBCongdon/platform
that referenced
this pull request
May 24, 2026
🤖 I have created a release *beep* *boop* --- ## [0.14.0](opentdf/platform@service/v0.13.0...service/v0.14.0) (2026-04-21) ### ⚠ BREAKING CHANGES * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ### Features * **authz:** Namespaced policy in decisioning ([opentdf#3226](opentdf#3226)) ([0355934](opentdf@0355934)) * **cli:** migrate otdfctl into platform monorepo ([opentdf#3205](opentdf#3205)) ([5177bec](opentdf@5177bec)) * fix tracing ([opentdf#3242](opentdf#3242)) ([57e5680](opentdf@57e5680)) * **policy:** add GetObligationTrigger RPC ([opentdf#3318](opentdf#3318)) ([d68e39d](opentdf@d68e39d)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) ([bedc9b3](opentdf@bedc9b3)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ([04fd85d](opentdf@04fd85d)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) ([c0443f1](opentdf@c0443f1)) * **policy:** add sort ListSubjectMappings API ([opentdf#3255](opentdf#3255)) ([9d5d757](opentdf@9d5d757)) * **policy:** Add sort support listregisteredresources api ([opentdf#3312](opentdf#3312)) ([91a3ff3](opentdf@91a3ff3)) * **policy:** add sort support to ListAttributes API ([opentdf#3223](opentdf#3223)) ([ec3312f](opentdf@ec3312f)) * **policy:** add sort support to ListKeyAccessServer ([opentdf#3287](opentdf#3287)) ([7fae2d7](opentdf@7fae2d7)) * **policy:** Add sort support to ListNamespaces API ([opentdf#3192](opentdf#3192)) ([aac86cd](opentdf@aac86cd)) * **policy:** add sort support to listobligations api ([opentdf#3300](opentdf#3300)) ([9221cac](opentdf@9221cac)) * **policy:** add sort support to ListSubjectConditionSets API ([opentdf#3272](opentdf#3272)) ([9010f12](opentdf@9010f12)) * **policy:** add SortField proto and update PageRequest for sort support ([opentdf#3187](opentdf#3187)) ([6cf1862](opentdf@6cf1862)) * **policy:** Enforce same namespace when actions referenced downstream ([opentdf#3206](opentdf#3206)) ([4b5463a](opentdf@4b5463a)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) ([6db1883](opentdf@6db1883)) * **policy:** Namespace subject mappings and condition sets ([opentdf#3172](opentdf#3172)) ([6deed50](opentdf@6deed50)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) ([3006780](opentdf@3006780)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) ([8948018](opentdf@8948018)) * **policy:** rollback migration strategy for namespaced actions ([opentdf#3235](opentdf#3235)) ([f7e5e01](opentdf@f7e5e01)) * **policy:** Seed existing namespaces with standard actions ([opentdf#3228](opentdf#3228)) ([12136b0](opentdf@12136b0)) * **policy:** Seed namespaces with standard actions on creation + namespaced actions for obligation triggers ([opentdf#3161](opentdf#3161)) ([984d76b](opentdf@984d76b)) ### Bug Fixes * **ci:** Upgrade toolchain version to 1.25.8 ([opentdf#3116](opentdf#3116)) ([e1b7882](opentdf@e1b7882)) * **core:** do not concat slashes directly in url/file paths ([opentdf#3290](opentdf#3290)) ([114c2a7](opentdf@114c2a7)) * **deps:** bump github.com/jackc/pgx/v5 from 5.7.5 to 5.9.0 in /service ([opentdf#3316](opentdf#3316)) ([017362e](opentdf@017362e)) * **deps:** bump github.com/opentdf/platform/lib/identifier from 0.2.0 to 0.3.0 in /service ([opentdf#3162](opentdf#3162)) ([8bc5dcd](opentdf@8bc5dcd)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.16.0 to 0.17.0 in /service ([opentdf#3125](opentdf#3125)) ([29fec61](opentdf@29fec61)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.17.0 to 0.21.0 in /service ([opentdf#3220](opentdf#3220)) ([e63add2](opentdf@e63add2)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.21.0 to 0.22.0 in /service ([opentdf#3248](opentdf#3248)) ([1ebce73](opentdf@1ebce73)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.22.0 to 0.23.0 in /service ([opentdf#3271](opentdf#3271)) ([3338b8e](opentdf@3338b8e)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.23.0 to 0.24.0 in /service ([opentdf#3321](opentdf#3321)) ([78e6022](opentdf@78e6022)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.24.0 to 0.25.0 in /service ([opentdf#3333](opentdf#3333)) ([3940bf8](opentdf@3940bf8)) * **deps:** bump github.com/opentdf/platform/sdk from 0.13.0 to 0.16.0 in /service ([opentdf#3356](opentdf#3356)) ([5617077](opentdf@5617077)) * **deps:** bump go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp from 1.42.0 to 1.43.0 in /service ([opentdf#3282](opentdf#3282)) ([046374a](opentdf@046374a)) * **deps:** bump go.opentelemetry.io/otel/sdk from 1.42.0 to 1.43.0 in /service ([opentdf#3281](opentdf#3281)) ([56b33f2](opentdf@56b33f2)) * **deps:** bump google.golang.org/grpc from 1.77.0 to 1.79.3 in /service ([opentdf#3176](opentdf#3176)) ([3289502](opentdf@3289502)) * **deps:** remove direct github.com/docker/docker dependency ([opentdf#3229](opentdf#3229)) ([2becb27](opentdf@2becb27)) * **deps:** upgrade testcontainers-go to resolve vulns ([opentdf#3299](opentdf#3299)) ([72c6f9b](opentdf@72c6f9b)) * **ers:** include standard JWT claims in claims mode entity resolution ([opentdf#3196](opentdf#3196)) ([6d50da1](opentdf@6d50da1)) * **ers:** ldap multi-strategy ers ([opentdf#3117](opentdf#3117)) ([d3aaf1a](opentdf@d3aaf1a)) * **policy:** deprecate ListAttributeValues in favor of existing GetAttribute ([opentdf#3108](opentdf#3108)) ([7e17c2d](opentdf@7e17c2d)) * **policy:** make obligation trigger uniqueness client-aware ([opentdf#3114](opentdf#3114)) ([9265bc3](opentdf@9265bc3)) * **policy:** omit empty attribute values from create responses ([opentdf#3193](opentdf#3193)) ([d298378](opentdf@d298378)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) ([10d0c0f](opentdf@10d0c0f)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) ([c20f039](opentdf@c20f039)) * **policy:** order List* results by created_at ([opentdf#3088](opentdf#3088)) ([ea90ac2](opentdf@ea90ac2)) * **sdk:** normalize issuer URL before OIDC discovery ([opentdf#3261](opentdf#3261)) ([61f98c9](opentdf@61f98c9)) * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) ([f04a385](opentdf@f04a385)) * **sdk:** remove testcontainers from consumer dependency graph ([opentdf#3129](opentdf#3129)) ([f17dcdd](opentdf@f17dcdd)) --- 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>
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
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
sdk/auth/oauth/oauth_test.goto a newtest/integration/module with its owngo.modtestcontainers-goand 60+ transitive container dependencies (docker, containerd, moby, etc.) fromsdk/go.modDetails
The
sdk/go.modpreviously listedtestcontainers-goas a direct dependency becauseoauth_test.goimported it for keycloak integration tests. Even though it was only used in test files, Go modules include all dependencies (test and non-test) ingo.mod. Any downstream consumer runninggo get github.com/opentdf/platform/sdkwould inherit these deps.This PR (SDK only): Unit test (
TestTokenExpiration_RespectsLeeway) stays in the SDK; keycloak integration suite moves totest/integration/oauth/.Service module: Still has testcontainers in
service/go.modfromservice/integration/andservice/entityresolution/integration/tests. This is being addressed in #3132 (export internal packages) and a follow-up PR to move service integration tests to their own module.Part of DSPX-2582
Test plan
go test ./...insdk/)testcontainers-gono longer insdk/go.modsdk/go.modtest/integration/modulego mod why -m github.com/testcontainers/testcontainers-gofrom a downstream consumer after release🤖 Generated with Claude Code