Skip to content

Commit

Permalink
Revert "Allow registry credentials for job/service containers (#694)"
Browse files Browse the repository at this point in the history 
Don't include this for the 2.273.2 release just yet

This reverts commit 4e85b8f.
  • Loading branch information
@juliobbv
juliobbv committed Sep 14, 2020
1 parent c18643e commit a41a9ba
Show file tree
Hide file tree
Showing 9 changed files with 4 additions and 241 deletions.
6 changes: 0 additions & 6 deletions src/Runner.Worker/Container/ContainerInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down Expand Up @@ -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<string, string> ContainerEnvironmentVariables
Expand Down
38 changes: 2 additions & 36 deletions src/Runner.Worker/Container/DockerCommandManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -18,7 +17,6 @@ public interface IDockerCommandManager : IRunnerService
string DockerInstanceLabel { get; }
Task<DockerVersion> DockerVersion(IExecutionContext context);
Task<int> DockerPull(IExecutionContext context, string image);
Task<int> DockerPull(IExecutionContext context, string image, string configFileDirectory);
Task<int> DockerBuild(IExecutionContext context, string workingDirectory, string dockerFile, string dockerContext, string tag);
Task<string> DockerCreate(IExecutionContext context, ContainerInfo container);
Task<int> DockerRun(IExecutionContext context, ContainerInfo container, EventHandler<ProcessDataReceivedEventArgs> stdoutDataReceived, EventHandler<ProcessDataReceivedEventArgs> stderrDataReceived);
Expand All @@ -33,7 +31,6 @@ public interface IDockerCommandManager : IRunnerService
Task<int> DockerExec(IExecutionContext context, string containerId, string options, string command, List<string> outputs);
Task<List<string>> DockerInspect(IExecutionContext context, string dockerObject, string options);
Task<List<PortMapping>> DockerPort(IExecutionContext context, string containerId);
Task<int> DockerLogin(IExecutionContext context, string configFileDirectory, string registry, string username, string password);
}

public class DockerCommandManager : RunnerService, IDockerCommandManager
Expand Down Expand Up @@ -85,18 +82,9 @@ public async Task<DockerVersion> DockerVersion(IExecutionContext context)
return new DockerVersion(serverVersion, clientVersion);
}

public Task<int> DockerPull(IExecutionContext context, string image)
public async Task<int> DockerPull(IExecutionContext context, string image)
{
return DockerPull(context, image, null);
}

public async Task<int> 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<int> DockerBuild(IExecutionContext context, string workingDirectory, string dockerFile, string dockerContext, string tag)
Expand Down Expand Up @@ -358,28 +346,6 @@ public async Task<List<PortMapping>> DockerPort(IExecutionContext context, strin
return DockerUtil.ParseDockerPort(portMappingLines);
}

public Task<int> 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<string>(new BoundedChannelOptions(1) { SingleReader = true, SingleWriter = true });
input.Writer.TryWrite(password);

var processInvoker = HostContext.CreateService<IProcessInvoker>();

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<int> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, CancellationToken cancellationToken = default(CancellationToken))
{
return ExecuteDockerCommandAsync(context, command, options, null, cancellationToken);
Expand Down
16 changes: 0 additions & 16 deletions src/Runner.Worker/Container/DockerUtil.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,5 @@ public static string ParsePathFromConfigEnv(IList<string> 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 "";
}
}
}
89 changes: 1 addition & 88 deletions src/Runner.Worker/ContainerOperationProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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}");
Expand Down Expand Up @@ -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<string> 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");
}
}
}
}
31 changes: 0 additions & 31 deletions src/Sdk/DTPipelines/Pipelines/JobContainer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,36 +56,5 @@ public IList<String> Ports
get;
set;
}

/// <summary>
/// Gets or sets the credentials used for pulling the container iamge.
/// </summary>
public ContainerRegistryCredentials Credentials
{
get;
set;
}
}

[EditorBrowsable(EditorBrowsableState.Never)]
public sealed class ContainerRegistryCredentials
{
/// <summary>
/// Gets or sets the user to authenticate to a registry with
/// </summary>
public String Username
{
get;
set;
}

/// <summary>
/// Gets or sets the password to authenticate to a registry with
/// </summary>
public String Password
{
get;
set;
}
}
}
3 changes: 0 additions & 3 deletions src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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";
Expand All @@ -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";
Expand Down
27 changes: 0 additions & 27 deletions src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,30 +209,6 @@ internal static Dictionary<String, String> 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,
Expand Down Expand Up @@ -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;
Expand Down
17 changes: 1 addition & 16 deletions src/Sdk/DTPipelines/workflow-v1.0.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
},
Expand Down Expand Up @@ -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",
Expand Down
18 changes: 0 additions & 18 deletions src/Test/L0/Container/DockerUtilL0.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

0 comments on commit a41a9ba

Please sign in to comment.