From a41a9ba8c73916ff0b2cfa790daca6cb8243b7fe Mon Sep 17 00:00:00 2001 From: Julio Barba Date: Mon, 14 Sep 2020 14:08:26 -0400 Subject: [PATCH] Revert "Allow registry credentials for job/service containers (#694)" Don't include this for the 2.273.2 release just yet This reverts commit 4e85b8f3b7493aa1adf9dd0a574a9fa2bf1add71. --- src/Runner.Worker/Container/ContainerInfo.cs | 6 -- .../Container/DockerCommandManager.cs | 38 +------- src/Runner.Worker/Container/DockerUtil.cs | 16 ---- .../ContainerOperationProvider.cs | 89 +------------------ src/Sdk/DTPipelines/Pipelines/JobContainer.cs | 31 ------- .../PipelineTemplateConstants.cs | 3 - .../PipelineTemplateConverter.cs | 27 ------ src/Sdk/DTPipelines/workflow-v1.0.json | 17 +--- src/Test/L0/Container/DockerUtilL0.cs | 18 ---- 9 files changed, 4 insertions(+), 241 deletions(-) diff --git a/src/Runner.Worker/Container/ContainerInfo.cs b/src/Runner.Worker/Container/ContainerInfo.cs index bd906c5a20b..695364c4a96 100644 --- a/src/Runner.Worker/Container/ContainerInfo.cs +++ b/src/Runner.Worker/Container/ContainerInfo.cs @@ -34,9 +34,6 @@ public ContainerInfo(IHostContext hostContext, Pipelines.JobContainer container, _environmentVariables = container.Environment; this.IsJobContainer = isJobContainer; this.ContainerNetworkAlias = networkAlias; - this.RegistryAuthUsername = container.Credentials?.Username; - this.RegistryAuthPassword = container.Credentials?.Password; - this.RegistryServer = DockerUtil.ParseRegistryHostnameFromImageName(this.ContainerImage); #if OS_WINDOWS _pathMappings.Add(new PathMapping(hostContext.GetDirectory(WellKnownDirectory.Work), "C:\\__w")); @@ -82,9 +79,6 @@ public ContainerInfo(IHostContext hostContext, Pipelines.JobContainer container, public string ContainerWorkDirectory { get; set; } public string ContainerCreateOptions { get; private set; } public string ContainerRuntimePath { get; set; } - public string RegistryServer { get; set; } - public string RegistryAuthUsername { get; set; } - public string RegistryAuthPassword { get; set; } public bool IsJobContainer { get; set; } public IDictionary ContainerEnvironmentVariables diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index 8663e934048..fd2d1051764 100644 --- a/src/Runner.Worker/Container/DockerCommandManager.cs +++ b/src/Runner.Worker/Container/DockerCommandManager.cs @@ -4,7 +4,6 @@ using System.Linq; using System.Text.RegularExpressions; using System.Threading; -using System.Threading.Channels; using System.Threading.Tasks; using GitHub.Runner.Common; using GitHub.Runner.Sdk; @@ -18,7 +17,6 @@ public interface IDockerCommandManager : IRunnerService string DockerInstanceLabel { get; } Task DockerVersion(IExecutionContext context); Task DockerPull(IExecutionContext context, string image); - Task DockerPull(IExecutionContext context, string image, string configFileDirectory); Task DockerBuild(IExecutionContext context, string workingDirectory, string dockerFile, string dockerContext, string tag); Task DockerCreate(IExecutionContext context, ContainerInfo container); Task DockerRun(IExecutionContext context, ContainerInfo container, EventHandler stdoutDataReceived, EventHandler stderrDataReceived); @@ -33,7 +31,6 @@ public interface IDockerCommandManager : IRunnerService Task DockerExec(IExecutionContext context, string containerId, string options, string command, List outputs); Task> DockerInspect(IExecutionContext context, string dockerObject, string options); Task> DockerPort(IExecutionContext context, string containerId); - Task DockerLogin(IExecutionContext context, string configFileDirectory, string registry, string username, string password); } public class DockerCommandManager : RunnerService, IDockerCommandManager @@ -85,18 +82,9 @@ public async Task DockerVersion(IExecutionContext context) return new DockerVersion(serverVersion, clientVersion); } - public Task DockerPull(IExecutionContext context, string image) + public async Task DockerPull(IExecutionContext context, string image) { - return DockerPull(context, image, null); - } - - public async Task DockerPull(IExecutionContext context, string image, string configFileDirectory) - { - if (string.IsNullOrEmpty(configFileDirectory)) - { - return await ExecuteDockerCommandAsync(context, $"pull", image, context.CancellationToken); - } - return await ExecuteDockerCommandAsync(context, $"--config {configFileDirectory} pull", image, context.CancellationToken); + return await ExecuteDockerCommandAsync(context, "pull", image, context.CancellationToken); } public async Task DockerBuild(IExecutionContext context, string workingDirectory, string dockerFile, string dockerContext, string tag) @@ -358,28 +346,6 @@ public async Task> DockerPort(IExecutionContext context, strin return DockerUtil.ParseDockerPort(portMappingLines); } - public Task DockerLogin(IExecutionContext context, string configFileDirectory, string registry, string username, string password) - { - string args = $"--config {configFileDirectory} login {registry} -u {username} --password-stdin"; - context.Command($"{DockerPath} {args}"); - - var input = Channel.CreateBounded(new BoundedChannelOptions(1) { SingleReader = true, SingleWriter = true }); - input.Writer.TryWrite(password); - - var processInvoker = HostContext.CreateService(); - - return processInvoker.ExecuteAsync( - workingDirectory: context.GetGitHubContext("workspace"), - fileName: DockerPath, - arguments: args, - environment: null, - requireExitCodeZero: false, - outputEncoding: null, - killProcessOnCancel: false, - redirectStandardIn: input, - cancellationToken: context.CancellationToken); - } - private Task ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, CancellationToken cancellationToken = default(CancellationToken)) { return ExecuteDockerCommandAsync(context, command, options, null, cancellationToken); diff --git a/src/Runner.Worker/Container/DockerUtil.cs b/src/Runner.Worker/Container/DockerUtil.cs index 02c2ece5b79..638a89cce51 100644 --- a/src/Runner.Worker/Container/DockerUtil.cs +++ b/src/Runner.Worker/Container/DockerUtil.cs @@ -45,21 +45,5 @@ public static string ParsePathFromConfigEnv(IList configEnvLines) } return ""; } - - public static string ParseRegistryHostnameFromImageName(string name) - { - var nameSplit = name.Split('/'); - // Single slash is implictly from Dockerhub, unless first part has .tld or :port - if (nameSplit.Length == 2 && (nameSplit[0].Contains(":") || nameSplit[0].Contains("."))) - { - return nameSplit[0]; - } - // All other non Dockerhub registries - else if (nameSplit.Length > 2) - { - return nameSplit[0]; - } - return ""; - } } } diff --git a/src/Runner.Worker/ContainerOperationProvider.cs b/src/Runner.Worker/ContainerOperationProvider.cs index bf107648014..8ec8a9644f0 100644 --- a/src/Runner.Worker/ContainerOperationProvider.cs +++ b/src/Runner.Worker/ContainerOperationProvider.cs @@ -198,18 +198,12 @@ private async Task StartContainerAsync(IExecutionContext executionContext, Conta } } - // TODO: Add at a later date. This currently no local package registry to test with - // UpdateRegistryAuthForGitHubToken(executionContext, container); - - // Before pulling, generate client authentication if required - var configLocation = await ContainerRegistryLogin(executionContext, container); - // Pull down docker image with retry up to 3 times int retryCount = 0; int pullExitCode = 0; while (retryCount < 3) { - pullExitCode = await _dockerManger.DockerPull(executionContext, container.ContainerImage, configLocation); + pullExitCode = await _dockerManger.DockerPull(executionContext, container.ContainerImage); if (pullExitCode == 0) { break; @@ -226,9 +220,6 @@ private async Task StartContainerAsync(IExecutionContext executionContext, Conta } } - // Remove credentials after pulling - ContainerRegistryLogout(configLocation); - if (retryCount == 3 && pullExitCode != 0) { throw new InvalidOperationException($"Docker pull failed with exit code {pullExitCode}"); @@ -446,83 +437,5 @@ private async Task ContainerHealthcheck(IExecutionContext executionContext, Cont throw new InvalidOperationException($"Failed to initialize, {container.ContainerNetworkAlias} service is {serviceHealth}."); } } - - private async Task ContainerRegistryLogin(IExecutionContext executionContext, ContainerInfo container) - { - if (string.IsNullOrEmpty(container.RegistryAuthUsername) || string.IsNullOrEmpty(container.RegistryAuthPassword)) - { - // No valid client config can be generated - return ""; - } - var configLocation = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Temp), $".docker_{Guid.NewGuid()}"); - try - { - var dirInfo = Directory.CreateDirectory(configLocation); - } - catch (Exception e) - { - throw new InvalidOperationException($"Failed to create directory to store registry client credentials: {e.Message}"); - } - var loginExitCode = await _dockerManger.DockerLogin( - executionContext, - configLocation, - container.RegistryServer, - container.RegistryAuthUsername, - container.RegistryAuthPassword); - - if (loginExitCode != 0) - { - throw new InvalidOperationException($"Docker login for '{container.RegistryServer}' failed with exit code {loginExitCode}"); - } - return configLocation; - } - - private void ContainerRegistryLogout(string configLocation) - { - try - { - if (!string.IsNullOrEmpty(configLocation) && Directory.Exists(configLocation)) - { - Directory.Delete(configLocation, recursive: true); - } - } - catch (Exception e) - { - throw new InvalidOperationException($"Failed to remove directory containing Docker client credentials: {e.Message}"); - } - } - - private void UpdateRegistryAuthForGitHubToken(IExecutionContext executionContext, ContainerInfo container) - { - var registryIsTokenCompatible = container.RegistryServer.Equals("docker.pkg.github.com", StringComparison.OrdinalIgnoreCase); - if (!registryIsTokenCompatible) - { - return; - } - - var registryMatchesWorkflow = false; - - // REGISTRY/OWNER/REPO/IMAGE[:TAG] - var imageParts = container.ContainerImage.Split('/'); - if (imageParts.Length != 4) - { - executionContext.Warning($"Could not identify owner and repo for container image {container.ContainerImage}. Skipping automatic token auth"); - return; - } - var owner = imageParts[1]; - var repo = imageParts[2]; - var nwo = $"{owner}/{repo}"; - if (nwo.Equals(executionContext.GetGitHubContext("repository"), StringComparison.OrdinalIgnoreCase)) - { - registryMatchesWorkflow = true; - } - - var registryCredentialsNotSupplied = string.IsNullOrEmpty(container.RegistryAuthUsername) && string.IsNullOrEmpty(container.RegistryAuthPassword); - if (registryCredentialsNotSupplied && registryMatchesWorkflow) - { - container.RegistryAuthUsername = executionContext.GetGitHubContext("actor"); - container.RegistryAuthPassword = executionContext.GetGitHubContext("token"); - } - } } } diff --git a/src/Sdk/DTPipelines/Pipelines/JobContainer.cs b/src/Sdk/DTPipelines/Pipelines/JobContainer.cs index 901c4fed17d..ad952cf5320 100644 --- a/src/Sdk/DTPipelines/Pipelines/JobContainer.cs +++ b/src/Sdk/DTPipelines/Pipelines/JobContainer.cs @@ -56,36 +56,5 @@ public IList Ports get; set; } - - /// - /// Gets or sets the credentials used for pulling the container iamge. - /// - public ContainerRegistryCredentials Credentials - { - get; - set; - } - } - - [EditorBrowsable(EditorBrowsableState.Never)] - public sealed class ContainerRegistryCredentials - { - /// - /// Gets or sets the user to authenticate to a registry with - /// - public String Username - { - get; - set; - } - - /// - /// Gets or sets the password to authenticate to a registry with - /// - public String Password - { - get; - set; - } } } diff --git a/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs b/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs index f769b1719f3..894b1ba6c1e 100644 --- a/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs +++ b/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs @@ -14,7 +14,6 @@ public sealed class PipelineTemplateConstants public const String Clean= "clean"; public const String Container = "container"; public const String ContinueOnError = "continue-on-error"; - public const String Credentials = "credentials"; public const String Defaults = "defaults"; public const String Env = "env"; public const String Event = "event"; @@ -46,7 +45,6 @@ public sealed class PipelineTemplateConstants public const String Options = "options"; public const String Outputs = "outputs"; public const String OutputsPattern = "needs.*.outputs"; - public const String Password = "password"; public const String Path = "path"; public const String Pool = "pool"; public const String Ports = "ports"; @@ -70,7 +68,6 @@ public sealed class PipelineTemplateConstants public const String Success = "success"; public const String Template = "template"; public const String TimeoutMinutes = "timeout-minutes"; - public const String Username = "username"; public const String Uses = "uses"; public const String VmImage = "vmImage"; public const String Volumes = "volumes"; diff --git a/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConverter.cs b/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConverter.cs index 153e4e89b58..5a9bc11b95d 100644 --- a/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConverter.cs +++ b/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConverter.cs @@ -209,30 +209,6 @@ internal static Dictionary ConvertToStepInputs( return (Int32)numberToken.Value; } - internal static ContainerRegistryCredentials ConvertToContainerCredentials(TemplateToken token) - { - var credentials = token.AssertMapping(PipelineTemplateConstants.Credentials); - var result = new ContainerRegistryCredentials(); - foreach (var credentialProperty in credentials) - { - var propertyName = credentialProperty.Key.AssertString($"{PipelineTemplateConstants.Credentials} key"); - switch (propertyName.Value) - { - case PipelineTemplateConstants.Username: - result.Username = credentialProperty.Value.AssertString(PipelineTemplateConstants.Username).Value; - break; - case PipelineTemplateConstants.Password: - result.Password = credentialProperty.Value.AssertString(PipelineTemplateConstants.Password).Value; - break; - default: - propertyName.AssertUnexpectedValue($"{PipelineTemplateConstants.Credentials} key {propertyName}"); - break; - } - } - - return result; - } - internal static JobContainer ConvertToJobContainer( TemplateContext context, TemplateToken value, @@ -299,9 +275,6 @@ internal static JobContainer ConvertToJobContainer( } result.Volumes = volumeList; break; - case PipelineTemplateConstants.Credentials: - result.Credentials = ConvertToContainerCredentials(containerPropertyPair.Value); - break; default: propertyName.AssertUnexpectedValue($"{PipelineTemplateConstants.Container} key"); break; diff --git a/src/Sdk/DTPipelines/workflow-v1.0.json b/src/Sdk/DTPipelines/workflow-v1.0.json index 9902a36dcb7..1903a90f769 100644 --- a/src/Sdk/DTPipelines/workflow-v1.0.json +++ b/src/Sdk/DTPipelines/workflow-v1.0.json @@ -373,8 +373,7 @@ "options": "non-empty-string", "env": "container-env", "ports": "sequence-of-non-empty-string", - "volumes": "sequence-of-non-empty-string", - "credentials": "container-registry-credentials" + "volumes": "sequence-of-non-empty-string" } } }, @@ -405,20 +404,6 @@ ] }, - "container-registry-credentials": { - "context": [ - "secrets", - "env", - "github" - ], - "mapping": { - "properties": { - "username": "non-empty-string", - "password": "non-empty-string" - } - } - }, - "container-env": { "mapping": { "loose-key-type": "non-empty-string", diff --git a/src/Test/L0/Container/DockerUtilL0.cs b/src/Test/L0/Container/DockerUtilL0.cs index c069255a85c..4e9d0637341 100644 --- a/src/Test/L0/Container/DockerUtilL0.cs +++ b/src/Test/L0/Container/DockerUtilL0.cs @@ -126,23 +126,5 @@ public void RegexParsesPathFromDockerConfigEnv() Assert.NotNull(result5); Assert.Equal("/foo/bar:/baz", result5); } - - [Theory] - [Trait("Level", "L0")] - [Trait("Category", "Worker")] - [InlineData("dockerhub/repo", "")] - [InlineData("localhost/doesnt_work", "")] - [InlineData("localhost:port/works", "localhost:port")] - [InlineData("host.tld/works", "host.tld")] - [InlineData("ghcr.io/owner/image", "ghcr.io")] - [InlineData("gcr.io/project/image", "gcr.io")] - [InlineData("myregistry.azurecr.io/namespace/image", "myregistry.azurecr.io")] - [InlineData("account.dkr.ecr.region.amazonaws.com/image", "account.dkr.ecr.region.amazonaws.com")] - [InlineData("docker.pkg.github.com/owner/repo/image", "docker.pkg.github.com")] - public void ParseRegistryHostnameFromImageName(string input, string expected) - { - var actual = DockerUtil.ParseRegistryHostnameFromImageName(input); - Assert.Equal(expected, actual); - } } }