Lets allow up to 150 characters for services on linux/mac (#1710)

thboop · web-flow · commit 02d2cb8fcdf4 · 2022-02-24T21:05:51.000-05:00
* Lets allow up to 150 characters on linux/mac, just to avoid some issues with runner naming

* Add 4 randomized digits on mac/linux

* fix pragma issue

* fix test

* �Address pr feedback

* reduce complexity

* lets make it cleaner!

* fix test

* fix logic
diff --git a/src/Runner.Listener/Configuration/ServiceControlManager.cs b/src/Runner.Listener/Configuration/ServiceControlManager.cs
@@ -48,13 +48,12 @@ public void CalculateServiceName(RunnerSettings settings, string serviceNamePatt
             string repoOrOrgName = regex.Replace(settings.RepoOrOrgName, "-");
 
             serviceName = StringUtil.Format(serviceNamePattern, repoOrOrgName, settings.AgentName);
-
-            if (serviceName.Length > 80)
+            if (serviceName.Length > MaxServiceNameLength)
             {
-                Trace.Verbose($"Calculated service name is too long (> 80 chars). Trying again by calculating a shorter name.");
-
-                int exceededCharLength = serviceName.Length - 80;
-                string repoOrOrgNameSubstring = StringUtil.SubstringPrefix(repoOrOrgName, 45);
+                Trace.Verbose($"Calculated service name is too long (> {MaxServiceNameLength} chars). Trying again by calculating a shorter name.");
+                // Add 5 to add -xxxx random number on the end
+                int exceededCharLength = serviceName.Length - MaxServiceNameLength + 5;
+                string repoOrOrgNameSubstring = StringUtil.SubstringPrefix(repoOrOrgName, MaxRepoOrgCharacters);
 
                 exceededCharLength -= repoOrOrgName.Length - repoOrOrgNameSubstring.Length;
 
@@ -66,12 +65,23 @@ public void CalculateServiceName(RunnerSettings settings, string serviceNamePatt
                     runnerNameSubstring = StringUtil.SubstringPrefix(settings.AgentName, settings.AgentName.Length - exceededCharLength);
                 }
 
+                // Lets add a suffix with a random number to reduce the chance of collisions between runner names once we truncate
+                var random = new Random();
+                var num = random.Next(1000, 9999).ToString();
+                runnerNameSubstring +=$"-{num}";
                 serviceName = StringUtil.Format(serviceNamePattern, repoOrOrgNameSubstring, runnerNameSubstring);
             }
 
             serviceDisplayName = StringUtil.Format(serviceDisplayNamePattern, repoOrOrgName, settings.AgentName);
 
             Trace.Info($"Service name '{serviceName}' display name '{serviceDisplayName}' will be used for service configuration.");
         }
+        #if (OS_LINUX || OS_OSX)
+            const int MaxServiceNameLength = 150;
+            const int MaxRepoOrgCharacters = 70;
+        #elif OS_WINDOWS
+            const int MaxServiceNameLength = 80;
+            const int MaxRepoOrgCharacters = 45;
+        #endif
     }
 }
diff --git a/src/Test/L0/ServiceControlManagerL0.cs b/src/Test/L0/ServiceControlManagerL0.cs
@@ -1,5 +1,6 @@
 using System;
 using System.Runtime.CompilerServices;
+using System.Text.RegularExpressions;
 using GitHub.Runner.Common;
 using GitHub.Runner.Listener.Configuration;
 using Xunit;
@@ -15,7 +16,7 @@ public void CalculateServiceName()
         {
             RunnerSettings settings = new RunnerSettings();
 
-            settings.AgentName = "thisiskindofalongrunnername1";
+            settings.AgentName = "thisiskindofalongrunnerabcde";
             settings.ServerUrl = "https://example.githubusercontent.com/12345678901234567890123456789012345678901234567890";
             settings.GitHubUrl = "https://github.com/myorganizationexample/myrepoexample";
 
@@ -43,7 +44,7 @@ public void CalculateServiceName()
                 Assert.Equal("actions", serviceNameParts[0]);
                 Assert.Equal("runner", serviceNameParts[1]);
                 Assert.Equal("myorganizationexample-myrepoexample", serviceNameParts[2]); // '/' has been replaced with '-'
-                Assert.Equal("thisiskindofalongrunnername1", serviceNameParts[3]);
+                Assert.Equal("thisiskindofalongrunnerabcde", serviceNameParts[3]);
             }
         }
 
