Enable external DF SDK

[davidmrdavid]

This PR is the culmination of several smaller PRs that originated here: #746

This PR changes the Durable Functions logic in the worker to account for the possibility of an external Durable Functions SDK implementation. That SDK would be installed as a managed dependency, from the PowerShell Gallery.

If this external SDK were detected, we defer the invocation of orchestrator Function types to that external SDK. All other invocation types are still handled by the worker. In this refactoring, I've also simplified some of the orchestration-detection logic as shown in this PR's InvokeFunction method.

This PR is in collaboration with @michaelpeng36

Pull request checklist

• My changes do not require documentation changes
  • Otherwise: Documentation issue linked to PR
• My changes should not be added to the release notes for the next release
  • Otherwise: I've added my notes to release_notes.md
• My changes do not need to be backported to a previous version
  • Otherwise: Backport tracked by issue/PR #issue_or_pr
• I have added all required tests (Unit tests, E2E tests)

Additional information

Before merging this PR, we will need to decide on the namespace of the new external Durable Functions SDK. In fact, it may be best to fully reserve it prior to merging.

[davidmrdavid]

Unfortunately, the diff in this section of the code makes the edit seem more complicated than it is.
In short, the previous logic of the Invoke method in here is now contained within the InvokeInternalDurableSDK . However, for the rest of this file, the diff will show parts of the old method interleaved in weird ways with new methods.

[davidmrdavid]

Just for clarity: the pwsh.ClearStreamsAndCommand() call here didn't use to be in the CreateReturnValueFromFunctionOutput method. The implementation of this method has not changed at all, it's just that the diff is acting up and still showing code from the previous Invoke implementation.

[davidmrdavid]

@Francisco-Gamino: as per our last conversation, I've refactored the code so that it no longer imports the external Durable Functions SDK. Instead, this is now the responsibility of the user app to do that: they will have to run Import-Module on their profile.ps1 to import the Durable Functions SDK.

The main changes as of your latest review are in DurableController.cs and PowerShellServices.cs.

In DurableController.cs, all relevant logic is in tryEnablingExternalSDK(). This method will first check if the External DF SDK was imported onto the PowerShell session. Based on that information, and whether the customer enabled the external SDK using the environment variableExternalDurablePowerShellSDK, it may switch to use the external SDK and/or log warnings to the user about potential configuration errors.

This is all defined here:

azure-functions-powershell-worker/src/DurableWorker/DurableController.cs

