Allow non-Az modules in requirements.psd1 (#287)

AnatoliB · web-flow · commit 7a8548db21b8 · 2019-07-30T12:29:52.000-07:00
Allow non-Az modules in requirements.psd1
diff --git a/docs/designs/PowerShell-AzF-Overall-Design.md b/docs/designs/PowerShell-AzF-Overall-Design.md
@@ -466,13 +466,16 @@ Note that, checking out a PowerShell Manager instance from the pool is a blockin
 
 The goal is to let the user declare the dependencies required by functions, and rely on the service automatically locating and installing the dependencies from the PowerShell Gallery or other sources, taking care of selecting the proper versions, and automatically upgrading the dependencies to the latest versions (if allowed by the version specifications provided by the user).
 
-Dependencies are declared in the _requirements.psd1_ file (_manifest_) as a collection of pairs (<_name_>, <_version specification_>). Currently, only _Az_ module is allowed, and the version specification should strictly match the following pattern: `<major version>.*`, so a typical manifest looks like this:
+Dependencies are declared in the _requirements.psd1_ file (_manifest_) as a collection of pairs (<_name_>, <_version specification_>). Currently, the version specification should strictly match the following pattern: `<major version>.*`, so a typical manifest looks like this:
 
 ``` PowerShell
-@{ 'Az' = '2.*' }
+@{
+  'Az' = '2.*'
+  'PSDepend' = '0.*'
+}
 ```
 
-However, the design should accommodate multiple module entries and allow specifying exact versions.
+The number of entries in the _requirements.psd1_ file should not exceed **10**. This limit is not user-configurable.
 
 Installing and upgrading dependencies should be performed automatically, without requiring any interaction with the user, and without interfering with the currently running functions. This represents an important design challenge. In a different context, dependencies could be stored on a single location on the file system, managed by regular PowerShell tools (`Install-Module`/`Save-Module`, `PSDepend`, etc.), while having the same file system location added to _PSModulePath_ to make all the modules available to scripts running on this machine. This is what PowerShell users normally do, and this approach looks attractive because it is simple and conventional. However, in the contexts where multiple independent workers load modules and execute scripts concurrently, and at the same time some module versions are being added, upgraded, or removed, this simple approach causes many known problems. The root causes of these problems are in the fundamentals of PowerShell and PowerShell modules design. The managed dependencies design in Azure Functions must take this into account. The problems will be solved if we satisfy the following conditions:
 
diff --git a/src/DependencyManagement/DependencyManagerStorage.cs b/src/DependencyManagement/DependencyManagerStorage.cs
@@ -58,6 +58,11 @@ public IEnumerable<string> GetInstalledAndInstallingSnapshots()
         public IEnumerable<string> GetInstalledModuleVersions(string snapshotPath, string moduleName, string majorVersion)
         {
             var modulePath = Path.Join(snapshotPath, moduleName);
+            if (!Directory.Exists(modulePath))
+            {
+                return Enumerable.Empty<string>();
+            }
+
             return Directory.EnumerateDirectories(modulePath, $"{majorVersion}.*");
         }
 
diff --git a/src/DependencyManagement/DependencyManifest.cs b/src/DependencyManagement/DependencyManifest.cs
@@ -9,7 +9,6 @@ namespace Microsoft.Azure.Functions.PowerShellWorker.DependencyManagement
     using System.Collections;
     using System.Collections.Generic;
     using System.IO;
-    using System.Linq;
     using System.Management.Automation.Language;
     using System.Text;
     using System.Text.RegularExpressions;
@@ -18,21 +17,19 @@ internal class DependencyManifest
     {
         private const string RequirementsPsd1FileName = "requirements.psd1";
 
-        private const string AzModuleName = "Az";
-
-        // The list of managed dependencies supported in Azure Functions.
-        private static readonly List<string> SupportedManagedDependencies = new List<string> { AzModuleName };
-
         private readonly string _functionAppRootPath;
 
-        public DependencyManifest(string functionAppRootPath)
+        private readonly int _maxDependencyEntries;
+
+        public DependencyManifest(string functionAppRootPath, int maxDependencyEntries = 10)
         {
             if (string.IsNullOrWhiteSpace(functionAppRootPath))
             {
                 throw new ArgumentException("Argument is null or empty", nameof(functionAppRootPath));
             }
 
             _functionAppRootPath = functionAppRootPath;
+            _maxDependencyEntries = maxDependencyEntries;
         }
 
         public IEnumerable<DependencyManifestEntry> GetEntries()
@@ -90,27 +87,23 @@ private Hashtable ParsePowerShellDataFile()
                 throw new ArgumentException(errorMsg);
             }
 
+            if (hashtable.Count > _maxDependencyEntries)
+            {
+                var message = string.Format(PowerShellWorkerStrings.TooManyDependencies, RequirementsPsd1FileName, hashtable.Count, _maxDependencyEntries);
+                throw new ArgumentException(message);
+            }
+
             return hashtable;
         }
 
         /// <summary>