@@ -54,7 +55,7 @@ public void CalculateServiceName80Chars()
         {
             RunnerSettings settings = new RunnerSettings();
 
-            settings.AgentName = "thisiskindofalongrunnername12";
+            settings.AgentName = "thisiskindofalongrunnernabcde";
             settings.ServerUrl = "https://example.githubusercontent.com/12345678901234567890123456789012345678901234567890";
             settings.GitHubUrl = "https://github.com/myorganizationexample/myrepoexample";
 
@@ -82,88 +83,129 @@ public void CalculateServiceName80Chars()
                 Assert.Equal("actions", serviceNameParts[0]);
                 Assert.Equal("runner", serviceNameParts[1]);
                 Assert.Equal("myorganizationexample-myrepoexample", serviceNameParts[2]); // '/' has been replaced with '-'
-                Assert.Equal("thisiskindofalongrunnername12", serviceNameParts[3]);
+                Assert.Equal("thisiskindofalongrunnernabcde", serviceNameParts[3]); // should not have random numbers unless we exceed 80
             }
         }
 
-        [Fact]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Service")]
-        public void CalculateServiceNameLimitsServiceNameTo80Chars()
-        {
-            RunnerSettings settings = new RunnerSettings();
-
-            settings.AgentName = "thisisareallyreallylongbutstillvalidagentname";
-            settings.ServerUrl = "https://example.githubusercontent.com/12345678901234567890123456789012345678901234567890";
-            settings.GitHubUrl = "https://github.com/myreallylongorganizationexample/myreallylongrepoexample";
-
-            string serviceNamePattern = "actions.runner.{0}.{1}";
-            string serviceDisplayNamePattern = "GitHub Actions Runner ({0}.{1})";
-
-            using (TestHostContext hc = CreateTestContext())
+        #if OS_WINDOWS
+            [Fact]
+            [Trait("Level", "L0")]
+            [Trait("Category", "Service")]
+            public void CalculateServiceNameLimitsServiceNameTo80Chars()
             {
-                ServiceControlManager scm = new ServiceControlManager();
-
-                scm.Initialize(hc);
-                scm.CalculateServiceName(
-                    settings,
-                    serviceNamePattern,
-                    serviceDisplayNamePattern,
-                    out string serviceName,
-                    out string serviceDisplayName);
-
-                // Verify name has been shortened to 80 characters
-                Assert.Equal(80, serviceName.Length);
-
-                var serviceNameParts = serviceName.Split('.');
-
-                // Verify that each component has been shortened to a sensible length
-                Assert.Equal("actions", serviceNameParts[0]); // Never shortened
-                Assert.Equal("runner", serviceNameParts[1]); // Never shortened
-                Assert.Equal("myreallylongorganizationexample-myreallylongr", serviceNameParts[2]); // First 45 chars, '/' has been replaced with '-'
-                Assert.Equal("thisisareallyreally", serviceNameParts[3]); // Remainder of unused chars
+                RunnerSettings settings = new RunnerSettings();
+
+                settings.AgentName = "thisisareallyreallylongbutstillvalidagentname";
+                settings.ServerUrl = "https://example.githubusercontent.com/12345678901234567890123456789012345678901234567890";
+                settings.GitHubUrl = "https://github.com/myreallylongorganizationexample/myreallylongrepoexample";
+
+                string serviceNamePattern = "actions.runner.{0}.{1}";
+                string serviceDisplayNamePattern = "GitHub Actions Runner ({0}.{1})";
+
+                using (TestHostContext hc = CreateTestContext())
+                {
+                    ServiceControlManager scm = new ServiceControlManager();
+
+                    scm.Initialize(hc);
+                    scm.CalculateServiceName(
+                        settings,
+                        serviceNamePattern,
+                        serviceDisplayNamePattern,
+                        out string serviceName,
+                        out string serviceDisplayName);
+
+                    // Verify name has been shortened to 80 characters
+                    Assert.Equal(80, serviceName.Length);
+
+                    var serviceNameParts = serviceName.Split('.');
+
+                    // Verify that each component has been shortened to a sensible length
+                    Assert.Equal("actions", serviceNameParts[0]); // Never shortened
+                    Assert.Equal("runner", serviceNameParts[1]); // Never shortened
+                    Assert.Equal("myreallylongorganizationexample-myreallylongr", serviceNameParts[2]); // First 45 chars, '/' has been replaced with '-'
+                    Assert.Matches(@"^(thisisareallyr-[0-9]{4})$", serviceNameParts[3]); // Remainder of unused chars, 4 random numbers added at the end
+                }
             }
-        }
-
-        // Special 'defensive' test that verifies we can gracefully handle creating service names
-        // in case github.com changes its org/repo naming convention in the future,
-        // and some of these characters may be invalid for service names
-        // Not meant to test character set exhaustively -- it's just here to exercise the sanitizing logic
-        [Fact]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Service")]
-        public void CalculateServiceNameSanitizeOutOfRangeChars()
-        {
-            RunnerSettings settings = new RunnerSettings();
-
-            settings.AgentName = "name";
-            settings.ServerUrl = "https://example.githubusercontent.com/12345678901234567890123456789012345678901234567890";
-            settings.GitHubUrl = "https://github.com/org!@$*+[]()/repo!@$*+[]()";
 
-            string serviceNamePattern = "actions.runner.{0}.{1}";
-            string serviceDisplayNamePattern = "GitHub Actions Runner ({0}.{1})";
-
-            using (TestHostContext hc = CreateTestContext())
+            // Special 'defensive' test that verifies we can gracefully handle creating service names
+            // in case github.com changes its org/repo naming convention in the future,
+            // and some of these characters may be invalid for service names
+            // Not meant to test character set exhaustively -- it's just here to exercise the sanitizing logic
+            [Fact]
+            [Trait("Level", "L0")]
+            [Trait("Category", "Service")]
+            public void CalculateServiceNameSanitizeOutOfRangeChars()
             {
-                ServiceControlManager scm = new ServiceControlManager();
-
-                scm.Initialize(hc);
-                scm.CalculateServiceName(
-                    settings,
-                    serviceNamePattern,
-                    serviceDisplayNamePattern,
-                    out string serviceName,
-                    out string serviceDisplayName);
-
-                var serviceNameParts = serviceName.Split('.');
-
-                // Verify service name parts are sanitized correctly
-                Assert.Equal("actions", serviceNameParts[0]);
-                Assert.Equal("runner", serviceNameParts[1]);
-                Assert.Equal("org----------repo---------", serviceNameParts[2]); // Chars replaced with '-'
-                Assert.Equal("name", serviceNameParts[3]);
+                RunnerSettings settings = new RunnerSettings();
+
+                settings.AgentName = "name";
+                settings.ServerUrl = "https://example.githubusercontent.com/12345678901234567890123456789012345678901234567890";
+                settings.GitHubUrl = "https://github.com/org!@$*+[]()/repo!@$*+[]()";
+
+                string serviceNamePattern = "actions.runner.{0}.{1}";
+                string serviceDisplayNamePattern = "GitHub Actions Runner ({0}.{1})";
+
+                using (TestHostContext hc = CreateTestContext())
+                {
+                    ServiceControlManager scm = new ServiceControlManager();
+
+                    scm.Initialize(hc);
+                    scm.CalculateServiceName(
+                        settings,
+                        serviceNamePattern,
+                        serviceDisplayNamePattern,
+                        out string serviceName,
+                        out string serviceDisplayName);
+
+                    var serviceNameParts = serviceName.Split('.');
+
+                    // Verify service name parts are sanitized correctly
+                    Assert.Equal("actions", serviceNameParts[0]);
+                    Assert.Equal("runner", serviceNameParts[1]);
+                    Assert.Equal("org----------repo---------", serviceNameParts[2]); // Chars replaced with '-'
+                    Assert.Equal("name", serviceNameParts[3]);
+                }
             }
-        }
+        #else
+            [Fact]
+            [Trait("Level", "L0")]
+            [Trait("Category", "Service")]
+            public void CalculateServiceNameLimitsServiceNameTo150Chars()
+            {
+                RunnerSettings settings = new RunnerSettings();
+
+                settings.AgentName = "thisisareallyreallylongbutstillvalidagentnameiamusingforthisexampletotestverylongnamelimits";
+                settings.ServerUrl = "https://example.githubusercontent.com/12345678901234567890123456789012345678901234567890";
+                settings.GitHubUrl = "https://github.com/myreallylongorganizationexampleonlinux/myreallylongrepoexampleonlinux1234";
+
+                string serviceNamePattern = "actions.runner.{0}.{1}";
+                string serviceDisplayNamePattern = "GitHub Actions Runner ({0}.{1})";
+
+                using (TestHostContext hc = CreateTestContext())
+                {
+                    ServiceControlManager scm = new ServiceControlManager();
+
+                    scm.Initialize(hc);
+                    scm.CalculateServiceName(
+                        settings,
+                        serviceNamePattern,
+                        serviceDisplayNamePattern,
+                        out string serviceName,
+                        out string serviceDisplayName);
+
+                    // Verify name has been shortened to 150
+                    Assert.Equal(150, serviceName.Length);
+
+                    var serviceNameParts = serviceName.Split('.');
+
+                    // Verify that each component has been shortened to a sensible length
+                    Assert.Equal("actions", serviceNameParts[0]); // Never shortened
+                    Assert.Equal("runner", serviceNameParts[1]); // Never shortened
+                    Assert.Equal("myreallylongorganizationexampleonlinux-myreallylongrepoexampleonlinux1", serviceNameParts[2]); // First 70 chars, '/' has been replaced with '-'
+                    Assert.Matches(@"^(thisisareallyreallylongbutstillvalidagentnameiamusingforthi-[0-9]{4})$", serviceNameParts[3]);
+                }
+            }
+        #endif
 
         private TestHostContext CreateTestContext([CallerMemberName] string testName = "")
         {
