Skip to content
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

feat: Add DependsOn support #2035

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
3 changes: 2 additions & 1 deletion container.go
Expand Up @@ -132,7 +132,8 @@ type ContainerRequest struct {
Tmpfs map[string]string
RegistryCred string // Deprecated: Testcontainers will detect registry credentials automatically
WaitingFor wait.Strategy
Name string // for specifying container name
DependsOn []ContainerDependency // specify other containers that must be running before starting this container.
Name string // for specifying container name
Hostname string
WorkingDir string // specify the working directory of the container
ExtraHosts []string // Deprecated: Use HostConfigModifier instead
Expand Down
171 changes: 171 additions & 0 deletions dependency.go
@@ -0,0 +1,171 @@
package testcontainers

import (
"context"
"fmt"
"github.com/docker/docker/api/types/container"
"github.com/google/uuid"
"github.com/pkg/errors"
"golang.org/x/exp/slices"
)

// ContainerDependency represents a reliance that a container has on another container.
type ContainerDependency struct {
Request ContainerRequest
EnvKey string
Copy link
Contributor

Choose a reason for hiding this comment

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

The environment variable can be injected by the callback function and not all containers can take advantage of it, so I'm not convinced this should be implemented in the dependency struct as a default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, i'm not sure what you mean by the env variable can be injected by the callback func. The container would be already running by the time the callback is called and I'm not aware if injecting env vars into a live container would be possible at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that we should make Env injection optional through the use of something like WithEnvName(envKey string) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The container would be already running by the time the callback is called
The dependency will be running, but not the container that depends on it. See please an additional note that I made about the callback function signature.

Do you mean that we should make Env injection optional through the use of something like WithEnvName(envKey string) ?
Yes, that was my original proposal

// CallbackFunc is called after the dependency container is started.
CallbackFunc func(Container)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the callback should receive also the container request of the container that has the dependency. In this way the callback can modify the request (e.g. add env variables, setup the hosts file, etc)

// KeepAlive determines whether the dependency should be kept alive after the parent container is terminated.
KeepAlive bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a KeepAlive flag since I thought it'd be helpful to define whether or not the dependency should terminate alongside its parent.

I set the default to true in NewContainerDependency but I'd be interested to hear other's opinions of the flag itself and if true is an appropriate default.

}

// NewContainerDependency can be used to define a dependency and the environment variable that
// will be used to pass the DNS name to the parent container.
func NewContainerDependency(containerRequest ContainerRequest, envKey string) ContainerDependency {
return ContainerDependency{
Request: containerRequest,
EnvKey: envKey,
CallbackFunc: func(c Container) {},
KeepAlive: true,
}
}

func (c ContainerDependency) WithKeepAlive(keepAlive bool) ContainerDependency {
c.KeepAlive = keepAlive
return c
}

func (c ContainerDependency) WithCallback(callbackFunc func(Container)) ContainerDependency {
c.CallbackFunc = callbackFunc
return c
}

func (c ContainerDependency) StartDependency(ctx context.Context, network string) (Container, error) {
c.Request.Networks = append(c.Request.Networks, network)
dependency, err := GenericContainer(ctx, GenericContainerRequest{
ContainerRequest: c.Request,
Started: true,
Reuse: c.Request.Name != "", // reuse a running dependency container if a name is provided.
})
if err != nil {
return nil, err
}

c.CallbackFunc(dependency)
return dependency, nil
}

func resolveDNSName(ctx context.Context, container Container, network *DockerNetwork) (string, error) {
curNetworks, err := container.Networks(ctx)
if err != nil {
return "", fmt.Errorf("%w: could not retrieve networks for dependency container", err)
}
// The container may not be connected to the network if it was reused.
if slices.Index(curNetworks, network.Name) == -1 {
err = network.provider.client.NetworkConnect(ctx, network.ID, container.GetContainerID(), nil)
if err != nil {
return "", fmt.Errorf("%w: could not connect dependency container to network", err)
}
}

networkAlias, err := container.NetworkAliases(ctx)
if err != nil {
return "", err
}

aliases := networkAlias[network.Name]
if len(aliases) == 0 {
return "", errors.New("could not retrieve network alias for dependency container")
}
return aliases[0], nil
}

func cleanupDependencyNetwork(ctx context.Context, dependencies map[Container]bool, network *DockerNetwork) error {
if network == nil {
return nil
}

for dependency, keepAlive := range dependencies {
err := network.provider.client.NetworkDisconnect(ctx, network.ID, dependency.GetContainerID(), true)
if err != nil {
return err
}

if !keepAlive {
if err := dependency.Terminate(ctx); err != nil {
return err
}
}

}
defer network.provider.Close()
return network.Remove(ctx)
}

var defaultDependencyHook = func(dockerInput *container.Config) ContainerLifecycleHooks {
var depNetwork *DockerNetwork
depContainers := make(map[Container]bool)
return ContainerLifecycleHooks{
PreCreates: []ContainerRequestHook{
func(ctx context.Context, req ContainerRequest) (err error) {
if len(req.DependsOn) == 0 {
return nil
}
defer func() {
if err != nil {
// clean up dependencies that were created if an error occurred.
cleanupErr := cleanupDependencyNetwork(ctx, depContainers, depNetwork)
if cleanupErr != nil {
Logger.Printf("Could not cleanup dependencies after an error occured: %v", cleanupErr)
}
}
}()

net, err := GenericNetwork(ctx, GenericNetworkRequest{
NetworkRequest: NetworkRequest{
Driver: Bridge,
Labels: GenericLabels(),
Name: fmt.Sprintf("testcontainer-dependency-%v", uuid.NewString()),
Internal: false,
},
})
depNetwork = net.(*DockerNetwork)
Comment on lines +124 to +132
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to use network.New for this, but that causes a cyclical dependency - which is why I'm using these depreciated functions.

I'd appreciate any suggestions on how to use network.New instead of doing this.

if err != nil {
return err
}

for _, dep := range req.DependsOn {
if dep.EnvKey == "" {
return errors.New("cannot create dependency with empty environment key.")
}
container, err := dep.StartDependency(ctx, depNetwork.Name)
if err != nil {
return err
}
depContainers[container] = dep.KeepAlive
name, err := resolveDNSName(ctx, container, depNetwork)
if err != nil {
return err
}
dockerInput.Env = append(dockerInput.Env, dep.EnvKey+"="+name)
}
return nil
},
},
PostCreates: []ContainerHook{
func(ctx context.Context, container Container) error {
if depNetwork == nil {
return nil
}
err := depNetwork.provider.client.NetworkConnect(ctx, depNetwork.ID, container.GetContainerID(), nil)
defer depNetwork.provider.Close()
return err
},
},
PostTerminates: []ContainerHook{
func(ctx context.Context, container Container) error {
return cleanupDependencyNetwork(ctx, depContainers, depNetwork)
},
},
}
}