-        /// Validate that the module name is not null or empty,
-        /// and ensure that the module is a supported dependency.
+        /// Validate that the module name is not null or empty.
         /// </summary>
         private static void ValidateModuleName(string name)
         {
-            // Validate the name property.
-            if (string.IsNullOrEmpty(name))
+            if (string.IsNullOrWhiteSpace(name))
             {
-                var errorMessage = string.Format(PowerShellWorkerStrings.DependencyPropertyIsNullOrEmpty, "name");
-                throw new ArgumentException(errorMessage);
-            }
-
-            // If this is not a supported module, error out.
-            if (!SupportedManagedDependencies.Contains(name, StringComparer.OrdinalIgnoreCase))
-            {
-                var errorMessage = string.Format(PowerShellWorkerStrings.ManagedDependencyNotSupported, name);
-                throw new ArgumentException(errorMessage);
+                throw new ArgumentException(PowerShellWorkerStrings.DependencyNameIsNullOrEmpty);
             }
         }
 
@@ -126,10 +119,9 @@ private static string GetMajorVersion(string version)
 
         private static void ValidateVersionFormat(string version)
         {
-            if (string.IsNullOrEmpty(version))
+            if (version == null)
             {
-                var errorMessage = string.Format(PowerShellWorkerStrings.DependencyPropertyIsNullOrEmpty, "version");
-                throw new ArgumentException(errorMessage);
+                throw new ArgumentNullException(version);
             }
 
             if (!IsValidVersionFormat(version))
@@ -144,7 +136,7 @@ private static void ValidateVersionFormat(string version)
         /// </summary>
         private static bool IsValidVersionFormat(string version)
         {
-            var pattern = @"^(\d){1,2}(\.)(\*)";
+            var pattern = @"^(\d)+(\.)(\*)";
             return Regex.IsMatch(version, pattern);
         }
     }
