Revert "docker: escape key-value pair as -e KEY and VALUE being environment var (#2091)"

This reverts commit 95459dea5f.

docs/adrs/1891-container-hooks.md

@@ -16,7 +16,7 @@ We should give them that option, and publish examples how how they can create th
   - For example, the current runner overrides `HOME`, we can do that in the hook, but we shouldn't pass that hook as an ENV with the other env's the user has set, as that is not user input, it is how the runner invokes containers
 
 ## Interface
-- You will set the variable `ACTIONS_RUNNER_CONTAINER_HOOK=/Users/foo/runner/hooks.js` which is the entrypoint to your hook handler.
+- You will set the variable `ACTIONS_RUNNER_CONTAINER_HOOKS=/Users/foo/runner/hooks.js` which is the entrypoint to your hook handler.
   - There is no partial opt in, you must handle every hook 
 - We will pass a command and some args via `stdin`
 - An exit code of 0 is a success, every other exit code is a failure

releaseNote.md

@@ -1,9 +1,6 @@
 ## Bugs
-- Avoid key based command injection via Docker command arguments (#2062)
+- Fixed an issue where job and service container envs were corrupted (#2091)
 ## 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,8 +92,6 @@ 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/ContainerOperationProvider.cs

@@ -98,41 +98,12 @@ 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))
             {
-                var healthcheck = await ContainerHealthcheck(executionContext, container);
+                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)
@@ -328,8 +299,9 @@ namespace GitHub.Runner.Worker
 
             if (!string.IsNullOrEmpty(container.ContainerId))
             {
-                if (!container.IsJobContainer && !container.FailedInitialization)
+                if (!container.IsJobContainer)
                 {
+                    // 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);
@@ -423,14 +395,14 @@ namespace GitHub.Runner.Worker
             }
         }
 
-        private async Task<string> ContainerHealthcheck(IExecutionContext executionContext, ContainerInfo container)
+        private async Task 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 String.Empty;
+                return;
             }
             var retryCount = 0;
             while (string.Equals(serviceHealth, "starting", StringComparison.OrdinalIgnoreCase))
@@ -441,7 +413,14 @@ namespace GitHub.Runner.Worker
                 serviceHealth = (await _dockerManager.DockerInspect(context: executionContext, dockerObject: container.ContainerId, options: healthCheck)).FirstOrDefault();
                 retryCount++;
             }
-            return serviceHealth;
+            if (string.Equals(serviceHealth, "healthy", StringComparison.OrdinalIgnoreCase))
+            {
+                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/Test/L0/Worker/ContainerOperationProviderL0.cs

@@ -1,126 +0,0 @@
-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.0
+2.296.1