Try to infer runner is on hosted/ghes when githuburl is empty.#4254
Try to infer runner is on hosted/ghes when githuburl is empty.#4254TingluoHuang merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds inference logic to determine whether a runner is on a hosted server or GHES when the GitHubUrl field is empty (as occurs with JIT-configured runners). The change attempts to infer the server type by examining ServerUrl and ServerUrlV2 fields and checking their domain patterns against known hosted service domains.
Changes:
- Added fallback logic in
RunnerSettings.IsHostedServerproperty getter to infer hosted/GHES status fromServerUrlandServerUrlV2whenGitHubUrlis empty - Added support for
GITHUB_ACTIONS_RUNNER_FORCE_GHESenvironment variable override in the new inference path - Added domain pattern checks for various hosted service domains (actions.githubusercontent.com, codedev.ms, githubapp.com, ghe.com, actions.localhost, ghe.localhost)
Comments suppressed due to low confidence (1)
src/Runner.Common/ConfigurationStore.cs:99
- Creating a UriBuilder with an invalid or malformed URL will throw an exception (UriFormatException). If ServerUrlV2 contains an invalid URL, this will cause an unhandled exception. While this scenario might be rare in production, consider adding error handling or validation to gracefully handle malformed URLs and prevent unexpected crashes.
var serverUrlV2 = new UriBuilder(ServerUrlV2);
| else | ||
| { | ||
| // feature flag env in case the new logic is wrong. | ||
| if (StringUtil.ConvertToBoolean(Environment.GetEnvironmentVariable("GITHUB_ACTIONS_RUNNER_FORCE_EMPTY_GITHUB_URL_IS_HOSTED"))) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // GitHubUrl will be empty for jit configured runner | ||
| // We will try to infer it from the ServerUrl/ServerUrlV2 | ||
| if (StringUtil.ConvertToBoolean(Environment.GetEnvironmentVariable("GITHUB_ACTIONS_RUNNER_FORCE_GHES"))) | ||
| { | ||
| // Allow env to override and force GHES in case the inference logic is wrong. | ||
| return false; | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(ServerUrl)) | ||
| { | ||
| // pipelines services | ||
| var serverUrl = new UriBuilder(ServerUrl); | ||
| return serverUrl.Host.EndsWith(".actions.githubusercontent.com", StringComparison.OrdinalIgnoreCase) | ||
| || serverUrl.Host.EndsWith(".codedev.ms", StringComparison.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(ServerUrlV2)) | ||
| { | ||
| // broker-listener | ||
| var serverUrlV2 = new UriBuilder(ServerUrlV2); | ||
| return serverUrlV2.Host.EndsWith(".actions.githubusercontent.com", StringComparison.OrdinalIgnoreCase) |
There was a problem hiding this comment.
According to the project's coding guidelines, changes should be safeguarded by a feature flag wherever possible. This new inference logic for determining IsHostedServer introduces a significant behavioral change that could affect how runners are classified. Consider gating this new inference logic behind a feature flag to allow for gradual rollout and easy rollback if issues are discovered. You would need to declare a new feature flag in src/Runner.Common/Constants.cs in the Constants.Runner.Features class.
There was a problem hiding this comment.
Do we need a new feature flag? Could add a new getter IsHostedServer_New and put the burden on caller to check feature flag and call the correct getter.
There was a problem hiding this comment.
i added the GITHUB_ACTIONS_RUNNER_FORCE_EMPTY_GITHUB_URL_IS_HOSTED in case any of the new code is broken.
https://github.com/github/c2c-actions-support/issues/5708