[LocalDeploy] Migrate to extensibility v2 contract #14438

jorgecotillo · 2024-06-28T23:46:07Z

Contributing a Pull Request

If you haven't already, read the full contribution guide. The guide may have changed since the last time you read it, so please double-check. Once you are done and ready to submit your PR, run through the relevant checklist below.

Contributing to documentation

All documentation contributions should be made directly in the Bicep documentation on Microsoft Docs.

Contributing an example

We are integrating the Bicep examples into the Azure QuickStart Templates. If you'd like to contribute new example .bicep files that showcase abilities of the language, please follow these instructions to add them directly there. We can still take bug reports and fixes for the existing examples for the time being.

This is a bug fix for an existing example
I have resolved all warnings and errors shown by the Bicep VS Code extension
I have checked that all tests are passing by running dotnet test
I have consistent casing for all of my identifiers and am using camelCasing unless I have a justification to use another casing style

Contributing a feature

I have opened a new issue for the proposal, or commented on an existing one, and ensured that the Bicep maintainers are good with the design of the feature being implemented
I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
I have appropriate test coverage of my new feature

Contributing a snippet

I have a snippet that is either a single, generic resource or multi resource that uses parent-child syntax
I have checked that there is not an equivalent snippet already submitted
I have used camelCasing unless I have a justification to use another casing style
I have placeholders values that correspond to their property names (e.g. dnsPrefix: 'dnsPrefix'), unless it's a property that MUST be changed or parameterized in order to deploy. In that case, I use 'REQUIRED' e.g. keyData
I have my symbolic name as the first tab stop ($1) in the snippet. e.g. res-aks-cluster.bicep
I have a resource name property equal to "name"
If applicable, I have set the location property to location: /*${<id>:location}*/'location' (not resourceGroup().location) where <id> is a placeholder id, and added param location string to the test's main.bicep file so that the resulting main.combined.bicep file used in the tests compiles without errors
I have verified that the snippet deploys correctly when used in the context of an actual bicep file

e.g.
```
resource aksCluster 'Microsoft.ContainerService/managedClusters@2021-03-01' = {
  name: 'name'
```

Microsoft Reviewers: Open in CodeFlow

github-actions · 2024-06-28T23:48:59Z

Test this change out locally with the following install scripts (Action run 9843996390)

VSCode

Mac/Linux

bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 9843996390

Windows

iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 9843996390"

Azure CLI

Mac/Linux

bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 9843996390

Windows

iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 9843996390"

github-actions · 2024-06-28T23:53:16Z

Dotnet Test Results

72 files - 36 72 suites - 36 23m 57s ⏱️ - 9m 5s
10 938 tests - 14 10 938 ✅ - 14 0 💤 ±0 0 ❌ ±0
25 772 runs - 12 864 25 772 ✅ - 12 864 0 💤 ±0 0 ❌ ±0

Results for commit 232ef21. ± Comparison against base commit 9864b14.

♻️ This comment has been updated with latest results.

src/Bicep.Local.Extension/extension.proto

anthony-c-martin · 2024-06-29T17:24:56Z

src/Bicep.Local.Extension/Protocol/IResourceHandler.cs

    string Message,
-    string Target);
+    ErrorDetail[]? Details,
+    JsonObject? InnerError);


Is there an expected contract for the InnerError field?

Yes, that's defined by the OData 4.0 error response format: https://docs.oasis-open.org/odata/odata-json-format/v4.0/os/odata-json-format-v4.0-os.html#_Toc372793091, which is used by the Graph API and some Azure data plane APIs. The deployment engine converts it to error.details[*].additionalInfo to ensure compatibility with ARM RPC.

Thanks - so sounds like using JsonObject rather than defining a custom record for it is the way to go?

src/Bicep.Core/Emit/TemplateWriter.cs

src/Bicep.Local.Deploy/LocalDeploymentEngineHost.cs

shenglol · 2024-07-01T17:50:45Z

src/Bicep.Core/Emit/TemplateWriter.cs

