diff --git a/cmd/crossplane/render/engine_docker.go b/cmd/crossplane/render/engine_docker.go index 9beb3e8c..ac60193d 100644 --- a/cmd/crossplane/render/engine_docker.go +++ b/cmd/crossplane/render/engine_docker.go @@ -63,6 +63,13 @@ type dockerRenderEngine struct { // (exit-3 partial output, *docker.ContainerExitError vs non-exit errors) // without a real Docker daemon. runner containerRunner + + // networks creates and removes the temporary Docker network Setup owns. + // Production callers leave it nil and Setup builds a real client from the + // environment only when it needs to create a network. Tests substitute a + // fake to exercise the create-network branch without a real Docker + // daemon. + networks networkClient } func (e *dockerRenderEngine) CheckContextSupport() error { @@ -95,7 +102,16 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f return func() {}, nil } - networkID, networkName, err := createRenderNetwork(ctx) + cli := e.networks + if cli == nil { + c, err := newNetworkClient() + if err != nil { + return func() {}, errors.Wrap(err, "cannot create Docker network for rendering") + } + cli = c + } + + networkID, networkName, err := createRenderNetwork(ctx, cli) if err != nil { return func() {}, errors.Wrap(err, "cannot create Docker network for rendering") } @@ -104,7 +120,7 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f injectNetworkAnnotation(fns, networkName) cleanup := func() { //nolint:contextcheck // Detached context for cleanup. - _ = removeRenderNetwork(context.Background(), networkID) + _ = removeRenderNetwork(context.Background(), cli, networkID) } return cleanup, nil diff --git a/cmd/crossplane/render/engine_docker_test.go b/cmd/crossplane/render/engine_docker_test.go index 73fcdddb..4d6d0d8a 100644 --- a/cmd/crossplane/render/engine_docker_test.go +++ b/cmd/crossplane/render/engine_docker_test.go @@ -23,10 +23,13 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/moby/moby/client" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" @@ -222,9 +225,8 @@ func TestDockerRenderEngineSetup(t *testing.T) { // call on the same engine stored its created network there. The branch // must annotate the supplied functions so their containers join the // network, never create a second network, and always return a no-op - // cleanup. The create-new-network branch is not covered here because it - // depends on a live Docker daemon; the broader render command tests - // exercise it integration-style. + // cleanup. The create-new-network branch is covered separately by + // TestDockerRenderEngineSetupCreatesNetwork. // // The MultiBatchAnnotatesAdditionalFunctions case simulates the // in-process multi-composition use case from crossplane/cli#96: a @@ -346,6 +348,95 @@ func TestDockerRenderEngineSetup(t *testing.T) { } } +func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) { + // When e.network is unset, Setup must create a temporary network through + // the engine's network client, record its name, annotate the supplied + // functions to join it, and return a cleanup that removes it through the + // same client. + errBoom := errors.New("boom") + + type args struct { + create func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) + } + type want struct { + err error + // created is whether Setup should create a network, annotate the + // functions to join it, and return a cleanup that removes it. + created bool + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "CreatesNetwork": { + reason: "Setup should create a network, annotate the functions to join it, and return a cleanup that removes it.", + args: args{ + create: createRenderNetworkReturns("network-id", nil), + }, + want: want{ + created: true, + }, + }, + "NetworkCreateError": { + reason: "Setup should return an error and a no-op cleanup, leaving the functions unannotated, when it cannot create the network.", + args: args{ + create: createRenderNetworkReturns("", errBoom), + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // Cleanup discards NetworkRemove's result, so record whether it + // was called. A cleanup that shouldn't remove anything leaves + // MockNetworkRemove nil. + removed := false + cli := &mockNetworkClient{MockNetworkCreate: tc.args.create} + if tc.want.created { + cli.MockNetworkRemove = func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + if diff := cmp.Diff("network-id", networkID); diff != "" { + t.Errorf("\n%s\nNetworkRemove(...): -want network ID, +got network ID:\n%s", tc.reason, diff) + } + removed = true + return client.NetworkRemoveResult{}, nil + } + } + e := &dockerRenderEngine{log: logging.NewNopLogger(), networks: cli} + fns := []pkgv1.Function{functionWithAnnotations(nil)} + + cleanup, err := e.Setup(t.Context(), fns) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nSetup(...): -want error, +got error:\n%s", tc.reason, diff) + } + + wantFns := []pkgv1.Function{functionWithAnnotations(nil)} + if tc.want.created { + if !strings.HasPrefix(e.network, renderNetworkPrefix) { + t.Errorf("\n%s\nSetup(...): e.network = %q, want a network with prefix %q", tc.reason, e.network, renderNetworkPrefix) + } + wantFns = []pkgv1.Function{functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: e.network})} + } else if e.network != "" { + t.Errorf("\n%s\nSetup(...): e.network = %q, want it unset", tc.reason, e.network) + } + if diff := cmp.Diff(wantFns, fns); diff != "" { + t.Errorf("\n%s\nSetup(...): -want fns, +got fns:\n%s", tc.reason, diff) + } + + cleanup() + + if diff := cmp.Diff(tc.want.created, removed); diff != "" { + t.Errorf("\n%s\nSetup(...) cleanup: -want network removed, +got network removed:\n%s", tc.reason, diff) + } + }) + } +} + // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image // pull errors) returned by docker.RunContainer. type nonExitError struct{ msg string } diff --git a/cmd/crossplane/render/network.go b/cmd/crossplane/render/network.go index 454b122a..8ef17d68 100644 --- a/cmd/crossplane/render/network.go +++ b/cmd/crossplane/render/network.go @@ -46,15 +46,28 @@ func (f *EngineFlags) SetDefaultCrossplaneDockerNetwork(fns []pkgv1.Function) { } } -// createRenderNetwork creates a temporary Docker bridge network for render. -// Function containers and the Crossplane render container join this network so -// they can reach each other. Returns the network ID and name. -func createRenderNetwork(ctx context.Context) (string, string, error) { +// networkClient is the subset of the Docker client the render engine uses to +// manage its temporary Docker network. +type networkClient interface { + NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) + NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) +} + +var _ networkClient = (*client.Client)(nil) + +// newNetworkClient returns a real Docker client built from the environment. +func newNetworkClient() (networkClient, error) { cli, err := docker.NewClient() if err != nil { - return "", "", errors.Wrap(err, "cannot create Docker client") + return nil, errors.Wrap(err, "cannot create Docker client") } + return cli, nil +} +// createRenderNetwork creates a temporary Docker bridge network for render. +// Function containers and the Crossplane render container join this network so +// they can reach each other. Returns the network ID and name. +func createRenderNetwork(ctx context.Context, cli networkClient) (string, string, error) { name := fmt.Sprintf("crossplane-render-%s", rand.String(8)) resp, err := cli.NetworkCreate(ctx, name, client.NetworkCreateOptions{ @@ -68,11 +81,7 @@ func createRenderNetwork(ctx context.Context) (string, string, error) { } // removeRenderNetwork removes a temporary Docker network. -func removeRenderNetwork(ctx context.Context, networkID string) error { - cli, err := docker.NewClient() - if err != nil { - return errors.Wrap(err, "cannot create Docker client") - } - _, err = cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{}) +func removeRenderNetwork(ctx context.Context, cli networkClient, networkID string) error { + _, err := cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{}) return errors.Wrap(err, "cannot remove Docker network") } diff --git a/cmd/crossplane/render/network_test.go b/cmd/crossplane/render/network_test.go index fde13f0f..843f16f4 100644 --- a/cmd/crossplane/render/network_test.go +++ b/cmd/crossplane/render/network_test.go @@ -1,9 +1,15 @@ package render import ( + "context" + "strings" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/moby/moby/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" ) @@ -74,3 +80,152 @@ func TestSetDefaultCrossplaneDockerNetwork(t *testing.T) { }) } } + +type mockNetworkClient struct { + MockNetworkCreate func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) + MockNetworkRemove func(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) +} + +func (m *mockNetworkClient) NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + return m.MockNetworkCreate(ctx, name, options) +} + +func (m *mockNetworkClient) NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + return m.MockNetworkRemove(ctx, networkID, options) +} + +var _ networkClient = &mockNetworkClient{} + +// renderNetworkPrefix is the prefix of the temporary network name +// createRenderNetwork generates. +const renderNetworkPrefix = "crossplane-render-" + +// createRenderNetworkReturns returns a MockNetworkCreate that returns the +// supplied network ID and error. It returns an error instead when it is not +// asked to create a render bridge network. +func createRenderNetworkReturns(id string, err error) func(context.Context, string, client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + return func(_ context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + if !strings.HasPrefix(name, renderNetworkPrefix) { + return client.NetworkCreateResult{}, errors.Errorf("NetworkCreate(...): name %q does not have prefix %q", name, renderNetworkPrefix) + } + if diff := cmp.Diff(client.NetworkCreateOptions{Driver: "bridge"}, options); diff != "" { + return client.NetworkCreateResult{}, errors.Errorf("NetworkCreate(...): -want options, +got options:\n%s", diff) + } + return client.NetworkCreateResult{ID: id}, err + } +} + +func TestCreateRenderNetwork(t *testing.T) { + errBoom := errors.New("boom") + + type args struct { + cli networkClient + } + type want struct { + id string + // namePrefix is a prefix the returned network name must have. The + // rest of the name is random. + namePrefix string + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "CreatesBridgeNetwork": { + reason: "createRenderNetwork should create a uniquely named bridge network through the supplied client and return its ID and name.", + args: args{ + cli: &mockNetworkClient{MockNetworkCreate: createRenderNetworkReturns("network-id", nil)}, + }, + want: want{ + id: "network-id", + namePrefix: renderNetworkPrefix, + }, + }, + "NetworkCreateError": { + reason: "createRenderNetwork should return an error when the client cannot create the network.", + args: args{ + cli: &mockNetworkClient{MockNetworkCreate: createRenderNetworkReturns("", errBoom)}, + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + id, networkName, err := createRenderNetwork(t.Context(), tc.args.cli) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\ncreateRenderNetwork(...): -want error, +got error:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.id, id); diff != "" { + t.Errorf("\n%s\ncreateRenderNetwork(...): -want ID, +got ID:\n%s", tc.reason, diff) + } + if !strings.HasPrefix(networkName, tc.want.namePrefix) { + t.Errorf("\n%s\ncreateRenderNetwork(...): name %q does not have prefix %q", tc.reason, networkName, tc.want.namePrefix) + } + }) + } +} + +func TestRemoveRenderNetwork(t *testing.T) { + errBoom := errors.New("boom") + + // removeNetworkReturns returns a MockNetworkRemove that returns the + // supplied error, or an error of its own when asked to remove any network + // other than network-id. + removeNetworkReturns := func(err error) func(context.Context, string, client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + return func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + if diff := cmp.Diff("network-id", networkID); diff != "" { + return client.NetworkRemoveResult{}, errors.Errorf("NetworkRemove(...): -want network ID, +got network ID:\n%s", diff) + } + return client.NetworkRemoveResult{}, err + } + } + + type args struct { + cli networkClient + networkID string + } + type want struct { + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "RemovesNetwork": { + reason: "removeRenderNetwork should remove the network with the supplied ID through the supplied client.", + args: args{ + cli: &mockNetworkClient{MockNetworkRemove: removeNetworkReturns(nil)}, + networkID: "network-id", + }, + }, + "NetworkRemoveError": { + reason: "removeRenderNetwork should return an error when the client cannot remove the network.", + args: args{ + cli: &mockNetworkClient{MockNetworkRemove: removeNetworkReturns(errBoom)}, + networkID: "network-id", + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := removeRenderNetwork(t.Context(), tc.args.cli, tc.args.networkID) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nremoveRenderNetwork(...): -want error, +got error:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/cmd/crossplane/render/op/cmd.go b/cmd/crossplane/render/op/cmd.go index 2274af1d..63a045e2 100644 --- a/cmd/crossplane/render/op/cmd.go +++ b/cmd/crossplane/render/op/cmd.go @@ -218,7 +218,11 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte if err != nil { return errors.Wrap(err, "cannot start function runtimes") } - defer render.StopFunctionRuntimes(log, fnAddrs) + defer func() { + if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { + log.Info("Error stopping function runtimes", "error", err) + } + }() // Build and execute the render request. in := render.OperationInputs{ diff --git a/cmd/crossplane/render/render.go b/cmd/crossplane/render/render.go index 90d406ea..ff5f664f 100644 --- a/cmd/crossplane/render/render.go +++ b/cmd/crossplane/render/render.go @@ -94,6 +94,17 @@ type OperationOutputs struct { RequiredSchemas []*fnv1.SchemaSelector } +const ( + // runtimeStopMargin is how much longer than containerStopGracePeriod + // StopFunctionRuntimes waits for each runtime to stop. It covers killing + // and removing the container once the grace period has expired. + runtimeStopMargin = 5 * time.Second + + // runtimeStopTimeout bounds how long StopFunctionRuntimes waits for each + // runtime to stop. + runtimeStopTimeout = containerStopGracePeriod + runtimeStopMargin +) + // FunctionAddresses maps function names to their gRPC target addresses. type FunctionAddresses struct { addrs map[string]string @@ -105,14 +116,30 @@ func (fa *FunctionAddresses) Addresses() map[string]string { return fa.addrs } -// Stop all function runtimes. +// Stop all function runtimes. Every runtime is stopped even if some fail; the +// returned error joins all failures. func (fa *FunctionAddresses) Stop(ctx context.Context) error { + return fa.stop(ctx, 0) +} + +// stop stops every function runtime and returns all failures joined. If +// timeout is positive each runtime gets its own timeout derived from ctx. +func (fa *FunctionAddresses) stop(ctx context.Context, timeout time.Duration) error { + var errs []error for name, rctx := range fa.contexts { - if err := rctx.Stop(ctx); err != nil { - return errors.Wrapf(err, "cannot stop function %q runtime (target %q)", name, rctx.Target) + sctx, cancel := ctx, context.CancelFunc(func() {}) + if timeout > 0 { + sctx, cancel = context.WithTimeout(ctx, timeout) } + if err := rctx.Stop(sctx); err != nil { + errs = append(errs, errors.Wrapf(err, "cannot stop function %q runtime (target %q)", name, rctx.Target)) + } + cancel() } - return nil + if len(errs) == 0 { + return nil + } + return errors.Join(errs...) } // StartFunctionRuntimes starts the runtime for each function and returns their @@ -167,16 +194,15 @@ func injectNetworkAnnotation(fns []pkgv1.Function, networkName string) { } } -// StopFunctionRuntimes stops all function runtimes with a timeout. -func StopFunctionRuntimes(log logging.Logger, fa *FunctionAddresses) { +// StopFunctionRuntimes stops all function runtimes and returns all failures +// joined. Cleanup runs even if ctx is already cancelled: each runtime gets its +// own timeout derived from ctx without its cancellation, so a slow runtime +// can't starve the others and cleanup stays bounded. +func StopFunctionRuntimes(ctx context.Context, fa *FunctionAddresses) error { if fa == nil { - return - } - stopCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - if err := fa.Stop(stopCtx); err != nil { - log.Info("Error stopping function runtimes", "error", err) + return nil } + return fa.stop(context.WithoutCancel(ctx), runtimeStopTimeout) } // OverrideFunctionAnnotations applies annotation overrides from flags to diff --git a/cmd/crossplane/render/render_test.go b/cmd/crossplane/render/render_test.go index 6d87ae2d..a47e4621 100644 --- a/cmd/crossplane/render/render_test.go +++ b/cmd/crossplane/render/render_test.go @@ -1,12 +1,15 @@ package render import ( + "context" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" ) @@ -91,3 +94,150 @@ func TestOverrideFunctionAnnotations(t *testing.T) { func functionWithAnnotations(annotations map[string]string) pkgv1.Function { return pkgv1.Function{ObjectMeta: metav1.ObjectMeta{Annotations: annotations}} } + +func TestFunctionAddressesStop(t *testing.T) { + errA := errors.New("boom a") + errB := errors.New("boom b") + + stopReturns := func(err error) RuntimeContext { + return RuntimeContext{Target: "fn:9443", Stop: func(context.Context) error { return err }} + } + + type args struct { + contexts map[string]RuntimeContext + } + type want struct { + // errs are the errors the returned error must wrap. When empty, Stop + // must return nil. + errs []error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "AllSucceed": { + reason: "Stop should return nil when every runtime stops.", + args: args{ + contexts: map[string]RuntimeContext{ + "ok-a": stopReturns(nil), + "ok-b": stopReturns(nil), + }, + }, + }, + "SomeFail": { + reason: "Stop should stop every runtime even if some fail, and return every failure.", + args: args{ + contexts: map[string]RuntimeContext{ + "ok-a": stopReturns(nil), + "fail-a": stopReturns(errA), + "ok-b": stopReturns(nil), + "fail-b": stopReturns(errB), + }, + }, + want: want{ + errs: []error{errA, errB}, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // Repeat so Go's randomized map iteration can't hide a regression + // to returning on the first error. + for range 20 { + fa := &FunctionAddresses{contexts: tc.args.contexts} + + err := fa.Stop(t.Context()) + + wantErrs := tc.want.errs + if len(wantErrs) == 0 { + wantErrs = []error{nil} + } + for _, want := range wantErrs { + if diff := cmp.Diff(want, err, cmpopts.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nStop(...): -want error, +got error:\n%s", tc.reason, diff) + } + } + } + }) + } +} + +func TestStopFunctionRuntimes(t *testing.T) { + errBoom := errors.New("boom") + + type args struct { + // stopErr is what the runtime's Stop returns. + stopErr error + // cancelParent cancels the context passed to StopFunctionRuntimes + // before calling it. + cancelParent bool + // noRuntimes passes nil FunctionAddresses. + noRuntimes bool + } + type want struct { + // ctxErr and hasDeadline describe the context the runtime's Stop + // was called with. + ctxErr error + hasDeadline bool + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "ParentContextLive": { + reason: "StopFunctionRuntimes should stop each runtime with a context bounded by its own timeout.", + want: want{hasDeadline: true}, + }, + "ParentContextCancelled": { + reason: "StopFunctionRuntimes should still stop each runtime, with a live but bounded context, when the parent context is already cancelled.", + args: args{cancelParent: true}, + want: want{hasDeadline: true}, + }, + "StopError": { + reason: "StopFunctionRuntimes should return the error a runtime fails to stop with.", + args: args{stopErr: errBoom}, + want: want{hasDeadline: true, err: errBoom}, + }, + "NilFunctionAddresses": { + reason: "StopFunctionRuntimes should do nothing when there are no runtimes.", + args: args{noRuntimes: true}, + want: want{}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // The context the runtime is stopped with is the property under + // test, so record it. + var got want + var fa *FunctionAddresses + if !tc.args.noRuntimes { + fa = &FunctionAddresses{contexts: map[string]RuntimeContext{ + "fn": {Target: "fn:9443", Stop: func(ctx context.Context) error { + got.ctxErr = ctx.Err() + _, got.hasDeadline = ctx.Deadline() + return tc.args.stopErr + }}, + }} + } + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + if tc.args.cancelParent { + cancel() + } + + got.err = StopFunctionRuntimes(ctx, fa) + + if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(want{}), cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStopFunctionRuntimes(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/cmd/crossplane/render/runtime_docker.go b/cmd/crossplane/render/runtime_docker.go index 169fe3f5..414a0509 100644 --- a/cmd/crossplane/render/runtime_docker.go +++ b/cmd/crossplane/render/runtime_docker.go @@ -24,6 +24,7 @@ import ( "net" "net/netip" "strings" + "time" "github.com/containerd/errdefs" "github.com/google/go-containerregistry/pkg/authn" @@ -87,12 +88,15 @@ type DockerCleanup string // Supported AnnotationKeyRuntimeDockerCleanup values. const ( - // AnnotationValueRuntimeDockerCleanupStop is the default. It stops the - // container once rendering is done. + // AnnotationValueRuntimeDockerCleanupStop stops the container once + // rendering is done, waiting up to containerStopGracePeriod for it to exit + // on SIGTERM before Docker kills it. AnnotationValueRuntimeDockerCleanupStop DockerCleanup = "Stop" - // AnnotationValueRuntimeDockerCleanupRemove stops and removes the - // container once rendering is done. + // AnnotationValueRuntimeDockerCleanupRemove is the default. It stops the + // container once rendering is done, waiting up to containerStopGracePeriod + // for it to exit on SIGTERM, then force removes it. The container is + // removed even if the graceful stop fails. AnnotationValueRuntimeDockerCleanupRemove DockerCleanup = "Remove" // AnnotationValueRuntimeDockerCleanupOrphan leaves the container running @@ -102,6 +106,10 @@ const ( AnnotationValueRuntimeDockerCleanupDefault = AnnotationValueRuntimeDockerCleanupRemove ) +// containerStopGracePeriod is how long the Stop and Remove cleanup policies +// wait for a Function container to exit on SIGTERM before Docker kills it. +const containerStopGracePeriod = 3 * time.Second + // AnnotationKeyRuntimeDockerPullPolicy can be added to a Function to control how its runtime // image is pulled. const AnnotationKeyRuntimeDockerPullPolicy = "render.crossplane.io/runtime-docker-pull-policy" @@ -161,8 +169,33 @@ type RuntimeDocker struct { // and is reached via host port bindings. When set, the container joins // the specified network and is reached via its Docker hostname on port 9443. Network string + + // dockerClient manages the Function's container. Production callers leave + // it nil and Start builds a real client from the environment. Tests + // substitute a fake to exercise container lifecycle and cleanup handling + // without a real Docker daemon. + dockerClient containerClient +} + +// containerClient is the subset of the Docker client RuntimeDocker uses to +// pull images and manage a Function's container. +type containerClient interface { + pullClient + containerCleanupClient + ContainerInspect(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) + ContainerCreate(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) + ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) +} + +// containerCleanupClient is the subset of the Docker client RuntimeDocker's +// stop function uses to clean up a Function's container. +type containerCleanupClient interface { + ContainerStop(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + ContainerRemove(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) } +var _ containerClient = (*client.Client)(nil) + // GetDockerPullPolicy extracts PullPolicy configuration from the supplied // Function. func GetDockerPullPolicy(fn pkgv1.Function) (DockerPullPolicy, error) { @@ -250,7 +283,7 @@ func GetRuntimeDocker(fn pkgv1.Function, log logging.Logger) (*RuntimeDocker, er var _ Runtime = &RuntimeDocker{} -func (r *RuntimeDocker) findContainer(ctx context.Context, cli *client.Client) (string, error) { +func (r *RuntimeDocker) findContainer(ctx context.Context, cli containerClient) (string, error) { if r.Name == "" { return "", nil } @@ -266,7 +299,7 @@ func (r *RuntimeDocker) findContainer(ctx context.Context, cli *client.Client) ( return inspect.Container.ID, nil } -func (r *RuntimeDocker) createContainer(ctx context.Context, cli *client.Client) (string, error) { +func (r *RuntimeDocker) createContainer(ctx context.Context, cli containerClient) (string, error) { r.log.Debug("Starting Docker container runtime setup", "image", r.Image) // Let Docker automatically allocate an available port on the bind address. @@ -377,7 +410,7 @@ func (r *RuntimeDocker) createContainer(ctx context.Context, cli *client.Client) } // startContainer ensures the container is running and returns its address. -func (r *RuntimeDocker) startContainer(ctx context.Context, cli *client.Client, containerID string) (string, error) { +func (r *RuntimeDocker) startContainer(ctx context.Context, cli containerClient, containerID string) (string, error) { // Start the container (idempotent - safe to call on running containers) if _, err := cli.ContainerStart(ctx, containerID, client.ContainerStartOptions{}); err != nil { return "", errors.Wrap(err, "cannot start Docker container") @@ -457,9 +490,13 @@ func (r *RuntimeDocker) getPullOptions() (client.ImagePullOptions, error) { // Start a Function as a Docker container. func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) { - cli, err := client.New(client.FromEnv) - if err != nil { - return RuntimeContext{}, errors.Wrap(err, "cannot create Docker client using environment variables") + cli := r.dockerClient + if cli == nil { + c, err := client.New(client.FromEnv) + if err != nil { + return RuntimeContext{}, errors.Wrap(err, "cannot create Docker client using environment variables") + } + cli = c } // Try to find an existing container with the supplied container name. @@ -483,19 +520,27 @@ func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) { // Inline stop function stop := func(ctx context.Context) error { + grace := int(containerStopGracePeriod / time.Second) + stopOpts := client.ContainerStopOptions{Timeout: &grace} + switch r.Cleanup { case AnnotationValueRuntimeDockerCleanupOrphan: return nil case AnnotationValueRuntimeDockerCleanupStop: - if _, err := cli.ContainerStop(ctx, containerID, client.ContainerStopOptions{}); err != nil { + if _, err := cli.ContainerStop(ctx, containerID, stopOpts); err != nil { return errors.Wrap(err, "cannot stop Docker container") } case AnnotationValueRuntimeDockerCleanupRemove: - if _, err := cli.ContainerStop(ctx, containerID, client.ContainerStopOptions{}); err != nil { - return errors.Wrap(err, "cannot stop Docker container") + // Give the container a chance to exit gracefully, then force + // remove it whether or not the stop succeeded, so a container + // that's slow to exit on SIGTERM can't cause removal to be + // skipped. A stop failure only matters if removal fails too. + var stopErr error + if _, err := cli.ContainerStop(ctx, containerID, stopOpts); err != nil { + stopErr = errors.Wrap(err, "cannot stop Docker container") } - if _, err := cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{}); err != nil { - return errors.Wrap(err, "cannot remove Docker container") + if _, err := cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{Force: true}); err != nil { + return errors.Join(errors.Wrap(err, "cannot remove Docker container"), stopErr) } } diff --git a/cmd/crossplane/render/runtime_docker_test.go b/cmd/crossplane/render/runtime_docker_test.go index a05a6079..b8544e3a 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -18,13 +18,20 @@ package render import ( "context" + "io" + "net" + "strconv" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" "github.com/moby/moby/client" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" @@ -40,6 +47,95 @@ func (m *mockPullClient) ImagePull(ctx context.Context, ref string, options clie var _ pullClient = &mockPullClient{} +type mockContainerClient struct { + MockImagePull func(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) + MockContainerInspect func(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) + MockContainerCreate func(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) + MockContainerStart func(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) + MockContainerStop func(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + MockContainerRemove func(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) +} + +func (m *mockContainerClient) ImagePull(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { + return m.MockImagePull(ctx, ref, options) +} + +func (m *mockContainerClient) ContainerInspect(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return m.MockContainerInspect(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerCreate(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return m.MockContainerCreate(ctx, options) +} + +func (m *mockContainerClient) ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) { + return m.MockContainerStart(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerStop(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) { + return m.MockContainerStop(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerRemove(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return m.MockContainerRemove(ctx, containerID, options) +} + +var _ containerClient = &mockContainerClient{} + +// imagePullDone is a MockImagePull that reports an image pull that has +// already completed. +func imagePullDone(context.Context, string, client.ImagePullOptions) (client.ImagePullResponse, error) { + return pullDoneResponse{}, nil +} + +// pullDoneResponse is an ImagePullResponse whose body is empty. Only the +// io.ReadCloser methods PullImage uses are implemented; the embedded +// interface is nil. +type pullDoneResponse struct { + client.ImagePullResponse +} + +func (pullDoneResponse) Read([]byte) (int, error) { return 0, io.EOF } + +func (pullDoneResponse) Close() error { return nil } + +// createContainerReturns returns a MockContainerCreate that creates a +// container with the supplied ID. +func createContainerReturns(id string) func(context.Context, client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return func(context.Context, client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return client.ContainerCreateResult{ID: id}, nil + } +} + +// startContainer returns a MockContainerStart that starts the container with +// the supplied ID, and returns an error for any other container. +func startContainer(id string) func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + return func(_ context.Context, containerID string, _ client.ContainerStartOptions) (client.ContainerStartResult, error) { + if diff := cmp.Diff(id, containerID); diff != "" { + return client.ContainerStartResult{}, errors.Errorf("ContainerStart(...): -want container ID, +got container ID:\n%s", diff) + } + return client.ContainerStartResult{}, nil + } +} + +// inspectContainerOnNetwork returns a MockContainerInspect that reports the +// container with the supplied ID as running with the supplied name, attached +// to the supplied Docker network. It returns an error for any other container. +func inspectContainerOnNetwork(id, name, networkName string) func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return func(_ context.Context, containerID string, _ client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + if diff := cmp.Diff(id, containerID); diff != "" { + return client.ContainerInspectResult{}, errors.Errorf("ContainerInspect(...): -want container ID, +got container ID:\n%s", diff) + } + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: id, + Name: "/" + name, + NetworkSettings: &container.NetworkSettings{ + Networks: map[string]*network.EndpointSettings{networkName: {}}, + }, + }}, nil + } +} + func TestGetRuntimeDocker(t *testing.T) { type args struct { fn pkgv1.Function @@ -246,3 +342,297 @@ func TestGetRuntimeDocker(t *testing.T) { }) } } + +func TestRuntimeDockerStart(t *testing.T) { + const ( + image = "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0" + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + errBoom := errors.New("boom") + + type args struct { + pullPolicy DockerPullPolicy + cli *mockContainerClient + } + type want struct { + rctx RuntimeContext + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "CreatesAndStartsContainer": { + reason: "Start should create, start, and inspect the Function's container through the injected client, without pulling an image that is present.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + cli: &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + want: want{ + rctx: RuntimeContext{Target: net.JoinHostPort(containerName, strconv.Itoa(FunctionPort))}, + }, + }, + "PullsImage": { + reason: "Start should pull the Function's image through the injected client when the pull policy is Always.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyAlways, + cli: &mockContainerClient{ + MockImagePull: func(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { + if diff := cmp.Diff(image, ref); diff != "" { + return nil, errors.Errorf("ImagePull(...): -want ref, +got ref:\n%s", diff) + } + return imagePullDone(ctx, ref, options) + }, + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + want: want{ + rctx: RuntimeContext{Target: net.JoinHostPort(containerName, strconv.Itoa(FunctionPort))}, + }, + }, + "PullError": { + reason: "Start should return an error, without creating a container, when the injected client cannot pull the image.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyAlways, + cli: &mockContainerClient{ + MockImagePull: func(context.Context, string, client.ImagePullOptions) (client.ImagePullResponse, error) { + return nil, errBoom + }, + }, + }, + want: want{ + err: errBoom, + }, + }, + "StartError": { + reason: "Start should return an error when the injected client cannot start the container.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + cli: &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + return client.ContainerStartResult{}, errBoom + }, + }, + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := &RuntimeDocker{ + Image: image, + Cleanup: AnnotationValueRuntimeDockerCleanupRemove, + PullPolicy: tc.args.pullPolicy, + Keychain: authn.NewMultiKeychain(), + Network: dockerNetwork, + log: logging.NewNopLogger(), + dockerClient: tc.args.cli, + } + + rctx, err := r.Start(t.Context()) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStart(...): -want error, +got error:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.rctx, rctx, cmpopts.IgnoreFields(RuntimeContext{}, "Stop")); diff != "" { + t.Errorf("\n%s\nStart(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestRuntimeDockerStop(t *testing.T) { + const ( + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + errStop := errors.New("stop boom") + errRemove := errors.New("remove boom") + + // The Stop and Remove policies give the container containerStopGracePeriod + // (3s) to exit, and Remove force removes it. + grace := 3 + wantStopOptions := client.ContainerStopOptions{Timeout: &grace} + wantRemoveOptions := client.ContainerRemoveOptions{Force: true} + + type args struct { + cleanup DockerCleanup + // stop and remove are the cleanup client's ContainerStop and + // ContainerRemove. A nil mock must not be called. The test checks + // the arguments each non-nil mock is called with. + stop func(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + remove func(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) + } + type want struct { + // errs are the errors the returned error must wrap. When empty, Stop + // must return nil. + errs []error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "Stop": { + reason: "The Stop cleanup policy should stop the container with the grace period and leave it in place.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupStop, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + }, + }, + "StopError": { + reason: "The Stop cleanup policy should return an error when the container cannot be stopped.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupStop, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + }, + want: want{ + errs: []error{errStop}, + }, + }, + "Remove": { + reason: "The Remove cleanup policy should stop the container with the grace period, then force remove it.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, nil + }, + }, + }, + "RemoveStopError": { + reason: "The Remove cleanup policy should still force remove the container when the graceful stop fails, and succeed if removal does.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, nil + }, + }, + }, + "RemoveError": { + reason: "The Remove cleanup policy should return an error when the container cannot be removed.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, errRemove + }, + }, + want: want{ + errs: []error{errRemove}, + }, + }, + "RemoveStopAndRemoveError": { + reason: "The Remove cleanup policy should return both errors when the container can be neither stopped nor removed.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, errRemove + }, + }, + want: want{ + errs: []error{errRemove, errStop}, + }, + }, + "Orphan": { + reason: "The Orphan cleanup policy should leave the container running without calling Docker.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupOrphan, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cli := &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + } + // A stop error is discarded when removal succeeds, so report + // unexpected arguments with t.Errorf rather than as an error. + if stop := tc.args.stop; stop != nil { + cli.MockContainerStop = func(ctx context.Context, id string, options client.ContainerStopOptions) (client.ContainerStopResult, error) { + if diff := cmp.Diff(containerID, id); diff != "" { + t.Errorf("\n%s\nContainerStop(...): -want container ID, +got container ID:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(wantStopOptions, options); diff != "" { + t.Errorf("\n%s\nContainerStop(...): -want options, +got options:\n%s", tc.reason, diff) + } + return stop(ctx, id, options) + } + } + if remove := tc.args.remove; remove != nil { + cli.MockContainerRemove = func(ctx context.Context, id string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + if diff := cmp.Diff(containerID, id); diff != "" { + t.Errorf("\n%s\nContainerRemove(...): -want container ID, +got container ID:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(wantRemoveOptions, options); diff != "" { + t.Errorf("\n%s\nContainerRemove(...): -want options, +got options:\n%s", tc.reason, diff) + } + return remove(ctx, id, options) + } + } + r := &RuntimeDocker{ + Image: "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0", + Cleanup: tc.args.cleanup, + PullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + Keychain: authn.NewMultiKeychain(), + Network: dockerNetwork, + log: logging.NewNopLogger(), + dockerClient: cli, + } + + rctx, err := r.Start(t.Context()) + if err != nil { + t.Fatalf("\n%s\nStart(...): unexpected error: %v", tc.reason, err) + } + + err = rctx.Stop(t.Context()) + + // The returned error may join several errors, so check that it + // wraps each wanted error in turn. + wantErrs := tc.want.errs + if len(wantErrs) == 0 { + wantErrs = []error{nil} + } + for _, want := range wantErrs { + if diff := cmp.Diff(want, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStop(...): -want error, +got error:\n%s", tc.reason, diff) + } + } + }) + } +} diff --git a/cmd/crossplane/render/xr/cmd.go b/cmd/crossplane/render/xr/cmd.go index 0a41121d..0c2435cb 100644 --- a/cmd/crossplane/render/xr/cmd.go +++ b/cmd/crossplane/render/xr/cmd.go @@ -278,7 +278,11 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte if err != nil { return errors.Wrap(err, "cannot start function runtimes") } - defer render.StopFunctionRuntimes(log, fnAddrs) + defer func() { + if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { + log.Info("Error stopping function runtimes", "error", err) + } + }() // Build and execute the render request. in := render.CompositionInputs{