Lines 65 to 92 in a1e5993

	private void tryEnablingExternalSDK()
	{
	var isExternalSdkLoaded = _powerShellServices.isExternalDurableSdkLoaded();
	if (isExternalDFSdkEnabled)
	{
	if (isExternalSdkLoaded)
	{
	// Enable external SDK only when customer has opted-in
	_powerShellServices.EnableExternalDurableSDK();
	}
	else
	{
	// Customer attempted to enable external SDK but the module not in session. Default to built-in SDK.
	_logger.Log(isUserOnlyLog: false, LogLevel.Error, string.Format(PowerShellWorkerStrings.ExternalSDKWasNotLoaded, Utils.ExternalDurableSdkName));
	}
	}
	else if (isExternalSdkLoaded)
	{
	// External SDK is in session, but customer does not mean to enable it. Report potential clashes
	_logger.Log(isUserOnlyLog: false, LogLevel.Error, String.Format(PowerShellWorkerStrings.PotentialDurableSDKClash, Utils.ExternalDurableSdkName));
	}
	}
	public void InitializeBindings(IList<ParameterBinding> inputData, out bool hasExternalSDK)
	{
	this.tryEnablingExternalSDK();
	// If the function is an durable client, then we set the DurableClient

DurableController leverages various changes in PowerShellServices.cs. The relevant methods are all here in the snippet below:

azure-functions-powershell-worker/src/DurableSDK/PowerShellServices.cs

Lines 44 to 86 in a1e5993

	public bool isExternalDurableSdkLoaded()
	{
	// Search for the external DF SDK in the current session
	var matchingModules = _pwsh.AddCommand(Utils.GetModuleCmdletInfo)
	.AddParameter("FullyQualifiedName", Utils.ExternalDurableSdkName)
	.InvokeAndClearCommands<PSModuleInfo>();
	// If we get at least one result, we know the external SDK was imported
	var numCandidates = matchingModules.Count();
	var isModuleInCurrentSession = numCandidates > 0;
	if (isModuleInCurrentSession)
	{
	var candidatesInfo = matchingModules.Select(module => string.Format(
	PowerShellWorkerStrings.FoundExternalDurableSdkInSession, module.Name, module.Version, module.Path));
	var externalSDKModuleInfo = string.Join('\n', candidatesInfo);
	if (numCandidates > 1)
	{
	// If there's more than 1 result, there may be runtime conflicts
	// warn user of potential conflicts
	_logger.Log(isUserOnlyLog: false, LogLevel.Warning, String.Format(
	PowerShellWorkerStrings.MultipleExternalSDKsInSession,
	numCandidates, Utils.ExternalDurableSdkName, externalSDKModuleInfo));
	}
	else
	{
	// a single external SDK is in session. Report its metadata
	_logger.Log(isUserOnlyLog: false, LogLevel.Trace, externalSDKModuleInfo);
	}
	}
	return isModuleInCurrentSession;
	}
	public void EnableExternalDurableSDK()
	{
	_usesExternalDurableSDK = true;
	// assign SetFunctionInvocationContextCommand to the corresponding external SDK's CmdLet
	SetFunctionInvocationContextCommand = string.Format(
	_setFunctionInvocationContextCommandTemplate,
	Utils.ExternalDurableSdkName);
	}

[Francisco-Gamino]

Could you please add a test case that validates this logic?

[davidmrdavid]

Discussed offline, sharing update:

The methods in PowerShellServices aren't easy to test and given that this logic is behind a feature flag, we feel it should be safe to release as is. I have created this ticket to track the work of refactoring PowerShellServices so that unit testing these methods is easier. Prior to GA'ing this work, we will need to have unit tests for these methods.

FYI @michaelpeng36

[Francisco-Gamino]

I left a couple more comments. Thank you.

[davidmrdavid]

Thanks @Francisco-Gamino for your prompt reviews. I believe I have addressed all your comments except the unit test, to which I responded here: #839 (comment)

No need to merge this at this point as it won't make a difference timeline wise, although it would be good to release this with the next worker release in early January. I also won't block if people want to merge this though :) . @Francisco-Gamino please let us know if this has your approval but I'm also happy to take on more feedback as this is an important feature. Thanks all.

[davidmrdavid]

Hi @Francisco-Gamino, I think in our last conversation we deemed this PR ready to merge. Please let me know if that's still the case (or let me know if you have new suggestions :) ) so that we can keep the ball rolling here and merge promptly. Thanks!

[Francisco-Gamino]

Should we include the profile.ps1 file name in this line Please ensure your script only imports a single version of '{0}' ?

[davidmrdavid]

Sure, I added an explicit reference to profile.ps1 in my latest commit: 8e0e103

Does that look good?

[Francisco-Gamino]

I left one last minor comment. Otherwise, LGTM.

[Francisco-Gamino]

@davidmrdavid -- One more thing: As per our conversation, the DF SDK will need to be imported in the profile.ps1. Please open an issue to track the template changes for the Functions Host, the CoreTools, and the VSCode Extension. Thank you.

[davidmrdavid]

Thank you @Francisco-Gamino.
I have created the template-tracking ticket here: Azure/azure-functions-durable-powershell#27 #906
I wasn't sure what you meant by "template changes for the Functions Host" (aren't all templates technically for the Functions Host?), but I did write down that we need to modify the templates for at least core-tools and the VSCode plugin.

Please let me know if my latest edit to the log message regarding having to import the module in profile.ps1 looks good to you. Thanks!

[davidmrdavid]

FYI @Francisco-Gamino:
I'm tracking editing the profile.ps1 templates here, there you'll find sub-tickets for each repo where the profile.ps1 templates are held.
And I simplified the warning log as we discussed: e85c19e

As the CI has passed and as per our conversation, I'll go ahead and merge this! Thank you for all your help.