feat(cassandra): add ssl option cassandra#3151
feat(cassandra): add ssl option cassandra#3151mdelapenya merged 35 commits intotestcontainers:mainfrom
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
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
Looking good, although I added some comments about using the same approach as in the Redis module. Could you please take a look?
Cheers!
I made changes, can you please check again? |
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
Thanks for the quick updates, here's some suggestion which focus on simplification and reducing API surface area.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@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
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds optional TLS support to Cassandra containers: generates CA and server certificates, creates a PKCS#12 keystore, exposes SSL port 9142, introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test / Client
participant Runner as CassandraContainer.Run
participant TLSGen as createTLSCerts()
participant Container as Testcontainers GenericContainer
participant Cassandra as Cassandra process
participant ClientLib as gocql (TLS client)
Test->>Runner: Run(..., WithTLS())
Runner->>TLSGen: generate CA, server cert, PKCS#12 keystore
TLSGen-->>Runner: tlsCerts (keystore bytes, tls.Config)
Runner->>Container: Start with mounted keystore & cassandra-ssl.yaml, expose 9142
Container->>Cassandra: Cassandra starts using client_encryption_options (keystore)
Runner->>Runner: store settings.TLSConfig
Test->>ClientLib: build cluster using TLSConfig()
ClientLib->>Cassandra: Establish TLS connection on 9142
Cassandra-->>ClientLib: TLS handshake + query responses
ClientLib-->>Test: Query results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-09-29T13:57:14.636ZApplied to files:
📚 Learning: 2025-09-29T15:08:18.694ZApplied to files:
📚 Learning: 2025-11-05T11:57:09.224ZApplied to files:
🪛 LanguageTooldocs/modules/cassandra.md[grammar] ~73-~73: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) ⏰ 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). (3)
🔇 Additional comments (10)
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.
|
@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? |
# Conflicts: # modules/cassandra/cassandra.go
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/cassandra/cassandra.go (1)
80-107:Runhas multiple compile-time and logic issues (invalid literal, unused vars, wrong identifiers)This block currently won’t compile and also doesn’t appear to use the new
ContainerRequest:
req := testcontainers.ContainerRequest{ ... }:
- The struct literal is syntactically invalid:
Env: map[string]string{...}), testcontainers.WithWaitStrategy(...),leaves an extra)after the map literal and introducestestcontainers.WithWaitStrategy(...)as a bare value in the struct, which is not keyed to any field. This matches the golangci-lint “unexpected ) in composite literal” error.reqis never used, so even once syntax is fixed you’ll getreq declared and not used.ctr, err := testcontainers.Run(ctx, img, moduleOpts...):
ctris never used; later you referencecontainerinstead, which is undefined in this function.- In the next lines:
if container != nil { c = &CassandraContainer{Container: container, settings: settings} }
containerisn’t declared in this scope; you probably meantctr.settingsmust come from the Options processing path; make sure it’s in scope here and set accordingly.You’ll need to decide whether
Runshould:
- Use
testcontainers.Run(ctx, img, ...)withContainerCustomizers that set env and wait strategy (in which case you should remove the unusedreqentirely), or- Build and pass a
ContainerRequestthrough the lower-level API (GenericContainerRequest), in which casetestcontainers.Run’s signature might not be the right call.At minimum, to fix the immediate issues:
- Make the struct literal valid or remove
reqif not used.- Replace
containerwithctr(and actually usectr), and ensuresettingsis in scope and populated.- Confirm
moduleOptsis defined in this package and includes any TLS/SSL customizers you intend.Until these are addressed, this function will not compile.
♻️ Duplicate comments (2)
modules/cassandra/cassandra.go (2)
115-121: Consider returning(*tls.Config, error)fromTLSConfiginstead of a bare pointerThe new
TLSConfig()accessor returnsnilwhen TLS is not enabled. That works, but callers then have to remember to check for nil vs. treat it as a hard error.Given prior discussion on similar methods in Redis/Valkey, it would be more explicit to return an error when TLS isn’t enabled, e.g.:
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 }This makes misuse easier to detect and keeps patterns consistent across modules.
Check the current Redis and Valkey modules in testcontainers-go to confirm the preferred TLSConfig API shape (returning (*tls.Config, error) vs *tls.Config).
3-19: Missingnatimport breaks the new port constants
portandsecurePortare declared asnat.Port("9042/tcp")/"9142/tcp", butgithub.com/docker/go-connections/natis not imported. This file will not compile (undefined: nat).Add the missing import to the import block:
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" )
🧹 Nitpick comments (1)
modules/cassandra/cassandra.go (1)
21-37: Ensuresettingsis correctly initialised before using it inConnectionHostThe new TLS-aware
ConnectionHostpickssecurePortwhenc.settings.tlsConfig != nil, otherwise falls back to the plainport. That logic is fine, but it assumes:
c.settingsis always initialised with the effectiveOptionswhen the container is created.tlsConfigis only non‑nil when TLS is actually enabled.Please double-check that
Run(and any other constructors) always populatesettingscorrectly, including the non‑TLS path, soConnectionHostnever sees a zero‑valueOptionsdue to missed wiring. This will also keep behavior consistent with the TLSConfig accessor below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/cassandra/cassandra.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
modules/cassandra/cassandra.go
📚 Learning: 2025-09-29T15:08:18.694Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3320
File: modules/artemis/artemis.go:98-103
Timestamp: 2025-09-29T15:08:18.694Z
Learning: In testcontainers-go, nat.Port is a type alias for string, so untyped string constants can be passed directly to functions expecting nat.Port (like wait.ForListeningPort) without explicit type conversion - the Go compiler handles the implicit conversion automatically.
Applied to files:
modules/cassandra/cassandra.go
🪛 GitHub Actions: Main pipeline
modules/cassandra/cassandra.go
[error] 1-1: syntax error: unexpected ) in composite literal; possibly missing comma or } (from golangci-lint)
[error] 90-90: syntax error: unexpected ) in composite literal; possibly missing comma or } (from golangci-lint)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
modules/cassandra/cassandra.go (1)
40-44: Consider returning(*tls.Config, error)instead of*tls.ConfigRight now callers have to infer “TLS not enabled” from a nil pointer. Exposing:
func (c *CassandraContainer) TLSConfig() (*tls.Config, error)with an explicit error when TLS wasn’t enabled would align better with the pattern discussed for other modules (e.g. Redis/Valkey) and avoid silent misuse.
🧹 Nitpick comments (1)
modules/cassandra/cassandra.go (1)
120-161: Clarify interaction betweenWithTLSandWithConfigFileforcassandra.yamlIn the TLS branch you mount an embedded
cassandra-ssl.yamlto/etc/cassandra/cassandra.yaml, and user options are appended afterwards:moduleOpts = append(moduleOpts, opts...)
WithConfigFilealso writes to/etc/cassandra/cassandra.yaml, so combiningWithTLS()andWithConfigFile(...)means whichever runs last wins, which could silently disable the TLS configuration.Consider either:
- documenting that
WithTLSshould not be combined withWithConfigFile, or- detecting this combination and returning an error (or choosing a clear precedence) to avoid surprising behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
modules/cassandra/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
modules/cassandra/cassandra.go(4 hunks)modules/cassandra/cassandra_test.go(1 hunks)modules/cassandra/examples_test.go(1 hunks)modules/cassandra/go.mod(1 hunks)modules/cassandra/options.go(1 hunks)modules/cassandra/testdata/cassandra-ssl.yaml(1 hunks)modules/cassandra/tls.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/cassandra/testdata/cassandra-ssl.yaml
- modules/cassandra/options.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
modules/cassandra/go.mod
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
modules/cassandra/cassandra.go
📚 Learning: 2025-09-29T15:08:18.694Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3320
File: modules/artemis/artemis.go:98-103
Timestamp: 2025-09-29T15:08:18.694Z
Learning: In testcontainers-go, nat.Port is a type alias for string, so untyped string constants can be passed directly to functions expecting nat.Port (like wait.ForListeningPort) without explicit type conversion - the Go compiler handles the implicit conversion automatically.
Applied to files:
modules/cassandra/cassandra.go
🧬 Code graph analysis (3)
modules/cassandra/examples_test.go (3)
modules/cassandra/cassandra.go (1)
Run(90-176)modules/cassandra/options.go (1)
WithTLS(42-47)cleanup.go (1)
TerminateContainer(97-108)
modules/cassandra/cassandra_test.go (2)
modules/cassandra/cassandra.go (1)
Run(90-176)modules/cassandra/options.go (1)
WithTLS(42-47)
modules/cassandra/cassandra.go (3)
modules/cassandra/options.go (1)
Option(19-19)options.go (1)
WithFiles(524-529)modules/redis/redis.go (1)
Run(58-139)
⏰ 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). (3)
- GitHub Check: test (1.24.x, modules/cassandra) / test: modules/cassandra/1.24.x
- GitHub Check: test (1.25.x, modules/cassandra) / test: modules/cassandra/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/cassandra/go.mod (1)
7-13: TLS dependencies for Cassandra module look appropriateThe added module dependencies line up with the new TLS certificate and PKCS#12 handling in this module; no issues from a modularity or usage standpoint.
modules/cassandra/cassandra_test.go (1)
121-170: TLS integration test gives good end‑to‑end coverageThis test exercises the TLS option, validates that a TLS config is exposed, and runs real queries over the TLS connection, which is exactly what we need to guard the new behavior.
modules/cassandra/examples_test.go (1)
71-133: TLS example is clear and consistent with the non‑TLS exampleThe example mirrors the existing non‑TLS flow while showing how to enable TLS and use the exposed TLSConfig, so it’s a useful and consistent addition.
modules/cassandra/tls.go (1)
80-96: The proposed change to removeInsecureSkipVerifymay introduce hostname verification failuresSetting
InsecureSkipVerify: trueis generally a security concern, but removing it without ensuring the self-signed certificate includes "localhost" as a Subject Alternative Name (SAN) or Common Name (CN) will cause TLS handshake failures. Certificates generated in container environments are typically issued for the container's hostname, not "localhost".To properly address this:
- Either regenerate the self-signed certificate to include "localhost" as a SAN, then remove
InsecureSkipVerify: true- Or document why
InsecureSkipVerify: trueis necessary for this test-only setup and consider an alternative hostname that matches the certSimply removing the line without fixing the underlying hostname mismatch will break the tests.
modules/cassandra/cassandra.go (1)
91-99: Verify that theOptiontype includes theCustomizemethod to implementContainerCustomizerThe code at lines 91-99 correctly performs a type assertion to apply module-specific options, but this pattern requires
Optionto implement thetestcontainers.ContainerCustomizerinterface. According to the testcontainers-go module pattern,Optionmust define aCustomize(req *testcontainers.GenericContainerRequest) errormethod (typically a no-op that returns nil) alongside its function signaturetype Option func(*options) error.Without this method, callers passing
cassandra.WithTLS()and similar option functions toRunastestcontainers.ContainerCustomizerparameters will either fail to compile or silently skip the option, leaving settings liketlsEnabledunset.
|
@mdelapenya @stevenh thanks for patience, i've fixed all issues please have a look again and let me know if any changes :) |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM! Thanks @MitulShah1 for your work and sorry for the late review. I just added a few comments regarding the missing docs, which are needed. But I'm already approving the PR waiting for the docs to be in place.
Cheers!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/modules/cassandra.md(1 hunks)modules/cassandra/cassandra.go(4 hunks)modules/cassandra/cassandra_test.go(1 hunks)modules/cassandra/examples_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/cassandra/examples_test.go
- modules/cassandra/cassandra_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
modules/cassandra/cassandra.go
📚 Learning: 2025-09-29T15:08:18.694Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3320
File: modules/artemis/artemis.go:98-103
Timestamp: 2025-09-29T15:08:18.694Z
Learning: In testcontainers-go, nat.Port is a type alias for string, so untyped string constants can be passed directly to functions expecting nat.Port (like wait.ForListeningPort) without explicit type conversion - the Go compiler handles the implicit conversion automatically.
Applied to files:
modules/cassandra/cassandra.go
📚 Learning: 2025-11-05T11:57:09.224Z
Learnt from: natsoman
Repo: testcontainers/testcontainers-go PR: 3452
File: modules/cosmosdb/cosmosdb.go:11-14
Timestamp: 2025-11-05T11:57:09.224Z
Learning: In the testcontainers-go CosmosDB module (modules/azure/cosmosdb/), the defaultProtocol is intentionally set to "http" (not "https") to avoid requiring users to perform certificate setup steps (downloading and trusting the emulator's self-signed certificate). This design prioritizes ease of use for test containers over strict emulator fidelity.
Applied to files:
modules/cassandra/cassandra.go
🪛 GitHub Actions: Main pipeline
modules/cassandra/cassandra.go
[error] 44-44: golangci-lint: error-format: fmt.Errorf can be replaced with errors.New (perfsprint)
🪛 LanguageTool
docs/modules/cassandra.md
[grammar] ~73-~73: Ensure spelling is correct
Context: ...the container will fail to start. #### WithTLS - Not available until the next release <a ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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 (9)
modules/cassandra/cassandra.go (9)
4-7: LGTM!The added imports support the TLS implementation correctly:
bytesfor config reader,crypto/tlsfor TLS types, andembedfor bundling the SSL config file.
18-19: LGTM!Standard Cassandra port definitions: 9042 for native protocol and 9142 for SSL-enabled native protocol.
22-23: LGTM!Embedding the SSL configuration file is a good practice that ensures the TLS setup is self-contained within the module.
28-28: LGTM!The
settingsfield stores the container's options, including TLS state, which is necessary for the TLS feature implementation.
34-36: LGTM!Correctly routes connection requests to the SSL port when TLS is enabled.
94-102: LGTM!The option processing correctly implements the options pattern, extracting and applying custom options before container creation.
159-162: LGTM!Adding a wait strategy for the SSL port ensures the container is fully ready before client connections are attempted. The use of
WithAdditionalWaitStrategyproperly complements the existing wait strategies.
105-105: Verify whether the non-SSL port should remain exposed when TLS is enabled.Both the regular port (9042) and SSL port (9142) are exposed when TLS is enabled. If the non-SSL port remains functional, clients could bypass encryption, creating a security gap.
Confirm whether:
- The embedded
cassandra-ssl.yamlconfiguration disables the non-SSL port (9042)- It is intentional to allow both SSL and non-SSL connections simultaneously
Also applies to: 136-136
138-138: Confirm whether CASSANDRA_BROADCAST_RPC_ADDRESS should be "127.0.0.1" in a testcontainers context.Setting
CASSANDRA_BROADCAST_RPC_ADDRESSto127.0.0.1will cause clients connecting from the host machine to attempt connecting back to their own localhost when using Cassandra's cluster metadata discovery. While this may work for single-node test setups where clients don't rely on cluster metadata, it will break multi-node configurations and certain Cassandra driver behaviors that fetch node information from the cluster.The standard pattern for testcontainers is to either use the container's internal bridge network address or rely on port mapping for discovery. Verify with a test that clients can successfully connect and discover nodes when this configuration is active.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/modules/cassandra.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/modules/cassandra.md
[grammar] ~73-~73: Ensure spelling is correct
Context: ...the container will fail to start. #### WithTLS - Not available until the next release <a ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (3)
- GitHub Check: test (1.25.x, modules/cassandra) / test: modules/cassandra/1.25.x
- GitHub Check: test (1.24.x, modules/cassandra) / test: modules/cassandra/1.24.x
- GitHub Check: Analyze (go)
MitulShah1
force-pushed
the
add-ssl-option-cassandra
branch
from
178c16f to
50579f9
Compare
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, thanks for the great hard work during the review.
Cheers!
mdelapenya
added
the
feature
4 participants
#3124
What does this PR do?
Why is it important?
Related issues
@mdelapenya Can you please review