diff --git a/src/DependencyManagement/DependencySnapshotInstaller.cs b/src/DependencyManagement/DependencySnapshotInstaller.cs
@@ -48,6 +48,8 @@ public void InstallSnapshot(
                     string moduleName = module.Name;
                     string latestVersion = module.LatestVersion;
 
+                    logger.Log(isUserOnlyLog: false, LogLevel.Trace, string.Format(PowerShellWorkerStrings.StartedInstallingModule, moduleName, latestVersion));
+
                     int tries = 1;
 
                     while (true)
diff --git a/src/resources/PowerShellWorkerStrings.resx b/src/resources/PowerShellWorkerStrings.resx
@@ -172,18 +172,18 @@
   <data name="FunctionAppDoesNotHaveDependentModulesToInstall" xml:space="preserve">
     <value>FunctionApp does have dependent modules to install.</value>
   </data>
-  <data name="DependencyPropertyIsNullOrEmpty" xml:space="preserve">
-    <value>Dependency '{0}' is null or empty.</value>
+  <data name="DependencyNameIsNullOrEmpty" xml:space="preserve">
+    <value>Dependency name is null or empty.</value>
+  </data>
+  <data name="StartedInstallingModule" xml:space="preserve">
+    <value>Started installing module '{0}' version '{1}'.</value>
   </data>
   <data name="ModuleHasBeenInstalled" xml:space="preserve">
     <value>Module name '{0}' version '{1}' has been installed.</value>
   </data>
   <data name="AcceptableFunctionAppDependenciesAlreadyInstalled" xml:space="preserve">
     <value>The function app has existing dependencies installed. Updating the dependencies to the latest versions will be performed in the background. New function app instances will pick up any new dependencies.</value>
   </data>
-  <data name="ManagedDependencyNotSupported" xml:space="preserve">
-    <value>Managed dependency name '{0}' is not supported.</value>
-  </data>
   <data name="InvalidPowerShellDataFile" xml:space="preserve">
     <value>The PowerShell data file '{0}' is invalid since it cannot be evaluated into a Hashtable object.</value>
   </data>
@@ -259,4 +259,7 @@
   <data name="FailedToInstallDependenciesSnapshot" xml:space="preserve">
     <value>Failed to install dependencies into '{0}', removing the folder.</value>
   </data>
+  <data name="TooManyDependencies" xml:space="preserve">
+    <value>The number of entries in the '{0}' file is {1}, which exceeds the maximum supported number of entries ({2}).</value>
+  </data>
 </root>
diff --git a/test/Unit/DependencyManagement/DependencyManifestTests.cs b/test/Unit/DependencyManagement/DependencyManifestTests.cs
@@ -0,0 +1,162 @@
+﻿//
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+//
+
+namespace Microsoft.Azure.Functions.PowerShellWorker.Test.DependencyManagement
+{
+    using System;
+    using System.IO;
+    using System.Linq;
+    using Xunit;
+
+    using PowerShellWorker.DependencyManagement;
+
+    public class DependencyManifestTests : IDisposable
+    {
+        private readonly string _appRootPath;
+
+        public DependencyManifestTests()
+        {
+            _appRootPath = Path.Join(Path.GetTempPath(), Guid.NewGuid().ToString());
+            Directory.CreateDirectory(_appRootPath);
+        }
+
+        public void Dispose()
+        {
+            Directory.Delete(_appRootPath, recursive: true);
+        }
+
+        [Fact]
+        public void CanBeConstructedWithAnyAppRootPath()
+        {
+            new DependencyManifest("This path does not have to exist");
+        }
+
+        [Fact]
+        public void GetEntriesThrowsWhenRequirementsFileDoesNotExist()
+        {
+            var manifest = new DependencyManifest(_appRootPath);
+            var exception = Assert.Throws<ArgumentException>(() => manifest.GetEntries().ToList());
+            Assert.Contains("No 'requirements.psd1' is found at the FunctionApp root folder", exception.Message);
+            Assert.Contains(_appRootPath, exception.Message);
+        }
+
+        [Fact]
+        public void GetEntriesThrowsWhenRequirementsFileIsEmpty()
+        {
+            CreateRequirementsFile(string.Empty);
+
+            var manifest = new DependencyManifest(_appRootPath);
+            var exception = Assert.Throws<ArgumentException>(() => manifest.GetEntries().ToList());
+            Assert.Equal(
+                "The PowerShell data file 'requirements.psd1' is invalid since it cannot be evaluated into a Hashtable object.",
+                exception.Message);
+        }
+
+        [Fact]
+        public void GetEntriesParsesRequirementsFileWithNoEntries()
+        {
+            CreateRequirementsFile("@{ }");
+
+            var manifest = new DependencyManifest(_appRootPath);
+            Assert.Empty(manifest.GetEntries());
+        }
+
+        [Theory]
+        [InlineData("@{ MyModule = '0.*' }", "MyModule", "0")]
+        [InlineData("@{ MyModule = '1.*' }", "MyModule", "1")]
+        [InlineData("@{ MyModule = '23.*' }", "MyModule", "23")]
+        [InlineData("@{ MyModule = '456.*' }", "MyModule", "456")]
+        public void GetEntriesParsesRequirementsFileWithSingleEntry(string content, string moduleName, string majorVersion)
+        {
+            CreateRequirementsFile(content);
+
+            var manifest = new DependencyManifest(_appRootPath);
+            var entries = manifest.GetEntries().ToList();
+
+            Assert.Single(entries);
+            Assert.Equal(moduleName, entries.Single().Name);
+            Assert.Equal(majorVersion, entries.Single().MajorVersion);
+        }
+
+        [Fact]
+        public void GetEntriesParsesRequirementsFileWithMultipleEntries()
+        {
+            CreateRequirementsFile("@{ A = '3.*'; B = '7.*'; C = '0.*' }");
+
+            var manifest = new DependencyManifest(_appRootPath, maxDependencyEntries: 3);
+            var entries = manifest.GetEntries().ToList();
+
+            Assert.Equal(3, entries.Count);
+            Assert.Equal("3", entries.Single(entry => entry.Name == "A").MajorVersion);
+            Assert.Equal("7", entries.Single(entry => entry.Name == "B").MajorVersion);
+            Assert.Equal("0", entries.Single(entry => entry.Name == "C").MajorVersion);
+        }
+
+        [Theory]
+        [InlineData("@{ MyModule = '' }")]
+        [InlineData("@{ MyModule = 'a' }")]
+        [InlineData("@{ MyModule = '.' }")]
+        [InlineData("@{ MyModule = '1' }")]
+        [InlineData("@{ MyModule = '1.' }")]
+        [InlineData("@{ MyModule = '1.0' }")]
+        [InlineData("@{ MyModule = '1.2' }")]
+        [InlineData("@{ MyModule = '2.3.4' }")]
+        public void GetEntriesThrowsOnInvalidVersionSpecification(string content)
+        {
+            CreateRequirementsFile(content);
+
+            var manifest = new DependencyManifest(_appRootPath);
+
+            var exception = Assert.Throws<ArgumentException>(() => manifest.GetEntries().ToList());
+            Assert.Contains("Version is not in the correct format", exception.Message);
+        }
+
+        [Theory]
+        [InlineData("@{ '' = '1.*' }")]
+        [InlineData("@{ ' ' = '1.*' }")]
+        [InlineData("@{ ' ' = '' }")]
+        public void GetEntriesThrowsOnInvalidModuleName(string content)
+        {
+            CreateRequirementsFile(content);
+
+            var manifest = new DependencyManifest(_appRootPath);
+
+            var exception = Assert.Throws<ArgumentException>(() => manifest.GetEntries().ToList());
+            Assert.Contains("Dependency name is null or empty", exception.Message);
+        }
+
+        [Fact]
+        public void GetEntriesThrowsOnNullModuleName()
+        {
+            CreateRequirementsFile("@{ $null = '1.0' }");
+
+            var manifest = new DependencyManifest(_appRootPath);
+
+            Assert.ThrowsAny<Exception>(() => manifest.GetEntries().ToList());
+        }
+
+        [Fact]
+        public void GetEntriesThrowsWhenTooManyEntries()
+        {
+            CreateRequirementsFile("@{ A = '3.*'; B = '7.*'; C = '0.*' }");
+
+            var manifest = new DependencyManifest(_appRootPath, maxDependencyEntries: 2);
+
+            var exception = Assert.Throws<ArgumentException>(() => manifest.GetEntries().ToList());
+
+            const string ExpectedMessage = "The number of entries in the 'requirements.psd1' file is 3,"
+                                           + " which exceeds the maximum supported number of entries (2).";
+            Assert.Equal(ExpectedMessage, exception.Message);
+        }
+
+        private void CreateRequirementsFile(string content)
+        {
+            using (var writer = new StreamWriter(Path.Join(_appRootPath, "requirements.psd1")))
+            {
+                writer.Write(content);
+            }
+        }
+    }
+}
diff --git a/test/Unit/DependencyManagement/DependencySnapshotInstallerTests.cs b/test/Unit/DependencyManagement/DependencySnapshotInstallerTests.cs
@@ -15,6 +15,8 @@ namespace Microsoft.Azure.Functions.PowerShellWorker.Test.DependencyManagement
     using Microsoft.Azure.Functions.PowerShellWorker.DependencyManagement;
     using Microsoft.Azure.Functions.PowerShellWorker.Utility;
 
+    using LogLevel = Microsoft.Azure.WebJobs.Script.Grpc.Messages.RpcLog.Types.Level;
+
     public class DependencySnapshotInstallerTests
     {
         private readonly Mock<IModuleProvider> _mockModuleProvider = new Mock<IModuleProvider>(MockBehavior.Strict);
@@ -85,9 +87,31 @@ public void InstallsDependencySnapshots()
 
             foreach (var entry in _testDependencyManifestEntries)
             {
+                _mockLogger.Verify(
+                    _ => _.Log(
+                        false,
+                        LogLevel.Trace,
+                        It.Is<string>(
+                            message => message.Contains("Started installing")
+                                       && message.Contains(entry.Name)
+                                       && message.Contains(_testLatestPublishedModuleVersions[entry.Name])),
+                        null),
+                    Times.Once);
+
                 _mockModuleProvider.Verify(
                     _ => _.SaveModule(dummyPowerShell, entry.Name, _testLatestPublishedModuleVersions[entry.Name], _targetPathInstalling),
                     Times.Once);
+
+                _mockLogger.Verify(
+                    _ => _.Log(
+                        false,
+                        LogLevel.Trace,
+                        It.Is<string>(
+                            message => message.Contains("has been installed")
+                                       && message.Contains(entry.Name)
+                                       && message.Contains(_testLatestPublishedModuleVersions[entry.Name])),
+                        null),
+                    Times.Once);
             }
 
             _mockStorage.Verify(_ => _.PromoteInstallingSnapshotToInstalledAtomically(_targetPathInstalled), Times.Once);

Original file line number	Diff line number	Diff line change
`@@ -58,6 +58,11 @@ public IEnumerable<string> GetInstalledAndInstallingSnapshots()`
`58`	`58`	`public IEnumerable<string> GetInstalledModuleVersions(string snapshotPath, string moduleName, string majorVersion)`
`59`	`59`	`{`
`60`	`60`	`var modulePath = Path.Join(snapshotPath, moduleName);`
	`61`	`+ if (!Directory.Exists(modulePath))`
	`62`	`+ {`
	`63`	`+ return Enumerable.Empty<string>();`
	`64`	`+ }`
	`65`	`+`
`61`	`66`	`return Directory.EnumerateDirectories(modulePath, $"{majorVersion}.*");`
`62`	`67`	`}`
`63`	`68`
Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,6 @@ namespace Microsoft.Azure.Functions.PowerShellWorker.DependencyManagement`
`9`	`9`	`using System.Collections;`
`10`	`10`	`using System.Collections.Generic;`
`11`	`11`	`using System.IO;`
`12`		`- using System.Linq;`
`13`	`12`	`using System.Management.Automation.Language;`
`14`	`13`	`using System.Text;`
`15`	`14`	`using System.Text.RegularExpressions;`
`@@ -18,21 +17,19 @@ internal class DependencyManifest`
`18`	`17`	`{`
`19`	`18`	`private const string RequirementsPsd1FileName = "requirements.psd1";`
`20`	`19`
`21`		`- private const string AzModuleName = "Az";`
`22`		`-`
`23`		`- // The list of managed dependencies supported in Azure Functions.`
`24`		`- private static readonly List<string> SupportedManagedDependencies = new List<string> { AzModuleName };`
`25`		`-`
`26`	`20`	`private readonly string _functionAppRootPath;`
`27`	`21`
`28`		`- public DependencyManifest(string functionAppRootPath)`
	`22`	`+ private readonly int _maxDependencyEntries;`
	`23`	`+`
	`24`	`+ public DependencyManifest(string functionAppRootPath, int maxDependencyEntries = 10)`
`29`	`25`	`{`
`30`	`26`	`if (string.IsNullOrWhiteSpace(functionAppRootPath))`
`31`	`27`	`{`
`32`	`28`	`throw new ArgumentException("Argument is null or empty", nameof(functionAppRootPath));`
`33`	`29`	`}`
`34`	`30`
`35`	`31`	`_functionAppRootPath = functionAppRootPath;`
	`32`	`+ _maxDependencyEntries = maxDependencyEntries;`
`36`	`33`	`}`
`37`	`34`
`38`	`35`	`public IEnumerable<DependencyManifestEntry> GetEntries()`
`@@ -90,27 +87,23 @@ private Hashtable ParsePowerShellDataFile()`
`90`	`87`	`throw new ArgumentException(errorMsg);`
`91`	`88`	`}`
`92`	`89`
	`90`	`+ if (hashtable.Count > _maxDependencyEntries)`
	`91`	`+ {`
	`92`	`+ var message = string.Format(PowerShellWorkerStrings.TooManyDependencies, RequirementsPsd1FileName, hashtable.Count, _maxDependencyEntries);`
	`93`	`+ throw new ArgumentException(message);`
	`94`	`+ }`
	`95`	`+`
`93`	`96`	`return hashtable;`
`94`	`97`	`}`
`95`	`98`
`96`	`99`	`/// <summary>`
`97`		`- /// Validate that the module name is not null or empty,`
`98`		`- /// and ensure that the module is a supported dependency.`
	`100`	`+ /// Validate that the module name is not null or empty.`
`99`	`101`	`/// </summary>`
`100`	`102`	`private static void ValidateModuleName(string name)`
`101`	`103`	`{`
`102`		`- // Validate the name property.`
`103`		`- if (string.IsNullOrEmpty(name))`
	`104`	`+ if (string.IsNullOrWhiteSpace(name))`
`104`	`105`	`{`
`105`		`- var errorMessage = string.Format(PowerShellWorkerStrings.DependencyPropertyIsNullOrEmpty, "name");`
`106`		`- throw new ArgumentException(errorMessage);`
`107`		`- }`
`108`		`-`
`109`		`- // If this is not a supported module, error out.`
`110`		`- if (!SupportedManagedDependencies.Contains(name, StringComparer.OrdinalIgnoreCase))`
`111`		`- {`
`112`		`- var errorMessage = string.Format(PowerShellWorkerStrings.ManagedDependencyNotSupported, name);`
`113`		`- throw new ArgumentException(errorMessage);`
	`106`	`+ throw new ArgumentException(PowerShellWorkerStrings.DependencyNameIsNullOrEmpty);`
`114`	`107`	`}`
`115`	`108`	`}`
`116`	`109`
`@@ -126,10 +119,9 @@ private static string GetMajorVersion(string version)`
`126`	`119`
`127`	`120`	`private static void ValidateVersionFormat(string version)`
`128`	`121`	`{`
`129`		`- if (string.IsNullOrEmpty(version))`
	`122`	`+ if (version == null)`
`130`	`123`	`{`
`131`		`- var errorMessage = string.Format(PowerShellWorkerStrings.DependencyPropertyIsNullOrEmpty, "version");`
`132`		`- throw new ArgumentException(errorMessage);`
	`124`	`+ throw new ArgumentNullException(version);`
`133`	`125`	`}`
`134`	`126`
`135`	`127`	`if (!IsValidVersionFormat(version))`
`@@ -144,7 +136,7 @@ private static void ValidateVersionFormat(string version)`
`144`	`136`	`/// </summary>`
`145`	`137`	`private static bool IsValidVersionFormat(string version)`
`146`	`138`	`{`
`147`		`- var pattern = @"^(\d){1,2}(\.)(\*)";`
	`139`	`+ var pattern = @"^(\d)+(\.)(\*)";`
`148`	`140`	`return Regex.IsMatch(version, pattern);`
`149`	`141`	`}`
`150`	`142`	`}`