+                        {
+                            case StringType:
+                                emitter.EmitProperty("type", "string");
+                                break;


Do we need to support the other built-in types as well?

Good point, does config items will support all types - like in Template parameters?

Yeah, the runtime (deployment engine) supports all types except for user-defined types.

src/Bicep.Local.Deploy/Extensibility/LocalExtensibilityHandler.cs

jorgecotillo · 2024-07-01T22:13:54Z

src/Bicep.Local.Deploy/Extensibility/AzExtensibilityExtension.cs

+
+namespace Bicep.Local.Deploy.Extensibility;
+
+public class AzExtensibilityExtension : LocalExtensibilityExtension


Extension suffix might be confusing as well, people might confuse with C# extension classes.

I think it's fine, the convention for C# extension classes is to pluralize.

Although looking at the existing name which I picked, I'm wondering if we could call it something more indicative of what it's actually doing like NestedDeploymentExtension?

See my other comment on LocalExtensibilityExtension.cs - I'm learning more towards NestedDeploymentExtensibilityHost as a name for this class.

Does it make more sense to make this a built-in local extension rather than an extension host?

NestedDeploymentBuiltInExtension ?

jorgecotillo · 2024-07-02T13:14:08Z

...cep.VSLanguageServerClient.IntegrationTests/TestSolution/BicepTestProject/packages.lock.json

I noticed a bunch of deletions - packages.lock.json - I cleaned the solution and rebuilt but files are gone, doesn't seem expected.

I would recommend just reverting this part of the change - the bicep-vs solution should be unaffected. It's a separate .sln file, so rebuilding the main solution file won't cause these files to be updated.

anthony-c-martin · 2024-07-02T13:22:53Z

src/Bicep.Core/Emit/EmitterSettings.cs

+        /// <summary>
+        /// Enables bicep local deploy
+        /// </summary>
+        public bool LocalDeployEnabled { get; }


Since this is just a convenience property, and you don't want the value to differ from what is on the model, could we replace it with the following?

public bool LocalDeployEnabled => model.Features.LocalDeployEnabled;

I moved to use Context.SemanticModel.Features.LocalDeployEnabled instead.

anthony-c-martin · 2024-07-02T13:25:45Z

src/Bicep.Core/Emit/TemplateWriter.cs

        {
            if (!providers.Any())
            {
                return;
            }

+            // TODO: Remove if statement once all providers got migrated to extensions (extensibility v2 contract).
            if (this.Context.SemanticModel.Features.LocalDeployEnabled)


For consistency, could we either access this property with Context.Settings.LocalDeployEnabled everywhere (the convenience property you added), or with Context.SemanticModel.Features.LocalDeployEnabled everywhere?

Got it, I didn't know you get access to Settings or SemanticModel from Context. I'll remove the convenience property and use Context.SemanticModel.Features.LocalDeployEnabled everywhere instead.

anthony-c-martin · 2024-07-02T13:28:09Z

src/Bicep.Core/Emit/TemplateWriter.cs

+                            case StringType:
+                                emitter.EmitProperty("type", "string");
+                                break;


Is it worth handling secureString here?

I am avoiding it because secureString requires bicep to read the value from KeyVault and not sure if all the wiring is in place right now.

after chatting with Shenglong, will be including the same types that we support today in Deployments

anthony-c-martin · 2024-07-02T13:32:22Z

src/Bicep.Local.Deploy/Extensibility/LocalExtensibilityExtension.cs

+
+namespace Bicep.Local.Deploy.Extensibility;
+
+public abstract class LocalExtensibilityExtension : IAsyncDisposable


...ExtensibilityExtension is a bit strange - what about LocalDeployExtension?

On second thoughts, I actually think it's fine to keep Provider here (GrpcExtensibilityProvider, NestedDeploymentExtensibilityProvider). This is a different concept to the extension which a user would think about - the GrpcExtensibilityProvider is a host for multiple user-defined extensions, rather than being an extension in itself. Maybe GrpcExtensibilityHost / LocalExtensibilityHost would be a more appropriate name? @shenglol thoughts?

Agree that the class functions more as an extension host / dispatcher. I think either 'LocalExtensibilityHost' or 'LocalExtensionHost' would be suitable names.

anthony-c-martin · 2024-07-02T14:14:34Z

src/Bicep.Local.Deploy/LocalDeploymentEngineHost.cs

+    /*var extensionName = requestUri.Segments[^4].TrimEnd('/');
+    var extensionVersion = requestUri.Segments[^3].TrimEnd('/');
+    var method = requestUri.Segments[^1].TrimEnd('/');*/


[nit] Remove this

(facepalm) moved to copy the arg names and omit to remove these.

anthony-c-martin · 2024-07-02T14:16:51Z

src/Bicep.Local.Deploy/LocalDeploymentEngineHost.cs

+        response.Headers.Add("Location", "local");
+        response.Headers.Add("Version", extensionVersion);


What are these headers used for? Would you mind adding a code comment to explain?

@shenglol are we going to relax echo these header values in deployment engine? If so, these headers are just temporal additions and can add a comment but if is permanent, are these values ok?

To give you some context Anthony, deployment engine performs validations on header values and expects these two to be present, but it might not apply to local extensibility.

Yeah, I'll send a PR to relax the validation for the headers. We can remove them from LocalDeploymentEngineHost later.

anthony-c-martin · 2024-07-02T14:20:10Z

src/Bicep.Local.Deploy/Extensibility/LocalExtensibilityHandler.cs

+        return method switch
+        {
+            "get" => await provider.Get(await content.ReadAsAsync<ResourceReferenceRequestBody>(cancellationToken), cancellationToken),
+            "delete" => await provider.Delete(await content.ReadAsAsync<ResourceReferenceRequestBody >(cancellationToken), cancellationToken),
+            "createOrUpdate" => await provider.CreateOrUpdate(await content.ReadAsAsync<ResourceRequestBody>(cancellationToken), cancellationToken),
+            "preview" => await provider.Preview(await content.ReadAsAsync<ResourceRequestBody>(cancellationToken), cancellationToken),
+            _ => throw new NotImplementedException($"Unsupported method {method}"),
+        };


It feels odd that this method deserializes the request (content.ReadAsAsync<ResourceReferenceRequestBody>) but then returns an unserialized response (ResourceResponseBody). Should we instead just have it return a HttpContent or equivalent?

I also noticed that for response serialization you are using ResourceStack (.ToJson()), but for request deserialization you are using the ASP.NET defaults (content.ReadAsAsync()). This may yield different results.

My preference here would be to remove the dependency on HttpContent and its built-in serializer at this layer, and instead take a Stream for the body, and return a Stream.

This removes the coupling with the HTTP layer, and puts us in full control the serialization + deserialization in one place (here).

@shenglol should we be using NewtonSoft or STJ for the classes under Azure.Deployments.Engine.Host.Azure.ExtensibilityV2.Contract.Models?

I would prefer using STJ for better performance. @jorgecotillo, we discussed using v2 models from Azure.Deployments.Extensibility instead. Could we switch to using those models?

anthony-c-martin · 2024-07-02T14:34:54Z

src/Bicep.Local.Deploy/Extensibility/LocalExtensibilityHandler.cs

 using Azure.Deployments.Extensibility.Contract;
 using Azure.Deployments.Extensibility.Messages;
 using Bicep.Core.Extensions;
 using Bicep.Core.Registry;
 using Bicep.Core.Semantics;
 using Bicep.Core.Semantics.Namespaces;
 using Bicep.Core.TypeSystem.Types;
+using Microsoft.WindowsAzure.ResourceStack.Common.Json;
 using Microsoft.WindowsAzure.ResourceStack.Common.Utilities;
 using IAsyncDisposable = System.IAsyncDisposable;

 namespace Bicep.Local.Deploy.Extensibility;

 public class LocalExtensibilityHandler : IAsyncDisposable


What about ExtensionHostManager?

I'm just thinking out loud here - what if we tweak the architecture? To me it seems a bit odd to have a manager or registry of extension hosts. Does it make sense to do the opposite? Instead of having a manager invoking an extension host, the LocalExtensionHost could reference a LocalExtensionRegistry. The LocalExtensionHost would use the LocalExtensionRegistry to resolve an extension based on a given extension name and version. Then, the LocalExtensionHost would dispatch the resolved extension to handle resource operations.

Created a diagram to illustrate the idea:

This shouldn't block the PR, though. Since it's a design change, we may need to discuss it first and implement it later in subsequent PRs.

Agreed, feels like there's definitely scope to simplify the design, and I like the above proposal. Working off it, some additional things to think about:

Lifecycle management of IExtension instances - I assume we'd want this to be the responsibility of the ExtensionRegistry?

Activating extensions that aren't currently running (currently I believe this happens implicitly when an IExtension method call happens, but I think it should be more explicitly managed)

Deactivating unused extensions

Keepalives (not currently implemented, but may need to be to avoid leaving orphaned processes running)

Resolving extensions by name & version leaves room for mixups to occur. I think we should key off the file system (cache) path to avoid this. This would need to be represented in the intermediate template somehow for the deployment layer to understand it.

anthony-c-martin · 2024-07-02T14:37:23Z

src/Bicep.Local.Deploy.IntegrationTests/ProviderExtensionTests.cs

+        handlerMock.SetupGet(x => x.ResourceType).Returns("apps/Deployment");

-        handlerMock.Setup(x => x.Save(It.IsAny<Protocol.ExtensibilityOperationRequest>(), It.IsAny<CancellationToken>()))
-            .Returns<Protocol.ExtensibilityOperationRequest, CancellationToken>((req, _) =>
-                Task.FromResult(new Protocol.ExtensibilityOperationResponse(req.Resource, null, null)));
+        handlerMock.Setup(x => x.CreateOrUpdate(It.IsAny<Protocol.ResourceRequestBody>(), It.IsAny<CancellationToken>()))
+            .Returns<Protocol.ResourceRequestBody, CancellationToken>((req, _) =>
+                Task.FromResult(new Protocol.ResourceResponseBody(null, identifiers, req.Type, "Succeeded", req.Properties)));

        await RunExtensionTest(
            builder => builder.AddHandler(handlerMock.Object),
            async (client, token) =>
            {
-                var request = new Extension.Rpc.ExtensibilityOperationRequest
+                var request = new Extension.Rpc.ResourceRequestBody
                {
-                    Import = new()
-                    {
-                        Provider = "Kubernetes",
-                        Version = "1.0.0",
-                        Config = """
-                        {
-                          "kubeConfig": "redacted",
-                          "namespace": "default"
-                        }
-                        """
-                    },


Rather than deleting this, could we move it to the Config property to be more representative of a real-life scenario?

anthony-c-martin · 2024-07-02T14:45:31Z

src/Bicep.Local.Extension/Rpc/BicepExtensionImpl.cs

+    private Protocol.ResourceRequestBody Convert(ResourceRequestBody request)
+        => new(!string.IsNullOrEmpty(request.Config) ? JsonNode.Parse(request.Config) as JsonObject : null,
+                request.Type,
+                JsonNode.Parse(request.Properties)!.AsObject(), // TODO: Is there a better way to parse to ensure result might not be null?


It's a risk when parsing untrusted data, so it makes sense that we're forced to handle null. My recommendation would be to just throw an exception if it happens.

I also think there's a lot going on in this function, so might be worth simplifying by using a traditional function rather than the expression body syntax:

private Protocol.ResourceRequestBody Convert(ResourceRequestBody request) { JsonObject? config = null; if (request.Config is {}) { config = JsonNode.Parse(request.Config)?.AsObject() ?? throw new ...; } var properties = JsonNode.Parse(request.Properties)?.AsObject() ?? throw new ...; return new(config, request.Type, properties, request.ApiVersion); }

This was intended as a response to your comment // TODO: Is there a better way to parse to ensure result might not be null? FYI

yup, I updated the code and will push in the upcoming iteration

anthony-c-martin · 2024-07-02T14:47:01Z

src/Bicep.Local.Extension/Rpc/BicepExtensionImpl.cs

@@ -5,6 +5,7 @@
 using System.Text.Json.Nodes;
 using Bicep.Local.Extension.Protocol;
 using Grpc.Core;
+using Grpc.Net.Client;


Do you know why this was necessary?

No clue, might be VS adding references when performing autocompletion, I just removed the unnecessary using statements btw.

src/Bicep.Local.Deploy/Extensibility/AzExtensibilityExtension.cs

This reverts commit 765d520.

anthony-c-martin

LGTM!

…c-martin/bicep-local-providers

This reverts commit e280973.

jorgecotillo added 7 commits June 25, 2024 23:58

initial commit

f6de99f

Merge branch 'main' into jcotillo/local_extensibility_v2

34e374e

update test

4bc5707

added missing fields

ff0e209

updated grpc test

29d2d3c

removed v2 suffix

e0c8ac2

Merge branch 'main' into jcotillo/local_extensibility_v2

132ff99

anthony-c-martin reviewed Jun 29, 2024

View reviewed changes

src/Bicep.Local.Extension/extension.proto Outdated Show resolved Hide resolved

anthony-c-martin reviewed Jun 29, 2024

View reviewed changes

src/Bicep.Core/Emit/TemplateWriter.cs Outdated Show resolved Hide resolved

anthony-c-martin reviewed Jun 29, 2024

View reviewed changes

src/Bicep.Local.Deploy/LocalDeploymentEngineHost.cs Show resolved Hide resolved

shenglol reviewed Jul 1, 2024

View reviewed changes

src/Bicep.Local.Deploy/Extensibility/LocalExtensibilityHandler.cs Outdated Show resolved Hide resolved

fixed emitter

eb89bb0

jorgecotillo changed the title ~~Jcotillo/local extensibility v2~~ [LocalDeploy] Migrate to extensibility v2 contract Jul 1, 2024

fixed expected error messages

3132ee1

jorgecotillo marked this pull request as ready for review July 1, 2024 19:59

jorgecotillo added 2 commits July 1, 2024 14:57

updated missing rename and updated code based on feedback

13e2016

nit message update

3cd7169

jorgecotillo commented Jul 1, 2024

View reviewed changes

jorgecotillo added 2 commits July 2, 2024 05:48

merge from master

ec5c6eb

merge from master

765d520

jorgecotillo commented Jul 2, 2024

View reviewed changes

anthony-c-martin reviewed Jul 2, 2024

View reviewed changes

shenglol reviewed Jul 2, 2024

View reviewed changes

src/Bicep.Local.Deploy/Extensibility/AzExtensibilityExtension.cs Outdated Show resolved Hide resolved

jorgecotillo added 5 commits July 4, 2024 12:01

use extensibility pkg models.

aa8e07b

Revert "merge from master"

c61c3f0

This reverts commit 765d520.

merge from master

8467c11

added missing feature override

d0dc4b4

minor updates and fixed tests

4b1ecf2

anthony-c-martin approved these changes Jul 5, 2024

View reviewed changes

Fixes for issues noticed integrating with https://github.com/anthony-…

232ef21

…c-martin/bicep-local-providers

jorgecotillo merged commit e280973 into main Jul 8, 2024
44 checks passed

jorgecotillo deleted the jcotillo/local_extensibility_v2 branch July 8, 2024 17:13

anthony-c-martin added a commit that referenced this pull request Jul 8, 2024

Revert "[LocalDeploy] Migrate to extensibility v2 contract (#14438)"

15ee663

This reverts commit e280973.


		namespace Bicep.Local.Deploy.Extensibility;

		public class AzExtensibilityExtension : LocalExtensibilityExtension


		namespace Bicep.Local.Deploy.Extensibility;

		public abstract class LocalExtensibilityExtension : IAsyncDisposable

		response.Headers.Add("Location", "local");
		response.Headers.Add("Version", extensionVersion);

[LocalDeploy] Migrate to extensibility v2 contract #14438

[LocalDeploy] Migrate to extensibility v2 contract #14438

Conversation

jorgecotillo commented Jun 28, 2024 • edited Loading

Contributing a Pull Request

Contributing to documentation

Contributing an example

Contributing a feature

Contributing a snippet

Microsoft Reviewers: Open in CodeFlow

github-actions bot commented Jun 28, 2024 • edited Loading

github-actions bot commented Jun 28, 2024 • edited Loading

Dotnet Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenglol Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin left a comment

Choose a reason for hiding this comment

jorgecotillo commented Jun 28, 2024 •

edited

Loading

github-actions bot commented Jun 28, 2024 •

edited

Loading

github-actions bot commented Jun 28, 2024 •

edited

Loading

anthony-c-martin Jul 2, 2024 •

edited

Loading

anthony-c-martin Jul 2, 2024 •

edited

Loading

shenglol Jul 2, 2024 •

edited

Loading

anthony-c-martin Jul 3, 2024 •

edited

Loading