Skip to content

Conversation

@adborroto
Copy link

What does this PR do?

This PR adds a new SpiceDB module to the Testcontainers for .NET library, providing seamless integration for testing applications that use SpiceDB (Google Zanzibar-inspired permissions database).

Key components added:

  • SpiceDBContainer: Container wrapper with SpiceDB-specific functionality including connection string generation
  • SpiceDBBuilder: Fluent builder for configuring SpiceDB containers with sensible defaults
  • SpiceDBConfiguration: Configuration class following the established Testcontainers pattern
  • Comprehensive test suite: Full test coverage with container lifecycle, port mapping, and command execution tests
  • Documentation: Complete module documentation following the established format used by other modules

Technical details:

  • Uses authzed/spicedb:v1.45.1 Docker image
  • Exposes gRPC API on port 50051
  • Implements wait strategy using SpiceDB CLI ping command
  • Follows the standard Testcontainers .NET module patterns and conventions

Why is it important?

SpiceDB is gaining significant adoption as a robust authorization service, especially for applications requiring fine-grained permissions at scale. This module enables developers to:

  1. Test authorization logic reliably: Write integration tests against a real SpiceDB instance without complex setup
  2. Improve development velocity: Spin up SpiceDB containers effortlessly in test environments
  3. Ensure consistency: Test against the same SpiceDB version used in production
  4. Simplify CI/CD: No need for external SpiceDB dependencies in testing pipelines

This addition expands Testcontainers' coverage into the authorization/permissions domain, complementing the existing database and messaging modules.

Related issues

  • Relates to expanding Testcontainers module ecosystem
  • Addresses community requests for SpiceDB testing support

How to test this PR

Prerequisites:

  • Docker installed and running
  • .NET 9.0 SDK

Testing steps:

  1. Build the solution:

    dotnet build
  2. Run SpiceDB tests specifically:

    dotnet test tests/Testcontainers.SpiceDB.Tests/
  3. Run all tests to ensure no regressions:

    dotnet test
  4. Verify documentation builds correctly:

    # Check that the documentation is properly formatted
    cat docs/modules/spicedb.md

Expected test results:

  • ✅ Container starts successfully
  • ✅ gRPC port (50051) is properly mapped
  • ✅ SpiceDB commands execute successfully
  • ✅ Container lifecycle management works correctly

Follow-ups

Potential enhancements for future PRs:

  1. Enhanced connection string implementation: Currently returns null - should return proper gRPC connection string format
  2. SpiceDB-specific helper methods: Add methods for schema management, relationship operations
  3. Configuration presets: Add builder methods for common SpiceDB configurations (development, testing, etc.)
  4. Client library integration examples: Demonstrate usage with popular SpiceDB client libraries
  5. Performance testing utilities: Add helpers for load testing authorization scenarios

Documentation improvements:

  • Add code examples showing integration with SpiceDB client libraries
  • Include

@netlify
Copy link

netlify bot commented Jul 31, 2025

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 3b49614
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-dotnet/deploys/69135ee2f0f3c50008495804
😎 Deploy Preview https://deploy-preview-1498--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@adborroto adborroto marked this pull request as draft July 31, 2025 12:17
@adborroto adborroto marked this pull request as ready for review August 1, 2025 08:13
@HofmeisterAn HofmeisterAn added module An official Testcontainers module enhancement New feature or request labels Sep 21, 2025
@HofmeisterAn HofmeisterAn force-pushed the develop branch 3 times, most recently from 4900ecd to 8fa5f1b Compare October 3, 2025 20:17
@coderabbitai
Copy link

coderabbitai bot commented Nov 11, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Added SpiceDB container module support enabling gRPC connections for test environments
  • Documentation

    • Added SpiceDB module documentation with configuration examples and setup instructions
  • Chores

    • Improved CI/CD pipeline with branch wildcards and path filtering
    • Updated package dependencies with Docker and gRPC components

Walkthrough

This PR introduces SpiceDB support to the Testcontainers library. It adds the Testcontainers.SpiceDB module with builder, configuration, and container classes; updates CI/CD pipelines with branch and path filtering; updates package dependencies; adds documentation and tests; and registers new projects in the solution.

Changes

