feat(cassandra): add ssl option cassandra #3151
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
MitulShah1
changed the title
mdelapenya
changed the title
# Conflicts: # go.sum
modules/cassandra/cassandra.go
Outdated
| ), | ||
| } | ||
|
|
||
| c := &CassandraContainer{} |
There was a problem hiding this comment.
suggestion: keep this initialisation next to its usage. Seems moved here for no reasons
modules/cassandra/cassandra.go
Outdated
| data, _ := io.ReadAll(body) | ||
| return strings.Contains(string(data), "COMPLETED") | ||
| }), | ||
| }).WithStartupTimeout(2*time.Minute), |
There was a problem hiding this comment.
question: is 2 minutes too long? Can we use a shorter duration?
modules/cassandra/cassandra.go
Outdated
| "CASSANDRA_SKIP_WAIT_FOR_GOSSIP": "1", | ||
| "CASSANDRA_START_NATIVE_TRANSPORT": "true", |
There was a problem hiding this comment.
question: are these new env vars related to the SSL options? If not, I'd separate the changes into a new PR, if possible
modules/cassandra/cassandra.go
Outdated
| } | ||
|
|
||
| // WithTLS is an alias for WithSSL for user convenience | ||
| func WithTLS(opts SSLOptions) testcontainers.CustomizeRequestOption { |
There was a problem hiding this comment.
question: why adding two flavours of the same option? I'd just choose one to avoid creating confusion on users
modules/cassandra/cassandra.go
Outdated
| } | ||
|
|
||
| // Mark that SSL is enabled for later use | ||
| req.Labels = mergeMap(req.Labels, map[string]string{"testcontainers.cassandra.ssl": "true"}) |
There was a problem hiding this comment.
question: I guess this is needed to notify the Run function about the SSL addition, right? I'd choose a different pattern for this, creating an options type as in the redis module. Then, you can simply generate the SSL certs on-the-fly as in that module.
I made changes, can you please check again? |
modules/cassandra/options.go
Outdated
| IsTLSEnabled bool | ||
| TLSConfig *tls.Config |
There was a problem hiding this comment.
suggestion: make these private so there's less API surface area allowing for refactoring without causing breaking changes. Given it's typed nature we can skip the Is prefix for boolean flag too.
| IsTLSEnabled bool | |
| TLSConfig *tls.Config | |
| tlsEnabled bool | |
| tlsConfig *tls.Config |
More fixes to align with this change obviously, so best to just rename in your editor.
| // GenerateJKSKeystore generates a JKS keystore with a self-signed cert using keytool, and extracts the public cert for Go client trust. | ||
| func GenerateJKSKeystore() (keystorePath, certPath string, err error) { |
There was a problem hiding this comment.
suggestion: make private, again less API surface area to maintain. Also needs to use go not an external tool which might not be present on all platforms. See other modules on how they generate keys, hopefully that's compatible with cassandra.
There was a problem hiding this comment.
@stevenh making this function private, i need to change in options_test.go, its ok?
There was a problem hiding this comment.
Either that or make a seperate test file.
There was a problem hiding this comment.
options_test.go is created by me only but i cannot access that function as testcases are depend on it, do you want me to make this function private and remove options_test.go?
There was a problem hiding this comment.
Up to you, having integration and unit tests are both good. In go its not a problem to use the main package for tests, as long as you're conscious about the seperation.
However can we should first see if we can eliminate this external dependency. You can see a similar example here: https://github.com/testcontainers/testcontainers-go/blob/main/modules/redis/options.go#L86 which uses https://pkg.go.dev/github.com/mdelapenya/tlscert
There was a problem hiding this comment.
It seems you're suggesting we either minimize SSL-related dependencies or simply support SSL with minimal setup. I always try to follow the existing repository codebase and coding style as much possible.
Initially, I tried using only tlscert, but it has some limitations. If we choose to use tlscert, we can eliminate the keytool dependency, but we'd then need to rely on openssl.
So, we have two options:
Use keytool as we currently do.
Follow an approach similar to Cassandra's Go driver, where certificates are added in a testdata folder and used for SSL testing, as seen in the official Go driver.
There was a problem hiding this comment.
tlscert would be the preferred option as it's what a lot of the other modules use.
If it needs changes that should be quick and easy as that module is written by @mdelapenya one of this repositories maintainers.
There was a problem hiding this comment.
i know but it can handle this cassandra cert generation? @mdelapenya ? i already tried that let me know if ok to use openssl then i will make change to tlscert.
There was a problem hiding this comment.
Yes, let's use it for consistency. If there is a missing feature in that library, let's open an issue in there and work on fix and release it
Cheers!
There was a problem hiding this comment.
A Quick Look and it seems like it can use standard pen formatted certs so it should just work
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // GenerateJKSKeystore generates a JKS keystore with a self-signed cert using keytool, and extracts the public cert for Go client trust. | ||
| func GenerateJKSKeystore() (keystorePath, certPath string, err error) { |
There was a problem hiding this comment.
Up to you, having integration and unit tests are both good. In go its not a problem to use the main package for tests, as long as you're conscious about the seperation.
However can we should first see if we can eliminate this external dependency. You can see a similar example here: https://github.com/testcontainers/testcontainers-go/blob/main/modules/redis/options.go#L86 which uses https://pkg.go.dev/github.com/mdelapenya/tlscert
| const ( | ||
| port = nat.Port("9042/tcp") | ||
| port = nat.Port("9042/tcp") | ||
| securePort = nat.Port("9142/tcp") // Common port for SSL/TLS connections |
There was a problem hiding this comment.
question: do we need to two ports or can it just be one which is either SSL or not?
| waitStrategies := []wait.Strategy{ | ||
| wait.ForListeningPort(port), | ||
| wait.ForExec([]string{"cqlsh", "-e", "SELECT bootstrapped FROM system.local"}).WithResponseMatcher(func(body io.Reader) bool { | ||
| data, _ := io.ReadAll(body) | ||
| return strings.Contains(string(data), "COMPLETED") | ||
| }).WithStartupTimeout(1 * time.Minute), | ||
| } |
There was a problem hiding this comment.
@mdelapenya looks like we have an issue with WithWaitStrategy, it sets vs appending to WaitFor preventing easy initialisation here.
There was a problem hiding this comment.
Indeed. I think the use case here would be appending. I can submit a fix now
|
|
||
| // Apply wait strategy using the correct method | ||
| if err := testcontainers.WithWaitStrategy(wait.ForAll(waitStrategies...)).Customize(&genericContainerReq); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
suggestion: wrap the error here
| return nil, err | |
| return nil, fmt.Errorf("with wait strategy: %w", err) |
| // TLSConfig returns the TLS configuration for the Cassandra container, nil if TLS is not enabled. | ||
| func (c *CassandraContainer) TLSConfig() *tls.Config { |
There was a problem hiding this comment.
@mdelapenya quite a common pattern in modules, we should consider returning an error instead.
There was a problem hiding this comment.
You mean the pointer to the config and the error, right? I think we changed the tlscert library precisely for that, so good to me to change it
There was a problem hiding this comment.
Nope, these container methods, have them return an error if the config is nil vs just returning nil as the caller should know if they enabled TLS or not.
func (c *CassandraContainer) TLSConfig() (*tls.Config, error) {
if c.settings.tlsConfig == nil {
return nil, errors.New("tls not enabled")
}
return c.settings.tlsConfig.Config, nil
}There was a problem hiding this comment.
Oh I see, the recently added methods for Redis and Valkey. For Valkey, we are in the same release cycle, we can change it without BC, for Redis, it's a BC
| func (o Option) Customize(req *testcontainers.GenericContainerRequest) error { | ||
| return o(req, &Options{}) |
There was a problem hiding this comment.
bug: while this will run there options won't be accessible, so should still be just a noop.
We're looking to refactor how this works in a future version to avoid this, but that will be a bit.
| Started: true, | ||
| } | ||
|
|
||
| var settings Options |
There was a problem hiding this comment.
bug: settings is never set, you still need to check if you have an Option in the for loop and process it to ensure it set correctly.
If your tests are passing they shouldn't so might need a fix there too.
| // TLSConfig represents the TLS configuration for Cassandra | ||
| type TLSConfig struct { |
There was a problem hiding this comment.
question: are the paths needed? If not we can just return *tls.Config. I suspect the answer is dependent on eliminating the need for keytool
|
@MitulShah1 did you have the chance to take a look a the comments here? |
Sorry lil busy, i will check and try to finish this week. |
|
@mdelapenya @MitulShah1 any progress on this? |
Hey @gaby @mdelapenya sorry for delay but i tried with simple tlc certy which @stevenh mentioned with @mdelapenya library seems its not working with cassandra. so just stuck on this point only. |
# Conflicts: # modules/cassandra/cassandra.go
Summary by CodeRabbit
WalkthroughAdds TLS/SSL support and option-driven configuration to the Cassandra testcontainer module, including secure port exposure, TLS-aware connection host selection, keystore/cert generation, TLS wait strategies, and a public TLSConfig accessor. Updates tests and examples to cover SSL flows and introduces a Cassandra SSL YAML test config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant CassandraContainer
participant Options
participant Keytool
participant Testcontainers as Testcontainers-Go
participant Cassandra as Cassandra (container)
participant Client as gocql Client
Caller->>CassandraContainer: Run(ctx, req, WithSSL(), WithConfigFile(...))
CassandraContainer->>Options: Apply WithSSL
Options->>Keytool: GenerateJKSKeystore()
Keytool-->>Options: keystore.jks, cert.pem
Options-->>CassandraContainer: TLSConfig, secure port (9142), files
CassandraContainer->>Testcontainers: GenericContainerRequest (ExposedPorts 9042,9142 + waits)
Testcontainers->>Cassandra: Start container
Cassandra-->>Testcontainers: Service up
Testcontainers-->>CassandraContainer: Container handle
note over CassandraContainer,Client: TLS-enabled flow
Caller->>CassandraContainer: TLSConfig()
CassandraContainer-->>Caller: *tls.Config (or nil)
Caller->>Client: Create cluster with SslOptions/TLSConfig and host:9142
Client->>Cassandra: Connect, query system.local
Cassandra-->>Client: release_version
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
modules/cassandra/options.go (3)
15-20: Make TLSConfig fields private to reduce API surface area.The public fields increase the API surface area and make future refactoring difficult. Since these fields are accessed through the TLSConfig() method, they should be private.
Based on past review feedback, if the tlscert library is adopted (as requested), the paths may not be needed at all—only the
*tls.Configwould be necessary.Apply this diff to make the fields private:
// TLSConfig represents the TLS configuration for Cassandra type TLSConfig struct { - KeystorePath string - CertificatePath string - Config *tls.Config + keystorePath string + certificatePath string + config *tls.Config }Then update the accessor method and any references accordingly.
31-33: Critical bug: Options are discarded on every call.Line 32 creates a fresh
Options{}instead of accumulating settings across multiple option calls. This means any TLS configuration set byWithSSL()will be lost, and the container won't have access to the TLS settings.This exact issue was flagged in previous reviews: "while this will run there options won't be accessible."
The current implementation should be treated as a no-op until the broader options refactoring is completed. However, to make it functional in the interim, the accumulated settings need to be passed through. The
WithSSLfunction should directly modify theGenericContainerRequestwithout relying on theOptionsstruct being carried forward byCustomize.One interim approach is to store the accumulated options outside the
Customizecall chain, but this requires broader changes to howRunaccesses the settings. Seemodules/cassandra/cassandra.golines 79-135 for how settings are currently extracted.
81-122: Make function private and plan for removal.This function was requested to be made private in previous reviews to reduce API surface area. More importantly, it should be removed entirely once the tlscert library is adopted (see comment on lines 35-79).
The reliance on external
keytoolcreates several issues:
- Not available on all platforms (requires Java/JDK installation)
- Harder to maintain and test
- Inconsistent with other testcontainers-go modules
If keeping this temporarily during the transition:
-// GenerateJKSKeystore generates a JKS keystore with a self-signed cert using keytool, and extracts the public cert for Go client trust. -func GenerateJKSKeystore() (keystorePath, certPath string, err error) { +// generateJKSKeystore generates a JKS keystore with a self-signed cert using keytool, and extracts the public cert for Go client trust. +// Deprecated: Will be removed once tlscert-based implementation is complete. +func generateJKSKeystore() (keystorePath, certPath string, err error) {Then update the call in
WithSSL(line 40) accordingly. However, prioritize replacing this entirely with the tlscert approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
modules/cassandra/cassandra.go(3 hunks)modules/cassandra/cassandra_test.go(7 hunks)modules/cassandra/examples_test.go(2 hunks)modules/cassandra/options.go(1 hunks)modules/cassandra/options_test.go(1 hunks)modules/cassandra/testdata/cassandra-ssl.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
modules/cassandra/examples_test.go (4)
modules/cassandra/cassandra.go (2)
Run(80-136)WithConfigFile(42-52)modules/cassandra/options.go (2)
WithSSL(36-79)TLSConfig(16-20)cleanup.go (1)
TerminateContainer(97-108)log/logger.go (1)
Printf(47-49)
modules/cassandra/options.go (3)
generic.go (1)
GenericContainerRequest(21-27)container.go (1)
ContainerFile(110-115)modules/cassandra/cassandra.go (1)
Run(80-136)
modules/cassandra/cassandra_test.go (3)
modules/cassandra/cassandra.go (3)
Run(80-136)WithConfigFile(42-52)WithInitScripts(55-71)modules/cassandra/options.go (2)
WithSSL(36-79)TLSConfig(16-20)testing.go (1)
CleanupContainer(91-97)
modules/cassandra/cassandra.go (7)
modules/cassandra/options.go (2)
Options(23-25)TLSConfig(16-20)container.go (1)
ContainerRequest(131-171)generic.go (2)
GenericContainerRequest(21-27)GenericContainer(52-98)wait/host_port.go (1)
ForListeningPort(67-69)wait/exec.go (1)
ForExec(71-73)options.go (1)
WithWaitStrategy(366-368)wait/all.go (1)
ForAll(44-48)
modules/cassandra/options_test.go (3)
generic.go (1)
GenericContainerRequest(21-27)container.go (1)
ContainerRequest(131-171)modules/cassandra/options.go (4)
Options(23-25)WithSSL(36-79)TLSConfig(16-20)GenerateJKSKeystore(82-122)
🪛 GitHub Actions: Main pipeline
modules/cassandra/cassandra.go
[error] 17-17: undefined: nat
⏰ 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 (go)
🔇 Additional comments (1)
modules/cassandra/options.go (1)
35-79: Cannot confirm PEM-based TLS support in test data
I didn’t find anystore_type: PEMentries inmodules/cassandra/testdata. Please verify that your Cassandra version (4.0+) and testdata configuration support PEM-based TLS or update the testdata accordingly.
| import ( | ||
| "context" | ||
| "crypto/tls" | ||
| "fmt" | ||
| "io" | ||
| "path/filepath" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/testcontainers/testcontainers-go" | ||
| "github.com/testcontainers/testcontainers-go/wait" | ||
| ) |
There was a problem hiding this comment.
Add the nat import to fix compilation.
nat.Port is used for port/securePort, but github.com/docker/go-connections/nat is not imported, so the branch fails to build (undefined: nat in the pipeline). Please add the missing import:
import (
"context"
"crypto/tls"
"fmt"
"io"
"path/filepath"
"strings"
"time"
+ "github.com/docker/go-connections/nat"
"github.com/testcontainers/testcontainers-go"
"github.com/testcontainers/testcontainers-go/wait"
)Based on pipeline failure logs.
📝 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.
| import ( | |
| "context" | |
| "crypto/tls" | |
| "fmt" | |
| "io" | |
| "path/filepath" | |
| "strings" | |
| "time" | |
| "github.com/testcontainers/testcontainers-go" | |
| "github.com/testcontainers/testcontainers-go/wait" | |
| ) | |
| import ( | |
| "context" | |
| "crypto/tls" | |
| "fmt" | |
| "io" | |
| "path/filepath" | |
| "strings" | |
| "time" | |
| "github.com/docker/go-connections/nat" | |
| "github.com/testcontainers/testcontainers-go" | |
| "github.com/testcontainers/testcontainers-go/wait" | |
| ) |
🤖 Prompt for AI Agents
In modules/cassandra/cassandra.go around lines 3 to 14, the code uses nat.Port
for port/securePort but does not import github.com/docker/go-connections/nat,
causing a compile error (undefined: nat); fix by adding the missing import
("github.com/docker/go-connections/nat") to the import block (keep the import
alias as nat if needed) so nat.Port is available to the file.
| require.Equal(t, keystorePath, newKeystorePath) | ||
| require.Equal(t, certPath, newCertPath) | ||
|
|
||
| // Get new file info | ||
| newKeystoreInfo, err := os.Stat(keystorePath) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify that the file was modified | ||
| require.NotEqual(t, initialKeystoreInfo.ModTime(), newKeystoreInfo.ModTime(), "keystore should be overwritten") |
There was a problem hiding this comment.
Avoid shared keystore paths across invocations.
GenerateJKSKeystore always returns /tmp/keystore.jks and cert.pem. When two Cassandra containers enable SSL at the same time, the second call removes and regenerates those files while the first container still holds the original certificate in its TLS config. The container then ships with the new cert while the client still trusts the old one, so the TLS handshake breaks. Please generate unique temp files per invocation (for example via os.CreateTemp) and update the tests to expect distinct paths so concurrent runs stay correct.
🤖 Prompt for AI Agents
In modules/cassandra/options_test.go around lines 66 to 74, the test and
implementation rely on fixed paths (/tmp/keystore.jks and cert.pem) causing race
conditions when multiple invocations overwrite the same files; change
GenerateJKSKeystore to create unique temporary files (e.g., use os.CreateTemp or
os.MkdirTemp + unique filenames) for both the keystore and cert, return those
paths, and ensure the function still writes the keystore/cert and cleans up
where appropriate; update the test to assert that the returned keystore and cert
paths are distinct from one another and from any previous run (use the returned
paths to stat the files and compare mod times) instead of assuming fixed /tmp
paths so concurrent runs do not clobber each other.
|
@gaby @MitulShah1 thanks for the ping. I'll resolve the conflicts locally and will try to check the TLS issues mentioned above. @MitulShah1 can you elaborate on those issues with tlscert? |
4 participants
#3124
What does this PR do?
Why is it important?
Related issues
@mdelapenya Can you please review