Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't try to login to ghcr.io with GHES tokens #1291

Merged
merged 3 commits into from
Aug 30, 2021
Merged

Don't try to login to ghcr.io with GHES tokens #1291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c96febc
Don't try GHXX tokens for ghcr.io login
fhammerl Aug 26, 2021
e34043d
Explain hosted / onpremise in comment
fhammerl Aug 26, 2021
6d153fc
Nitfix variable name
fhammerl Aug 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Runner.Listener/Configuration/ConfigurationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ public async Task ConfigureAsync(CommandSettings command)
try
{
// Determine the service deployment type based on connection data. (Hosted/OnPremises)
// Hosted usually means github.com or localhost, while OnPremises means GHES or GHAE
runnerSettings.IsHostedServer = runnerSettings.GitHubUrl == null || UrlUtil.IsHostedServer(new UriBuilder(runnerSettings.GitHubUrl));

// Warn if the Actions server url and GHES server url has different Host
Expand Down
3 changes: 2 additions & 1 deletion src/Runner.Worker/ContainerOperationProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,8 @@ private void ContainerRegistryLogout(string configLocation)
private void UpdateRegistryAuthForGitHubToken(IExecutionContext executionContext, ContainerInfo container)
{
var registryIsTokenCompatible = container.RegistryServer.Equals("ghcr.io", StringComparison.OrdinalIgnoreCase) || container.RegistryServer.Equals("containers.pkg.github.com", StringComparison.OrdinalIgnoreCase);
if (!registryIsTokenCompatible)
var isFallbackTokenFromHostedGithub = HostContext.GetService<IConfigurationStore>().GetSettings().IsHostedServer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mind want to check what's the runner version that we introduced IsHostedServer in the setting file and does that version included in the earliest GHES release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, the IsHostedServer might return true on those GHES self-hosted runners.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runner/src/Runner.Worker/JobExtension.cs

Lines 172 to 175 in 62d5686

// Temporary hack for GHES alpha
var configurationStore = HostContext.GetService<IConfigurationStore>();
var runnerSettings = configurationStore.GetSettings();
if (string.IsNullOrEmpty(context.GetGitHubContext("server_url")) && !runnerSettings.IsHostedServer && !string.IsNullOrEmpty(runnerSettings.GitHubUrl))

Good thoughts, thanks for the feedback. These lines above gave me the confidence that IsHostedServer has been in runner versions since GHES Alpha.

Digging a bit more, this PR (#386) from 25 Mar 2020 confirms that IsHostedServer has been there since GHES Alpha.
Which was then rolled out in 2.168.0 (#420)

@TingluoHuang So any runners that were of a version lesser than 2.168.0 at the time they were configured would not benefit from this fix. Should we extend support that far back? If the life expectancy of a GHES runner instance is indeed very long (>~1y), we should either update their .runner with IsHostedServer, or calculate it runtime.

We could perhaps also consider adding settingsVersion: 2.xx.x to the settings so we could more easily handle differences like this.

if (!registryIsTokenCompatible || !isFallbackTokenFromHostedGithub)
{
return;
}
Expand Down