Cohort / File(s) Summary
CI/CD Pipeline Configuration
.github/workflows/cicd.yml
Adds branch wildcards (bugfix/*, feature/*) and path ignore rules (docs/**, examples/**) to push and pull_request triggers.
Solution Configuration
Testcontainers.sln
Registers two new projects: Testcontainers.SpiceDB and Testcontainers.SpiceDB.Tests, with corresponding build configurations and nested project relationships.
Package Management
Directory.Packages.props
Adds package versions for Docker.DotNet.Enhanced.X509 3.128.3, Docker.DotNet.Enhanced 3.128.3, Grpc.Core 2.46.6, Grpc.HealthCheck 2.60.0, and Grpc.Net.Client 2.60.0.
Module Documentation
docs/modules/index.md, docs/modules/spicedb.md
Adds SpiceDB entry to modules index and introduces comprehensive SpiceDB documentation page with setup instructions, usage examples, and recommended dependencies.
SpiceDB Module Core
src/Testcontainers.SpiceDB/SpiceDBBuilder.cs, src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs, src/Testcontainers.SpiceDB/SpiceDBContainer.cs
Implements builder pattern, configuration management (TslEnabled, GrpcPresharedKey, DatastoreEngine), and container class with gRPC connection string generation.
SpiceDB Project Setup
src/Testcontainers.SpiceDB/Testcontainers.SpiceDB.csproj, src/Testcontainers.SpiceDB/Usings.cs, src/Testcontainers.SpiceDB/.editorconfig
Establishes project structure with multi-framework targeting (net8.0, net9.0, netstandard2.0/2.1), global using directives, and editor configuration.
SpiceDB Test Suite
tests/Testcontainers.SpiceDB.Tests/Testcontainers.SpiceDB.Tests.csproj, tests/Testcontainers.SpiceDB.Tests/SpiceDBContainerTest.cs, tests/Testcontainers.SpiceDB.Tests/Usings.cs, tests/Testcontainers.SpiceDB.Tests/.editorconfig
Adds test project with xUnit tests validating port mapping, container version command execution, gRPC connection string generation, and connectivity.

Sequence Diagram

sequenceDiagram
    participant Test as Test Code
    participant Builder as SpiceDBBuilder
    participant Config as SpiceDBConfiguration
    participant Container as SpiceDBContainer
    participant Docker as Docker Engine

    Test->>Builder: new SpiceDBBuilder()
    Builder->>Config: new SpiceDBConfiguration(...)
    Config-->>Builder: config instance
    Test->>Builder: Build()
    Builder->>Docker: CreateContainer(...)
    Docker-->>Container: container
    Container-->>Test: SpiceDBContainer
    Test->>Container: InitializeAsync()
    Container->>Docker: Start container
    Docker-->>Container: running
    Container-->>Test: ready
    Test->>Container: GetGrpcConnectionString()
    Container-->>Test: grpc://host:port
    Test->>Container: DisposeAsync()
    Container->>Docker: Stop container
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20-25 minutes

  • Builder Logic: Review initialization, validation, and configuration merging patterns in SpiceDBBuilder.cs
  • Configuration Design: Verify default values (preshared key, datastore engine, TLS flag) in SpiceDBConfiguration.cs and constructor chaining
  • Container Integration: Ensure gRPC connection string generation correctly handles TLS and port mapping in SpiceDBContainer.cs
  • Test Coverage: Validate that test cases properly assert container lifecycle, command execution, and gRPC connectivity
  • Dependency Alignment: Confirm added Grpc.* packages align with project requirements and version compatibility

Suggested reviewers

  • HofmeisterAn
  • hamidjahad

Poem

🐰 A spicy new module hops into the warren,
With gRPC secrets and containers to borrow,
Configuration dances, builders align,
Tests validate magic—a permissions design!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(spicedb): add testcontainer module for SpiceDB' directly summarizes the main change: adding a new SpiceDB module to Testcontainers.
Description check ✅ Passed The PR description covers all mandatory sections (What, Why), recommended sections (Related issues, How to test), and optional sections (Follow-ups) with comprehensive detail throughout.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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 and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🧹 Nitpick comments (3)
tests/Testcontainers.SpiceDB.Tests/SpiceDBContainerTest.cs (3)

37-50: Consider using ConfigureAwait(false) on Line 46.

While ConfigureAwait(true) works in test contexts, the standard practice is to use ConfigureAwait(false) in library code and tests to avoid potential deadlocks and improve performance.

        var execResult = await _spicedbContainer.ExecAsync(commands, TestContext.Current.CancellationToken)
-            .ConfigureAwait(true);
+            .ConfigureAwait(false);

67-77: SSL validation bypass is acceptable for tests but consider documenting it.

While disabling SSL certificate validation is common in integration tests, consider adding a comment explaining this is intentional for test environments.

        var handler = new SocketsHttpHandler();
+        // Disable SSL validation for test environments
        handler.SslOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true;

78-84: Remove redundant assertion and consider testing actual connectivity.

Line 79 redundantly asserts connectionString is not null (already validated in the previous test). Additionally, checking channel.State only verifies the channel reached the Ready state but doesn't confirm actual RPC connectivity to SpiceDB.

        // Then
-        Assert.NotNull(connectionString);
        Assert.NotNull(channel);

-        // Test connectivity by attempting to connect
        await channel.ConnectAsync();
        Assert.Equal(ConnectivityState.Ready, channel.State);
+        
+        // Optionally: Perform an actual RPC call to verify true connectivity
+        // For example, call a SpiceDB health check or version endpoint
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c8ff7dd and 3b49614.

📒 Files selected for processing (15)
  • .github/workflows/cicd.yml (1 hunks)
  • Directory.Packages.props (2 hunks)
  • Testcontainers.sln (3 hunks)
  • docs/modules/index.md (2 hunks)
  • docs/modules/spicedb.md (1 hunks)
  • src/Testcontainers.SpiceDB/.editorconfig (1 hunks)
  • src/Testcontainers.SpiceDB/SpiceDBBuilder.cs (1 hunks)
  • src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs (1 hunks)
  • src/Testcontainers.SpiceDB/SpiceDBContainer.cs (1 hunks)
  • src/Testcontainers.SpiceDB/Testcontainers.SpiceDB.csproj (1 hunks)
  • src/Testcontainers.SpiceDB/Usings.cs (1 hunks)
  • tests/Testcontainers.SpiceDB.Tests/.editorconfig (1 hunks)
  • tests/Testcontainers.SpiceDB.Tests/SpiceDBContainerTest.cs (1 hunks)
  • tests/Testcontainers.SpiceDB.Tests/Testcontainers.SpiceDB.Tests.csproj (1 hunks)
  • tests/Testcontainers.SpiceDB.Tests/Usings.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs (2)
src/Testcontainers.SpiceDB/SpiceDBBuilder.cs (1)
  • PublicAPI (4-71)
src/Testcontainers.SpiceDB/SpiceDBContainer.cs (1)
  • PublicAPI (7-28)
tests/Testcontainers.SpiceDB.Tests/SpiceDBContainerTest.cs (2)
src/Testcontainers.SpiceDB/SpiceDBBuilder.cs (7)
  • SpiceDBContainer (37-41)
  • SpiceDBBuilder (17-21)
  • SpiceDBBuilder (27-31)
  • SpiceDBBuilder (44-52)
  • SpiceDBBuilder (55-58)
  • SpiceDBBuilder (61-64)
  • SpiceDBBuilder (67-70)
src/Testcontainers.SpiceDB/SpiceDBContainer.cs (2)
  • SpiceDBContainer (16-20)
  • GetGrpcConnectionString (22-27)
src/Testcontainers.SpiceDB/SpiceDBContainer.cs (2)
src/Testcontainers.SpiceDB/SpiceDBBuilder.cs (8)
  • PublicAPI (4-71)
  • SpiceDBContainer (37-41)
  • SpiceDBBuilder (17-21)
  • SpiceDBBuilder (27-31)
  • SpiceDBBuilder (44-52)
  • SpiceDBBuilder (55-58)
  • SpiceDBBuilder (61-64)
  • SpiceDBBuilder (67-70)
src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs (6)
  • PublicAPI (4-63)
  • SpiceDBConfiguration (10-15)
  • SpiceDBConfiguration (28-32)
  • SpiceDBConfiguration (38-42)
  • SpiceDBConfiguration (48-52)
  • SpiceDBConfiguration (59-62)
src/Testcontainers.SpiceDB/SpiceDBBuilder.cs (2)
src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs (6)
  • PublicAPI (4-63)
  • SpiceDBConfiguration (10-15)
  • SpiceDBConfiguration (28-32)
  • SpiceDBConfiguration (38-42)
  • SpiceDBConfiguration (48-52)
  • SpiceDBConfiguration (59-62)
src/Testcontainers.SpiceDB/SpiceDBContainer.cs (2)
  • PublicAPI (7-28)
  • SpiceDBContainer (16-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: analyze (csharp)
🔇 Additional comments (6)
docs/modules/index.md (1)

80-80: Thanks for wiring up the table entry.

NuGet and Source links follow the existing convention, so the index entry is ready to go once the module lands.

tests/Testcontainers.SpiceDB.Tests/SpiceDBContainerTest.cs (3)

11-24: LGTM!

The test class structure follows xUnit best practices with proper async lifecycle management using IAsyncLifetime.


26-35: LGTM!

Port mapping test is straightforward and validates the expected behavior.


52-61: LGTM!

Connection string validation test is clear and appropriate.

Testcontainers.sln (2)

283-286: LGTM!

Project declarations are correctly added to the solution with proper GUIDs and paths.


813-816: LGTM!

Build configurations and nesting for the SpiceDB projects are correctly structured and follow the solution's conventions.

Also applies to: 821-824, 961-962

Comment on lines +7 to 12
pull_request:
branches: [develop, main]
paths-ignore: ["docs/**", "examples/**"]
branches: [ develop, main, bugfix/*, feature/* ]
pull_request:
branches: [ develop, main ]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Remove duplicate pull_request event trigger and consolidate overlapping branches keys.

YAML does not support duplicate keys at the same nesting level. The current configuration has two pull_request triggers (lines 7–9 and lines 11–12), and the second one will override the first. Additionally, within the first pull_request block, there are two conflicting branches keys (lines 8 and 10), where line 10 will override line 8.

This means the paths-ignore filter and the feature/* branch pattern from lines 5–6 will be lost for pull_request events. The workflow will fall back to the legacy configuration on lines 11–12.

Apply this diff to remove the duplicate trigger and consolidate the configuration:

  pull_request:
    branches: [develop, main]
    paths-ignore: ["docs/**", "examples/**"]
-    branches: [ develop, main, bugfix/*, feature/* ]
-  pull_request:
-    branches: [ develop, main ]
🧰 Tools
🪛 actionlint (1.7.8)

10-10: key "branches" is duplicated in "pull_request" section. previously defined at line:8,col:5

(syntax-check)


11-11: key "pull_request" is duplicated in "on" section. previously defined at line:7,col:3

(syntax-check)

🪛 YAMLlint (1.37.1)

[error] 10-10: duplication of key "branches" in mapping

(key-duplicates)


[error] 11-11: duplication of key "pull_request" in mapping

(key-duplicates)

🤖 Prompt for AI Agents
In .github/workflows/cicd.yml around lines 7 to 12, remove the duplicate
pull_request block and consolidate the configuration into a single pull_request
trigger that includes the full branches list and the paths-ignore filter;
specifically, keep one pull_request key that lists branches: [develop, main,
bugfix/*, feature/*] and retain paths-ignore: ["docs/**", "examples/**"], and
delete the second redundant pull_request block so no keys are overridden.

Comment on lines +8 to 12
<PackageVersion Include="Docker.DotNet.Enhanced.X509" Version="3.128.3"/>
<PackageVersion Include="Docker.DotNet.Enhanced" Version="3.128.3"/>
<PackageVersion Include="Grpc.Core" Version="2.46.6" />
<PackageVersion Include="Docker.DotNet.Enhanced.X509" Version="3.129.0"/>
<PackageVersion Include="Docker.DotNet.Enhanced" Version="3.129.0"/>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Resolve duplicate central package versions.

Adding the 3.128.3 entries for Docker.DotNet.Enhanced(.X509) alongside the existing 3.129.0 definitions causes MSBuild to choke (PackageVersion items must be unique). Drop the new 3.128.3 lines (or bump the module to 3.129.0) so only one version remains.

-        <PackageVersion Include="Docker.DotNet.Enhanced.X509" Version="3.128.3"/>
-        <PackageVersion Include="Docker.DotNet.Enhanced" Version="3.128.3"/>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<PackageVersion Include="Docker.DotNet.Enhanced.X509" Version="3.128.3"/>
<PackageVersion Include="Docker.DotNet.Enhanced" Version="3.128.3"/>
<PackageVersion Include="Grpc.Core" Version="2.46.6" />
<PackageVersion Include="Docker.DotNet.Enhanced.X509" Version="3.129.0"/>
<PackageVersion Include="Docker.DotNet.Enhanced" Version="3.129.0"/>
<PackageVersion Include="Grpc.Core" Version="2.46.6" />
<PackageVersion Include="Docker.DotNet.Enhanced.X509" Version="3.129.0"/>
<PackageVersion Include="Docker.DotNet.Enhanced" Version="3.129.0"/>
🤖 Prompt for AI Agents
In Directory.Packages.props around lines 8 to 12 there are duplicate
PackageVersion entries for Docker.DotNet.Enhanced and
Docker.DotNet.Enhanced.X509 (both 3.128.3 and 3.129.0) which breaks MSBuild;
remove the 3.128.3 entries or update any dependent modules to use 3.129.0 so
each PackageVersion Include appears only once, leaving a single canonical
version for both Docker.DotNet.Enhanced and Docker.DotNet.Enhanced.X509.

Comment on lines +13 to +40
=== "Usage Example"

````csharp
public sealed class SpiceDBContainerTest : IAsyncLifetime
{
private readonly SpiceDBContainer \_spicedbContainer = new SpiceDBBuilder().Build();

public Task InitializeAsync()
{
return _spicedbContainer.StartAsync();
}

public Task DisposeAsync()
{
return _spicedbContainer.DisposeAsync().AsTask();
}

[Fact]
public async Task SpiceDBContainer_IsRunning_ReturnsTrue()
{
// Given
var containerState = await _spicedbContainer.GetStateAsync();

// ...
}
}
```

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix the tabbed code block fencing.

The === "Usage Example" section opens with ````csharp but closes with an indented ```; MkDocs will treat this as mismatched fences, so the code sample renders incorrectly. Indent the block and use matching triple backticks inside the tab (no need for the leading backslash on _). Suggested fix:

-=== "Usage Example"
-
-````csharp
-public sealed class SpiceDBContainerTest : IAsyncLifetime
-{
-private readonly SpiceDBContainer \_spicedbContainer = new SpiceDBBuilder().Build();
-
-        public Task InitializeAsync()
-        {
-            return _spicedbContainer.StartAsync();
-        }
-
-        public Task DisposeAsync()
-        {
-            return _spicedbContainer.DisposeAsync().AsTask();
-        }
-
-        [Fact]
-        public async Task SpiceDBContainer_IsRunning_ReturnsTrue()
-        {
-            // Given
-            var containerState = await _spicedbContainer.GetStateAsync();
-
-            // ...
-        }
-    }
-    ```
+=== "Usage Example"
+
+    ```csharp
+    public sealed class SpiceDBContainerTest : IAsyncLifetime
+    {
+        private readonly SpiceDBContainer _spicedbContainer = new SpiceDBBuilder().Build();
+
+        public Task InitializeAsync()
+        {
+            return _spicedbContainer.StartAsync();
+        }
+
+        public Task DisposeAsync()
+        {
+            return _spicedbContainer.DisposeAsync().AsTask();
+        }
+
+        [Fact]
+        public async Task SpiceDBContainer_IsRunning_ReturnsTrue()
+        {
+            // Given
+            var containerState = await _spicedbContainer.GetStateAsync();
+
+            // ...
+        }
+    }
+    ```
🤖 Prompt for AI Agents
In docs/modules/spicedb.md around lines 13 to 40, the tabbed "Usage Example"
block opens with ````csharp but closes with an indented ``` and contains an
escaped underscore, causing mismatched fences and wrong rendering; fix by
replacing the current block with a properly indented tab content: start the tab
with === "Usage Example", indent the entire code block by four spaces and use
matching triple backticks (```csharp ... ```), remove the backslash before
_spicedbContainer so the identifier is _spicedbContainer, and ensure the opening
and closing backticks align at the same indentation level inside the tab.

@@ -0,0 +1 @@
root = true
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Remove nested root = true.

Setting root = true in a subdirectory severs this project from the repo-wide .editorconfig, so all parent formatting/analyzer rules stop applying to the SpiceDB sources. Please drop this file (or at least the directive) so the shared configuration continues to flow down.

🤖 Prompt for AI Agents
In src/Testcontainers.SpiceDB/.editorconfig around line 1, the file sets "root =
true" which prevents the repo-wide .editorconfig from being inherited; remove
that directive (or delete this .editorconfig entirely) so the parent/shared
formatting and analyzer rules continue to apply to the SpiceDB sources, then
save and commit the change.

Comment on lines +46 to +51
return base.Init()
.WithImage(SpiceDBImage)
.WithPortBinding(SpiceDBgRPCPort, true)
.WithPortBinding(SpiceDBgHTTPPort, true)
.WithCommand("serve", $"--grpc-preshared-key={DockerResourceConfiguration.GrpcPresharedKey}", $"--datastore-engine={DockerResourceConfiguration.DatastoreEngine}", $"--log-level=info")
.WithWaitStrategy(Wait.ForUnixContainer().UntilMessageIsLogged("grpc server started serving"));
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

TLS flag in configuration is ignored, breaking HTTPS clients

SpiceDBConfiguration.TslEnabled flips the connection string to https:// (see SpiceDBContainer.GetGrpcConnectionString), but this start command never enables TLS on the server. SpiceDB still logs insecure=true, so any client honoring the HTTPS URI will try a TLS handshake against an insecure endpoint and fail. Please either propagate TslEnabled into the command (supplying the appropriate --grpc-tls-* flags or certificates) or block the flag until proper TLS support exists; right now setting it to true is a functional regression for consumers.

/// <summary>
/// Initializes a new instance of the <see cref="SpiceDBConfiguration" /> class.
/// </summary>
public SpiceDBConfiguration(string grpcPresharedKey = "mysecret", string datastoreEngine = "memory", bool? tslEnabled = false)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix typo: "Tsl" should be "Tls".

The parameter name tslEnabled and property name TslEnabled contain a typo. The correct abbreviation for "Transport Layer Security" is "TLS", not "TSL".

-    public SpiceDBConfiguration(string grpcPresharedKey = "mysecret", string datastoreEngine = "memory", bool? tslEnabled = false)
+    public SpiceDBConfiguration(string grpcPresharedKey = "mysecret", string datastoreEngine = "memory", bool? tlsEnabled = false)
    {
        GrpcPresharedKey = grpcPresharedKey;
        DatastoreEngine = datastoreEngine;
-        TslEnabled = tslEnabled.GetValueOrDefault(false);
+        TlsEnabled = tlsEnabled.GetValueOrDefault(false);
    }

-    public bool TslEnabled { get; set; }
+    public bool TlsEnabled { get; set; }

Note: This change will also require updates in SpiceDBContainer.cs where TslEnabled is referenced.

Also applies to: 17-17

🤖 Prompt for AI Agents
In src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs around lines 10 and 17,
the boolean parameter and property are misspelled as "tslEnabled"/"TslEnabled";
rename them to "tlsEnabled"/"TlsEnabled" (update the constructor parameter,
backing field/property name, and any XML/comments) and propagate the change to
all references (e.g., update SpiceDBContainer.cs where TslEnabled is referenced)
to ensure consistency and rebuild references/usages across the project.

Comment on lines +24 to +42
/// <summary>
/// Initializes a new instance of the <see cref="SpiceDBConfiguration" /> class.
/// </summary>
/// <param name="resourceConfiguration">The Docker resource configuration.</param>
public SpiceDBConfiguration(IResourceConfiguration<CreateContainerParameters> resourceConfiguration)
: base(resourceConfiguration)
{
// Passes the configuration upwards to the base implementations to create an updated immutable copy.
}

/// <summary>
/// Initializes a new instance of the <see cref="SpiceDBConfiguration" /> class.
/// </summary>
/// <param name="resourceConfiguration">The Docker resource configuration.</param>
public SpiceDBConfiguration(IContainerConfiguration resourceConfiguration)
: base(resourceConfiguration)
{
// Passes the configuration upwards to the base implementations to create an updated immutable copy.
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify property initialization in copy constructors.

The constructors at lines 28-32 and 38-42 rely on the base class to handle configuration copying. Ensure that ContainerConfiguration properly preserves derived class properties, or these constructors may need to explicitly initialize the SpiceDB-specific properties.

Run the following script to check how similar modules handle this pattern:


🏁 Script executed:

#!/bin/bash
# Search for similar configuration patterns in other modules
rg -A 10 'public.*Configuration.*IResourceConfiguration' --type cs -g 'src/Testcontainers.*/Configuration.cs' | head -100

Length of output: 148


I'll help verify this review comment by examining the codebase structure and comparing patterns. Let me gather the necessary information.


---

🏁 Script executed:

```shell
#!/bin/bash
# Find all Configuration.cs files in the Testcontainers modules
fd -t f 'Configuration\.cs$' 'src/Testcontainers'

Length of output: 1408


🏁 Script executed:

#!/bin/bash
# Examine the SpiceDBConfiguration.cs file
cat 'src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs'

Length of output: 2533


🏁 Script executed:

#!/bin/bash
# Find and examine the base ContainerConfiguration class
fd -t f 'ContainerConfiguration\.cs$' --type f

Length of output: 142


🏁 Script executed:

#!/bin/bash
# Look for similar configuration patterns in other Testcontainers modules
find src/Testcontainers.* -name 'Configuration.cs' -type f | head -20

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Search for how other modules define their configurations
rg 'class.*Configuration.*:.*Configuration' --type cs -A 5 -B 2 | head -150

Length of output: 13275


I need to examine the base class and compare with other similar modules to verify the concern. Let me inspect the implementations.


---

🏁 Script executed:

```shell
#!/bin/bash
# Examine the base ContainerConfiguration class
cat 'src/Testcontainers/Configurations/Containers/ContainerConfiguration.cs'

Length of output: 9313


🏁 Script executed:

#!/bin/bash
# Examine WeaviateConfiguration to see how it handles derived properties
cat 'src/Testcontainers.Weaviate/WeaviateConfiguration.cs'

Length of output: 2164


🏁 Script executed:

#!/bin/bash
# Examine RedisConfiguration for comparison
cat 'src/Testcontainers.Redis/RedisConfiguration.cs'

Length of output: 2116


🏁 Script executed:

#!/bin/bash
# Examine RavenDbConfiguration for comparison
cat 'src/Testcontainers.RavenDb/RavenDbConfiguration.cs'

Length of output: 2148


Add explicit property initialization to copy constructors to preserve SpiceDB-specific configuration.

The constructors at lines 28-32 and 38-42 rely on base class handling, but ContainerConfiguration doesn't know about TslEnabled, GrpcPresharedKey, and DatastoreEngine. When these constructors are called with an existing SpiceDBConfiguration, the derived properties are lost.

The copy constructor at lines 54-60 also needs to explicitly merge the derived properties like the base class does with BuildConfiguration.Combine().

Update all copy constructors to preserve SpiceDB-specific properties:

public SpiceDBConfiguration(SpiceDBConfiguration oldValue, SpiceDBConfiguration newValue)
    : base(oldValue, newValue)
{
    TslEnabled = newValue.TslEnabled;
    GrpcPresharedKey = newValue.GrpcPresharedKey;
    DatastoreEngine = newValue.DatastoreEngine;
}
🤖 Prompt for AI Agents
In src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs around lines 24-42 (and
also update the copy constructor around lines 54-60), the derived properties
TslEnabled, GrpcPresharedKey, and DatastoreEngine are not preserved because the
base constructors don’t know about them; change the two constructors that accept
IResourceConfiguration/CreateContainerParameters and IContainerConfiguration to
detect if the provided configuration is a SpiceDBConfiguration (or accept
SpiceDBConfiguration overloads) and explicitly copy those three properties from
the incoming instance to this instance, and update the
SpiceDBConfiguration(SpiceDBConfiguration oldValue, SpiceDBConfiguration
newValue) copy constructor to assign TslEnabled, GrpcPresharedKey, and
DatastoreEngine from newValue after calling base(oldValue, newValue).

Comment on lines +54 to +62
/// <summary>
/// Initializes a new instance of the <see cref="SpiceDBConfiguration" /> class.
/// </summary>
/// <param name="oldValue">The old Docker resource configuration.</param>
/// <param name="newValue">The new Docker resource configuration.</param>
public SpiceDBConfiguration(SpiceDBConfiguration oldValue, SpiceDBConfiguration newValue)
: base(oldValue, newValue)
{
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Merge constructor must copy SpiceDB-specific properties.

The merge constructor doesn't copy the SpiceDB-specific properties (GrpcPresharedKey, DatastoreEngine, TslEnabled) from oldValue and newValue. This will cause configuration loss when the builder clones or merges configurations.

    public SpiceDBConfiguration(SpiceDBConfiguration oldValue, SpiceDBConfiguration newValue)
        : base(oldValue, newValue)
    {
+        GrpcPresharedKey = BuildConfiguration.Combine(oldValue, newValue, @this => @this.GrpcPresharedKey);
+        DatastoreEngine = BuildConfiguration.Combine(oldValue, newValue, @this => @this.DatastoreEngine);
+        TslEnabled = BuildConfiguration.Combine(oldValue, newValue, @this => @this.TslEnabled);
    }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/Testcontainers.SpiceDB/SpiceDBConfiguration.cs around lines 54 to 62, the
merge constructor currently only calls the base ctor and fails to copy
SpiceDB-specific properties; update the constructor to assign
this.GrpcPresharedKey, this.DatastoreEngine and this.TslEnabled from the
incoming newValue (falling back to oldValue if newValue is null or its property
is not set) so merged/cloned configurations preserve those fields—use null
checks/conditional operators to prefer newValue values and fallback to oldValue
values, handling boolean/nullability appropriately.

Comment on lines +817 to +820
{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Debug|Any CPU.Build.0 = Debug|Any CPU
{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Release|Any CPU.ActiveCfg = Release|Any CPU
{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Release|Any CPU.Build.0 = Release|Any CPU
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: GUID mismatch detected.

Lines 817-820 reference GUID {A9F554E5-9183-4F8A-B226-58DD46BB2060}, but the project declaration on Line 283 uses {64B27088-14DC-4CA2-B24E-5D0D5BA14355}. This mismatch will cause build configuration issues.

-		{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
-		{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Debug|Any CPU.Build.0 = Debug|Any CPU
-		{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Release|Any CPU.ActiveCfg = Release|Any CPU
-		{A9F554E5-9183-4F8A-B226-58DD46BB2060}.Release|Any CPU.Build.0 = Release|Any CPU
+		{64B27088-14DC-4CA2-B24E-5D0D5BA14355}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{64B27088-14DC-4CA2-B24E-5D0D5BA14355}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{64B27088-14DC-4CA2-B24E-5D0D5BA14355}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{64B27088-14DC-4CA2-B24E-5D0D5BA14355}.Release|Any CPU.Build.0 = Release|Any CPU

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In Testcontainers.sln around lines 817 to 820, the project configuration entries
use GUID {A9F554E5-9183-4F8A-B226-58DD46BB2060} which does not match the
project's GUID {64B27088-14DC-4CA2-B24E-5D0D5BA14355} declared at line 283;
update the four configuration lines (Debug|Any CPU.ActiveCfg, Debug|Any
CPU.Build.0, Release|Any CPU.ActiveCfg, Release|Any CPU.Build.0) to use the
correct GUID {64B27088-14DC-4CA2-B24E-5D0D5BA14355} (or if the project GUID at
line 283 is wrong, update that one instead so they match), ensure all
occurrences across the .sln are consistent, then save and verify the solution
loads/builds successfully.

@@ -0,0 +1 @@
root = true
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Keep inheriting the repo .editorconfig.

Marking this test-project .editorconfig as root = true blocks all shared editor/analyzer settings from the rest of the repository. Please remove the directive (or the file) so the tests inherit the common rules.

🤖 Prompt for AI Agents
In tests/Testcontainers.SpiceDB.Tests/.editorconfig at line 1, the file sets
"root = true" which prevents inheriting the repository-wide .editorconfig;
remove the "root = true" directive (or delete the .editorconfig file entirely)
so this test project inherits the shared editor/analyzer settings from the repo
root, then run a quick format/lint check to confirm settings are applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request module An official Testcontainers module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants