Do not fail service containers without the healthcheck

This reverts commit fec24e8341.

releaseNote.md

@@ -1,6 +1,9 @@
 ## Bugs
-- Fixed an issue where job and service container envs were corrupted (#2091)
+- Avoid key based command injection via Docker command arguments (#2062)
 ## Misc
+- Added step context name and start/finish time in step telemetry (#2069)
+- Improved error logs when there is a missing 'using' token configuration in the metadata file  (#2052)
+- Added full job name and nested workflow details in log (#2049)
 
 ## Windows x64
 We recommend configuring the runner in a root folder of the Windows drive (e.g. "C:\actions-runner"). This will help avoid issues related to service identity folder permissions and long file path restrictions on Windows.

src/Runner.Worker/Container/ContainerInfo.cs

@@ -92,6 +92,8 @@ namespace GitHub.Runner.Worker.Container
         public bool IsJobContainer { get; set; }
         public bool IsAlpine { get; set; }
 
+        public bool FailedInitialization { get; set; }
+
         public IDictionary<string, string> ContainerEnvironmentVariables
         {
             get

src/Runner.Worker/Container/DockerCommandManager.cs

@@ -107,7 +107,6 @@ namespace GitHub.Runner.Worker.Container
         public async Task<string> DockerCreate(IExecutionContext context, ContainerInfo container)
         {
             IList<string> dockerOptions = new List<string>();
-            IDictionary<string, string> environment = new Dictionary<string, string>();
             // OPTIONS
             dockerOptions.Add($"--name {container.ContainerDisplayName}");
             dockerOptions.Add($"--label {DockerInstanceLabel}");
@@ -136,8 +135,7 @@ namespace GitHub.Runner.Worker.Container
                 }
                 else
                 {
-                    environment.Add(env.Key, env.Value);
+                    dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key, env.Value));
-                    dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key));
                 }
             }
 
@@ -185,7 +183,7 @@ namespace GitHub.Runner.Worker.Container
             dockerOptions.Add($"{container.ContainerEntryPointArgs}");
 
             var optionsString = string.Join(" ", dockerOptions);
-            List<string> outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString, environment);
+            List<string> outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString);
 
             return outputStrings.FirstOrDefault();
         }
@@ -445,11 +443,6 @@ namespace GitHub.Runner.Worker.Container
         }
 
         private async Task<List<string>> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options)
-        {
-            return await ExecuteDockerCommandAsync(context, command, options, null);
-        }
-
-        private async Task<List<string>> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, IDictionary<string, string> environment)
         {
             string arg = $"{command} {options}".Trim();
             context.Command($"{DockerPath} {arg}");
@@ -477,7 +470,7 @@ namespace GitHub.Runner.Worker.Container
                             workingDirectory: context.GetGitHubContext("workspace"),
                             fileName: DockerPath,
                             arguments: arg,
-                            environment: environment,
+                            environment: null,
                             requireExitCodeZero: true,
                             outputEncoding: null,
                             cancellationToken: CancellationToken.None);

src/Runner.Worker/Container/DockerUtil.cs

@@ -71,6 +71,15 @@ namespace GitHub.Runner.Worker.Container
             return $"{flag} \"{EscapeString(key)}\"";
         }
 
+        public static string CreateEscapedOption(string flag, string key, string value)
+        {
+            if (String.IsNullOrEmpty(key))
+            {
+                return "";
+            }
+            return $"{flag} \"{EscapeString(key)}={EscapeString(value)}\"";
+        }
+
         private static string EscapeString(string value)
         {
             return value.Replace("\\", "\\\\").Replace("\"", "\\\"");

src/Runner.Worker/ContainerOperationProvider.cs

@@ -98,12 +98,41 @@ namespace GitHub.Runner.Worker
                 await StartContainerAsync(executionContext, container);
             }
 
+            await RunContainersHealthcheck(executionContext, containers);
+        }
+
+        public async Task RunContainersHealthcheck(IExecutionContext executionContext, List<ContainerInfo> containers)
+        {
             executionContext.Output("##[group]Waiting for all services to be ready");
+
+            var unhealthyContainers = new List<ContainerInfo>();
             foreach (var container in containers.Where(c => !c.IsJobContainer))
             {
-                await ContainerHealthcheck(executionContext, container);
+                var healthcheck = await ContainerHealthcheck(executionContext, container);
+
+                if (!(string.Equals(healthcheck, "healthy", StringComparison.OrdinalIgnoreCase) || string.IsNullOrEmpty(healthcheck)))
+                {
+                    unhealthyContainers.Add(container);
+                }
+                else
+                {
+                    executionContext.Output($"{container.ContainerNetworkAlias} service is healthy.");
+                }
             }
             executionContext.Output("##[endgroup]");
+
+            if (unhealthyContainers.Count > 0)
+            {
+                foreach (var container in unhealthyContainers)
+                {
+                    executionContext.Output($"##[group]Service container {container.ContainerNetworkAlias} failed.");
+                    await _dockerManager.DockerLogs(context: executionContext, containerId: container.ContainerId);
+                    executionContext.Error($"Failed to initialize container {container.ContainerImage}");
+                    container.FailedInitialization = true;
+                    executionContext.Output("##[endgroup]");
+                }
+                throw new InvalidOperationException("One or more containers failed to start.");
+            }
         }
 
         public async Task StopContainersAsync(IExecutionContext executionContext, object data)
@@ -134,6 +163,7 @@ namespace GitHub.Runner.Worker
             ArgUtil.NotNull(executionContext, nameof(executionContext));
             ArgUtil.NotNull(container, nameof(container));
             ArgUtil.NotNullOrEmpty(container.ContainerImage, nameof(container.ContainerImage));
+
             Trace.Info($"Container name: {container.ContainerName}");
             Trace.Info($"Container image: {container.ContainerImage}");
             Trace.Info($"Container options: {container.ContainerCreateOptions}");
@@ -197,8 +227,6 @@ namespace GitHub.Runner.Worker
             container.ContainerId = await _dockerManager.DockerCreate(executionContext, container);
             ArgUtil.NotNullOrEmpty(container.ContainerId, nameof(container.ContainerId));
 
-
-
             // Start container
             int startExitCode = await _dockerManager.DockerStart(executionContext, container.ContainerId);
             if (startExitCode != 0)
@@ -208,36 +236,19 @@ namespace GitHub.Runner.Worker
 
             try
             {
-                
                 // Make sure container is up and running
                 var psOutputs = await _dockerManager.DockerPS(executionContext, $"--all --filter id={container.ContainerId} --filter status=running --no-trunc --format \"{{{{.ID}}}} {{{{.Status}}}}\"");
-                
                 if (psOutputs.FirstOrDefault(x => !string.IsNullOrEmpty(x))?.StartsWith(container.ContainerId) != true)
                 {
                     // container is not up and running, pull docker log for this container.
                     await _dockerManager.DockerPS(executionContext, $"--all --filter id={container.ContainerId} --no-trunc --format \"{{{{.ID}}}} {{{{.Status}}}}\"");
-                    
-                    
-                    //executionContext.Output("##[group]Getting docker logs..");
-
                     int logsExitCode = await _dockerManager.DockerLogs(executionContext, container.ContainerId);
-                    
                     if (logsExitCode != 0)
                     {
                         executionContext.Warning($"Docker logs fail with exit code {logsExitCode}");
                     }
-                    //executionContext.Output("##[endgroup]");
-                    executionContext.Warning($"Docker container {container.ContainerId} is not in running state.");
-                }
-                else {
-                        executionContext.Output($"##[group]Container {container.ContainerId} is not running!");
-                        string opt = "--format=\'{{.State.ExitCode}}\'";
-                        await _dockerManager.DockerInspect(executionContext, container.ContainerId, opt);
-                        await _dockerManager.DockerLogs(executionContext, container.ContainerId);
-                        //if exit code is not 0 we can maybe print a message?
-                        //executionContext.Output("Container exit code: ");
 
-                    executionContext.Output("##[endgroup]");
+                    executionContext.Warning($"Docker container {container.ContainerId} is not in running state.");
                 }
             }
             catch (Exception ex)
@@ -317,9 +328,8 @@ namespace GitHub.Runner.Worker
 
             if (!string.IsNullOrEmpty(container.ContainerId))
             {
-                if (!container.IsJobContainer)
+                if (!container.IsJobContainer && !container.FailedInitialization)
                 {
-                    // Print logs for service container jobs (not the "action" job itself b/c that's already logged).
                     executionContext.Output($"Print service container logs: {container.ContainerDisplayName}");
 
                     int logsExitCode = await _dockerManager.DockerLogs(executionContext, container.ContainerId);
@@ -413,14 +423,14 @@ namespace GitHub.Runner.Worker
             }
         }
 
-        private async Task ContainerHealthcheck(IExecutionContext executionContext, ContainerInfo container)
+        private async Task<string> ContainerHealthcheck(IExecutionContext executionContext, ContainerInfo container)
         {
             string healthCheck = "--format=\"{{if .Config.Healthcheck}}{{print .State.Health.Status}}{{end}}\"";
             string serviceHealth = (await _dockerManager.DockerInspect(context: executionContext, dockerObject: container.ContainerId, options: healthCheck)).FirstOrDefault();
             if (string.IsNullOrEmpty(serviceHealth))
             {
                 // Container has no HEALTHCHECK
-                return;
+                return String.Empty;
             }
             var retryCount = 0;
             while (string.Equals(serviceHealth, "starting", StringComparison.OrdinalIgnoreCase))
@@ -431,14 +441,7 @@ namespace GitHub.Runner.Worker
                 serviceHealth = (await _dockerManager.DockerInspect(context: executionContext, dockerObject: container.ContainerId, options: healthCheck)).FirstOrDefault();
                 retryCount++;
             }
-            if (string.Equals(serviceHealth, "healthy", StringComparison.OrdinalIgnoreCase))
+            return serviceHealth;
-            {
-                executionContext.Output($"{container.ContainerNetworkAlias} service is healthy.");
-            }
-            else
-            {
-                throw new InvalidOperationException($"Failed to initialize, {container.ContainerNetworkAlias} service is {serviceHealth}.");
-            }
         }
 
         private async Task<string> ContainerRegistryLogin(IExecutionContext executionContext, ContainerInfo container)

src/Runner.Worker/JobExtension.cs

@@ -279,7 +279,7 @@ namespace GitHub.Runner.Worker
 
                         preJobSteps.Add(new JobExtensionRunner(runAsync: containerProvider.StartContainersAsync,
                                                                           condition: $"{PipelineTemplateConstants.Success}()",
-                                                                          displayName: "**Initialize containers**",
+                                                                          displayName: "Initialize containers",
                                                                           data: (object)containers));
                     }
 

src/Test/L0/Container/DockerUtilL0.cs

@@ -171,5 +171,32 @@ namespace GitHub.Runner.Common.Tests.Worker.Container
             }
             Assert.Equal(expected, actual);
         }
+
+        [Theory]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        [InlineData("HOME", "", "HOME", "")]
+        [InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #")]
+        [InlineData("HOME \"alpine:3.8 sh -c id #", "HOME \"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #")]
+        [InlineData("HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #")]
+        [InlineData("HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #")]
+        [InlineData("HOME \"\"alpine:3.8 sh -c id #", "HOME \"\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #")]
+        [InlineData("HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #")]
+        [InlineData("HOME \"\\\"alpine:3.8 sh -c id #", "HOME \"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #")]
+        public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedKey, string escapedValue)
+        {
+            var flag = "--example";
+            var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput);
+            string expected;
+            if (String.IsNullOrEmpty(keyInput))
+            {
+                expected = "";
+            }
+            else
+            {
+                expected = $"{flag} \"{escapedKey}={escapedValue}\"";
+            }
+            Assert.Equal(expected, actual);
+        }
     }
 }

src/Test/L0/Worker/ContainerOperationProviderL0.cs

