Add function management APIs to the runtime #2005

ahmelsayed · 2017-10-06T21:08:51Z

This change adds the following APIs:

Function management APIs

GET /admin/functions - List available functions
GET /admin/functions/{name} - Get a specific function - FunctionsController
PUT /admin/functions/{name} - Create or update an existing function - FunctionsController
DELETE /admin/functions/{name} - Delete a function - FunctionsController

File management APIs (VFS)

GET /admin/vfs/* - Get a file or directory content - VirtualFileSystem
PUT /admin/vfs/* - Update file or directory - VirtualFileSystem
DELETE /admin/vfs/* - Delete a file or directory - VirtualFileSystem
GET /admin/zip/* - Download a zip archive - ZipFileSystem

The VFS implementation is mostly a copy of the one in kudu, but modified to work with ASP.NET Core and on Linux. However there is a difference in behavior between Unix vs Windows for vfs.
In Windows, the file system doesn’t have a root (at least not in win32) so the root of your virtual filesystem is your home. (%HOME%) or scriptRoot if HOME is not defined, and is a special path that looks like /admin/vfs/SystemDrive, this maps to the system drive.
In Linux the root is simply just / which is the root of the file system.

This PR also:

Renames and splits AdminController --> HostController for script host general APIs both runtime and management, FunctionsController for function specific APIs, both runtime and management.
Removes check for host running before any /admin API call. Basically whoever implements an Admin API has to expect the host to not be running. If it's a management API, then it should work. If it's a runtime api, it should fail with a proper error explaining why the host is not running.

//cc @davidebbo @btardif for FYI

fabiocav · 2017-11-18T00:11:23Z

@ahmelsayed apologies for the delay on this... could you rebase this on the latest before a review?

ahmelsayed · 2017-11-18T01:30:28Z

will do

I figured I should explain a bit about the PR and file structure since there are a lot of new files.

src/WebJobs.Script.WebHost/Management
|- IWebFunctionsManager.cs # interface implemented in WebFunctionsManager.cs below
|- VirtualFileSystem.cs # implementation of /vfs/ apis
|- VistualFileSystemBase.cs # shared code between VFS and ZIP file systems
|- WebFunctionsManager.cs # implementation that drives the management APIs in FunctionsController
|- ZipFileSystem.cs # allows uploading zip files, and downloading directories as zip files.

src/WebJobs.Script.WebHost/Extensions 
|- .. # Helper classes

src/WebJobs.Script.WebHost/Helpers
|- FileSystemHelpers.cs # You mentioned that you use System.IO.Abstraction (or want to use?) I couldn't find it. 

src/WebJobs.Script.WebHost/Middleware
|- # I moved VFS from kudu as a middleware rather than a controller because all the code is expecting to get an HttpRequestMessage, and produce an HttpResponseMessage. It doesn't map itself too easily to IActionResult from what I have seen. Though I'm open to feedback if you think differently. 

src/WebJobs.Script.WebHost/Models
|- FunctionMetadataResponse.cs # this class is how kudu describes a function right now. The runtime has it's own FunctionMetadata.cs class
# this class has a static mapper that converts FunctionMetadata -> FunctionMetadataResponse

fabiocav · 2017-11-22T00:38:13Z

src/WebJobs.Script.WebHost/Controllers/HostController.cs

    /// </summary>
-    public class AdminController : Controller
+    public class HostController : Controller
    {


Update description?

ahmelsayed · 2017-11-22T00:39:27Z

oh I thought I rebased this, but it looks like dev moved again :)

fabiocav · 2017-11-22T00:43:56Z

src/WebJobs.Script.WebHost/Middleware/VirtualFileSystemMiddleware.cs

+
+        private async Task<bool> AuthenticateAndAuthorize(HttpContext context)
+        {
+            var authorizationEvaluator = context.RequestServices.GetRequiredService<IAuthorizationPolicyProvider>();


nit: change variable name to authorizationPolicyProvider or prolicyProvider

fabiocav · 2017-11-22T00:46:07Z

src/WebJobs.Script.WebHost/Models/FunctionMetadataResponse.cs

+
+namespace WebJobs.Script.Management.Models
+{
+    public class FunctionMetadataResponse


How problematic would it be if we changed the response payloads to use the ApiModel approach used with other management APIs? That model handles things like ref links (self, hrefs, etc.) and other things to keep the way resources are exposed consistent

It's possible. It'll just require a UX change to handle both the kudu model and this new model.

I was thinking that eventually we would get rid of FunctionMetadataResponse object since it's just a map on top of FunctionMetadata. also kudu uses name_name instead of camelCasing like the runtime. I wanted to change the model but would rather just do it once so that the UX expects only one or the other.

so do you think it's better to keep both FunctionMetadata and FunctionMetadataResponse?

fabiocav · 2017-11-22T00:48:28Z

src/WebJobs.Script.WebHost/WebHostResolver.cs

            if (ScriptSettingsManager.Instance.IsAzureEnvironment)
            {
+                // When running on Azure, we kick this off on the background
+                // Q: why?


Not sure if the logic here was "run on background when running on Azure" or "run on background if there's work to do", which only happened on Azure. The intention here is not really clear.

it just seemed odd to me that this path is running in a task. I wasn't sure why so I put the comment. Still not really sure why it was done like this in the first place :)

fabiocav · 2017-11-22T00:49:04Z

src/WebJobs.Script.WebHost/WebHostSettings.cs


        public string SecretsPath { get; set; }

+        public string TestDataPath { get; set; }


Can you add a description? What is this for?

fabiocav · 2017-11-22T00:52:09Z

src/WebJobs.Script.WebHost/WebJobsServiceCollectionExtensions.cs

+            builder.RegisterType<WebFunctionsManager>().As<IWebFunctionsManager>().SingleInstance();
+            builder.RegisterType<VirtualFileSystem>();
+            builder.RegisterType<ZipFileSystem>();
+            builder.RegisterType<VirtualFileSystemMiddleware>();


Was the middleware registration here required? THe call to UseMiddleware should be sufficient

fabiocav · 2017-11-22T01:01:44Z

src/WebJobs.Script.WebHost/Extensions/TaskExtensions.cs

+
+namespace Microsoft.Azure.WebJobs.Script.WebHost.Extensions
+{
+    public static class TaskExtensions


Class name confused me at first. This is really an IEnumerable of Task extensions class

fabiocav · 2017-11-22T01:04:14Z

src/WebJobs.Script.WebHost/Helpers/DelegatingStream.cs

+        {
+            if (innerStream == null)
+            {
+                throw new ArgumentNullException("innerStream");


nit: use nameof for parameter name and throw expression for the null validation.

removed file

fabiocav · 2017-11-22T01:05:19Z

src/WebJobs.Script.WebHost/Helpers/DelegatingStream.cs

+    {
+        private Stream _innerStream;
+
+        protected DelegatingStream(Stream innerStream)


Why is this needed?

some random C# quirk with inheriting from Stream class I guess. It's only used in the ZipArchive scenario and I'm removing that since it doesn't make sense in the runtime.

fabiocav · 2017-11-22T01:07:23Z

src/WebJobs.Script.WebHost/Helpers/FileSystemHelpers.cs

+
+namespace Microsoft.Azure.WebJobs.Script.WebHost.Helpers
+{
+    public static class FileSystemHelpers


There's a FileUtility class already in the project. Some things here seem to overlap, but we should either move what is here there or vice-versa, but we shouldn't have both.

fabiocav · 2017-11-22T01:16:43Z

src/WebJobs.Script.WebHost/Extensions/HttpRequestExtensions.cs

+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Http.Extensions;
+
+namespace WebJobs.Script.WebHost.Extensions


We have an HttpRequestExtensions class in Script, would be good to just move this there.

fabiocav · 2017-11-22T01:19:49Z

src/WebJobs.Script.WebHost/Middleware/ScriptHostCheckMiddleware.cs

@@ -1,9 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the MIT License. See License.txt in the project root for license information.



Is the change here required? Just removing unused usings? Or was there something that got lost in a rebase?

obsolete since Mathew replaced this file with another implementation.

fabiocav

Took an initial look and provided some feedback.

I'm assuming a lot of the things here came from Kudu (the VFS stuff), is that correct? I didn't thoroughly review those because that was my assumption and that would be pretty stable code.

Were there any tests for that new surface that we could bring here as well?

fabiocav · 2017-12-11T13:53:51Z

@ahmelsayed let me know if you need any more information on the feedback provided. Would be good to have these changes merged in soon.

ahmelsayed · 2018-02-01T20:57:11Z

src/WebJobs.Script.WebHost/Extensions/FunctionMetadataExtensions.cs

+            {
+                Name = functionMetadata.Name,
+
+                // Q: can functionMetadata.ScriptFile be null or empty?


@fabiocav do you know the answer to this?

ahmelsayed · 2018-02-01T21:36:52Z

@fabiocav regarding tests, most of the VFS tests in kudu rely on System.IO.Abstraction mocks which the WebHost project doesn't use. Would you be okay with me switching FileUtility to use IFileSystem instead of directly using System.IO.*?

also sorry for the delay in addressing your comments :) I was swamped by so many different things the past couple of months.

ahmelsayed · 2018-03-08T01:13:49Z

I updated FileUtility to use System.IO.Abstraction and added kudu tests for vfs here

fabiocav

The changes look good. But looks like we're running into a bunch of test failures we don't see in dev. Missing a rebase? Or are there issues?

ahmelsayed · 2018-03-14T00:29:58Z

I just notices that all tests are now passing on dev. I just did a rebase and will take a look at any failed tests

ahmelsayed · 2018-03-14T01:23:37Z

okay, all tests should be passing now, but few things:

This commit 0518ba8df0d0b5d2ac338f78f273a886bbcc7a31 clears up the Mock<IFileSystem> that the vfs test cases use. This will need to be reconsidered if you ever plan on parallel test cases.
This commit 627e71660cda6649b41d2eca0bb8452a43259e4d however changes an existing test case. it seems that the test case is trying to handle the secret file being locked and verifying that the correct exception is raised. However, it looks like the original FileUtility.ReadAsync was just using a StreamReader which locks the file by default.

azure-functions-host/src/WebJobs.Script/Extensions/FileUtility.cs

Lines 71 to 75 in 8cf2deb

    
           encoding = encoding ?? Encoding.UTF8; 
        
           using (var reader = new StreamReader(path, encoding, true, 4096)) 
        
           { 
        
               return await reader.ReadToEndAsync(); 
        
           }

I updated FileUtility.ReadAsync to only grab a read lock to the file.

https://github.com/Azure/azure-functions-host/blob/627e71660cda6649b41d2eca0bb8452a43259e4d/src/WebJobs.Script/Extensions/FileUtility.cs#L82-L87

I can either revert that without changing the test, or changing the test to allow FileShare.None when locking the file.

fabiocav

Will be nice to have this finally merged in! Thanks @ahmelsayed !

ahmelsayed · 2018-03-14T21:05:08Z

yay! Thanks @fabiocav!!

/cc @christopheranderson, @bala16

brettsam · 2018-03-15T21:15:48Z

It looks like this change adds two matching routes for GET /admin/functions/{name}/status.
https://github.com/Azure/azure-functions-host/blob/dev/src/WebJobs.Script.WebHost/Controllers/HostController.cs#L79
https://github.com/Azure/azure-functions-host/blob/dev/src/WebJobs.Script.WebHost/Controllers/FunctionsController.cs#L116

The e2e tests I'm working on caught it as I rebased. I'm removing the one in HostController locally to unblock myself.

brettsam · 2018-03-15T21:18:02Z

Same with POST admin/functions/{name} (Invoke).

ahmelsayed · 2018-03-15T21:19:32Z

sorry about that. It looks like one of the rebases I did brought it back and I didn't notice

fabiocav · 2018-03-15T22:47:27Z

Missed on the last review as well... That's the risk when those changes become large. We'll just adjust, get it fixed and be happy :)

ahmelsayed requested a review from fabiocav October 6, 2017 21:08

dnfclas added the cla-already-signed label Oct 6, 2017

fabiocav force-pushed the core branch from 4080da1 to 568cbb5 Compare October 9, 2017 23:23

ahmelsayed force-pushed the ahmels-management branch 2 times, most recently from b59a37b to 9394601 Compare October 11, 2017 19:06

fabiocav force-pushed the core branch 2 times, most recently from d177c52 to 3f54f80 Compare October 12, 2017 21:38

ahmelsayed force-pushed the ahmels-management branch 2 times, most recently from 7078d3a to c6a3528 Compare October 16, 2017 20:42

fabiocav changed the base branch from core to dev October 19, 2017 01:29

ahmelsayed force-pushed the ahmels-management branch 2 times, most recently from 3a70672 to daa4414 Compare October 26, 2017 00:03

This was referenced Nov 8, 2017

Functions API needs to be fully accessible over ARM #1824

Closed

V1 runtime should not fail all any /admin requests if the host is not running #2114

Closed

ahmelsayed force-pushed the ahmels-management branch from 40a4f59 to 385131e Compare November 18, 2017 00:59

ahmelsayed force-pushed the ahmels-management branch 2 times, most recently from 0b5e744 to 48b164b Compare November 18, 2017 02:08

fabiocav reviewed Nov 22, 2017

View reviewed changes

msftgits removed the cla-already-signed label Dec 11, 2017

ahmelsayed commented Feb 1, 2018

View reviewed changes

ahmelsayed force-pushed the ahmels-management branch from 48b164b to 008fe36 Compare February 1, 2018 21:26

ahmelsayed force-pushed the ahmels-management branch from 008fe36 to d6c101a Compare March 6, 2018 22:54

ahmelsayed force-pushed the ahmels-management branch from 6dca084 to d1ace53 Compare March 12, 2018 22:46

fabiocav reviewed Mar 14, 2018

View reviewed changes

ahmelsayed force-pushed the ahmels-management branch from d1ace53 to 2c82667 Compare March 14, 2018 00:29

fabiocav approved these changes Mar 14, 2018

View reviewed changes

Add function management APIs to the runtime

be325d7

ahmelsayed force-pushed the ahmels-management branch from 627e716 to be325d7 Compare March 14, 2018 20:40

ahmelsayed merged commit 9345363 into dev Mar 14, 2018

ahmelsayed deleted the ahmels-management branch April 11, 2018 16:24


		public string SecretsPath { get; set; }

		public string TestDataPath { get; set; }

		@@ -1,9 +1,6 @@
		// Copyright (c) .NET Foundation. All rights reserved.
		// Licensed under the MIT License. See License.txt in the project root for license information.

Add function management APIs to the runtime #2005

Add function management APIs to the runtime #2005

Uh oh!

Conversation

ahmelsayed commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Function management APIs

File management APIs (VFS)

Uh oh!

fabiocav commented Nov 18, 2017

Uh oh!

ahmelsayed commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmelsayed commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiocav left a comment

Choose a reason for hiding this comment

Uh oh!

fabiocav commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmelsayed commented Feb 1, 2018

Uh oh!

ahmelsayed commented Mar 8, 2018

Uh oh!

fabiocav left a comment

Choose a reason for hiding this comment

Uh oh!

ahmelsayed commented Mar 14, 2018

Uh oh!

ahmelsayed commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ahmelsayed commented Oct 6, 2017 •

edited

Loading

ahmelsayed commented Nov 18, 2017 •

edited

Loading

ahmelsayed commented Mar 14, 2018 •

edited

Loading

ahmelsayed commented Mar 14, 2018 •

edited

Loading