bundle and handle Azure PowerShell #47
Conversation
requirements.psd1
Outdated
| @@ -0,0 +1,21 @@ | |||
| @{ | |||
| # Packaged with the PowerShell Language Worker | |||
| 'PowerShellGet' = @{ | |||
There was a problem hiding this comment.
Can we put this file in the tools folder? I guess we can call Invoke-PSDepend "$PSScriptRoot/tools/requirements.psd1
There was a problem hiding this comment.
We synced offline and think it should go in src
TylerLeonhardt
changed the title
|
@daxian-dbw I've addressed your comment. This PR was updated to fix Azure PowerShell being brought in and enables a step that will auto connect to Azure if the correct environment variables are present. |
src/PowerShell/PowerShellManager.cs
Outdated
| string.IsNullOrEmpty(applicationSecret) || | ||
| string.IsNullOrEmpty(tenantId)) | ||
| { | ||
| _logger.Log(LogLevel.Warning, "Required environment variables to authenticate to Azure were not present", isUserLog: true); |
There was a problem hiding this comment.
We won't be able to see the log if it's user log, but I think this message should be visible to us too.
We probably shouldn't just return in this case, but should indicate a failure status so we can send a FunctionLoadResponse message with the status and the error message.
There was a problem hiding this comment.
I think this actually should be a Warning because we shouldn't require connecting to Azure to run the function locally - especially if their function doesn't interact Azure at all.
Additionally, this shouldn't be a user log, it should be a system log because in the local scenario the user sees the system logs... so I'll make that change.
There was a problem hiding this comment.
The local scenario makes sense. But I don't see the logging gets changed to the system log.
src/Messaging/RpcLogger.cs
Outdated
| @@ -47,7 +47,7 @@ public async void Log(LogLevel logLevel, string message, Exception exception = n | |||
| RpcLog = new RpcLog() | |||
| { | |||
| Exception = exception?.ToRpcException(), | |||
| InvocationId = _invocationId, | |||
| InvocationId = _invocationId ?? _requestId, | |||
There was a problem hiding this comment.
Do we have to set the InvocationId? What will happen if leaving it null or empty string?
There was a problem hiding this comment.
It has to be set. I've tried setting it to empty string or null but it still fails by saying can't be null or empty.
There was a problem hiding this comment.
As far as I understand, we will hit this code only for user logging, which only happens when the function runs. _invocationId will not be null by that time. But when doing system logging, _invocationId could be null, so we should handle it there. And I think it should be _invocationId ?? "N/A".
src/RequestProcessor.cs
Outdated
| default: | ||
| throw new InvalidOperationException($"Not supportted message type: {request.ContentCase}"); | ||
| } | ||
| await _msgStream.WriteAsync(response); |
There was a problem hiding this comment.
This async operation can be moved out of the using block.
src/PowerShell/PowerShellManager.cs
Outdated
| @@ -103,6 +106,9 @@ internal void InitializeRunspace() | |||
| { | |||
| Dictionary<string, ParameterMetadata> parameterMetadata; | |||
|
|
|||
| // We attempt to authenticate to Azure with every invocation | |||
| AuthenticateToAzure(); | |||
There was a problem hiding this comment.
Can you please explain why we are doing this for every function invocation? Are you expecting the environment variables may change for each function invocation?
There was a problem hiding this comment.
we talked offline. I will move it to WorkerInit for the demo but also opened #49 for understanding.
This PR bundles modules into the
src/Modulesdirectory so that they can be distributed with the worker.The modules are as followed:
AzureRm- to interact with Azure resources inside of an Azure FunctionMicrosoft.PowerShell.Archive- so that users can expand and archive zipsPowerShellGet- mainly forPublish-Module. User's will want to use AzF as a CICD pipeline for their modulesThis psd1 also handles dev dependencies - so Pester has been moved to that.
Also, the format of the
.psd1is in the format that PSDepend uses. I wanted to leverage this package because it does exactly what we want but I was afraid that folks wouldn't like the extra dependency.This also is an evaluation of the Module as a possible solution to specifying dependencies IN a user's function.
In addition to bundling, this PR includes changes to get Azure PowerShell working. Some work was needed to get Azure PowerShell to work at all (see #48)... but this PR includes lets you use it without needing to do any interactive work using Serivce Principals.
See this doc on how to set one up:
https://docs.microsoft.com/en-us/powershell/azure/create-azure-service-principal-azureps?view=azurermps-6.8.1
If the user specifies the environment variables:
ApplicationId,ApplicationSecret&TenantIdThen the PowerShell Worker will attempt to connect to Azure using that information without you needing to explicitly say
Connect-AzureRmAccount. This is similar to what Azure Automation does today.I've also included a working example of an HttpTrigger that can grab some VM's from Azure and send a response back.