- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 583
 
feat(cassandra): add ssl option cassandra #3151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ba9ac99
              a87e16b
              d7c7be5
              22c2887
              dfe89e6
              59a1733
              4e7b372
              875e63f
              8e1b349
              26a38e1
              7d76d50
              cc1dd06
              71100a9
              f07aea2
              ac19eb4
              5b636f7
              2ad9f7b
              7409635
              4c2e3dc
              6da1e1e
              9a4f4f2
              5242c32
              18214a0
              8ba3319
              60622d4
              38320ac
              d1724a5
              b05ef84
              86f661b
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| 
          
            
          
           | 
    @@ -2,28 +2,38 @@ package cassandra | |||||
| 
     | 
||||||
| import ( | ||||||
| "context" | ||||||
| "crypto/tls" | ||||||
| "fmt" | ||||||
| "io" | ||||||
| "path/filepath" | ||||||
| "strings" | ||||||
| "time" | ||||||
| 
     | 
||||||
| "github.com/testcontainers/testcontainers-go" | ||||||
| "github.com/testcontainers/testcontainers-go/wait" | ||||||
| ) | ||||||
| 
     | 
||||||
| const ( | ||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: do we need to two ports or can it just be one which is either SSL or not?  | 
||||||
| ) | ||||||
| 
     | 
||||||
| // CassandraContainer represents the Cassandra container type used in the module | ||||||
| type CassandraContainer struct { | ||||||
| testcontainers.Container | ||||||
| settings Options | ||||||
| } | ||||||
| 
     | 
||||||
| // ConnectionHost returns the host and port of the cassandra container, using the default, native 9042 port, and | ||||||
| // ConnectionHost returns the host and port of the cassandra container, using the default, native port, | ||||||
| // obtaining the host and exposed port from the container | ||||||
| func (c *CassandraContainer) ConnectionHost(ctx context.Context) (string, error) { | ||||||
| return c.PortEndpoint(ctx, port, "") | ||||||
| // Use the secure port if TLS is enabled | ||||||
| portToUse := port | ||||||
| if c.settings.tlsConfig != nil { | ||||||
| portToUse = securePort | ||||||
| } | ||||||
| 
     | 
||||||
| return c.PortEndpoint(ctx, portToUse, "") | ||||||
| } | ||||||
| 
     | 
||||||
| // WithConfigFile sets the YAML config file to be used for the cassandra container | ||||||
| 
        
          
        
         | 
    @@ -37,7 +47,6 @@ func WithConfigFile(configFile string) testcontainers.CustomizeRequestOption { | |||||
| FileMode: 0o755, | ||||||
| } | ||||||
| req.Files = append(req.Files, cf) | ||||||
| 
     | 
||||||
| return nil | ||||||
| } | ||||||
| } | ||||||
| 
        
          
        
         | 
    @@ -54,10 +63,8 @@ func WithInitScripts(scripts ...string) testcontainers.CustomizeRequestOption { | |||||
| FileMode: 0o755, | ||||||
| } | ||||||
| initScripts = append(initScripts, cf) | ||||||
| 
     | 
||||||
| execs = append(execs, initScript{File: cf.ContainerFilePath}) | ||||||
| } | ||||||
| 
     | 
||||||
| req.Files = append(req.Files, initScripts...) | ||||||
| return testcontainers.WithAfterReadyCommand(execs...)(req) | ||||||
| } | ||||||
| 
        
          
        
         | 
    @@ -71,31 +78,54 @@ func RunContainer(ctx context.Context, opts ...testcontainers.ContainerCustomize | |||||
| 
     | 
||||||
| // Run creates an instance of the Cassandra container type | ||||||
| func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustomizer) (*CassandraContainer, error) { | ||||||
| moduleOpts := []testcontainers.ContainerCustomizer{ | ||||||
| testcontainers.WithExposedPorts(port), | ||||||
| testcontainers.WithEnv(map[string]string{ | ||||||
| req := testcontainers.ContainerRequest{ | ||||||
| Image: img, | ||||||
| Env: map[string]string{ | ||||||
| "CASSANDRA_SNITCH": "GossipingPropertyFileSnitch", | ||||||
| "JVM_OPTS": "-Dcassandra.skip_wait_for_gossip_to_settle=0 -Dcassandra.initial_token=0", | ||||||
| "HEAP_NEWSIZE": "128M", | ||||||
| "MAX_HEAP_SIZE": "1024M", | ||||||
| "CASSANDRA_ENDPOINT_SNITCH": "GossipingPropertyFileSnitch", | ||||||
| "CASSANDRA_DC": "datacenter1", | ||||||
| }), | ||||||
| testcontainers.WithWaitStrategy(wait.ForAll( | ||||||
| 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") | ||||||
| }), | ||||||
| )), | ||||||
| }, | ||||||
| ExposedPorts: []string{string(port)}, | ||||||
| } | ||||||
| 
     | 
||||||
| moduleOpts = append(moduleOpts, opts...) | ||||||
| genericContainerReq := testcontainers.GenericContainerRequest{ | ||||||
| ContainerRequest: req, | ||||||
| Started: true, | ||||||
| } | ||||||
| 
     | 
||||||
| ctr, err := testcontainers.Run(ctx, img, moduleOpts...) | ||||||
| var settings Options | ||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.  | 
||||||
| for _, opt := range opts { | ||||||
| if err := opt.Customize(&genericContainerReq); err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
| } | ||||||
| 
     | 
||||||
| // Set up wait strategies | ||||||
| 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), | ||||||
| } | ||||||
| 
         
      Comment on lines
    
      +107
     to 
      +113
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. I think the use case here would be appending. I can submit a fix now  | 
||||||
| 
     | 
||||||
| // Add TLS wait strategy if TLS config exists | ||||||
| if settings.tlsConfig != nil { | ||||||
| waitStrategies = append(waitStrategies, wait.ForListeningPort(securePort).WithStartupTimeout(1*time.Minute)) | ||||||
| } | ||||||
| 
     | 
||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: wrap the error here 
        Suggested change
       
    
  | 
||||||
| } | ||||||
| 
     | 
||||||
| container, err := testcontainers.GenericContainer(ctx, genericContainerReq) | ||||||
| var c *CassandraContainer | ||||||
| if ctr != nil { | ||||||
| c = &CassandraContainer{Container: ctr} | ||||||
| if container != nil { | ||||||
| c = &CassandraContainer{Container: container, settings: settings} | ||||||
| } | ||||||
| 
     | 
||||||
| if err != nil { | ||||||
| 
        
          
        
         | 
    @@ -104,3 +134,11 @@ func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustom | |||||
| 
     | 
||||||
| return c, nil | ||||||
| } | ||||||
| 
     | 
||||||
| // TLSConfig returns the TLS configuration for the Cassandra container, nil if TLS is not enabled. | ||||||
| func (c *CassandraContainer) TLSConfig() *tls.Config { | ||||||
| 
         
      Comment on lines
    
      +138
     to 
      +139
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mdelapenya quite a common pattern in modules, we should consider returning an error instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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  | 
||||||
| if c.settings.tlsConfig == nil { | ||||||
| return nil | ||||||
| } | ||||||
| return c.settings.tlsConfig.Config | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the nat import to fix compilation.
nat.Portis used forport/securePort, butgithub.com/docker/go-connections/natis not imported, so the branch fails to build (undefined: natin 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
🤖 Prompt for AI Agents