@@ -0,0 +1,126 @@
+using GitHub.Runner.Worker;
+using GitHub.Runner.Worker.Container;
+using Xunit;
+using Moq;
+using GitHub.Runner.Worker.Container.ContainerHooks;
+using System.Threading.Tasks;
+using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+using GitHub.DistributedTask.WebApi;
+using System;
+
+namespace GitHub.Runner.Common.Tests.Worker
+{
+
+    public sealed class ContainerOperationProviderL0
+    {
+
+        private TestHostContext _hc;
+        private Mock<IExecutionContext> _ec;
+        private Mock<IDockerCommandManager> _dockerManager;
+        private Mock<IContainerHookManager> _containerHookManager;
+        private ContainerOperationProvider containerOperationProvider;
+        private Mock<IJobServerQueue> serverQueue;
+        private Mock<IPagingLogger> pagingLogger;
+        private List<string> healthyDockerStatus = new List<string> { "healthy" };
+        private List<string> emptyDockerStatus = new List<string> { string.Empty };
+        private List<string> unhealthyDockerStatus = new List<string> { "unhealthy" };
+        private List<string> dockerLogs = new List<string> { "log1", "log2", "log3" };
+
+        List<ContainerInfo> containers = new List<ContainerInfo>();
+
+        [Fact]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        public async void RunServiceContainersHealthcheck_UnhealthyServiceContainer_AssertFailedTask()
+        {
+            //Arrange
+            Setup();
+            _dockerManager.Setup(x => x.DockerInspect(_ec.Object, It.IsAny<string>(), It.IsAny<string>())).Returns(Task.FromResult(unhealthyDockerStatus));
+
+            //Act
+            try
+            {
+                await containerOperationProvider.RunContainersHealthcheck(_ec.Object, containers);
+            }
+            catch (InvalidOperationException)
+            {
+
+                //Assert
+                Assert.Equal(TaskResult.Failed, _ec.Object.Result ?? TaskResult.Failed);
+            }
+        }
+
+        [Fact]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        public async void RunServiceContainersHealthcheck_UnhealthyServiceContainer_AssertExceptionThrown()
+        {
+            //Arrange
+            Setup();
+            _dockerManager.Setup(x => x.DockerInspect(_ec.Object, It.IsAny<string>(), It.IsAny<string>())).Returns(Task.FromResult(unhealthyDockerStatus));
+
+            //Act and Assert
+            await Assert.ThrowsAsync<InvalidOperationException>(() => containerOperationProvider.RunContainersHealthcheck(_ec.Object, containers));
+
+        }
+
+        [Fact]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        public async void RunServiceContainersHealthcheck_healthyServiceContainer_AssertSucceededTask()
+        {
+            //Arrange
+            Setup();
+            _dockerManager.Setup(x => x.DockerInspect(_ec.Object, It.IsAny<string>(), It.IsAny<string>())).Returns(Task.FromResult(healthyDockerStatus));
+
+            //Act
+            await containerOperationProvider.RunContainersHealthcheck(_ec.Object, containers);
+
+            //Assert
+            Assert.Equal(TaskResult.Succeeded, _ec.Object.Result ?? TaskResult.Succeeded);
+
+        }
+
+        [Fact]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        public async void RunServiceContainersHealthcheck_healthyServiceContainerWithoutHealthcheck_AssertSucceededTask()
+        {
+            //Arrange
+            Setup();
+            _dockerManager.Setup(x => x.DockerInspect(_ec.Object, It.IsAny<string>(), It.IsAny<string>())).Returns(Task.FromResult(emptyDockerStatus));
+
+            //Act
+            await containerOperationProvider.RunContainersHealthcheck(_ec.Object, containers);
+
+            //Assert
+            Assert.Equal(TaskResult.Succeeded, _ec.Object.Result ?? TaskResult.Succeeded);
+
+        }
+
+        private void Setup([CallerMemberName] string testName = "")
+        {
+            containers.Add(new ContainerInfo() { ContainerImage = "ubuntu:16.04" });
+            _hc = new TestHostContext(this, testName);
+            _ec = new Mock<IExecutionContext>();
+            serverQueue = new Mock<IJobServerQueue>();
+            pagingLogger = new Mock<IPagingLogger>();
+
+            _dockerManager = new Mock<IDockerCommandManager>();
+            _containerHookManager = new Mock<IContainerHookManager>();
+            containerOperationProvider = new ContainerOperationProvider();
+
+            _hc.SetSingleton<IDockerCommandManager>(_dockerManager.Object);
+            _hc.SetSingleton<IJobServerQueue>(serverQueue.Object);
+            _hc.SetSingleton<IPagingLogger>(pagingLogger.Object);
+
+            _hc.SetSingleton<IDockerCommandManager>(_dockerManager.Object);
+            _hc.SetSingleton<IContainerHookManager>(_containerHookManager.Object);
+
+            _ec.Setup(x => x.Global).Returns(new GlobalContext());
+
+            containerOperationProvider.Initialize(_hc);
+        }
+    }
+}

src/runnerversion

@@ -1 +1 @@
-2.296.1
+2.296.0