Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design proposal for signature provider plugins #640

Closed
wants to merge 1 commit into from

Conversation

dtivel
Copy link
Collaborator

@dtivel dtivel commented Jul 26, 2023

Progress on #639.

CC @clairernovotny

@dtivel dtivel requested a review from a team as a code owner July 26, 2023 18:39
"name": "azure-key-vault",
"description": "Use Azure Key Vault.",
"entryPoint": {
"filePath": "lib/net6.0/Microsoft.Azure.KeyVault.Sign.dll",
Copy link

@agr agr Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be a reason for a "plugin package" to target multiple frameworks? If yes, how would that plugin.json would look like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about multiple frameworks as the tool is >.NET 6 only. I believe the AssemblyLoadContext will automatically pickup runtimes if they're in subfolders to handle native dependencies. @dtivel?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a direct file link in the configuration, there doesn't seem to be any room for automatic choices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario here is to enable Sign CLI and plugins to keep pace with .NET releases.

This can be achieved partially using .NET's roll forward feature. Rolling forward offers convenience and simplicity at the increased risk of runtime compatibility issues. Sign CLI currently targets net6.0 and will likely run just fine on net7.0; however, the chance of runtime compatibility issues, including breaking changes, generally increases from runtime major version x to x+1 or x to x+2. The gold standard is to retarget, recompile, and retest on the newer .NET runtime, but that isn't always feasible. This is a good read.

Currently, Sign CLI targets net6.0. With .NET's default policy of Minor, Sign CLI would run only if a .NET 6 runtime is installed. Sign CLI overrides the default policy with Major, enabling Sign CLI to run on a newer major runtime if the requested runtime isn't installed. The Major policy has an increased risk (over Minor) of runtime compatibility issues, but the trade-offs will probably usually be worth it. In the event of runtime compatibility issues resulting from running on a newer major version than what Sign CLI targets, the solution is simple: install a .NET 6 runtime. In time, .NET 6 will sunset and by then Sign CLI should target a newer runtime.

It's worth noting that the .NET team recognizes the impact of the default roll forward policy (Minor) on .NET tools. You install a newer major version of the .NET SDK and suddenly all .NET tools are unavailable. They are thinking about how to solve it.

Separate from the roll forward policy, in this proposal Sign CLI becomes a (plugin) host and must make decisions similar to the .NET host (dotnet.exe) as to which plugin assembly to load. We use .NET's AssemblyLoadContext to explicitly load one of the plugin's assemblies. We provide a file path and tell it, "Load that one." As @agr said, there's no room for automatic choices. The roll forward policy doesn't apply all the way down. After we tell the runtime which plugin assembly to load, the runtime will use its deps.json file to load its dependencies.

I think Sign CLI should load the plugin that matches the host (Sign CLI) not the runtime and deviate only if it's a sound fallback. For example, if a plugin contains net6.0, net7.0, and net8.0 assemblies and Sign CLI's net7.0 assemblies are loaded by the .NET 8 runtime, Sign CLI should load the plugin's net7.0 assemblies. If the plugin only had net6.0 and net8.0 assemblies, Sign CLI should load the net6.0 assemblies or report that plugin isn't available for the current runtime. If we keep Sign CLI's current roll forward policy, it seems we should try to load the net6.0 assemblies.

If we make the following assumptions:

  1. The plugin's entry point assembly is under the lib/<tfm>/ directory.
  2. If the plugin ships support for multiple target frameworks, the plugin's entry point assembly name is the same across all of them.

...then filePath can be the relative path to the plugin's entry point assembly relative from the TFM directory (lib/<tfm>/). If the plugin's assembly is directly located in the lib/<tfm>/ directory, then filePath is simply the assembly's file name. Sign CLI can look under lib/ for a TFM matching its own and load lib/<tfm>/<filePath>. Optionally, Sign CLI can fall back to using the greatest TFM not greater than its own.

Thoughts?

`name` | yes | string | N/A | N/A
`description` | yes | string | N/A | N/A
`aliases` | yes | array of strings | N/A | N/A
`dataType` | no | string | `Text` | `Text`, `Boolean`, `Uri`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine, plugin would have to validate parameters anyway, so why not just read and pass them to a plugin as strings, then let it sort out the conversion/interpretation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts, @clairernovotny?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this other than having common parameters validated in the base could provide a more consistent experience.

</Target>
```

#### <a name="plugin-json-file"></a>The `plugin.json` file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the design can be simplified by moving the plugin.json content elsewhere. For example: name can be inferred from package name or from class name, or it can be part of the ISignatureProviderPlugin. Path can be inferred from looking at all the classes that implement ISignatureProviderPlugin and auto-adding them.
The reason I'm suggesting this, is because editing an additional file can be error prone for developers and increase plugin development time.

@dtivel
Copy link
Collaborator Author

dtivel commented Nov 9, 2023

Closing this in favor of dotnet/designs#304.

CC @clairernovotny

@dtivel dtivel closed this Nov 9, 2023
@dtivel dtivel deleted the dtivel/plugin-proposal branch November 9, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants