From 2cd75c8ee026ab265ee0525c38ebed0ca31f2b1a Mon Sep 17 00:00:00 2001 From: aryanmehrotra Date: Sat, 26 Sep 2026 19:06:10 +0530 Subject: [PATCH 1/5] feat(azure-disks): support disk update (PATCH) armcompute DisksClient.BeginUpdate against Microsoft.Compute/disks/{name} returned 501 NotImplemented. PATCH now applies a DiskUpdate body as a partial update and answers 202 + Azure-AsyncOperation, like CreateOrUpdate, so the SDK poller completes and its final GET reads the updated disk. - tags: replaced wholesale when present (ARM Compute PATCH semantics); internal bookkeeping tags are kept - sku / tier: switched in place; moving off UltraSSD_LRS/PremiumV2_LRS drops provisioned IOPS/MBps - diskSizeGB: grow only; a shrink is a 400 BadRequest - diskIOPSReadWrite / diskMBpsReadWrite: only on UltraSSD_LRS and PremiumV2_LRS, otherwise a 400 InvalidParameter - the volume is updated in place, so id, uniqueId, timeCreated and any attachment are preserved, and Resource Graph reflects the new size/tags --- docs/coverage/README.md | 2 +- docs/coverage/azure/README.md | 2 +- docs/coverage/azure/disks.md | 3 +- docs/coverage/coverage.json | 3 + docs/sdk-server.md | 2 +- internal/coveragegen/wireops.go | 2 +- server/azure/disks/handler.go | 3 + server/azure/disks/types.go | 16 ++ server/azure/disks/update.go | 186 +++++++++++++++++ server/azure/disks/update_test.go | 337 ++++++++++++++++++++++++++++++ 10 files changed, 551 insertions(+), 5 deletions(-) create mode 100644 server/azure/disks/update.go create mode 100644 server/azure/disks/update_test.go diff --git a/docs/coverage/README.md b/docs/coverage/README.md index 96c4e64b3..07c6a30a6 100644 --- a/docs/coverage/README.md +++ b/docs/coverage/README.md @@ -76,7 +76,7 @@ code does not implement. Machine-readable: [`coverage.json`](./coverage.json). | `datastream` | - | - | [Datastream](./gcp/datastream.md) | - | 11 | | `devcenter` | - | [DevCenter](./azure/devcenter.md) | - | - | 8 | | `digitaltwins` | - | [DigitalTwins](./azure/digitaltwins.md) | - | - | 8 | -| `disks` | - | [Disks](./azure/disks.md) | - | - | 7 | +| `disks` | - | [Disks](./azure/disks.md) | - | - | 8 | | `dns` | [Route53](./aws/route53.md) | [DNS](./azure/dns.md) | [CloudDNS](./gcp/clouddns.md) | - | 16 | | `ecs` | [ECS](./aws/ecs.md) | - | - | - | 37 | | `efs` | [EFS](./aws/efs.md) | - | - | - | 27 | diff --git a/docs/coverage/azure/README.md b/docs/coverage/azure/README.md index 8f0b55897..1f64e6dc5 100644 --- a/docs/coverage/azure/README.md +++ b/docs/coverage/azure/README.md @@ -29,7 +29,7 @@ Services cloudemu emulates for Azure, by native name. Back to the [cross-provide | [Databricks](./databricks.md) | `databricks` | 46 | | [DevCenter](./devcenter.md) | (provider-native) | 8 | | [DigitalTwins](./digitaltwins.md) | (provider-native) | 8 | -| [Disks](./disks.md) | (provider-native) | 7 | +| [Disks](./disks.md) | (provider-native) | 8 | | [ElasticSan](./elasticsan.md) | (provider-native) | 8 | | [EventGrid](./eventgrid.md) | `eventbus` | 16 | | [Eventhub](./eventhub.md) | (provider-native) | 14 | diff --git a/docs/coverage/azure/disks.md b/docs/coverage/azure/disks.md index b45d3e747..ca932c387 100644 --- a/docs/coverage/azure/disks.md +++ b/docs/coverage/azure/disks.md @@ -3,7 +3,7 @@ provider-native `disks` wire service (Azure-only) · no portable driver · [Azure index](./README.md) -## Operations (7) +## Operations (8) | Operation | Description | | --- | --- | @@ -14,6 +14,7 @@ provider-native `disks` wire service (Azure-only) · no portable driver · [Azur | `List` | | | `ListByResourceGroup` | | | `RevokeAccess` | | +| `Update` | | ## Not in scope diff --git a/docs/coverage/coverage.json b/docs/coverage/coverage.json index 8d520510c..f3b45ca50 100644 --- a/docs/coverage/coverage.json +++ b/docs/coverage/coverage.json @@ -5906,6 +5906,9 @@ }, { "name": "RevokeAccess" + }, + { + "name": "Update" } ], "providers": { diff --git a/docs/sdk-server.md b/docs/sdk-server.md index 346aa102b..9f300d03a 100644 --- a/docs/sdk-server.md +++ b/docs/sdk-server.md @@ -191,7 +191,7 @@ All handlers speak ARM JSON over HTTPS unless noted. | Service | ARM provider / operations | |---------|--------------------------| | **Virtual Machines** | `Microsoft.Compute/virtualMachines`: CreateOrUpdate, Get, List, Delete, start, powerOff, restart | -| **Disks / Snapshots / Images / SSH Public Keys** | `Microsoft.Compute/{disks,snapshots,images,sshPublicKeys}`: full CRUD | +| **Disks / Snapshots / Images / SSH Public Keys** | `Microsoft.Compute/{disks,snapshots,images,sshPublicKeys}`: full CRUD. Disks also serve Update (PATCH, 202 + Azure-AsyncOperation, so `armcompute` `DisksClient.BeginUpdate` pollers complete): tags, `sku`, `tier`, `diskSizeGB` (grow only; a shrink is a 400) and `diskIOPSReadWrite`/`diskMBpsReadWrite` (UltraSSD_LRS / PremiumV2_LRS only), plus beginGetAccess/endGetAccess. | | **Blob Storage** *(data plane)* | Containers + Blobs: Create/Delete/List, PutBlob, GetBlob, DeleteBlob, CopyBlob | | **Cosmos DB** *(data plane)* | Databases, Containers, Documents: full CRUD with `x-ms-documentdb-*` headers | | **Cosmos DB (SQL ARM control plane)** | `Microsoft.DocumentDB/databaseAccounts/{acct}/sqlDatabases[/containers]`: SQL databases (CreateUpdate/Get/List/Delete, cascading container delete), containers (CreateUpdate/Get/List/Delete with partitionKey, defaultTtl, uniqueKeyPolicy, indexingPolicy), and `throughputSettings/default` at database and container level (Get/Update manual RU/s or autoscale maxThroughput + migrateToAutoscale/migrateToManualThroughput). Real `armcosmos` `SQLResources` clients round-trip end-to-end, including the LRO pollers, so Terraform/Bicep/`az cosmosdb sql` can manage the data model. Shares state with the Cosmos data plane above; a control-plane database/container/throughput is visible to data-plane clients and vice versa. | diff --git a/internal/coveragegen/wireops.go b/internal/coveragegen/wireops.go index 49abed02c..849c2b4ad 100644 --- a/internal/coveragegen/wireops.go +++ b/internal/coveragegen/wireops.go @@ -63,7 +63,7 @@ var nativeWireOperations = map[string][]string{ //nolint:gochecknoglobals // gen "GetMetadataEndpoints", "GetToken", }, "azure/disks": { - "CreateOrUpdate", "Delete", "Get", "GrantAccess", "List", "ListByResourceGroup", "RevokeAccess", + "CreateOrUpdate", "Delete", "Get", "GrantAccess", "List", "ListByResourceGroup", "RevokeAccess", "Update", }, "azure/snapshots": { "CreateOrUpdate", "Delete", "Get", "List", "ListByResourceGroup", diff --git a/server/azure/disks/handler.go b/server/azure/disks/handler.go index 3c6ec816f..e1f76ccf2 100644 --- a/server/azure/disks/handler.go +++ b/server/azure/disks/handler.go @@ -4,6 +4,7 @@ // Supported operations: // // PUT .../disks/{name} : CreateOrUpdate (returns 202 + Azure-AsyncOperation) +// PATCH .../disks/{name} : Update (returns 202 + Azure-AsyncOperation) // GET .../disks/{name} : Get // GET .../disks : List in resource group // DELETE .../disks/{name} : Delete (returns 202 + Azure-AsyncOperation) @@ -97,6 +98,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodPut: h.createOrUpdate(w, r, rp) + case http.MethodPatch: + h.update(w, r, rp) case http.MethodGet: h.get(w, r, rp) case http.MethodDelete: diff --git a/server/azure/disks/types.go b/server/azure/disks/types.go index 501f18b90..bf51ad713 100644 --- a/server/azure/disks/types.go +++ b/server/azure/disks/types.go @@ -24,6 +24,22 @@ type diskRequestProps struct { Tier string `json:"tier"` } +// diskUpdateRequest is the PATCH body (armcompute.DiskUpdate). Every field is +// optional; pointers and a nil map distinguish "omitted" from a zero value, so +// an omitted field leaves the disk unchanged while "tags":{} clears the tags. +type diskUpdateRequest struct { + SKU *diskSKU `json:"sku"` + Tags map[string]string `json:"tags"` + Properties *diskUpdateRequestProps `json:"properties"` +} + +type diskUpdateRequestProps struct { + DiskSizeGB *int `json:"diskSizeGB"` + DiskIOPSReadWrite *int `json:"diskIOPSReadWrite"` + DiskMBpsReadWrite *int `json:"diskMBpsReadWrite"` + Tier *string `json:"tier"` +} + type creationData struct { CreateOption string `json:"createOption,omitempty"` SourceURI string `json:"sourceUri,omitempty"` diff --git a/server/azure/disks/update.go b/server/azure/disks/update.go new file mode 100644 index 000000000..e0cf99b47 --- /dev/null +++ b/server/azure/disks/update.go @@ -0,0 +1,186 @@ +package disks + +import ( + "fmt" + "net/http" + "strings" + + "github.com/stackshy/cloudemu/v2/server/wire/azurearm" + computedriver "github.com/stackshy/cloudemu/v2/services/compute/driver" +) + +// Disk SKUs whose provisioned performance (diskIOPSReadWrite / +// diskMBpsReadWrite) is caller-settable. Every other SKU derives IOPS and +// throughput from its size/tier, and Azure rejects an explicit value. +const ( + skuUltraSSD = "UltraSSD_LRS" + skuPremiumV2 = "PremiumV2_LRS" + errCodeBadReq = "BadRequest" + errCodeInvalid = "InvalidParameter" +) + +// diskPatchError is a validation failure of a disk PATCH body, rendered as an +// ARM 400 with the given code. +type diskPatchError struct { + code, msg string +} + +// update handles PATCH .../disks/{name} (armcompute DisksClient.BeginUpdate). +// The body is a DiskUpdate: every field is optional and only the supplied ones +// change. Like CreateOrUpdate it answers 202 + Azure-AsyncOperation, and the +// SDK poller's final GET reads the updated disk. +// +//nolint:gocritic // rp is a request-scoped value +func (h *Handler) update(w http.ResponseWriter, r *http.Request, rp azurearm.ResourcePath) { + if rp.ResourceGroup == "" { + azurearm.WriteError(w, http.StatusBadRequest, "InvalidPath", "missing resourceGroups segment") + return + } + + existing, err := findDiskByName(r.Context(), h.compute, rp.ResourceGroup, rp.ResourceName) + if err != nil { + azurearm.WriteCErr(w, err) + return + } + + var req diskUpdateRequest + + if !azurearm.DecodeJSON(w, r, &req) { + return + } + + cfg, perr := applyDiskPatch(existing, &req) + if perr != nil { + azurearm.WriteError(w, http.StatusBadRequest, perr.code, perr.msg) + return + } + + vol, err := h.updateExistingDisk(r.Context(), existing, cfg) + if err != nil { + azurearm.WriteCErr(w, err) + return + } + + writeDiskAsync(w, r, rp.Subscription, "disk-update-"+rp.ResourceName, + h.toDiskResponse(r.Context(), vol, rp, "")) +} + +// applyDiskPatch folds a DiskUpdate body over the existing volume and returns +// the full VolumeConfig to store. Omitted fields keep their current value. +// Tags, when present, replace the user tag set wholesale (ARM Compute PATCH +// semantics, as for virtualMachines); the cloudemu-internal bookkeeping tags +// are always preserved. +func applyDiskPatch(vol *computedriver.VolumeInfo, req *diskUpdateRequest) (computedriver.VolumeConfig, *diskPatchError) { + cfg := computedriver.VolumeConfig{ + Size: vol.Size, + VolumeType: vol.VolumeType, + IOPS: vol.IOPS, + Throughput: vol.Throughput, + Tier: vol.Tier, + Location: vol.Location, + AvailabilityZone: vol.AvailabilityZone, + Tags: patchDiskTags(vol.Tags, req.Tags), + } + + if req.SKU != nil { + applySKUPatch(&cfg, req.SKU) + } + + if req.Properties == nil { + return cfg, nil + } + + perr := applyPropsPatch(&cfg, vol.Size, req.Properties) + + return cfg, perr +} + +// applySKUPatch switches the disk's storage account type (and tier, when +// given). Moving to a SKU without caller-settable performance drops any +// provisioned IOPS/throughput, which only UltraSSD_LRS/PremiumV2_LRS carry. +func applySKUPatch(cfg *computedriver.VolumeConfig, sku *diskSKU) { + if sku.Name != "" { + cfg.VolumeType = sku.Name + } + + if sku.Tier != "" { + cfg.Tier = sku.Tier + } + + if !provisionedPerfSKU(cfg.VolumeType) { + cfg.IOPS = 0 + cfg.Throughput = 0 + } +} + +// applyPropsPatch applies diskSizeGB (grow only), tier and the provisioned +// performance fields. Azure never shrinks a managed disk, and only accepts +// diskIOPSReadWrite/diskMBpsReadWrite on UltraSSD_LRS and PremiumV2_LRS. +func applyPropsPatch(cfg *computedriver.VolumeConfig, currentSize int, p *diskUpdateRequestProps) *diskPatchError { + if p.DiskSizeGB != nil { + if *p.DiskSizeGB < currentSize { + return &diskPatchError{code: errCodeBadReq, msg: fmt.Sprintf( + "Disk size can only be increased. Current size is %d GB, requested size is %d GB.", + currentSize, *p.DiskSizeGB)} + } + + cfg.Size = *p.DiskSizeGB + } + + if p.Tier != nil && *p.Tier != "" { + cfg.Tier = *p.Tier + } + + if p.DiskIOPSReadWrite == nil && p.DiskMBpsReadWrite == nil { + return nil + } + + if !provisionedPerfSKU(cfg.VolumeType) { + return &diskPatchError{code: errCodeInvalid, msg: fmt.Sprintf( + "Property 'diskIOPSReadWrite'/'diskMBpsReadWrite' can only be set on %s and %s disks; disk SKU is %q.", + skuUltraSSD, skuPremiumV2, cfg.VolumeType)} + } + + if p.DiskIOPSReadWrite != nil { + cfg.IOPS = *p.DiskIOPSReadWrite + } + + if p.DiskMBpsReadWrite != nil { + cfg.Throughput = *p.DiskMBpsReadWrite + } + + return nil +} + +// provisionedPerfSKU reports whether a disk SKU lets the caller set IOPS and +// throughput independently of size. +func provisionedPerfSKU(sku string) bool { + return strings.EqualFold(sku, skuUltraSSD) || strings.EqualFold(sku, skuPremiumV2) +} + +// patchDiskTags returns the tag set to store: the existing tags when the PATCH +// omitted tags, otherwise the supplied user tags plus the existing internal +// bookkeeping tags. +func patchDiskTags(existing, patch map[string]string) map[string]string { + if patch == nil { + out := make(map[string]string, len(existing)) + for k, v := range existing { + out[k] = v + } + + return out + } + + out := make(map[string]string, len(patch)+diskExtraSlots) + for k, v := range patch { + out[k] = v + } + + for _, k := range []string{armNameTag, rgTag, createOptionTag, sourceIDTag} { + if v, ok := existing[k]; ok { + out[k] = v + } + } + + return out +} diff --git a/server/azure/disks/update_test.go b/server/azure/disks/update_test.go new file mode 100644 index 000000000..2ead0099f --- /dev/null +++ b/server/azure/disks/update_test.go @@ -0,0 +1,337 @@ +package disks_test + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph" + + "github.com/stackshy/cloudemu/v2" + azureserver "github.com/stackshy/cloudemu/v2/server/azure" +) + +var fastPoll = &runtime.PollUntilDoneOptions{Frequency: time.Millisecond} + +func newUpdateTestServer(t *testing.T) (*httptest.Server, *armcompute.DisksClient) { + t.Helper() + + cloudP := cloudemu.NewAzure() + srv := azureserver.New(azureserver.Drivers{ + VirtualMachines: cloudP.VirtualMachines, + Disks: cloudP.VirtualMachines, + ResourceDiscovery: cloudP.ResourceDiscovery, + }) + + ts := httptest.NewTLSServer(srv) + t.Cleanup(ts.Close) + ensureRG(t, ts, "sub-1", "rg-1") + + return ts, newDisksClient(t, ts) +} + +func createDiskForUpdate(t *testing.T, client *armcompute.DisksClient, name string, sku armcompute.DiskStorageAccountTypes) { + t.Helper() + + ctx := context.Background() + + poller, err := client.BeginCreateOrUpdate(ctx, "rg-1", name, armcompute.Disk{ + Location: to.Ptr("eastus"), + SKU: &armcompute.DiskSKU{Name: to.Ptr(sku)}, + Tags: map[string]*string{"env": to.Ptr("dev")}, + Properties: &armcompute.DiskProperties{ + CreationData: &armcompute.CreationData{CreateOption: to.Ptr(armcompute.DiskCreateOptionEmpty)}, + DiskSizeGB: to.Ptr[int32](64), + }, + }, nil) + if err != nil { + t.Fatalf("BeginCreateOrUpdate: %v", err) + } + + if _, err := poller.PollUntilDone(ctx, fastPoll); err != nil { + t.Fatalf("create poll: %v", err) + } +} + +func beginUpdate(t *testing.T, client *armcompute.DisksClient, name string, upd armcompute.DiskUpdate) (armcompute.Disk, error) { + t.Helper() + + ctx := context.Background() + + poller, err := client.BeginUpdate(ctx, "rg-1", name, upd, nil) + if err != nil { + return armcompute.Disk{}, err + } + + resp, err := poller.PollUntilDone(ctx, fastPoll) + if err != nil { + t.Fatalf("update poll: %v", err) + } + + return resp.Disk, nil +} + +// TestSDKDiskUpdate drives armcompute DisksClient.BeginUpdate + PollUntilDone +// end to end: size grows, SKU switches to PremiumV2_LRS with provisioned +// performance, tags are replaced, and identity (id/uniqueId/timeCreated) is kept. +func TestSDKDiskUpdate(t *testing.T) { + _, client := newUpdateTestServer(t) + ctx := context.Background() + + createDiskForUpdate(t, client, "upd-disk", armcompute.DiskStorageAccountTypesPremiumLRS) + + before, err := client.Get(ctx, "rg-1", "upd-disk", nil) + if err != nil { + t.Fatalf("Get before: %v", err) + } + + updated, err := beginUpdate(t, client, "upd-disk", armcompute.DiskUpdate{ + SKU: &armcompute.DiskSKU{Name: to.Ptr(armcompute.DiskStorageAccountTypesPremiumV2LRS)}, + Tags: map[string]*string{"env": to.Ptr("prod"), "team": to.Ptr("storage")}, + Properties: &armcompute.DiskUpdateProperties{ + DiskSizeGB: to.Ptr[int32](128), + DiskIOPSReadWrite: to.Ptr[int64](4000), + DiskMBpsReadWrite: to.Ptr[int64](250), + }, + }) + if err != nil { + t.Fatalf("BeginUpdate: %v", err) + } + + assertUpdatedDisk(t, "poller result", updated) + + got, err := client.Get(ctx, "rg-1", "upd-disk", nil) + if err != nil { + t.Fatalf("Get after: %v", err) + } + + assertUpdatedDisk(t, "get", got.Disk) + + if *got.ID != *before.ID || *got.Properties.UniqueID != *before.Properties.UniqueID { + t.Errorf("identity changed: id %s -> %s, uniqueId %s -> %s", + *before.ID, *got.ID, *before.Properties.UniqueID, *got.Properties.UniqueID) + } + + if !got.Properties.TimeCreated.Equal(*before.Properties.TimeCreated) { + t.Errorf("timeCreated changed: %v -> %v", before.Properties.TimeCreated, got.Properties.TimeCreated) + } +} + +func assertUpdatedDisk(t *testing.T, stage string, d armcompute.Disk) { + t.Helper() + + p := d.Properties + if p == nil { + t.Fatalf("%s: properties nil", stage) + } + + if p.DiskSizeGB == nil || *p.DiskSizeGB != 128 { + t.Errorf("%s: diskSizeGB=%v want 128", stage, p.DiskSizeGB) + } + + if p.DiskIOPSReadWrite == nil || *p.DiskIOPSReadWrite != 4000 { + t.Errorf("%s: diskIOPSReadWrite=%v want 4000", stage, p.DiskIOPSReadWrite) + } + + if p.DiskMBpsReadWrite == nil || *p.DiskMBpsReadWrite != 250 { + t.Errorf("%s: diskMBpsReadWrite=%v want 250", stage, p.DiskMBpsReadWrite) + } + + if d.SKU == nil || d.SKU.Name == nil || *d.SKU.Name != armcompute.DiskStorageAccountTypesPremiumV2LRS { + t.Errorf("%s: sku=%v want PremiumV2_LRS", stage, d.SKU) + } + + if len(d.Tags) != 2 || d.Tags["env"] == nil || *d.Tags["env"] != "prod" || d.Tags["team"] == nil { + t.Errorf("%s: tags=%v want {env:prod, team:storage}", stage, d.Tags) + } +} + +// TestSDKDiskUpdateOmittedFieldsKept checks that a tags-only PATCH leaves size +// and SKU alone, and that an empty-body PATCH is a no-op. +func TestSDKDiskUpdateOmittedFieldsKept(t *testing.T) { + _, client := newUpdateTestServer(t) + + createDiskForUpdate(t, client, "keep-disk", armcompute.DiskStorageAccountTypesStandardSSDLRS) + + got, err := beginUpdate(t, client, "keep-disk", armcompute.DiskUpdate{ + Tags: map[string]*string{"owner": to.Ptr("alice")}, + }) + if err != nil { + t.Fatalf("BeginUpdate: %v", err) + } + + if *got.Properties.DiskSizeGB != 64 { + t.Errorf("diskSizeGB=%d want 64 (unchanged)", *got.Properties.DiskSizeGB) + } + + if *got.SKU.Name != armcompute.DiskStorageAccountTypesStandardSSDLRS { + t.Errorf("sku=%s want StandardSSD_LRS (unchanged)", *got.SKU.Name) + } + + if len(got.Tags) != 1 || *got.Tags["owner"] != "alice" { + t.Errorf("tags=%v want exactly {owner:alice}", got.Tags) + } + + got, err = beginUpdate(t, client, "keep-disk", armcompute.DiskUpdate{}) + if err != nil { + t.Fatalf("empty BeginUpdate: %v", err) + } + + if *got.Properties.DiskSizeGB != 64 || len(got.Tags) != 1 { + t.Errorf("empty PATCH changed the disk: size=%d tags=%v", *got.Properties.DiskSizeGB, got.Tags) + } +} + +// TestSDKDiskUpdateRejections covers the 400s real Azure returns for a shrink +// and for provisioned performance on a SKU that does not support it, plus the +// 404 for a missing disk. A rejected PATCH must leave the disk untouched. +func TestSDKDiskUpdateRejections(t *testing.T) { + _, client := newUpdateTestServer(t) + ctx := context.Background() + + createDiskForUpdate(t, client, "rej-disk", armcompute.DiskStorageAccountTypesStandardLRS) + + cases := []struct { + name string + disk string + upd armcompute.DiskUpdate + status int + wantCode string + }{ + {"shrink", "rej-disk", armcompute.DiskUpdate{ + Properties: &armcompute.DiskUpdateProperties{DiskSizeGB: to.Ptr[int32](32)}, + }, http.StatusBadRequest, "BadRequest"}, + {"iops on Standard_LRS", "rej-disk", armcompute.DiskUpdate{ + Properties: &armcompute.DiskUpdateProperties{DiskIOPSReadWrite: to.Ptr[int64](3000)}, + }, http.StatusBadRequest, "InvalidParameter"}, + {"missing disk", "no-such-disk", armcompute.DiskUpdate{ + Tags: map[string]*string{"a": to.Ptr("b")}, + }, http.StatusNotFound, ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := beginUpdate(t, client, tc.disk, tc.upd) + + var respErr *azcore.ResponseError + if !errors.As(err, &respErr) { + t.Fatalf("err=%v, want *azcore.ResponseError", err) + } + + if respErr.StatusCode != tc.status { + t.Errorf("status=%d want %d", respErr.StatusCode, tc.status) + } + + if tc.wantCode != "" && respErr.ErrorCode != tc.wantCode { + t.Errorf("code=%q want %q", respErr.ErrorCode, tc.wantCode) + } + }) + } + + got, err := client.Get(ctx, "rg-1", "rej-disk", nil) + if err != nil { + t.Fatalf("Get: %v", err) + } + + if *got.Properties.DiskSizeGB != 64 || got.Properties.DiskIOPSReadWrite != nil { + t.Errorf("rejected PATCH mutated disk: size=%d iops=%v", *got.Properties.DiskSizeGB, got.Properties.DiskIOPSReadWrite) + } +} + +// TestSDKDiskUpdateSKUDowngradeDropsPerf checks that moving a PremiumV2_LRS +// disk to a SKU without settable performance drops the provisioned IOPS/MBps. +func TestSDKDiskUpdateSKUDowngradeDropsPerf(t *testing.T) { + _, client := newUpdateTestServer(t) + + createDiskForUpdate(t, client, "perf-disk", armcompute.DiskStorageAccountTypesPremiumV2LRS) + + if _, err := beginUpdate(t, client, "perf-disk", armcompute.DiskUpdate{ + Properties: &armcompute.DiskUpdateProperties{DiskIOPSReadWrite: to.Ptr[int64](5000)}, + }); err != nil { + t.Fatalf("set iops: %v", err) + } + + got, err := beginUpdate(t, client, "perf-disk", armcompute.DiskUpdate{ + SKU: &armcompute.DiskSKU{Name: to.Ptr(armcompute.DiskStorageAccountTypesPremiumLRS)}, + }) + if err != nil { + t.Fatalf("downgrade: %v", err) + } + + if got.Properties.DiskIOPSReadWrite != nil { + t.Errorf("diskIOPSReadWrite=%d after moving to Premium_LRS, want omitted", *got.Properties.DiskIOPSReadWrite) + } +} + +// TestSDKDiskUpdateVisibleInResourceGraph checks that a PATCHed size and tag +// set is what Resource Graph reports for the disk afterwards. +func TestSDKDiskUpdateVisibleInResourceGraph(t *testing.T) { + ts, client := newUpdateTestServer(t) + + createDiskForUpdate(t, client, "arg-disk", armcompute.DiskStorageAccountTypesPremiumLRS) + + if _, err := beginUpdate(t, client, "arg-disk", armcompute.DiskUpdate{ + Tags: map[string]*string{"env": to.Ptr("prod")}, + Properties: &armcompute.DiskUpdateProperties{DiskSizeGB: to.Ptr[int32](256)}, + }); err != nil { + t.Fatalf("BeginUpdate: %v", err) + } + + row := queryDiskRow(t, ts) + + props, _ := row["properties"].(map[string]any) + if size, _ := props["diskSizeGB"].(float64); size != 256 { + t.Errorf("resource graph diskSizeGB=%v want 256", props["diskSizeGB"]) + } + + tags, _ := row["tags"].(map[string]any) + if tags["env"] != "prod" { + t.Errorf("resource graph tags=%v want env=prod", tags) + } +} + +func queryDiskRow(t *testing.T, ts *httptest.Server) map[string]any { + t.Helper() + + opts := &arm.ClientOptions{ClientOptions: azcore.ClientOptions{ + Cloud: cloud.Configuration{ + ActiveDirectoryAuthorityHost: "https://login.microsoftonline.com/", + Services: map[cloud.ServiceName]cloud.ServiceConfiguration{ + cloud.ResourceManager: {Endpoint: ts.URL, Audience: "https://management.azure.com"}, + }, + }, + Transport: ts.Client(), + Retry: policy.RetryOptions{MaxRetries: -1}, + }} + + cf, err := armresourcegraph.NewClientFactory(fakeCred{}, opts) + if err != nil { + t.Fatal(err) + } + + out, err := cf.NewClient().Resources(context.Background(), armresourcegraph.QueryRequest{ + Query: to.Ptr("Resources | where type =~ 'microsoft.compute/disks'"), + }, nil) + if err != nil { + t.Fatalf("Resources: %v", err) + } + + data, _ := out.Data.([]any) + if len(data) != 1 { + t.Fatalf("resource graph rows=%d want 1", len(data)) + } + + row, _ := data[0].(map[string]any) + + return row +} From 5a0895ed94546c08815bf2949ca169058166dfd8 Mon Sep 17 00:00:00 2001 From: aryanmehrotra Date: Sat, 26 Sep 2026 19:06:10 +0530 Subject: [PATCH 2/5] feat(azure-functions): support App Service plan update (PATCH) armappservice PlansClient.Update against Microsoft.Web/serverfarms/{name} answered 405 MethodNotAllowed; only PUT, GET and DELETE were routed. PATCH now merges the body into the stored plan and answers 200 with the updated plan, as the sites PATCH does: - kind, sku.name/tier/capacity (a new SKU name re-derives the tier) - tags, replaced wholesale when present - properties.reserved, perSiteScaling, zoneRedundant and maximumElasticWorkerCount, which are now also modeled on PUT and rendered on every read A PATCH for a missing plan, or one in another resource group, is a 404. The provider gains PatchAppServicePlan, a read-modify-write under the store lock. --- docs/sdk-server.md | 2 +- providers/azure/functions/app_service_plan.go | 79 +++++++ .../azure/functions/app_service_plan_test.go | 36 ++++ server/azure/functions/handler.go | 41 +++- server/azure/functions/plan_patch.go | 73 +++++++ server/azure/functions/plan_patch_sdk_test.go | 196 ++++++++++++++++++ server/azure/functions/types.go | 43 +++- 7 files changed, 452 insertions(+), 18 deletions(-) create mode 100644 server/azure/functions/plan_patch.go create mode 100644 server/azure/functions/plan_patch_sdk_test.go diff --git a/docs/sdk-server.md b/docs/sdk-server.md index 9f300d03a..002c01df8 100644 --- a/docs/sdk-server.md +++ b/docs/sdk-server.md @@ -197,7 +197,7 @@ All handlers speak ARM JSON over HTTPS unless noted. | **Cosmos DB (SQL ARM control plane)** | `Microsoft.DocumentDB/databaseAccounts/{acct}/sqlDatabases[/containers]`: SQL databases (CreateUpdate/Get/List/Delete, cascading container delete), containers (CreateUpdate/Get/List/Delete with partitionKey, defaultTtl, uniqueKeyPolicy, indexingPolicy), and `throughputSettings/default` at database and container level (Get/Update manual RU/s or autoscale maxThroughput + migrateToAutoscale/migrateToManualThroughput). Real `armcosmos` `SQLResources` clients round-trip end-to-end, including the LRO pollers, so Terraform/Bicep/`az cosmosdb sql` can manage the data model. Shares state with the Cosmos data plane above; a control-plane database/container/throughput is visible to data-plane clients and vice versa. | | **Virtual Network** | `Microsoft.Network/{virtualNetworks,networkSecurityGroups,publicIPAddresses,networkInterfaces}`: CRUD + nested subnets; NICs bind a subnet and get a private IP | | **Azure Monitor** | `microsoft.insights/metricAlerts` and metric data ingest/read | -| **Functions** | `Microsoft.Web/sites` (Function Apps): CreateOrUpdate, Get, List, Delete + non-ARM `/api/{name}` invoke | +| **Functions** | `Microsoft.Web/sites` (Function Apps): CreateOrUpdate, Get, List, Update (PATCH), Delete + non-ARM `/api/{name}` invoke. `Microsoft.Web/serverfarms` (App Service plans): CreateOrUpdate, Get, List, ListWebApps, Update (PATCH: `kind`, `sku`, tags, `reserved`, `perSiteScaling`, `zoneRedundant`, `maximumElasticWorkerCount`), Delete. | | **Service Bus** | `Microsoft.ServiceBus/namespaces[/queues]` ARM CRUD + raw-HTTP REST data plane (`POST /{ns}/{queue}/messages`, `DELETE /messages/head`) | | **SQL Database** | `Microsoft.Sql/servers[/databases]`: servers and databases, full CRUD lifecycle | | **Managed Cassandra** | `Microsoft.DocumentDB/cassandraClusters[/dataCenters]`: clusters (CreateOrUpdate, Get, ListByResourceGroup, ListBySubscription, Update, Delete, deallocate, start, invokeCommand, status) and datacenters (CreateOrUpdate, Get, List, Update, Delete). Real `armcosmos` `CassandraClusters`/`CassandraDataCenters` clients round-trip end-to-end, including the LRO pollers. | diff --git a/providers/azure/functions/app_service_plan.go b/providers/azure/functions/app_service_plan.go index 6abc1b7e1..05c06e419 100644 --- a/providers/azure/functions/app_service_plan.go +++ b/providers/azure/functions/app_service_plan.go @@ -28,6 +28,13 @@ type AppServicePlan struct { Kind string // app / functionapp / linux Capacity int Tags map[string]string + + // Plan properties a caller can change after create (armappservice + // PlanProperties / PlanPatchResourceProperties). + Reserved bool // true for a Linux plan + PerSiteScaling bool + ZoneRedundant bool + MaximumElasticWorkerCount int // Elastic Premium burst ceiling } // App Service plan pricing tiers, the tier real Azure derives from a SKU name @@ -224,3 +231,75 @@ func (m *Mock) ListAppServicePlans(_ context.Context, subscription, resourceGrou return out, nil } + +// AppServicePlanPatch is a partial App Service plan update (ARM PATCH +// .../serverfarms/{name}). A nil field leaves the stored value untouched; a +// non-nil Tags map replaces the plan's tags wholesale. +type AppServicePlanPatch struct { + Kind *string + SKUName *string + SKUTier *string + Capacity *int + Tags map[string]string + Reserved *bool + PerSiteScaling *bool + ZoneRedundant *bool + MaximumElasticWorkerCount *int +} + +// PatchAppServicePlan applies a partial update to one App Service plan scoped +// to the given subscription and resource group, returning the stored result or +// NotFound. The read-modify-write runs under the store lock. A SKU name change +// without an explicit tier re-derives the tier, as a create does. +func (m *Mock) PatchAppServicePlan( + _ context.Context, subscription, resourceGroup, name string, patch AppServicePlanPatch, +) (*AppServicePlan, error) { + var out AppServicePlan + + found := m.plans.Update(planKey(subscription, resourceGroup, name), func(p *AppServicePlan) *AppServicePlan { + next := *p + applyPlanPatch(&next, &patch) + out = next + + return &next + }) + if !found { + return nil, cerrors.Newf(cerrors.NotFound, "app service plan %s not found", name) + } + + return &out, nil +} + +func applyPlanPatch(p *AppServicePlan, patch *AppServicePlanPatch) { + setIf(&p.Kind, patch.Kind) + setIf(&p.Capacity, patch.Capacity) + setIf(&p.Reserved, patch.Reserved) + setIf(&p.PerSiteScaling, patch.PerSiteScaling) + setIf(&p.ZoneRedundant, patch.ZoneRedundant) + setIf(&p.MaximumElasticWorkerCount, patch.MaximumElasticWorkerCount) + + if patch.SKUName != nil && *patch.SKUName != "" && *patch.SKUName != p.SKUName { + p.SKUName = *patch.SKUName + p.SKUTier = deriveSKUTier(p.SKUName) + } + + if patch.SKUTier != nil && *patch.SKUTier != "" { + p.SKUTier = *patch.SKUTier + } + + if patch.Tags != nil { + tags := make(map[string]string, len(patch.Tags)) + for k, v := range patch.Tags { + tags[k] = v + } + + p.Tags = tags + } +} + +// setIf assigns *src to *dst when src is non-nil. +func setIf[T any](dst, src *T) { + if src != nil { + *dst = *src + } +} diff --git a/providers/azure/functions/app_service_plan_test.go b/providers/azure/functions/app_service_plan_test.go index bc2060349..7c42a254f 100644 --- a/providers/azure/functions/app_service_plan_test.go +++ b/providers/azure/functions/app_service_plan_test.go @@ -211,3 +211,39 @@ func TestAppServicePlanResourceGroupIsolation(t *testing.T) { // Deleting an already-gone (or never-scoped) plan is NotFound. require.True(t, cerrors.IsNotFound(m.DeleteAppServicePlan(ctx, "sub1", "rgA", "default"))) } + +func TestPatchAppServicePlan(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + _, err := m.CreateAppServicePlan(ctx, AppServicePlan{ + Name: "p", Subscription: "sub", ResourceGroup: "rg", SKUName: "S1", Tags: map[string]string{"env": "dev"}, + }) + require.NoError(t, err) + + reserved, workers, sku := true, 10, "P1v3" + + got, err := m.PatchAppServicePlan(ctx, "sub", "rg", "p", AppServicePlanPatch{ + SKUName: &sku, Reserved: &reserved, MaximumElasticWorkerCount: &workers, + }) + require.NoError(t, err) + + assert.Equal(t, "P1v3", got.SKUName) + assert.Equal(t, "PremiumV3", got.SKUTier, "a new SKU name re-derives the tier") + assert.True(t, got.Reserved) + assert.Equal(t, 10, got.MaximumElasticWorkerCount) + assert.Equal(t, map[string]string{"env": "dev"}, got.Tags, "omitted tags are kept") + assert.Equal(t, 1, got.Capacity, "omitted capacity is kept") + + got, err = m.PatchAppServicePlan(ctx, "sub", "rg", "p", AppServicePlanPatch{Tags: map[string]string{}}) + require.NoError(t, err) + assert.Empty(t, got.Tags, "an empty tags map clears the tags") + assert.True(t, got.Reserved, "an omitted property is kept") + + stored, err := m.GetAppServicePlan(ctx, "sub", "rg", "p") + require.NoError(t, err) + assert.Equal(t, got, stored) + + _, err = m.PatchAppServicePlan(ctx, "sub", "other-rg", "p", AppServicePlanPatch{Reserved: &reserved}) + assert.True(t, cerrors.IsNotFound(err), "a PATCH in another resource group is NotFound, got %v", err) +} diff --git a/server/azure/functions/handler.go b/server/azure/functions/handler.go index c315ce2a7..d11b07ec4 100644 --- a/server/azure/functions/handler.go +++ b/server/azure/functions/handler.go @@ -10,6 +10,7 @@ // GET .../sites : List in resource group / subscription // DELETE .../sites/{name} : Delete // PUT .../serverfarms/{name} : CreateOrUpdate App Service plan +// PATCH .../serverfarms/{name} : Update App Service plan (partial) // GET .../serverfarms/{name} : Get App Service plan // POST /api/{name} : Synchronous invoke (non-ARM, mirrors how // real Function Apps are hit at @@ -65,6 +66,9 @@ type appServicePlanStore interface { GetAppServicePlan(ctx context.Context, subscription, resourceGroup, name string) (*azfunctions.AppServicePlan, error) DeleteAppServicePlan(ctx context.Context, subscription, resourceGroup, name string) error ListAppServicePlans(ctx context.Context, subscription, resourceGroup string) ([]azfunctions.AppServicePlan, error) + PatchAppServicePlan( + ctx context.Context, subscription, resourceGroup, name string, patch azfunctions.AppServicePlanPatch, + ) (*azfunctions.AppServicePlan, error) } // azureFunctionApps is the Azure-only site surface the handler layers on top of @@ -669,6 +673,8 @@ func (h *Handler) servePlan(w http.ResponseWriter, r *http.Request, rp azurearm. switch r.Method { case http.MethodPut: createPlan(w, r, rp, store) + case http.MethodPatch: + patchPlan(w, r, rp, store) case http.MethodGet: getPlan(w, r, rp, store) case http.MethodDelete: @@ -718,16 +724,25 @@ func createPlan(w http.ResponseWriter, r *http.Request, rp azurearm.ResourcePath return } + props := req.Properties + if props == nil { + props = &serverFarmPatchProperties{} + } + plan, err := store.CreateAppServicePlan(r.Context(), azfunctions.AppServicePlan{ - Name: rp.ResourceName, - Subscription: rp.Subscription, - ResourceGroup: rp.ResourceGroup, - Location: req.Location, - SKUName: req.SKU.Name, - SKUTier: req.SKU.Tier, - Kind: req.Kind, - Capacity: req.SKU.Capacity, - Tags: req.Tags, + Name: rp.ResourceName, + Subscription: rp.Subscription, + ResourceGroup: rp.ResourceGroup, + Location: req.Location, + SKUName: req.SKU.Name, + SKUTier: req.SKU.Tier, + Kind: req.Kind, + Capacity: req.SKU.Capacity, + Tags: req.Tags, + Reserved: boolOr(props.Reserved), + PerSiteScaling: boolOr(props.PerSiteScaling), + ZoneRedundant: boolOr(props.ZoneRedundant), + MaximumElasticWorkerCount: intOr(props.MaximumElasticWorkerCount), }) if err != nil { azurearm.WriteCErr(w, err) @@ -835,8 +850,12 @@ func toServerFarmResource(rp azurearm.ResourcePath, plan *azfunctions.AppService Capacity: plan.Capacity, }, Properties: serverFarmProperties{ - ProvisioningState: "Succeeded", - Status: "Ready", + ProvisioningState: "Succeeded", + Status: "Ready", + Reserved: plan.Reserved, + PerSiteScaling: plan.PerSiteScaling, + ZoneRedundant: plan.ZoneRedundant, + MaximumElasticWorkerCount: plan.MaximumElasticWorkerCount, }, } } diff --git a/server/azure/functions/plan_patch.go b/server/azure/functions/plan_patch.go new file mode 100644 index 000000000..f86e412fc --- /dev/null +++ b/server/azure/functions/plan_patch.go @@ -0,0 +1,73 @@ +package functions + +import ( + "encoding/json" + "io" + "net/http" + + azfunctions "github.com/stackshy/cloudemu/v2/providers/azure/functions" + "github.com/stackshy/cloudemu/v2/server/wire/azurearm" +) + +// patchPlan serves PATCH .../serverfarms/{name} (armappservice +// PlansClient.Update). Only the fields present in the body change; the rest of +// the plan is kept. Like the sites PATCH it answers 200 synchronously with the +// updated plan, one of the two statuses (200/202) the SDK accepts. A PATCH +// against a missing plan, or the wrong resource group, is a 404. +// +//nolint:gocritic // rp travels the dispatch chain once per request. +func patchPlan(w http.ResponseWriter, r *http.Request, rp azurearm.ResourcePath, store appServicePlanStore) { + if rp.ResourceGroup == "" { + azurearm.WriteError(w, http.StatusBadRequest, "InvalidPath", "missing resourceGroups segment") + return + } + + r.Body = http.MaxBytesReader(w, r.Body, maxControlBytes) + + var req patchServerFarmRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil && err != io.EOF { + azurearm.WriteError(w, http.StatusBadRequest, "InvalidRequestContent", err.Error()) + return + } + + plan, err := store.PatchAppServicePlan(r.Context(), rp.Subscription, rp.ResourceGroup, rp.ResourceName, + toPlanPatch(&req)) + if err != nil { + azurearm.WriteCErr(w, err) + return + } + + azurearm.WriteJSON(w, http.StatusOK, toServerFarmResource(rp, plan)) +} + +// toPlanPatch maps the wire PATCH body onto the provider's partial update. +func toPlanPatch(req *patchServerFarmRequest) azfunctions.AppServicePlanPatch { + patch := azfunctions.AppServicePlanPatch{Kind: req.Kind, Tags: req.Tags} + + if req.SKU != nil { + patch.SKUName = req.SKU.Name + patch.SKUTier = req.SKU.Tier + patch.Capacity = req.SKU.Capacity + } + + if req.Properties != nil { + patch.Reserved = req.Properties.Reserved + patch.PerSiteScaling = req.Properties.PerSiteScaling + patch.ZoneRedundant = req.Properties.ZoneRedundant + patch.MaximumElasticWorkerCount = req.Properties.MaximumElasticWorkerCount + } + + return patch +} + +func boolOr(p *bool) bool { + return p != nil && *p +} + +func intOr(p *int) int { + if p == nil { + return 0 + } + + return *p +} diff --git a/server/azure/functions/plan_patch_sdk_test.go b/server/azure/functions/plan_patch_sdk_test.go new file mode 100644 index 000000000..7a20768a7 --- /dev/null +++ b/server/azure/functions/plan_patch_sdk_test.go @@ -0,0 +1,196 @@ +package functions_test + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appservice/armappservice/v3" + + "github.com/stackshy/cloudemu/v2" + azureserver "github.com/stackshy/cloudemu/v2/server/azure" +) + +func newPlanPatchServer(t *testing.T) (*httptest.Server, *armappservice.PlansClient) { + t.Helper() + + cloudP := cloudemu.NewAzure() + ts := httptest.NewTLSServer(azureserver.New(azureserver.Drivers{Functions: cloudP.Functions})) + t.Cleanup(ts.Close) + + ensureRG(t, ts.Client(), ts.URL, subID, rgName) + + client := newPlansClient(t, ts) + + poller, err := client.BeginCreateOrUpdate(context.Background(), rgName, "patch-plan", armappservice.Plan{ + Kind: to.Ptr("elastic"), + Location: to.Ptr("eastus"), + Tags: map[string]*string{"env": to.Ptr("dev")}, + SKU: &armappservice.SKUDescription{Name: to.Ptr("EP1"), Capacity: to.Ptr[int32](1)}, + Properties: &armappservice.PlanProperties{ + MaximumElasticWorkerCount: to.Ptr[int32](5), + }, + }, nil) + if err != nil { + t.Fatalf("BeginCreateOrUpdate: %v", err) + } + + if _, err := poller.PollUntilDone(context.Background(), &runtimePollerOptions); err != nil { + t.Fatalf("PollUntilDone: %v", err) + } + + return ts, client +} + +// TestSDKAzureAppServicePlanUpdate drives armappservice PlansClient.Update +// (PATCH): the changed properties land, and everything the body omitted +// (SKU, tags, kind) is kept. +func TestSDKAzureAppServicePlanUpdate(t *testing.T) { + _, client := newPlanPatchServer(t) + ctx := context.Background() + + resp, err := client.Update(ctx, rgName, "patch-plan", armappservice.PlanPatchResource{ + Properties: &armappservice.PlanPatchResourceProperties{ + MaximumElasticWorkerCount: to.Ptr[int32](20), + Reserved: to.Ptr(true), + ZoneRedundant: to.Ptr(true), + }, + }, nil) + if err != nil { + t.Fatalf("Update: %v", err) + } + + assertPatchedPlan(t, "update response", resp.Plan) + + got, err := client.Get(ctx, rgName, "patch-plan", nil) + if err != nil { + t.Fatalf("Get: %v", err) + } + + assertPatchedPlan(t, "get", got.Plan) +} + +func assertPatchedPlan(t *testing.T, stage string, p armappservice.Plan) { + t.Helper() + + if p.Properties == nil { + t.Fatalf("%s: properties nil", stage) + } + + pp := p.Properties + + if pp.MaximumElasticWorkerCount == nil || *pp.MaximumElasticWorkerCount != 20 { + t.Errorf("%s: maximumElasticWorkerCount=%v want 20", stage, pp.MaximumElasticWorkerCount) + } + + if pp.Reserved == nil || !*pp.Reserved { + t.Errorf("%s: reserved=%v want true", stage, pp.Reserved) + } + + if pp.ZoneRedundant == nil || !*pp.ZoneRedundant { + t.Errorf("%s: zoneRedundant=%v want true", stage, pp.ZoneRedundant) + } + + if p.SKU == nil || *p.SKU.Name != "EP1" || *p.SKU.Tier != "ElasticPremium" { + t.Errorf("%s: sku=%+v want EP1/ElasticPremium (unchanged)", stage, p.SKU) + } + + if p.Kind == nil || *p.Kind != "elastic" { + t.Errorf("%s: kind=%v want elastic (unchanged)", stage, p.Kind) + } + + if len(p.Tags) != 1 || *p.Tags["env"] != "dev" { + t.Errorf("%s: tags=%v want {env:dev} (unchanged)", stage, p.Tags) + } +} + +// TestAzureAppServicePlanPatchSKUAndTags sends the sku and tags a raw ARM +// PATCH may carry (armappservice.PlanPatchResource has no fields for them), and +// reads the result back through the SDK. A new SKU name re-derives the tier; +// tags are replaced wholesale. +func TestAzureAppServicePlanPatchSKUAndTags(t *testing.T) { + ts, client := newPlanPatchServer(t) + + body := `{"sku":{"name":"EP2","capacity":3},"tags":{"team":"payments"}}` + + status, out := rawPlanPatch(t, ts, "patch-plan", body) + if status != http.StatusOK { + t.Fatalf("PATCH status=%d body=%s", status, out) + } + + got, err := client.Get(context.Background(), rgName, "patch-plan", nil) + if err != nil { + t.Fatalf("Get: %v", err) + } + + if *got.SKU.Name != "EP2" || *got.SKU.Tier != "ElasticPremium" || *got.SKU.Capacity != 3 { + t.Errorf("sku=%s/%s/%d want EP2/ElasticPremium/3", *got.SKU.Name, *got.SKU.Tier, *got.SKU.Capacity) + } + + if len(got.Tags) != 1 || got.Tags["team"] == nil || *got.Tags["team"] != "payments" { + t.Errorf("tags=%v want exactly {team:payments}", got.Tags) + } + + if got.Properties.MaximumElasticWorkerCount == nil || *got.Properties.MaximumElasticWorkerCount != 5 { + t.Errorf("maximumElasticWorkerCount=%v want 5 (unchanged)", got.Properties.MaximumElasticWorkerCount) + } + + var resp map[string]any + if err := json.Unmarshal(out, &resp); err != nil { + t.Fatalf("decode PATCH body: %v", err) + } + + if resp["name"] != "patch-plan" { + t.Errorf("PATCH response name=%v want patch-plan", resp["name"]) + } +} + +// TestSDKAzureAppServicePlanUpdateMissing checks a PATCH against a plan that +// does not exist (or lives in another resource group) is a 404. +func TestSDKAzureAppServicePlanUpdateMissing(t *testing.T) { + _, client := newPlanPatchServer(t) + + _, err := client.Update(context.Background(), rgName, "no-such-plan", armappservice.PlanPatchResource{ + Properties: &armappservice.PlanPatchResourceProperties{Reserved: to.Ptr(true)}, + }, nil) + + var respErr *azcore.ResponseError + if !errors.As(err, &respErr) || respErr.StatusCode != http.StatusNotFound { + t.Fatalf("err=%v, want 404 ResponseError", err) + } +} + +func rawPlanPatch(t *testing.T, ts *httptest.Server, name, body string) (int, []byte) { + t.Helper() + + url := ts.URL + "/subscriptions/" + subID + "/resourceGroups/" + rgName + + "/providers/Microsoft.Web/serverfarms/" + name + "?api-version=2023-01-01" + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPatch, url, strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + + req.Header.Set("Content-Type", "application/json") + + resp, err := ts.Client().Do(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + + var buf strings.Builder + + if _, err := io.Copy(&buf, resp.Body); err != nil { + t.Fatal(err) + } + + return resp.StatusCode, []byte(buf.String()) +} diff --git a/server/azure/functions/types.go b/server/azure/functions/types.go index fbd6cf0a3..f8aa1e04d 100644 --- a/server/azure/functions/types.go +++ b/server/azure/functions/types.go @@ -167,8 +167,12 @@ type serverFarmSKU struct { } type serverFarmProperties struct { - ProvisioningState string `json:"provisioningState,omitempty"` - Status string `json:"status,omitempty"` + ProvisioningState string `json:"provisioningState,omitempty"` + Status string `json:"status,omitempty"` + Reserved bool `json:"reserved"` + PerSiteScaling bool `json:"perSiteScaling"` + ZoneRedundant bool `json:"zoneRedundant"` + MaximumElasticWorkerCount int `json:"maximumElasticWorkerCount,omitempty"` } // serverFarmListResponse is the {value:[...]} envelope for the serverfarms @@ -179,10 +183,37 @@ type serverFarmListResponse struct { // createServerFarmRequest captures the fields read from a serverfarms PUT body. type createServerFarmRequest struct { - Kind string `json:"kind"` - Location string `json:"location"` - Tags map[string]string `json:"tags"` - SKU serverFarmSKU `json:"sku"` + Kind string `json:"kind"` + Location string `json:"location"` + Tags map[string]string `json:"tags"` + SKU serverFarmSKU `json:"sku"` + Properties *serverFarmPatchProperties `json:"properties"` +} + +// patchServerFarmRequest captures a serverfarms PATCH body +// (armappservice.PlanPatchResource plus the sku/tags the ARM REST API also +// accepts). Every field is optional: a nil pointer or map leaves the stored +// value unchanged, and a present tags map replaces the tags wholesale. +type patchServerFarmRequest struct { + Kind *string `json:"kind"` + Tags map[string]string `json:"tags"` + SKU *serverFarmPatchSKU `json:"sku"` + Properties *serverFarmPatchProperties `json:"properties"` +} + +type serverFarmPatchSKU struct { + Name *string `json:"name"` + Tier *string `json:"tier"` + Capacity *int `json:"capacity"` +} + +// serverFarmPatchProperties are the mutable plan properties read from a PUT +// or PATCH body. +type serverFarmPatchProperties struct { + Reserved *bool `json:"reserved"` + PerSiteScaling *bool `json:"perSiteScaling"` + ZoneRedundant *bool `json:"zoneRedundant"` + MaximumElasticWorkerCount *int `json:"maximumElasticWorkerCount"` } // stringDictionary is the ARM StringDictionary shape returned by From 3de8b772d0ac8419368e9a8d4f91ad38fad1ae84 Mon Sep 17 00:00:00 2001 From: aryanmehrotra Date: Sun, 27 Sep 2026 16:28:36 +0530 Subject: [PATCH 3/5] fix(azure-disks): merge disk updates in the provider and enforce Azure's resize rules What was wrong: - The PATCH handler built a full VolumeConfig from a DescribeVolumes snapshot and UpdateVolume mutated the stored *VolumeInfo in place with no lock, then Set it back. That raced DescribeVolumes and overwrote a concurrent AttachVolume copy-on-write: under -race, 277 of 300 disks lost their attachment when attach and update ran together. Two concurrent PATCHes (size, tags) also overwrote each other. - Grow-only and the IOPS/MBps-SKU rule lived only in the wire PATCH handler, so a re-PUT (and the Go library) could shrink a disk. - A shrink answered 400 BadRequest; real Azure answers 400 InvalidParameter (InvalidResizeWithName, "Troubleshoot Azure disk resize failures"). - Attached-disk and active-SAS restrictions were not enforced, sku.tier was copied into the performance tier, a tier was accepted on any SKU, and conversions to/from UltraSSD_LRS were accepted. What the real API does, and what this does now: - New optional driver capability AzureDiskPatcher.PatchVolume. The merge and every rule run inside m.volumes.Update with copy-on-write, like AttachVolume. UpdateVolume (the re-PUT) goes through the same path. - Shrink -> InvalidArgument (400 InvalidParameter); size above the SKU maximum (32767 GiB, 65536 for Ultra/PremiumV2) -> 400. - Active, unexpired SAS (beginGetAccess) -> 409 ChangeDiskSizeWhileActiveSasNotAllowed for a resize, 409 OperationNotAllowed for other property changes; tags still update. - Attached disk (Microsoft Learn troubleshoot-disk-resize, expand-disks, disks-convert-types): an OS disk is only resized, and any disk only converted, while its VM is deallocated (409 OperationNotAllowed); data disks still grow online; a Standard/Premium disk of 4 TiB or less cannot grow past 4 TiB while attached (409 InvalidResizeForLargeDisks). - SKU conversion: Ultra is neither source nor target, PremiumV2 cannot be converted away, an OS disk cannot become PremiumV2, unknown SKUs are 400. - Performance tier (disks-change-performance): Premium SSD only, not below the size's baseline, P60-P80 only above 4 TiB; a move to Premium sets the baseline tier, a move off clears it, a grow raises it to the new baseline. sku.tier is read-only and ignored on PATCH and re-PUT. - The wire layer echoes the provider's ARM code via *driver.AzureDiskError. --- .../azure/virtualmachines/disk_update.go | 500 ++++++++++++++++++ .../azure/virtualmachines/disk_update_test.go | 268 ++++++++++ providers/azure/virtualmachines/vm.go | 37 +- server/azure/disks/handler.go | 6 +- server/azure/disks/update.go | 175 ++---- server/azure/disks/update_rules_test.go | 364 +++++++++++++ server/azure/disks/update_test.go | 48 +- services/compute/driver/driver.go | 49 ++ 8 files changed, 1285 insertions(+), 162 deletions(-) create mode 100644 providers/azure/virtualmachines/disk_update.go create mode 100644 providers/azure/virtualmachines/disk_update_test.go create mode 100644 server/azure/disks/update_rules_test.go diff --git a/providers/azure/virtualmachines/disk_update.go b/providers/azure/virtualmachines/disk_update.go new file mode 100644 index 000000000..5f3ff7d01 --- /dev/null +++ b/providers/azure/virtualmachines/disk_update.go @@ -0,0 +1,500 @@ +package virtualmachines + +import ( + "net/url" + "strings" + "time" + + cerrors "github.com/stackshy/cloudemu/v2/errors" + "github.com/stackshy/cloudemu/v2/services/compute/driver" +) + +// Managed disk SKUs (armcompute DiskStorageAccountTypes). +const ( + skuStandardLRS = "Standard_LRS" + skuStandardSSDLRS = "StandardSSD_LRS" + skuStandardSSDZRS = "StandardSSD_ZRS" + skuPremiumLRS = "Premium_LRS" + skuPremiumZRS = "Premium_ZRS" + skuPremiumV2LRS = "PremiumV2_LRS" + skuUltraSSDLRS = "UltraSSD_LRS" +) + +const ( + // osDiskDevice mirrors the Device marker server/azure/virtualmachines + // attaches a VM's OS disk at, so the disk rules can tell an OS disk from a + // data disk. + osDiskDevice = "osdisk" + + // largeDiskBoundaryGiB is where Standard HDD/SSD and Premium SSD move to a + // different storage back end. Crossing it needs the disk detached. + largeDiskBoundaryGiB = 4096 + + // maxDiskSizeGiB / maxProvisionedDiskSizeGiB are the largest managed disk + // sizes (Standard/Premium SSD, and Ultra Disk / Premium SSD v2). + maxDiskSizeGiB = 32767 + maxProvisionedDiskSizeGiB = 65536 + + // sasTimeLayout is the se= (signed expiry) format GrantDiskAccess writes. + sasTimeLayout = "2006-01-02T15:04:05Z" + + // ARM error codes for a disk update Azure refuses because of the disk's + // state rather than the request. + codeOperationNotAllowed = "OperationNotAllowed" + codeResizeWhileActiveSAS = "ChangeDiskSizeWhileActiveSasNotAllowed" + codeInvalidResizeLargeDisks = "InvalidResizeForLargeDisks" +) + +// knownDiskSKUs are the storage account types a disk can be converted to, by +// lower-cased name, mapped to their canonical spelling. +var knownDiskSKUs = map[string]string{ //nolint:gochecknoglobals // lookup table + strings.ToLower(skuStandardLRS): skuStandardLRS, + strings.ToLower(skuStandardSSDLRS): skuStandardSSDLRS, + strings.ToLower(skuStandardSSDZRS): skuStandardSSDZRS, + strings.ToLower(skuPremiumLRS): skuPremiumLRS, + strings.ToLower(skuPremiumZRS): skuPremiumZRS, + strings.ToLower(skuPremiumV2LRS): skuPremiumV2LRS, + strings.ToLower(skuUltraSSDLRS): skuUltraSSDLRS, +} + +// premiumTiers are the Premium SSD performance tiers in ascending order, each +// with the largest disk size (GiB) it is the baseline for. From the +// "Performance tiers for Azure Premium SSD managed disks" table on Microsoft +// Learn (virtual-machines/disks-change-performance). +var premiumTiers = []struct { //nolint:gochecknoglobals // lookup table + name string + maxSize int +}{ + {"P1", 4}, {"P2", 8}, {"P3", 16}, {"P4", 32}, {"P6", 64}, {"P10", 128}, + {"P15", 256}, {"P20", 512}, {"P30", 1024}, {"P40", 2048}, {"P50", 4096}, + {"P60", 8192}, {"P70", 16384}, {"P80", maxDiskSizeGiB}, +} + +// p50Index is the position of P50, the highest tier a disk of 4 TiB or less +// may use: P60, P70 and P80 need a disk larger than 4,096 GiB. +const p50Index = 10 + +// patchVolume merges patch over the stored volume id and validates the result +// against Azure's managed-disk update rules, all inside one store-lock span +// with copy-on-write (like AttachVolume), so a concurrent attach, detach or +// update is never overwritten with a stale copy. location, when non-empty, +// replaces the stored region (the PUT path). +func (m *Mock) patchVolume(id string, patch *driver.AzureDiskPatch, location string) (*driver.VolumeInfo, error) { + var ( + out driver.VolumeInfo + patchErr error + ) + + ok := m.volumes.Update(id, func(cur *driver.VolumeInfo) *driver.VolumeInfo { + next, err := m.mergeVolumePatch(cur, patch) + if err != nil { + patchErr = err + return cur + } + + if location != "" { + next.Location = location + } + + out = *next + out.Tags = copyTags(next.Tags) + + return next + }) + if !ok { + return nil, cerrors.Newf(cerrors.NotFound, "disk %q not found", id) + } + + if patchErr != nil { + return nil, patchErr + } + + return &out, nil +} + +// volumeChange records which disk properties a patch actually changes. +type volumeChange struct { + size, sku, tier, perf bool +} + +func (c volumeChange) any() bool { return c.size || c.sku || c.tier || c.perf } + +// mergeVolumePatch returns a new VolumeInfo with patch applied over cur, or +// the error real Azure answers the update with. It never mutates cur. Request +// errors (400) are checked before state conflicts (409). +func (m *Mock) mergeVolumePatch(cur *driver.VolumeInfo, patch *driver.AzureDiskPatch) (*driver.VolumeInfo, error) { + next := *cur + next.Tags = patchVolumeTags(cur.Tags, patch.Tags, patch.KeepTags) + + var chg volumeChange + + if err := applySizePatch(&next, cur, patch.Size, &chg); err != nil { + return nil, err + } + + if err := applySKUPatch(&next, cur, patch.VolumeType, &chg); err != nil { + return nil, err + } + + if err := applyPerfPatch(&next, cur, patch, &chg); err != nil { + return nil, err + } + + if err := applyTierPatch(&next, cur, patch.Tier, &chg); err != nil { + return nil, err + } + + if !chg.any() { + return &next, nil + } + + if chg.size { + if err := checkMaxSize(&next); err != nil { + return nil, err + } + } + + if err := m.checkVolumeState(cur, &next, chg); err != nil { + return nil, err + } + + return &next, nil +} + +// applySizePatch grows the disk. Azure never shrinks a managed disk +// (InvalidParameter / InvalidResizeWithName) and rejects a size above the +// SKU's maximum. +func applySizePatch(next, cur *driver.VolumeInfo, size *int, chg *volumeChange) error { + if size == nil || *size == cur.Size { + return nil + } + + if *size < cur.Size { + return cerrors.Newf(cerrors.InvalidArgument, + "Disk size can only be increased. Current size is %d GB, requested size is %d GB.", cur.Size, *size) + } + + next.Size = *size + chg.size = true + + return nil +} + +// applySKUPatch converts the disk to another storage account type, subject to +// checkSKUConversion. A conversion drops the provisioned IOPS/throughput of a +// SKU that does not carry them. +func applySKUPatch(next, cur *driver.VolumeInfo, sku *string, chg *volumeChange) error { + if sku == nil || *sku == "" || strings.EqualFold(*sku, cur.VolumeType) { + return nil + } + + target, ok := knownDiskSKUs[strings.ToLower(*sku)] + if !ok { + return cerrors.Newf(cerrors.InvalidArgument, "The value '%s' of parameter 'sku.name' is not valid.", *sku) + } + + if err := checkSKUConversion(cur, target); err != nil { + return err + } + + next.VolumeType = target + chg.sku = true + + if !provisionedPerfSKU(target) { + next.IOPS = 0 + next.Throughput = 0 + } + + return nil +} + +// checkSKUConversion applies Microsoft Learn (virtual-machines/ +// disks-convert-types): an Ultra Disk can be neither the source nor the target +// of a conversion, a Premium SSD v2 cannot be converted to anything else, and +// an OS disk cannot become a Premium SSD v2. +func checkSKUConversion(cur *driver.VolumeInfo, target string) error { + switch { + case strings.EqualFold(cur.VolumeType, skuUltraSSDLRS) || target == skuUltraSSDLRS: + return cerrors.Newf(cerrors.InvalidArgument, + "Changing the SKU of disk %q from %s to %s is not supported. Create a new disk from a snapshot instead.", + cur.ID, cur.VolumeType, target) + case strings.EqualFold(cur.VolumeType, skuPremiumV2LRS): + return cerrors.Newf(cerrors.InvalidArgument, + "A %s disk cannot be converted to another disk type. Migrate it using a snapshot instead.", skuPremiumV2LRS) + case target == skuPremiumV2LRS && cur.Device == osDiskDevice: + return cerrors.Newf(cerrors.InvalidArgument, "An OS disk cannot be converted to %s.", skuPremiumV2LRS) + } + + return nil +} + +// applyPerfPatch sets diskIOPSReadWrite / diskMBpsReadWrite, which Azure only +// accepts on UltraSSD_LRS and PremiumV2_LRS. Clearing them is always allowed +// (a re-PUT that omits them). +func applyPerfPatch(next, cur *driver.VolumeInfo, patch *driver.AzureDiskPatch, chg *volumeChange) error { + iops := changedInt(patch.IOPS, cur.IOPS) + mbps := changedInt(patch.Throughput, cur.Throughput) + + if iops == nil && mbps == nil { + return nil + } + + provisioned := provisionedPerfSKU(next.VolumeType) + + if !provisioned && (nonZero(iops) || nonZero(mbps)) { + return cerrors.Newf(cerrors.InvalidArgument, + "Property 'diskIOPSReadWrite'/'diskMBpsReadWrite' can only be set on %s and %s disks; disk SKU is %q.", + skuUltraSSDLRS, skuPremiumV2LRS, next.VolumeType) + } + + setInt(&next.IOPS, iops) + setInt(&next.Throughput, mbps) + + chg.perf = provisioned + + return nil +} + +// changedInt returns v when it is set and differs from cur, else nil. +func changedInt(v *int, cur int) *int { + if v == nil || *v == cur { + return nil + } + + return v +} + +func nonZero(v *int) bool { return v != nil && *v != 0 } + +func setInt(dst, v *int) { + if v != nil { + *dst = *v + } +} + +// applyTierPatch sets the performance tier and keeps it consistent with the +// SKU and size. Per Microsoft Learn (virtual-machines/disks-change-performance) +// a tier is only supported on Premium SSD, cannot be below the size's +// baseline tier, and P60-P80 need a disk larger than 4,096 GiB. Moving to +// Premium SSD sets the baseline tier, moving off it clears the tier, and +// growing a Premium SSD past its tier raises the tier to the new baseline. +func applyTierPatch(next, cur *driver.VolumeInfo, tier *string, chg *volumeChange) error { + if tier != nil && *tier != "" && !strings.EqualFold(*tier, cur.Tier) { + return applyExplicitTier(next, *tier, chg) + } + + premium := premiumSSD(next.VolumeType) + baseline := premiumTiers[baselineTierIndex(next.Size)].name + + switch { + case chg.sku && premium: + next.Tier = baseline + case chg.sku: + next.Tier = "" + case chg.size && premium && tierIndex(next.Tier) < baselineTierIndex(next.Size): + next.Tier = baseline + } + + return nil +} + +// applyExplicitTier sets a requested performance tier. +func applyExplicitTier(next *driver.VolumeInfo, tier string, chg *volumeChange) error { + if !premiumSSD(next.VolumeType) { + return cerrors.Newf(cerrors.InvalidArgument, + "Performance tier '%s' can only be set on %s and %s disks; disk SKU is %q.", + tier, skuPremiumLRS, skuPremiumZRS, next.VolumeType) + } + + idx := tierIndex(tier) + if err := validTierFor(tier, idx, next.Size); err != nil { + return err + } + + next.Tier = premiumTiers[idx].name + chg.tier = true + + return nil +} + +// validTierFor checks that the tier at idx is a valid performance tier for a +// Premium SSD of size GiB. +func validTierFor(tier string, idx, size int) error { + if idx < 0 { + return cerrors.Newf(cerrors.InvalidArgument, "The value '%s' of parameter 'tier' is not valid.", tier) + } + + base := baselineTierIndex(size) + if idx < base { + return cerrors.Newf(cerrors.InvalidArgument, + "Performance tier '%s' is below the baseline tier '%s' of a %d GiB disk.", tier, premiumTiers[base].name, size) + } + + if idx > p50Index && size <= largeDiskBoundaryGiB { + return cerrors.Newf(cerrors.InvalidArgument, + "Performance tier '%s' is only available on disks larger than %d GiB.", tier, largeDiskBoundaryGiB) + } + + return nil +} + +// checkVolumeState refuses a property change the disk's current state does not +// allow (Microsoft Learn: troubleshoot/azure/virtual-machines/windows/ +// troubleshoot-disk-resize and virtual-machines/windows/expand-disks). An +// active SAS (beginGetAccess) holds a lease and blocks every change; the +// attachment rules are in checkAttachedChange. +func (m *Mock) checkVolumeState(cur, next *driver.VolumeInfo, chg volumeChange) error { + if m.activeDiskSAS(cur.ID) { + code := codeOperationNotAllowed + if chg.size { + code = codeResizeWhileActiveSAS + } + + return diskConflict(code, "Cannot change disk %q while it has an active SAS URI. Revoke access to the disk "+ + "(endGetAccess) and retry the operation.", cur.ID) + } + + if cur.State != stateInUse || cur.AttachedTo == "" { + return nil + } + + return checkAttachedChange(cur, next, chg, m.vmAllocated(cur.AttachedTo)) +} + +// checkAttachedChange applies the rules for an attached disk: an OS disk is +// only resized, and any disk only converted to another SKU, while its VM is +// deallocated (data disks can be expanded online), and a Standard HDD/SSD or +// Premium SSD of 4 TiB or less cannot grow past 4 TiB while attached. +func checkAttachedChange(cur, next *driver.VolumeInfo, chg volumeChange, allocated bool) error { + if chg.sku && allocated { + return diskConflict(codeOperationNotAllowed, "Cannot change the SKU of disk %q while it is attached to running "+ + "VM %q. Changing the disk type requires the virtual machine to be deallocated.", cur.ID, cur.AttachedTo) + } + + if !chg.size { + return nil + } + + if cur.Device == osDiskDevice && allocated { + return diskConflict(codeOperationNotAllowed, "Cannot resize disk %q while it is attached to running VM %q. "+ + "Resizing a disk of an Azure Virtual Machine requires the virtual machine to be deallocated. "+ + "Please stop your VM and retry the operation.", cur.ID, cur.AttachedTo) + } + + if !provisionedPerfSKU(cur.VolumeType) && cur.Size <= largeDiskBoundaryGiB && next.Size > largeDiskBoundaryGiB { + return diskConflict(codeInvalidResizeLargeDisks, "Disk %q of %d GiB cannot be expanded beyond %d GiB while "+ + "it is attached. Detach the disk and retry the operation.", cur.ID, cur.Size, largeDiskBoundaryGiB) + } + + return nil +} + +// checkMaxSize rejects a size above the largest disk the SKU offers. +func checkMaxSize(vol *driver.VolumeInfo) error { + limit := maxDiskSizeGiB + if provisionedPerfSKU(vol.VolumeType) { + limit = maxProvisionedDiskSizeGiB + } + + if vol.Size > limit { + return cerrors.Newf(cerrors.InvalidArgument, + "Disk size %d GB is larger than the maximum %d GB for a %s disk.", vol.Size, limit, vol.VolumeType) + } + + return nil +} + +// diskConflict builds the 409 an ARM disk update is refused with. +func diskConflict(code, format string, args ...any) error { + return &driver.AzureDiskError{Code: code, Err: cerrors.Newf(cerrors.FailedPrecondition, format, args...)} +} + +// vmAllocated reports whether the VM holds compute (any power state but +// deallocated). A VM stopped from inside the guest is still allocated, and +// Azure still refuses the change. An unknown VM counts as not allocated. +func (m *Mock) vmAllocated(instanceID string) bool { + inst, ok := m.instances.Get(instanceID) + if !ok { + return false + } + + return inst.PowerState != powerStateDeallocated +} + +// activeDiskSAS reports whether a SAS granted by GrantDiskAccess is still +// live: not revoked and not past its se= expiry on the mock's clock. +func (m *Mock) activeDiskSAS(volumeID string) bool { + sas, ok := m.diskAccess.Get(volumeID) + if !ok { + return false + } + + u, err := url.Parse(sas) + if err != nil { + return true + } + + expiry, err := time.Parse(sasTimeLayout, u.Query().Get("se")) + if err != nil { + return true + } + + return m.opts.Clock.Now().Before(expiry) +} + +// patchVolumeTags returns the tag set to store: a copy of existing when the +// patch omits tags, otherwise the patch tags plus the existing keys in keep. +func patchVolumeTags(existing, patch map[string]string, keep []string) map[string]string { + if patch == nil { + return copyTags(existing) + } + + out := make(map[string]string, len(patch)+len(keep)) + for k, v := range patch { + out[k] = v + } + + for _, k := range keep { + if v, ok := existing[k]; ok { + out[k] = v + } + } + + return out +} + +// provisionedPerfSKU reports whether a disk SKU lets the caller set IOPS and +// throughput independently of size. +func provisionedPerfSKU(sku string) bool { + return strings.EqualFold(sku, skuUltraSSDLRS) || strings.EqualFold(sku, skuPremiumV2LRS) +} + +// premiumSSD reports whether a disk SKU is Premium SSD, the only type with +// performance tiers. +func premiumSSD(sku string) bool { + return strings.EqualFold(sku, skuPremiumLRS) || strings.EqualFold(sku, skuPremiumZRS) +} + +// tierIndex returns the position of a performance tier name in premiumTiers, +// or -1 for an unknown name (including ""). +func tierIndex(tier string) int { + for i := range premiumTiers { + if strings.EqualFold(premiumTiers[i].name, tier) { + return i + } + } + + return -1 +} + +// baselineTierIndex returns the position of the baseline tier Azure selects +// for a Premium SSD of size GiB. +func baselineTierIndex(size int) int { + for i := range premiumTiers { + if size <= premiumTiers[i].maxSize { + return i + } + } + + return len(premiumTiers) - 1 +} diff --git a/providers/azure/virtualmachines/disk_update_test.go b/providers/azure/virtualmachines/disk_update_test.go new file mode 100644 index 000000000..8a8c12945 --- /dev/null +++ b/providers/azure/virtualmachines/disk_update_test.go @@ -0,0 +1,268 @@ +package virtualmachines + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stackshy/cloudemu/v2/config" + cerrors "github.com/stackshy/cloudemu/v2/errors" + "github.com/stackshy/cloudemu/v2/services/compute/driver" +) + +// TestConcurrentAttachAndUpdateVolumeKeepsAttachment runs AttachVolume, +// UpdateVolume (a disk re-PUT) and DescribeVolumes on the same disks in +// parallel. The update must merge under the store lock with copy-on-write: +// mutating the stored *VolumeInfo in place is a data race with the reader, and +// writing back a pointer read before a concurrent attach silently drops the +// attachment (the disk reads "available" while the VM still owns it). +func TestConcurrentAttachAndUpdateVolumeKeepsAttachment(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + insts, err := m.RunInstances(ctx, driver.InstanceConfig{ImageID: "img-1", InstanceType: "Standard_D2s_v3"}, 1) + require.NoError(t, err) + + vmID := insts[0].ID + + const rounds = 300 + + ids := make([]string, rounds) + + for i := range ids { + vol, err := m.CreateVolume(ctx, driver.VolumeConfig{Size: 32, VolumeType: "Premium_LRS"}) + require.NoError(t, err) + + ids[i] = vol.ID + } + + var wg sync.WaitGroup + + for _, id := range ids { + wg.Add(3) + + go func() { + defer wg.Done() + + assert.NoError(t, m.AttachVolume(ctx, id, vmID, "0")) + }() + + go func() { + defer wg.Done() + + _, err := m.UpdateVolume(ctx, id, driver.VolumeConfig{ + Size: 64, VolumeType: "Premium_LRS", Tags: map[string]string{"env": "prod"}, + }) + assert.NoError(t, err) + }() + + go func() { + defer wg.Done() + + _, _ = m.DescribeVolumes(ctx, nil) + }() + } + + wg.Wait() + + vols, err := m.DescribeVolumes(ctx, ids) + require.NoError(t, err) + require.Len(t, vols, rounds) + + for i := range vols { + assert.Equal(t, stateInUse, vols[i].State, "disk %s lost its attachment", vols[i].ID) + assert.Equal(t, vmID, vols[i].AttachedTo, "disk %s lost its attachment", vols[i].ID) + assert.Equal(t, 64, vols[i].Size, "disk %s lost its resize", vols[i].ID) + } +} + +// TestConcurrentAttachAndPatchVolumeKeepsAttachment is the PATCH variant of +// the race above: PatchVolume must not drop a concurrent attachment either. +func TestConcurrentAttachAndPatchVolumeKeepsAttachment(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + insts, err := m.RunInstances(ctx, driver.InstanceConfig{ImageID: "img-1", InstanceType: "Standard_D2s_v3"}, 1) + require.NoError(t, err) + + const rounds = 200 + + ids := make([]string, rounds) + + for i := range ids { + vol, err := m.CreateVolume(ctx, driver.VolumeConfig{Size: 32, VolumeType: "Premium_LRS"}) + require.NoError(t, err) + + ids[i] = vol.ID + } + + size := 64 + + var wg sync.WaitGroup + + for _, id := range ids { + wg.Add(3) + + go func() { + defer wg.Done() + + assert.NoError(t, m.AttachVolume(ctx, id, insts[0].ID, "0")) + }() + + go func() { + defer wg.Done() + + _, err := m.PatchVolume(ctx, id, driver.AzureDiskPatch{Size: &size}) + assert.NoError(t, err) + }() + + go func() { + defer wg.Done() + + _, _ = m.DescribeVolumes(ctx, nil) + }() + } + + wg.Wait() + + vols, err := m.DescribeVolumes(ctx, ids) + require.NoError(t, err) + + for i := range vols { + assert.Equal(t, insts[0].ID, vols[i].AttachedTo, "disk %s lost its attachment", vols[i].ID) + assert.Equal(t, 64, vols[i].Size, "disk %s lost its resize", vols[i].ID) + } +} + +func ptrTo[T any](v T) *T { return &v } + +// TestPatchVolumeRules exercises the disk update rules through the Go library, +// the same code the ARM wire handler and a re-PUT go through. +func TestPatchVolumeRules(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + newVol := func(size int, sku string) string { + vol, err := m.CreateVolume(ctx, driver.VolumeConfig{ + Size: size, VolumeType: sku, Tags: map[string]string{"cloudemu:azureDiskName": "d", "env": "dev"}, + }) + require.NoError(t, err) + + return vol.ID + } + + premium := newVol(64, skuPremiumLRS) + standard := newVol(64, skuStandardLRS) + + for _, tc := range []struct { + name string + id string + patch driver.AzureDiskPatch + }{ + {"shrink", premium, driver.AzureDiskPatch{Size: ptrTo(32)}}, + {"unknown sku", premium, driver.AzureDiskPatch{VolumeType: ptrTo("Gold_LRS")}}, + {"above the 32767 GiB maximum", standard, driver.AzureDiskPatch{Size: ptrTo(40000)}}, + {"iops on Premium_LRS", premium, driver.AzureDiskPatch{IOPS: ptrTo(5000)}}, + {"tier on Standard_LRS", standard, driver.AzureDiskPatch{Tier: ptrTo("P10")}}, + {"unknown tier", premium, driver.AzureDiskPatch{Tier: ptrTo("P99")}}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := m.PatchVolume(ctx, tc.id, tc.patch) + assert.True(t, cerrors.IsInvalidArgument(err), "err = %v, want InvalidArgument", err) + }) + } + + // A re-PUT (UpdateVolume) is held to the grow-only rule too. + _, err := m.UpdateVolume(ctx, premium, driver.VolumeConfig{Size: 16, VolumeType: skuPremiumLRS}) + assert.True(t, cerrors.IsInvalidArgument(err), "re-PUT shrink: err = %v, want InvalidArgument", err) + + // Growing a Premium SSD past its tier raises the tier to the new baseline, + // and a tags replacement keeps the named bookkeeping keys. + got, err := m.PatchVolume(ctx, premium, driver.AzureDiskPatch{ + Size: ptrTo(600), Tags: map[string]string{"team": "x"}, KeepTags: []string{"cloudemu:azureDiskName"}, + }) + require.NoError(t, err) + assert.Equal(t, "P30", got.Tier) + assert.Equal(t, map[string]string{"team": "x", "cloudemu:azureDiskName": "d"}, got.Tags) + + // A Standard disk may grow past 4 TiB while detached, and a PremiumV2 disk + // up to 64 TiB. + _, err = m.PatchVolume(ctx, standard, driver.AzureDiskPatch{Size: ptrTo(8192)}) + require.NoError(t, err) + + v2 := newVol(64, skuPremiumV2LRS) + _, err = m.PatchVolume(ctx, v2, driver.AzureDiskPatch{Size: ptrTo(40000), IOPS: ptrTo(8000)}) + require.NoError(t, err) + + _, err = m.PatchVolume(ctx, "no-such-disk", driver.AzureDiskPatch{Size: ptrTo(1)}) + assert.True(t, cerrors.IsNotFound(err), "err = %v, want NotFound", err) +} + +// TestPatchVolumeOSDiskRules: an OS disk cannot become a Premium SSD v2, and +// is only resized while its VM is deallocated. +func TestPatchVolumeOSDiskRules(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + insts, err := m.RunInstances(ctx, driver.InstanceConfig{ImageID: "img-1", InstanceType: "Standard_D2s_v3"}, 1) + require.NoError(t, err) + + vol, err := m.CreateVolume(ctx, driver.VolumeConfig{Size: 64, VolumeType: skuPremiumLRS}) + require.NoError(t, err) + require.NoError(t, m.AttachVolume(ctx, vol.ID, insts[0].ID, osDiskDevice)) + + _, err = m.PatchVolume(ctx, vol.ID, driver.AzureDiskPatch{VolumeType: ptrTo(skuPremiumV2LRS)}) + assert.True(t, cerrors.IsInvalidArgument(err), "OS disk to PremiumV2: err = %v, want InvalidArgument", err) + + _, err = m.PatchVolume(ctx, vol.ID, driver.AzureDiskPatch{Size: ptrTo(128)}) + assertDiskErr(t, err, codeOperationNotAllowed) + + // Powered off from the guest is still allocated. + require.NoError(t, m.PowerOff(ctx, insts[0].ID)) + + _, err = m.PatchVolume(ctx, vol.ID, driver.AzureDiskPatch{Size: ptrTo(128)}) + assertDiskErr(t, err, codeOperationNotAllowed) + + require.NoError(t, m.Deallocate(ctx, insts[0].ID)) + + got, err := m.PatchVolume(ctx, vol.ID, driver.AzureDiskPatch{Size: ptrTo(128)}) + require.NoError(t, err) + assert.Equal(t, 128, got.Size) +} + +// TestPatchVolumeSASExpires: a granted SAS blocks a resize only until it is +// revoked or reaches its expiry. +func TestPatchVolumeSASExpires(t *testing.T) { + ctx := context.Background() + clk := config.NewFakeClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + m := New(config.NewOptions(config.WithClock(clk))) + + vol, err := m.CreateVolume(ctx, driver.VolumeConfig{Size: 64, VolumeType: skuStandardLRS}) + require.NoError(t, err) + + _, err = m.GrantDiskAccess(ctx, vol.ID, "Read", 60) + require.NoError(t, err) + + _, err = m.PatchVolume(ctx, vol.ID, driver.AzureDiskPatch{Size: ptrTo(128)}) + assertDiskErr(t, err, codeResizeWhileActiveSAS) + assert.True(t, cerrors.IsFailedPrecondition(err), "an active-SAS refusal is FailedPrecondition (409)") + + clk.Advance(61 * time.Second) + + _, err = m.PatchVolume(ctx, vol.ID, driver.AzureDiskPatch{Size: ptrTo(128)}) + require.NoError(t, err) +} + +func assertDiskErr(t *testing.T, err error, code string) { + t.Helper() + + var de *driver.AzureDiskError + if assert.True(t, errors.As(err, &de), "err = %v, want *driver.AzureDiskError", err) { + assert.Equal(t, code, de.Code) + } +} diff --git a/providers/azure/virtualmachines/vm.go b/providers/azure/virtualmachines/vm.go index 03a549a58..d5dccf220 100644 --- a/providers/azure/virtualmachines/vm.go +++ b/providers/azure/virtualmachines/vm.go @@ -35,6 +35,7 @@ var ( _ driver.ConsoleReader = (*Mock)(nil) _ driver.AzureVMController = (*Mock)(nil) _ driver.AzureDiskUpdater = (*Mock)(nil) + _ driver.AzureDiskPatcher = (*Mock)(nil) _ driver.KeyPairGenerator = (*Mock)(nil) _ driver.AzureDiskAccessor = (*Mock)(nil) _ driver.AzureSSHKeyUpdater = (*Mock)(nil) @@ -1152,40 +1153,38 @@ func (m *Mock) CreateVolume(_ context.Context, cfg driver.VolumeConfig) (*driver // uniqueId and timeCreated stay stable and an attached disk is not duplicated, // while updating the mutable cost fields from cfg. A non-zero Size, non-empty // VolumeType/Tier are applied; IOPS/Throughput and Tags are replaced from cfg -// (PUT is a full resource replacement). +// (PUT is a full resource replacement). The same Azure rules as PatchVolume +// apply (a re-PUT cannot shrink a disk), and the merge runs under the store +// lock with copy-on-write. // //nolint:gocritic // hugeParam: cfg mirrors the driver-interface signature. func (m *Mock) UpdateVolume(_ context.Context, id string, cfg driver.VolumeConfig) (*driver.VolumeInfo, error) { - vol, ok := m.volumes.Get(id) - if !ok { - return nil, cerrors.Newf(cerrors.NotFound, "disk %q not found", id) + tags := copyTags(cfg.Tags) + if tags == nil { + tags = map[string]string{} } + patch := driver.AzureDiskPatch{IOPS: &cfg.IOPS, Throughput: &cfg.Throughput, Tags: tags} + if cfg.Size != 0 { - vol.Size = cfg.Size + patch.Size = &cfg.Size } if cfg.VolumeType != "" { - vol.VolumeType = cfg.VolumeType + patch.VolumeType = &cfg.VolumeType } if cfg.Tier != "" { - vol.Tier = cfg.Tier + patch.Tier = &cfg.Tier } - if cfg.Location != "" { - vol.Location = cfg.Location - } - - vol.IOPS = cfg.IOPS - vol.Throughput = cfg.Throughput - vol.Tags = copyTags(cfg.Tags) - - m.volumes.Set(id, vol) - - result := *vol + return m.patchVolume(id, &patch, cfg.Location) +} - return &result, nil +// PatchVolume applies a partial managed-disk update (ARM Disks Update). See +// driver.AzureDiskPatcher. +func (m *Mock) PatchVolume(_ context.Context, id string, patch driver.AzureDiskPatch) (*driver.VolumeInfo, error) { + return m.patchVolume(id, &patch, "") } func (m *Mock) DeleteVolume(_ context.Context, id string) error { diff --git a/server/azure/disks/handler.go b/server/azure/disks/handler.go index e1f76ccf2..a40e17320 100644 --- a/server/azure/disks/handler.go +++ b/server/azure/disks/handler.go @@ -272,10 +272,14 @@ func (h *Handler) createOrUpdate(w http.ResponseWriter, r *http.Request, rp azur // attachment, rather than delete+recreate, which would leave a duplicate // phantom volume when the disk is attached (DeleteVolume rejects an attached // disk) and churn the uniqueId/timeCreated on every re-PUT. + // sku.tier is read-only in the ARM contract, so an update takes the + // performance tier from properties.tier alone. if existing, err := findDiskByName(r.Context(), h.compute, rp.ResourceGroup, rp.ResourceName); err == nil { + cfg.Tier = req.Properties.Tier + vol, err := h.updateExistingDisk(r.Context(), existing, cfg) if err != nil { - azurearm.WriteCErr(w, err) + writeDiskErr(w, err) return } diff --git a/server/azure/disks/update.go b/server/azure/disks/update.go index e0cf99b47..5300ed187 100644 --- a/server/azure/disks/update.go +++ b/server/azure/disks/update.go @@ -1,34 +1,24 @@ package disks import ( - "fmt" + "errors" "net/http" - "strings" + cerrors "github.com/stackshy/cloudemu/v2/errors" "github.com/stackshy/cloudemu/v2/server/wire/azurearm" computedriver "github.com/stackshy/cloudemu/v2/services/compute/driver" ) -// Disk SKUs whose provisioned performance (diskIOPSReadWrite / -// diskMBpsReadWrite) is caller-settable. Every other SKU derives IOPS and -// throughput from its size/tier, and Azure rejects an explicit value. -const ( - skuUltraSSD = "UltraSSD_LRS" - skuPremiumV2 = "PremiumV2_LRS" - errCodeBadReq = "BadRequest" - errCodeInvalid = "InvalidParameter" -) - -// diskPatchError is a validation failure of a disk PATCH body, rendered as an -// ARM 400 with the given code. -type diskPatchError struct { - code, msg string -} +// internalDiskTags are the cloudemu bookkeeping tags a disk carries. A PATCH +// that replaces the user tags keeps them. +var internalDiskTags = []string{armNameTag, rgTag, createOptionTag, sourceIDTag} //nolint:gochecknoglobals // fixed list // update handles PATCH .../disks/{name} (armcompute DisksClient.BeginUpdate). // The body is a DiskUpdate: every field is optional and only the supplied ones // change. Like CreateOrUpdate it answers 202 + Azure-AsyncOperation, and the -// SDK poller's final GET reads the updated disk. +// SDK poller's final GET reads the updated disk. The merge and Azure's rules +// (grow-only, SKU conversion, performance tier, attached disk, active SAS) run +// in the provider, so the Go library and serve behave the same. // //nolint:gocritic // rp is a request-scoped value func (h *Handler) update(w http.ResponseWriter, r *http.Request, rp azurearm.ResourcePath) { @@ -37,6 +27,12 @@ func (h *Handler) update(w http.ResponseWriter, r *http.Request, rp azurearm.Res return } + patcher, ok := h.compute.(computedriver.AzureDiskPatcher) + if !ok { + azurearm.WriteError(w, http.StatusNotImplemented, "NotImplemented", "disk update not supported") + return + } + existing, err := findDiskByName(r.Context(), h.compute, rp.ResourceGroup, rp.ResourceName) if err != nil { azurearm.WriteCErr(w, err) @@ -49,15 +45,9 @@ func (h *Handler) update(w http.ResponseWriter, r *http.Request, rp azurearm.Res return } - cfg, perr := applyDiskPatch(existing, &req) - if perr != nil { - azurearm.WriteError(w, http.StatusBadRequest, perr.code, perr.msg) - return - } - - vol, err := h.updateExistingDisk(r.Context(), existing, cfg) + vol, err := patcher.PatchVolume(r.Context(), existing.ID, toDiskPatch(&req)) if err != nil { - azurearm.WriteCErr(w, err) + writeDiskErr(w, err) return } @@ -65,122 +55,41 @@ func (h *Handler) update(w http.ResponseWriter, r *http.Request, rp azurearm.Res h.toDiskResponse(r.Context(), vol, rp, "")) } -// applyDiskPatch folds a DiskUpdate body over the existing volume and returns -// the full VolumeConfig to store. Omitted fields keep their current value. -// Tags, when present, replace the user tag set wholesale (ARM Compute PATCH -// semantics, as for virtualMachines); the cloudemu-internal bookkeeping tags -// are always preserved. -func applyDiskPatch(vol *computedriver.VolumeInfo, req *diskUpdateRequest) (computedriver.VolumeConfig, *diskPatchError) { - cfg := computedriver.VolumeConfig{ - Size: vol.Size, - VolumeType: vol.VolumeType, - IOPS: vol.IOPS, - Throughput: vol.Throughput, - Tier: vol.Tier, - Location: vol.Location, - AvailabilityZone: vol.AvailabilityZone, - Tags: patchDiskTags(vol.Tags, req.Tags), - } - - if req.SKU != nil { - applySKUPatch(&cfg, req.SKU) - } - - if req.Properties == nil { - return cfg, nil - } +// toDiskPatch maps a DiskUpdate body onto the provider's partial update. +// sku.tier is read-only in the ARM contract (it follows sku.name) and is +// ignored; the performance tier is properties.tier. +func toDiskPatch(req *diskUpdateRequest) computedriver.AzureDiskPatch { + patch := computedriver.AzureDiskPatch{Tags: req.Tags, KeepTags: internalDiskTags} - perr := applyPropsPatch(&cfg, vol.Size, req.Properties) - - return cfg, perr -} - -// applySKUPatch switches the disk's storage account type (and tier, when -// given). Moving to a SKU without caller-settable performance drops any -// provisioned IOPS/throughput, which only UltraSSD_LRS/PremiumV2_LRS carry. -func applySKUPatch(cfg *computedriver.VolumeConfig, sku *diskSKU) { - if sku.Name != "" { - cfg.VolumeType = sku.Name + if req.SKU != nil && req.SKU.Name != "" { + name := req.SKU.Name + patch.VolumeType = &name } - if sku.Tier != "" { - cfg.Tier = sku.Tier + if p := req.Properties; p != nil { + patch.Size = p.DiskSizeGB + patch.Tier = p.Tier + patch.IOPS = p.DiskIOPSReadWrite + patch.Throughput = p.DiskMBpsReadWrite } - if !provisionedPerfSKU(cfg.VolumeType) { - cfg.IOPS = 0 - cfg.Throughput = 0 - } + return patch } -// applyPropsPatch applies diskSizeGB (grow only), tier and the provisioned -// performance fields. Azure never shrinks a managed disk, and only accepts -// diskIOPSReadWrite/diskMBpsReadWrite on UltraSSD_LRS and PremiumV2_LRS. -func applyPropsPatch(cfg *computedriver.VolumeConfig, currentSize int, p *diskUpdateRequestProps) *diskPatchError { - if p.DiskSizeGB != nil { - if *p.DiskSizeGB < currentSize { - return &diskPatchError{code: errCodeBadReq, msg: fmt.Sprintf( - "Disk size can only be increased. Current size is %d GB, requested size is %d GB.", - currentSize, *p.DiskSizeGB)} - } - - cfg.Size = *p.DiskSizeGB - } - - if p.Tier != nil && *p.Tier != "" { - cfg.Tier = *p.Tier - } - - if p.DiskIOPSReadWrite == nil && p.DiskMBpsReadWrite == nil { - return nil - } - - if !provisionedPerfSKU(cfg.VolumeType) { - return &diskPatchError{code: errCodeInvalid, msg: fmt.Sprintf( - "Property 'diskIOPSReadWrite'/'diskMBpsReadWrite' can only be set on %s and %s disks; disk SKU is %q.", - skuUltraSSD, skuPremiumV2, cfg.VolumeType)} - } - - if p.DiskIOPSReadWrite != nil { - cfg.IOPS = *p.DiskIOPSReadWrite - } - - if p.DiskMBpsReadWrite != nil { - cfg.Throughput = *p.DiskMBpsReadWrite - } - - return nil -} - -// provisionedPerfSKU reports whether a disk SKU lets the caller set IOPS and -// throughput independently of size. -func provisionedPerfSKU(sku string) bool { - return strings.EqualFold(sku, skuUltraSSD) || strings.EqualFold(sku, skuPremiumV2) -} - -// patchDiskTags returns the tag set to store: the existing tags when the PATCH -// omitted tags, otherwise the supplied user tags plus the existing internal -// bookkeeping tags. -func patchDiskTags(existing, patch map[string]string) map[string]string { - if patch == nil { - out := make(map[string]string, len(existing)) - for k, v := range existing { - out[k] = v - } - - return out - } - - out := make(map[string]string, len(patch)+diskExtraSlots) - for k, v := range patch { - out[k] = v +// writeDiskErr writes a disk update error. A refusal the provider tagged with +// an ARM error code (OperationNotAllowed, ChangeDiskSizeWhileActiveSasNotAllowed, +// ...) is echoed with that code; anything else maps as usual. +func writeDiskErr(w http.ResponseWriter, err error) { + var de *computedriver.AzureDiskError + if !errors.As(err, &de) { + azurearm.WriteCErr(w, err) + return } - for _, k := range []string{armNameTag, rgTag, createOptionTag, sourceIDTag} { - if v, ok := existing[k]; ok { - out[k] = v - } + status := http.StatusConflict + if cerrors.IsInvalidArgument(err) { + status = http.StatusBadRequest } - return out + azurearm.WriteError(w, status, de.Code, cerrors.Message(err)) } diff --git a/server/azure/disks/update_rules_test.go b/server/azure/disks/update_rules_test.go new file mode 100644 index 000000000..880650c0b --- /dev/null +++ b/server/azure/disks/update_rules_test.go @@ -0,0 +1,364 @@ +package disks_test + +import ( + "context" + "fmt" + "io" + "net/http" + "strings" + "sync" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" + + "github.com/stackshy/cloudemu/v2/providers/azure/virtualmachines" + computedriver "github.com/stackshy/cloudemu/v2/services/compute/driver" +) + +// diskVolumeID resolves an ARM disk name to the driver volume behind it. +func diskVolumeID(t *testing.T, vm *virtualmachines.Mock, name string) string { + t.Helper() + + vols, err := vm.DescribeVolumes(context.Background(), nil) + if err != nil { + t.Fatal(err) + } + + for i := range vols { + if vols[i].Tags["cloudemu:azureDiskName"] == name { + return vols[i].ID + } + } + + t.Fatalf("no volume for disk %s", name) + + return "" +} + +// attachToNewVM starts a VM and attaches the named disk to it at device. +func attachToNewVM(t *testing.T, vm *virtualmachines.Mock, disk, device string) string { + t.Helper() + + ctx := context.Background() + + insts, err := vm.RunInstances(ctx, computedriver.InstanceConfig{ImageID: "img", InstanceType: "Standard_D2s_v3"}, 1) + if err != nil { + t.Fatal(err) + } + + if err := vm.AttachVolume(ctx, diskVolumeID(t, vm, disk), insts[0].ID, device); err != nil { + t.Fatal(err) + } + + return insts[0].ID +} + +func sizeUpdate(gb int32) armcompute.DiskUpdate { + return armcompute.DiskUpdate{Properties: &armcompute.DiskUpdateProperties{DiskSizeGB: to.Ptr(gb)}} +} + +func skuUpdate(sku armcompute.DiskStorageAccountTypes) armcompute.DiskUpdate { + return armcompute.DiskUpdate{SKU: &armcompute.DiskSKU{Name: to.Ptr(sku)}} +} + +func tierUpdate(tier string) armcompute.DiskUpdate { + return armcompute.DiskUpdate{Properties: &armcompute.DiskUpdateProperties{Tier: to.Ptr(tier)}} +} + +func diskSize(t *testing.T, client *armcompute.DisksClient, name string) int32 { + t.Helper() + + got, err := client.Get(context.Background(), "rg-1", name, nil) + if err != nil { + t.Fatalf("Get %s: %v", name, err) + } + + return *got.Properties.DiskSizeGB +} + +// TestSDKDiskRePutCannotShrink checks that the grow-only rule lives in the +// provider: a CreateOrUpdate re-PUT on an existing disk cannot shrink it +// either (it used to answer 202 and store the smaller size). +func TestSDKDiskRePutCannotShrink(t *testing.T) { + _, client := newUpdateTestServer(t) + ctx := context.Background() + + createDiskForUpdate(t, client, "shrink-disk", armcompute.DiskStorageAccountTypesPremiumLRS) + + if _, err := beginUpdate(t, client, "shrink-disk", sizeUpdate(128)); err != nil { + t.Fatalf("grow: %v", err) + } + + _, err := client.BeginCreateOrUpdate(ctx, "rg-1", "shrink-disk", armcompute.Disk{ + Location: to.Ptr("eastus"), + SKU: &armcompute.DiskSKU{Name: to.Ptr(armcompute.DiskStorageAccountTypesPremiumLRS)}, + Properties: &armcompute.DiskProperties{ + CreationData: &armcompute.CreationData{CreateOption: to.Ptr(armcompute.DiskCreateOptionEmpty)}, + DiskSizeGB: to.Ptr[int32](16), + }, + }, nil) + wantDiskErr(t, err, http.StatusBadRequest, "InvalidParameter") + + if got := diskSize(t, client, "shrink-disk"); got != 128 { + t.Errorf("diskSizeGB=%d after a rejected shrink, want 128", got) + } +} + +// TestSDKDiskUpdateAttachedToRunningVM covers the attached-disk rules from +// Microsoft Learn (troubleshoot-disk-resize, expand-disks): an OS disk is only +// resized and any disk only converted while the VM is deallocated, a data disk +// can grow online, and a Standard/Premium disk of 4 TiB or less cannot grow +// past 4 TiB while attached. +func TestSDKDiskUpdateAttachedToRunningVM(t *testing.T) { + _, client, vm := newUpdateTestEnv(t) + ctx := context.Background() + + createDiskForUpdate(t, client, "os-disk", armcompute.DiskStorageAccountTypesPremiumLRS) + createDiskForUpdate(t, client, "data-disk", armcompute.DiskStorageAccountTypesStandardLRS) + + osVM := attachToNewVM(t, vm, "os-disk", "osdisk") + attachToNewVM(t, vm, "data-disk", "0") + + _, err := beginUpdate(t, client, "os-disk", sizeUpdate(128)) + wantDiskErr(t, err, http.StatusConflict, "OperationNotAllowed") + + _, err = beginUpdate(t, client, "os-disk", skuUpdate(armcompute.DiskStorageAccountTypesStandardSSDLRS)) + wantDiskErr(t, err, http.StatusConflict, "OperationNotAllowed") + + _, err = beginUpdate(t, client, "data-disk", sizeUpdate(8192)) + wantDiskErr(t, err, http.StatusConflict, "InvalidResizeForLargeDisks") + + if _, err := beginUpdate(t, client, "data-disk", sizeUpdate(128)); err != nil { + t.Fatalf("online data-disk grow: %v", err) + } + + if _, err := beginUpdate(t, client, "os-disk", armcompute.DiskUpdate{ + Tags: map[string]*string{"k": to.Ptr("v")}, + }); err != nil { + t.Fatalf("tags on an attached disk: %v", err) + } + + if got := diskSize(t, client, "os-disk"); got != 64 { + t.Errorf("os-disk diskSizeGB=%d after rejected resize, want 64", got) + } + + if err := vm.Deallocate(ctx, osVM); err != nil { + t.Fatal(err) + } + + if _, err := beginUpdate(t, client, "os-disk", sizeUpdate(128)); err != nil { + t.Fatalf("resize after deallocate: %v", err) + } + + if _, err := beginUpdate(t, client, "os-disk", skuUpdate(armcompute.DiskStorageAccountTypesStandardSSDLRS)); err != nil { + t.Fatalf("sku change after deallocate: %v", err) + } +} + +// TestSDKDiskUpdateWithActiveSAS checks that an active beginGetAccess SAS +// blocks a resize (ChangeDiskSizeWhileActiveSasNotAllowed) and a conversion, +// while a tags-only update still goes through, and that endGetAccess lifts it. +func TestSDKDiskUpdateWithActiveSAS(t *testing.T) { + _, client := newUpdateTestServer(t) + ctx := context.Background() + + createDiskForUpdate(t, client, "sas-upd", armcompute.DiskStorageAccountTypesStandardLRS) + + grant, err := client.BeginGrantAccess(ctx, "rg-1", "sas-upd", armcompute.GrantAccessData{ + Access: to.Ptr(armcompute.AccessLevelRead), DurationInSeconds: to.Ptr[int32](3600), + }, nil) + if err != nil { + t.Fatalf("BeginGrantAccess: %v", err) + } + + if _, err := grant.PollUntilDone(ctx, fastPoll); err != nil { + t.Fatalf("grant poll: %v", err) + } + + _, err = beginUpdate(t, client, "sas-upd", sizeUpdate(256)) + wantDiskErr(t, err, http.StatusConflict, "ChangeDiskSizeWhileActiveSasNotAllowed") + + _, err = beginUpdate(t, client, "sas-upd", skuUpdate(armcompute.DiskStorageAccountTypesPremiumLRS)) + wantDiskErr(t, err, http.StatusConflict, "OperationNotAllowed") + + if _, err := beginUpdate(t, client, "sas-upd", armcompute.DiskUpdate{ + Tags: map[string]*string{"k": to.Ptr("v")}, + }); err != nil { + t.Fatalf("tags with an active SAS: %v", err) + } + + revoke, err := client.BeginRevokeAccess(ctx, "rg-1", "sas-upd", nil) + if err != nil { + t.Fatalf("BeginRevokeAccess: %v", err) + } + + if _, err := revoke.PollUntilDone(ctx, fastPoll); err != nil { + t.Fatalf("revoke poll: %v", err) + } + + if _, err := beginUpdate(t, client, "sas-upd", sizeUpdate(256)); err != nil { + t.Fatalf("resize after revoke: %v", err) + } +} + +// TestSDKDiskUpdatePerformanceTier covers the Premium SSD performance tier +// rules (Microsoft Learn, disks-change-performance) and the SKU conversion +// rules (disks-convert-types). +func TestSDKDiskUpdatePerformanceTier(t *testing.T) { + _, client := newUpdateTestServer(t) + + createDiskForUpdate(t, client, "tier-disk", armcompute.DiskStorageAccountTypesStandardSSDLRS) + createDiskForUpdate(t, client, "ultra-disk", armcompute.DiskStorageAccountTypesUltraSSDLRS) + + // A tier only applies to Premium SSD. + _, err := beginUpdate(t, client, "tier-disk", tierUpdate("P30")) + wantDiskErr(t, err, http.StatusBadRequest, "InvalidParameter") + + _, err = beginUpdate(t, client, "ultra-disk", tierUpdate("P50")) + wantDiskErr(t, err, http.StatusBadRequest, "InvalidParameter") + + // Moving to Premium SSD sets the baseline tier for 64 GiB (P6). + got, err := beginUpdate(t, client, "tier-disk", skuUpdate(armcompute.DiskStorageAccountTypesPremiumLRS)) + if err != nil { + t.Fatalf("to Premium_LRS: %v", err) + } + + wantTier(t, got, "P6") + + for _, tc := range []struct{ tier, why string }{ + {"P4", "below the P6 baseline"}, + {"P70", "P60-P80 need a disk above 4 TiB"}, + {"Premium", "sku.tier value, not a performance tier"}, + } { + t.Run(tc.why, func(t *testing.T) { + _, err := beginUpdate(t, client, "tier-disk", tierUpdate(tc.tier)) + wantDiskErr(t, err, http.StatusBadRequest, "InvalidParameter") + }) + } + + got, err = beginUpdate(t, client, "tier-disk", tierUpdate("P50")) + if err != nil { + t.Fatalf("tier P50: %v", err) + } + + wantTier(t, got, "P50") + + // sku.tier is read-only: sending it does not change the performance tier. + got, err = beginUpdate(t, client, "tier-disk", armcompute.DiskUpdate{ + SKU: &armcompute.DiskSKU{Name: to.Ptr(armcompute.DiskStorageAccountTypesPremiumLRS), Tier: to.Ptr("Premium")}, + }) + if err != nil { + t.Fatalf("sku.tier: %v", err) + } + + wantTier(t, got, "P50") + + // Moving off Premium SSD clears the tier. + got, err = beginUpdate(t, client, "tier-disk", skuUpdate(armcompute.DiskStorageAccountTypesStandardSSDLRS)) + if err != nil { + t.Fatalf("to StandardSSD_LRS: %v", err) + } + + if got.Properties.Tier != nil { + t.Errorf("tier=%s after moving off Premium SSD, want omitted", *got.Properties.Tier) + } + + // Ultra Disk is neither a conversion source nor a target. + _, err = beginUpdate(t, client, "tier-disk", skuUpdate(armcompute.DiskStorageAccountTypesUltraSSDLRS)) + wantDiskErr(t, err, http.StatusBadRequest, "InvalidParameter") + + _, err = beginUpdate(t, client, "ultra-disk", skuUpdate(armcompute.DiskStorageAccountTypesPremiumLRS)) + wantDiskErr(t, err, http.StatusBadRequest, "InvalidParameter") +} + +func wantTier(t *testing.T, d armcompute.Disk, tier string) { + t.Helper() + + if d.Properties.Tier == nil || *d.Properties.Tier != tier { + t.Errorf("tier=%v want %s", d.Properties.Tier, tier) + } +} + +// TestSDKDiskConcurrentPatchesBothLand runs a size PATCH and a tags PATCH on +// the same disks at once. The merge happens in the provider under the store +// lock, so neither overwrites the other with a stale snapshot. +func TestSDKDiskConcurrentPatchesBothLand(t *testing.T) { + _, client := newUpdateTestServer(t) + + const disks = 20 + + for i := range disks { + createDiskForUpdate(t, client, fmt.Sprintf("race-%d", i), armcompute.DiskStorageAccountTypesPremiumLRS) + } + + var wg sync.WaitGroup + + for i := range disks { + name := fmt.Sprintf("race-%d", i) + + wg.Add(2) + + go func() { + defer wg.Done() + + if _, err := beginUpdate(t, client, name, sizeUpdate(128)); err != nil { + t.Errorf("size PATCH %s: %v", name, err) + } + }() + + go func() { + defer wg.Done() + + if _, err := beginUpdate(t, client, name, armcompute.DiskUpdate{ + Tags: map[string]*string{"owner": to.Ptr("bob")}, + }); err != nil { + t.Errorf("tags PATCH %s: %v", name, err) + } + }() + } + + wg.Wait() + + for i := range disks { + name := fmt.Sprintf("race-%d", i) + + got, err := client.Get(context.Background(), "rg-1", name, nil) + if err != nil { + t.Fatalf("Get %s: %v", name, err) + } + + if *got.Properties.DiskSizeGB != 128 || got.Tags["owner"] == nil { + t.Errorf("%s: size=%d tags=%v, want both PATCHes applied", name, *got.Properties.DiskSizeGB, got.Tags) + } + } +} + +// TestDiskPatchMalformedBody checks a PATCH body that is not JSON is the 400 +// InvalidRequestContent ARM returns, and leaves the disk alone. +func TestDiskPatchMalformedBody(t *testing.T) { + ts, client := newUpdateTestServer(t) + + createDiskForUpdate(t, client, "bad-body", armcompute.DiskStorageAccountTypesPremiumLRS) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPatch, + ts.URL+diskPath("rg-1", "bad-body")+wireAPIVersion, strings.NewReader(`{"properties":`)) + if err != nil { + t.Fatal(err) + } + + resp, err := ts.Client().Do(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + + body, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusBadRequest || !strings.Contains(string(body), "InvalidRequestContent") { + t.Errorf("status=%d body=%s, want 400 InvalidRequestContent", resp.StatusCode, body) + } + + if got := diskSize(t, client, "bad-body"); got != 64 { + t.Errorf("diskSizeGB=%d, want 64", got) + } +} diff --git a/server/azure/disks/update_test.go b/server/azure/disks/update_test.go index 2ead0099f..e46004084 100644 --- a/server/azure/disks/update_test.go +++ b/server/azure/disks/update_test.go @@ -18,6 +18,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph" "github.com/stackshy/cloudemu/v2" + "github.com/stackshy/cloudemu/v2/providers/azure/virtualmachines" azureserver "github.com/stackshy/cloudemu/v2/server/azure" ) @@ -26,6 +27,16 @@ var fastPoll = &runtime.PollUntilDoneOptions{Frequency: time.Millisecond} func newUpdateTestServer(t *testing.T) (*httptest.Server, *armcompute.DisksClient) { t.Helper() + ts, client, _ := newUpdateTestEnv(t) + + return ts, client +} + +// newUpdateTestEnv is newUpdateTestServer plus the Azure VM mock behind it, so +// a test can attach disks to VMs and change power state directly. +func newUpdateTestEnv(t *testing.T) (*httptest.Server, *armcompute.DisksClient, *virtualmachines.Mock) { + t.Helper() + cloudP := cloudemu.NewAzure() srv := azureserver.New(azureserver.Drivers{ VirtualMachines: cloudP.VirtualMachines, @@ -37,7 +48,7 @@ func newUpdateTestServer(t *testing.T) (*httptest.Server, *armcompute.DisksClien t.Cleanup(ts.Close) ensureRG(t, ts, "sub-1", "rg-1") - return ts, newDisksClient(t, ts) + return ts, newDisksClient(t, ts), cloudP.VirtualMachines } func createDiskForUpdate(t *testing.T, client *armcompute.DisksClient, name string, sku armcompute.DiskStorageAccountTypes) { @@ -210,7 +221,7 @@ func TestSDKDiskUpdateRejections(t *testing.T) { }{ {"shrink", "rej-disk", armcompute.DiskUpdate{ Properties: &armcompute.DiskUpdateProperties{DiskSizeGB: to.Ptr[int32](32)}, - }, http.StatusBadRequest, "BadRequest"}, + }, http.StatusBadRequest, "InvalidParameter"}, {"iops on Standard_LRS", "rej-disk", armcompute.DiskUpdate{ Properties: &armcompute.DiskUpdateProperties{DiskIOPSReadWrite: to.Ptr[int64](3000)}, }, http.StatusBadRequest, "InvalidParameter"}, @@ -248,9 +259,11 @@ func TestSDKDiskUpdateRejections(t *testing.T) { } } -// TestSDKDiskUpdateSKUDowngradeDropsPerf checks that moving a PremiumV2_LRS -// disk to a SKU without settable performance drops the provisioned IOPS/MBps. -func TestSDKDiskUpdateSKUDowngradeDropsPerf(t *testing.T) { +// TestSDKDiskUpdatePremiumV2CannotConvertBack checks that a PremiumV2_LRS disk +// keeps its provisioned performance and refuses a conversion to another type: +// Microsoft Learn (disks-convert-types) says a Premium SSD v2 cannot be +// switched directly to another disk type, only migrated via a snapshot. +func TestSDKDiskUpdatePremiumV2CannotConvertBack(t *testing.T) { _, client := newUpdateTestServer(t) createDiskForUpdate(t, client, "perf-disk", armcompute.DiskStorageAccountTypesPremiumV2LRS) @@ -261,15 +274,32 @@ func TestSDKDiskUpdateSKUDowngradeDropsPerf(t *testing.T) { t.Fatalf("set iops: %v", err) } - got, err := beginUpdate(t, client, "perf-disk", armcompute.DiskUpdate{ + _, err := beginUpdate(t, client, "perf-disk", armcompute.DiskUpdate{ SKU: &armcompute.DiskSKU{Name: to.Ptr(armcompute.DiskStorageAccountTypesPremiumLRS)}, }) + wantDiskErr(t, err, http.StatusBadRequest, "InvalidParameter") + + got, err := client.Get(context.Background(), "rg-1", "perf-disk", nil) if err != nil { - t.Fatalf("downgrade: %v", err) + t.Fatalf("Get: %v", err) + } + + if *got.SKU.Name != armcompute.DiskStorageAccountTypesPremiumV2LRS || *got.Properties.DiskIOPSReadWrite != 5000 { + t.Errorf("rejected conversion changed the disk: sku=%s iops=%d", *got.SKU.Name, *got.Properties.DiskIOPSReadWrite) + } +} + +// wantDiskErr asserts err is an ARM error with the given status and code. +func wantDiskErr(t *testing.T, err error, status int, code string) { + t.Helper() + + var respErr *azcore.ResponseError + if !errors.As(err, &respErr) { + t.Fatalf("err=%v, want *azcore.ResponseError %d %s", err, status, code) } - if got.Properties.DiskIOPSReadWrite != nil { - t.Errorf("diskIOPSReadWrite=%d after moving to Premium_LRS, want omitted", *got.Properties.DiskIOPSReadWrite) + if respErr.StatusCode != status || respErr.ErrorCode != code { + t.Errorf("got %d %q, want %d %q", respErr.StatusCode, respErr.ErrorCode, status, code) } } diff --git a/services/compute/driver/driver.go b/services/compute/driver/driver.go index 131fe6e3d..f8965949d 100644 --- a/services/compute/driver/driver.go +++ b/services/compute/driver/driver.go @@ -821,6 +821,55 @@ type AzureDiskUpdater interface { UpdateVolume(ctx context.Context, id string, cfg VolumeConfig) (*VolumeInfo, error) } +// AzureDiskPatch is a partial managed-disk update (ARM Disks Update, the +// PATCH of a DiskUpdate body). A nil field leaves the stored value unchanged. +type AzureDiskPatch struct { + // Size is the new diskSizeGB. Azure only grows a disk. + Size *int + // VolumeType is the new sku.name (storage account type). + VolumeType *string + // Tier is the new Premium SSD performance tier (properties.tier, e.g. P30). + Tier *string + // IOPS / Throughput are the provisioned diskIOPSReadWrite / + // diskMBpsReadWrite, settable only on UltraSSD_LRS and PremiumV2_LRS. + IOPS *int + Throughput *int + // Tags, when non-nil, replaces the tag set wholesale (ARM resource-level + // PATCH semantics). KeepTags names existing keys that survive the + // replacement: the wire layer's own bookkeeping tags. + Tags map[string]string + KeepTags []string +} + +// AzureDiskPatcher is an optional Azure-only capability for a partial managed +// disk update. The merge and every Azure rule (grow-only, SKU conversion, +// performance tier, attached-disk and active-SAS restrictions) run inside the +// provider's store lock, so a concurrent attach/detach or another update is +// never overwritten with a stale copy. Only the Azure VM mock implements it. +type AzureDiskPatcher interface { + // PatchVolume applies patch to the volume id and returns the stored + // result. NotFound when id is unknown, InvalidArgument for a request Azure + // rejects with 400, and an *AzureDiskError for one it rejects with a + // specific ARM error code. + PatchVolume(ctx context.Context, id string, patch AzureDiskPatch) (*VolumeInfo, error) +} + +// AzureDiskError is a managed-disk update refusal that real Azure reports with +// a specific ARM error code (e.g. OperationNotAllowed, +// ChangeDiskSizeWhileActiveSasNotAllowed). It unwraps to the canonical cloudemu +// error, so a caller that only checks the code (FailedPrecondition → 409, +// InvalidArgument → 400) still sees it; the Azure wire layer echoes Code. +type AzureDiskError struct { + Code string + Err error +} + +// Error implements the error interface. +func (e *AzureDiskError) Error() string { return e.Err.Error() } + +// Unwrap returns the canonical cloudemu error this one stands for. +func (e *AzureDiskError) Unwrap() error { return e.Err } + // AzureDiskDeleteOptioner is an optional Azure-only capability that records a // disk attachment's ARM deleteOption on the attached volume, mapped onto the // shared VolumeInfo.DeleteOnTermination (deleteOption "Delete" ⟷ true). The From f0c5f2d9ddc527519f7522a8b4e2b276f0ffe376 Mon Sep 17 00:00:00 2001 From: aryanmehrotra Date: Sun, 27 Sep 2026 16:28:36 +0530 Subject: [PATCH 4/5] fix(azure-functions): refuse App Service plan OS changes and validate SKU/capacity on update What was wrong: - PATCH {"properties":{"reserved":false}} on a Linux plan, or {"kind":"app"}, was accepted, and a re-PUT without reserved silently turned a Linux plan into a Windows one. TestPatchAppServicePlan pinned the flip as success. - PATCH accepted sku.capacity -5 and 0, an unknown sku.name (keeping the stale tier), moves between Consumption/Elastic Premium and dedicated tiers, and an explicit tier that did not match the name. What the real API does, and what this does now: - Real Azure answers 400 BadRequest "You cannot change the OS hosting your app at this time. Please recreate your app with the desired OS." (Azure/bicep#5724; azurerm treats os_type as ForceNew). PATCH and re-PUT now share checkPlanUpdate under the store lock, and refuse a change of reserved or of a Linux/Windows kind with that 400 BadRequest. - On a change: capacity must be >= 1 and <= the tier maximum (Free/Shared 1, Basic 3, Standard 10, Premium 30, Isolated 100), the SKU must be known, the hosting family (Consumption, Elastic Premium, Workflow Standard, dedicated) cannot change, and the tier must match the name. A create with a negative capacity is 400. - The re-PUT is now an atomic update-or-insert rather than a blind Set. - New provider tests use hand-rolled helpers instead of testify. --- providers/azure/functions/app_service_plan.go | 62 +++++- .../azure/functions/app_service_plan_rules.go | 141 +++++++++++++ .../azure/functions/app_service_plan_test.go | 36 ---- .../functions/app_service_plan_update_test.go | 198 ++++++++++++++++++ server/azure/functions/handler.go | 9 +- server/azure/functions/plan_patch.go | 17 +- server/azure/functions/plan_patch_sdk_test.go | 99 ++++++++- 7 files changed, 511 insertions(+), 51 deletions(-) create mode 100644 providers/azure/functions/app_service_plan_rules.go create mode 100644 providers/azure/functions/app_service_plan_update_test.go diff --git a/providers/azure/functions/app_service_plan.go b/providers/azure/functions/app_service_plan.go index 05c06e419..acd090529 100644 --- a/providers/azure/functions/app_service_plan.go +++ b/providers/azure/functions/app_service_plan.go @@ -145,13 +145,48 @@ func (m *Mock) CreateAppServicePlan(_ context.Context, p AppServicePlan) (*AppSe p.Capacity = 1 } - stored := p + if p.Capacity < 0 { + return nil, cerrors.Newf(cerrors.InvalidArgument, "Invalid sku.capacity %d: it must be at least 1.", p.Capacity) + } - m.plans.Set(planKey(p.Subscription, p.ResourceGroup, p.Name), &stored) + return m.putAppServicePlan(&p) +} - out := stored +// putAppServicePlan stores p, creating the plan or replacing an existing one. +// A replacement (an ARM re-PUT) is subject to the same checks as a PATCH, run +// under the store lock: in particular it cannot flip the plan's OS. +func (m *Mock) putAppServicePlan(p *AppServicePlan) (*AppServicePlan, error) { + key := planKey(p.Subscription, p.ResourceGroup, p.Name) - return &out, nil + for { + var putErr error + + replaced := m.plans.Update(key, func(cur *AppServicePlan) *AppServicePlan { + if err := checkPlanUpdate(cur, p); err != nil { + putErr = err + return cur + } + + stored := *p + + return &stored + }) + + if putErr != nil { + return nil, putErr + } + + if !replaced { + stored := *p + if !m.plans.SetIfAbsent(key, &stored) { + continue // created concurrently: replace it instead + } + } + + out := *p + + return &out, nil + } } // GetAppServicePlan returns one App Service plan scoped to the given @@ -249,16 +284,25 @@ type AppServicePlanPatch struct { // PatchAppServicePlan applies a partial update to one App Service plan scoped // to the given subscription and resource group, returning the stored result or -// NotFound. The read-modify-write runs under the store lock. A SKU name change -// without an explicit tier re-derives the tier, as a create does. +// NotFound. The read-modify-write and the checks in checkPlanUpdate (no OS +// change, a known SKU in the same plan family, a matching tier, a capacity in +// range) run under the store lock. A SKU name change re-derives the tier. func (m *Mock) PatchAppServicePlan( _ context.Context, subscription, resourceGroup, name string, patch AppServicePlanPatch, ) (*AppServicePlan, error) { var out AppServicePlan + var patchErr error + found := m.plans.Update(planKey(subscription, resourceGroup, name), func(p *AppServicePlan) *AppServicePlan { next := *p applyPlanPatch(&next, &patch) + + if err := checkPlanUpdate(p, &next); err != nil { + patchErr = err + return p + } + out = next return &next @@ -267,6 +311,10 @@ func (m *Mock) PatchAppServicePlan( return nil, cerrors.Newf(cerrors.NotFound, "app service plan %s not found", name) } + if patchErr != nil { + return nil, patchErr + } + return &out, nil } @@ -278,7 +326,7 @@ func applyPlanPatch(p *AppServicePlan, patch *AppServicePlanPatch) { setIf(&p.ZoneRedundant, patch.ZoneRedundant) setIf(&p.MaximumElasticWorkerCount, patch.MaximumElasticWorkerCount) - if patch.SKUName != nil && *patch.SKUName != "" && *patch.SKUName != p.SKUName { + if patch.SKUName != nil && *patch.SKUName != "" && !strings.EqualFold(*patch.SKUName, p.SKUName) { p.SKUName = *patch.SKUName p.SKUTier = deriveSKUTier(p.SKUName) } diff --git a/providers/azure/functions/app_service_plan_rules.go b/providers/azure/functions/app_service_plan_rules.go new file mode 100644 index 000000000..173629a63 --- /dev/null +++ b/providers/azure/functions/app_service_plan_rules.go @@ -0,0 +1,141 @@ +package functions + +import ( + "regexp" + "strings" + + cerrors "github.com/stackshy/cloudemu/v2/errors" +) + +// codeBadRequest is the ARM error code real Azure answers an OS change on an +// existing plan with (Azure/bicep#5724: 400 BadRequest, "You cannot change the +// OS hosting your app at this time. Please recreate your app with the desired +// OS."). +const codeBadRequest = "BadRequest" + +// PlanError is an App Service plan refusal that real Azure reports with a +// specific ARM error code. It unwraps to the canonical cloudemu error, so a +// caller that only checks the code still sees it; the wire layer echoes Code. +type PlanError struct { + Code string + Err error +} + +// Error implements the error interface. +func (e *PlanError) Error() string { return e.Err.Error() } + +// Unwrap returns the canonical cloudemu error this one stands for. +func (e *PlanError) Unwrap() error { return e.Err } + +// knownPlanSKU matches the App Service plan SKU names a plan can be moved to +// and whose tier deriveSKUTier maps correctly: Free, Shared, Basic, Standard, +// Premium (v1/v2/v3), Isolated (v1/v2), Consumption, Elastic Premium and +// Workflow Standard. +var knownPlanSKU = regexp.MustCompile( + `^(F1|D1|B[1-3]|S[1-3]|P[1-3]|P[1-3]V2|P[0-3]V3|I[1-3]|I[1-6]V2|Y1|EP[1-3]|WS[1-3])$`) + +// maxPlanCapacity is the largest sku.capacity (instance count) of a dedicated +// tier, from the App Service limits (Azure subscription and service limits): +// Free and Shared 1, Basic 3, Standard 10, Premium 30, Isolated 100. Other +// tiers are not capped here. +var maxPlanCapacity = map[string]int{ //nolint:gochecknoglobals // lookup table + tierFree: 1, tierShared: 1, tierBasic: 3, tierStandard: 10, + tierPremium: 30, tierPremiumV2: 30, tierPremiumV3: 30, + tierIsolated: 100, tierIsolatedV2: 100, +} + +// checkPlanUpdate validates replacing plan cur with next (a PATCH or a +// re-PUT). Only what changes is checked, so a plan created with an unusual +// SKU can still be re-PUT unchanged. +func checkPlanUpdate(cur, next *AppServicePlan) error { + if err := checkPlanOS(cur, next); err != nil { + return err + } + + skuChanged := !strings.EqualFold(cur.SKUName, next.SKUName) + + if skuChanged { + if err := checkPlanSKUMove(cur, next); err != nil { + return err + } + } + + if skuChanged || !strings.EqualFold(cur.SKUTier, next.SKUTier) { + if want := deriveSKUTier(next.SKUName); !strings.EqualFold(next.SKUTier, want) { + return cerrors.Newf(cerrors.InvalidArgument, + "The sku tier '%s' does not match sku name '%s' (tier '%s').", next.SKUTier, next.SKUName, want) + } + } + + if skuChanged || cur.Capacity != next.Capacity { + return checkPlanCapacity(next) + } + + return nil +} + +// checkPlanOS refuses a change of the plan's OS: reserved (true for Linux), +// or a kind that moves between Linux and Windows. An omitted kind (a re-PUT +// without one) is not a change of OS. +func checkPlanOS(cur, next *AppServicePlan) error { + if cur.Reserved == next.Reserved && (next.Kind == "" || linuxKind(cur.Kind) == linuxKind(next.Kind)) { + return nil + } + + return &PlanError{Code: codeBadRequest, Err: cerrors.New(cerrors.InvalidArgument, + "You cannot change the OS hosting your app at this time. Please recreate your app with the desired OS.")} +} + +// checkPlanSKUMove rejects an unknown SKU, and a move between the +// Consumption, Elastic Premium, Workflow Standard and dedicated families: a +// plan's hosting model is fixed at create. +func checkPlanSKUMove(cur, next *AppServicePlan) error { + if !knownPlanSKU.MatchString(strings.ToUpper(next.SKUName)) { + return cerrors.Newf(cerrors.InvalidArgument, "The pricing tier '%s' is not allowed for this App Service plan.", + next.SKUName) + } + + from, to := planFamily(cur.SKUTier), planFamily(deriveSKUTier(next.SKUName)) + if from != to { + return cerrors.Newf(cerrors.InvalidArgument, + "Cannot change the App Service plan from %s (%s) to %s (%s). Create a new plan instead.", + cur.SKUName, cur.SKUTier, next.SKUName, deriveSKUTier(next.SKUName)) + } + + return nil +} + +// checkPlanCapacity rejects an instance count below 1 or above the tier's +// maximum. +func checkPlanCapacity(p *AppServicePlan) error { + if p.Capacity < 1 { + return cerrors.Newf(cerrors.InvalidArgument, "Invalid sku.capacity %d: it must be at least 1.", p.Capacity) + } + + if limit, ok := maxPlanCapacity[p.SKUTier]; ok && p.Capacity > limit { + return cerrors.Newf(cerrors.InvalidArgument, + "Invalid sku.capacity %d: the %s tier allows at most %d instances.", p.Capacity, p.SKUTier, limit) + } + + return nil +} + +// planFamily groups plan tiers by hosting model. +func planFamily(tier string) string { + switch { + case strings.EqualFold(tier, tierDynamic): + return tierDynamic + case strings.EqualFold(tier, tierElasticPremium): + return tierElasticPremium + case strings.EqualFold(tier, tierWorkflowStandard): + return tierWorkflowStandard + default: + return "Dedicated" + } +} + +// linuxKind reports whether a plan kind names a Linux plan ("linux", +// "functionapp,linux", ...). +func linuxKind(kind string) bool { + return strings.Contains(strings.ToLower(kind), "linux") +} diff --git a/providers/azure/functions/app_service_plan_test.go b/providers/azure/functions/app_service_plan_test.go index 7c42a254f..bc2060349 100644 --- a/providers/azure/functions/app_service_plan_test.go +++ b/providers/azure/functions/app_service_plan_test.go @@ -211,39 +211,3 @@ func TestAppServicePlanResourceGroupIsolation(t *testing.T) { // Deleting an already-gone (or never-scoped) plan is NotFound. require.True(t, cerrors.IsNotFound(m.DeleteAppServicePlan(ctx, "sub1", "rgA", "default"))) } - -func TestPatchAppServicePlan(t *testing.T) { - ctx := context.Background() - m := newTestMock() - - _, err := m.CreateAppServicePlan(ctx, AppServicePlan{ - Name: "p", Subscription: "sub", ResourceGroup: "rg", SKUName: "S1", Tags: map[string]string{"env": "dev"}, - }) - require.NoError(t, err) - - reserved, workers, sku := true, 10, "P1v3" - - got, err := m.PatchAppServicePlan(ctx, "sub", "rg", "p", AppServicePlanPatch{ - SKUName: &sku, Reserved: &reserved, MaximumElasticWorkerCount: &workers, - }) - require.NoError(t, err) - - assert.Equal(t, "P1v3", got.SKUName) - assert.Equal(t, "PremiumV3", got.SKUTier, "a new SKU name re-derives the tier") - assert.True(t, got.Reserved) - assert.Equal(t, 10, got.MaximumElasticWorkerCount) - assert.Equal(t, map[string]string{"env": "dev"}, got.Tags, "omitted tags are kept") - assert.Equal(t, 1, got.Capacity, "omitted capacity is kept") - - got, err = m.PatchAppServicePlan(ctx, "sub", "rg", "p", AppServicePlanPatch{Tags: map[string]string{}}) - require.NoError(t, err) - assert.Empty(t, got.Tags, "an empty tags map clears the tags") - assert.True(t, got.Reserved, "an omitted property is kept") - - stored, err := m.GetAppServicePlan(ctx, "sub", "rg", "p") - require.NoError(t, err) - assert.Equal(t, got, stored) - - _, err = m.PatchAppServicePlan(ctx, "sub", "other-rg", "p", AppServicePlanPatch{Reserved: &reserved}) - assert.True(t, cerrors.IsNotFound(err), "a PATCH in another resource group is NotFound, got %v", err) -} diff --git a/providers/azure/functions/app_service_plan_update_test.go b/providers/azure/functions/app_service_plan_update_test.go new file mode 100644 index 000000000..0dd5cee56 --- /dev/null +++ b/providers/azure/functions/app_service_plan_update_test.go @@ -0,0 +1,198 @@ +package functions + +import ( + "context" + "errors" + "maps" + "testing" + + cerrors "github.com/stackshy/cloudemu/v2/errors" +) + +func planMustNotErr(t *testing.T, err error) { + t.Helper() + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func planWantEqual[T comparable](t *testing.T, what string, got, want T) { + t.Helper() + + if got != want { + t.Errorf("%s = %v, want %v", what, got, want) + } +} + +// planWantInvalid asserts err is an InvalidArgument (400) refusal. +func planWantInvalid(t *testing.T, what string, err error) { + t.Helper() + + if !cerrors.IsInvalidArgument(err) { + t.Errorf("%s: err = %v, want InvalidArgument", what, err) + } +} + +// planWantOSChangeRefused asserts err is the 400 BadRequest Azure answers an +// OS change with. +func planWantOSChangeRefused(t *testing.T, what string, err error) { + t.Helper() + + var pe *PlanError + if !errors.As(err, &pe) || pe.Code != codeBadRequest || !cerrors.IsInvalidArgument(err) { + t.Errorf("%s: err = %v, want a BadRequest PlanError", what, err) + } + + if err != nil && err.Error() == "" { + t.Errorf("%s: empty error text", what) + } +} + +func createPlanForUpdate(t *testing.T, m *Mock, p AppServicePlan) { + t.Helper() + + p.Subscription, p.ResourceGroup = "sub", "rg" + if p.Name == "" { + p.Name = "p" + } + + _, err := m.CreateAppServicePlan(context.Background(), p) + planMustNotErr(t, err) +} + +func patchPlan(m *Mock, patch AppServicePlanPatch) (*AppServicePlan, error) { + return m.PatchAppServicePlan(context.Background(), "sub", "rg", "p", patch) +} + +func TestPatchAppServicePlan(t *testing.T) { + m := newTestMock() + createPlanForUpdate(t, m, AppServicePlan{SKUName: "S1", Tags: map[string]string{"env": "dev"}}) + + workers, sku, perSite := 10, "P1v3", true + + got, err := patchPlan(m, AppServicePlanPatch{ + SKUName: &sku, PerSiteScaling: &perSite, MaximumElasticWorkerCount: &workers, + }) + planMustNotErr(t, err) + + planWantEqual(t, "sku name", got.SKUName, "P1v3") + planWantEqual(t, "sku tier (re-derived)", got.SKUTier, tierPremiumV3) + planWantEqual(t, "perSiteScaling", got.PerSiteScaling, true) + planWantEqual(t, "maximumElasticWorkerCount", got.MaximumElasticWorkerCount, 10) + planWantEqual(t, "capacity (omitted, kept)", got.Capacity, 1) + + if !maps.Equal(got.Tags, map[string]string{"env": "dev"}) { + t.Errorf("tags = %v, want the omitted tags kept", got.Tags) + } + + got, err = patchPlan(m, AppServicePlanPatch{Tags: map[string]string{}}) + planMustNotErr(t, err) + + if len(got.Tags) != 0 { + t.Errorf("tags = %v, want an empty tags map to clear them", got.Tags) + } + + planWantEqual(t, "perSiteScaling (omitted, kept)", got.PerSiteScaling, true) + + stored, err := m.GetAppServicePlan(context.Background(), "sub", "rg", "p") + planMustNotErr(t, err) + planWantEqual(t, "stored sku", stored.SKUName, got.SKUName) + + _, err = m.PatchAppServicePlan(context.Background(), "sub", "other-rg", "p", AppServicePlanPatch{PerSiteScaling: &perSite}) + if !cerrors.IsNotFound(err) { + t.Errorf("PATCH in another resource group: err = %v, want NotFound", err) + } +} + +// TestPatchAppServicePlanRefusesOSChange: real Azure refuses to change the OS +// of an existing plan (Azure/bicep#5724), by reserved or by kind. +func TestPatchAppServicePlanRefusesOSChange(t *testing.T) { + m := newTestMock() + createPlanForUpdate(t, m, AppServicePlan{SKUName: "P1v3", Kind: "linux", Reserved: true}) + + windows, appKind, linuxFunc := false, "app", "functionapp,linux" + + _, err := patchPlan(m, AppServicePlanPatch{Reserved: &windows}) + planWantOSChangeRefused(t, "reserved:false on a Linux plan", err) + + _, err = patchPlan(m, AppServicePlanPatch{Kind: &appKind}) + planWantOSChangeRefused(t, "kind:app on a Linux plan", err) + + got, err := patchPlan(m, AppServicePlanPatch{Kind: &linuxFunc}) + planMustNotErr(t, err) + planWantEqual(t, "kind (same OS)", got.Kind, "functionapp,linux") + planWantEqual(t, "reserved", got.Reserved, true) +} + +// TestPutAppServicePlanRefusesOSChange: a re-PUT that omits reserved on a +// Linux plan used to turn it into a Windows plan silently. +func TestPutAppServicePlanRefusesOSChange(t *testing.T) { + m := newTestMock() + createPlanForUpdate(t, m, AppServicePlan{SKUName: "P1v3", Kind: "linux", Reserved: true}) + + _, err := m.CreateAppServicePlan(context.Background(), AppServicePlan{ + Name: "p", Subscription: "sub", ResourceGroup: "rg", SKUName: "P1v3", Kind: "linux", + }) + planWantOSChangeRefused(t, "re-PUT without reserved", err) + + stored, err := m.GetAppServicePlan(context.Background(), "sub", "rg", "p") + planMustNotErr(t, err) + planWantEqual(t, "reserved after refused re-PUT", stored.Reserved, true) + + got, err := m.CreateAppServicePlan(context.Background(), AppServicePlan{ + Name: "p", Subscription: "sub", ResourceGroup: "rg", SKUName: "P2v3", Kind: "linux", Reserved: true, Capacity: 3, + }) + planMustNotErr(t, err) + planWantEqual(t, "re-PUT sku", got.SKUName, "P2v3") + planWantEqual(t, "re-PUT capacity", got.Capacity, 3) +} + +// TestPatchAppServicePlanSKUAndCapacityRules covers the SKU and capacity +// checks: capacity at least 1 and at most the tier's instance limit, a known +// SKU, no move between hosting families, and a tier that matches the name. +func TestPatchAppServicePlanSKUAndCapacityRules(t *testing.T) { + m := newTestMock() + createPlanForUpdate(t, m, AppServicePlan{SKUName: "B1"}) + createPlanForUpdate(t, m, AppServicePlan{Name: "consumption", SKUName: "Y1"}) + + ptr := func(s string) *string { return &s } + n := func(i int) *int { return &i } + + for _, tc := range []struct { + what string + patch AppServicePlanPatch + }{ + {"capacity 0", AppServicePlanPatch{Capacity: n(0)}}, + {"capacity -5", AppServicePlanPatch{Capacity: n(-5)}}, + {"capacity above the Basic limit of 3", AppServicePlanPatch{Capacity: n(4)}}, + {"unknown sku", AppServicePlanPatch{SKUName: ptr("ZZ9")}}, + {"dedicated to Elastic Premium", AppServicePlanPatch{SKUName: ptr("EP1")}}, + {"dedicated to Consumption", AppServicePlanPatch{SKUName: ptr("Y1")}}, + {"mismatched explicit tier", AppServicePlanPatch{SKUName: ptr("S1"), SKUTier: ptr("Premium")}}, + {"tier alone", AppServicePlanPatch{SKUTier: ptr("Standard")}}, + {"sku move above the new tier limit", AppServicePlanPatch{SKUName: ptr("S1"), Capacity: n(11)}}, + } { + _, err := patchPlan(m, tc.patch) + planWantInvalid(t, tc.what, err) + } + + stored, err := m.GetAppServicePlan(context.Background(), "sub", "rg", "p") + planMustNotErr(t, err) + planWantEqual(t, "sku after refusals", stored.SKUName, "B1") + planWantEqual(t, "capacity after refusals", stored.Capacity, 1) + + got, err := patchPlan(m, AppServicePlanPatch{SKUName: ptr("S1"), SKUTier: ptr("Standard"), Capacity: n(10)}) + planMustNotErr(t, err) + planWantEqual(t, "tier", got.SKUTier, tierStandard) + planWantEqual(t, "capacity", got.Capacity, 10) + + _, err = m.PatchAppServicePlan(context.Background(), "sub", "rg", "consumption", + AppServicePlanPatch{SKUName: ptr("S1")}) + planWantInvalid(t, "Consumption to dedicated", err) + + _, err = m.CreateAppServicePlan(context.Background(), AppServicePlan{ + Name: "neg", Subscription: "sub", ResourceGroup: "rg", SKUName: "S1", Capacity: -1, + }) + planWantInvalid(t, "create with negative capacity", err) +} diff --git a/server/azure/functions/handler.go b/server/azure/functions/handler.go index d11b07ec4..98a20d28a 100644 --- a/server/azure/functions/handler.go +++ b/server/azure/functions/handler.go @@ -38,6 +38,9 @@ const ( resourceType = "sites" serverFarmsType = "serverfarms" + // provisioningSucceeded is the ARM provisioningState of a settled resource. + provisioningSucceeded = "Succeeded" + functionAppKind = "functionapp" defaultLocation = "eastus" invokePathPrefix = "/api/" @@ -745,7 +748,7 @@ func createPlan(w http.ResponseWriter, r *http.Request, rp azurearm.ResourcePath MaximumElasticWorkerCount: intOr(props.MaximumElasticWorkerCount), }) if err != nil { - azurearm.WriteCErr(w, err) + writePlanErr(w, err) return } @@ -850,7 +853,7 @@ func toServerFarmResource(rp azurearm.ResourcePath, plan *azfunctions.AppService Capacity: plan.Capacity, }, Properties: serverFarmProperties{ - ProvisioningState: "Succeeded", + ProvisioningState: provisioningSucceeded, Status: "Ready", Reserved: plan.Reserved, PerSiteScaling: plan.PerSiteScaling, @@ -931,7 +934,7 @@ func upsertFunction(r *http.Request, fn sdrv.Serverless, cfg sdrv.FunctionConfig //nolint:gocritic // rp is request-scoped. func toSiteResource(rp azurearm.ResourcePath, info *sdrv.FunctionInfo, meta *azfunctions.SiteMeta) siteResource { location := defaultLocation - provisioningState := "Succeeded" + provisioningState := provisioningSucceeded kind := functionAppKind state := siteStateRunning diff --git a/server/azure/functions/plan_patch.go b/server/azure/functions/plan_patch.go index f86e412fc..6ca3e6701 100644 --- a/server/azure/functions/plan_patch.go +++ b/server/azure/functions/plan_patch.go @@ -2,9 +2,11 @@ package functions import ( "encoding/json" + "errors" "io" "net/http" + cerrors "github.com/stackshy/cloudemu/v2/errors" azfunctions "github.com/stackshy/cloudemu/v2/providers/azure/functions" "github.com/stackshy/cloudemu/v2/server/wire/azurearm" ) @@ -33,7 +35,7 @@ func patchPlan(w http.ResponseWriter, r *http.Request, rp azurearm.ResourcePath, plan, err := store.PatchAppServicePlan(r.Context(), rp.Subscription, rp.ResourceGroup, rp.ResourceName, toPlanPatch(&req)) if err != nil { - azurearm.WriteCErr(w, err) + writePlanErr(w, err) return } @@ -71,3 +73,16 @@ func intOr(p *int) int { return *p } + +// writePlanErr writes a plan PUT/PATCH error. A refusal the provider tagged +// with an ARM error code (the 400 BadRequest for an OS change) is echoed with +// that code; anything else maps as usual. +func writePlanErr(w http.ResponseWriter, err error) { + var pe *azfunctions.PlanError + if !errors.As(err, &pe) { + azurearm.WriteCErr(w, err) + return + } + + azurearm.WriteError(w, http.StatusBadRequest, pe.Code, cerrors.Message(err)) +} diff --git a/server/azure/functions/plan_patch_sdk_test.go b/server/azure/functions/plan_patch_sdk_test.go index 7a20768a7..53f171da3 100644 --- a/server/azure/functions/plan_patch_sdk_test.go +++ b/server/azure/functions/plan_patch_sdk_test.go @@ -51,7 +51,7 @@ func newPlanPatchServer(t *testing.T) (*httptest.Server, *armappservice.PlansCli // TestSDKAzureAppServicePlanUpdate drives armappservice PlansClient.Update // (PATCH): the changed properties land, and everything the body omitted -// (SKU, tags, kind) is kept. +// (SKU, tags, kind, reserved) is kept. func TestSDKAzureAppServicePlanUpdate(t *testing.T) { _, client := newPlanPatchServer(t) ctx := context.Background() @@ -59,7 +59,7 @@ func TestSDKAzureAppServicePlanUpdate(t *testing.T) { resp, err := client.Update(ctx, rgName, "patch-plan", armappservice.PlanPatchResource{ Properties: &armappservice.PlanPatchResourceProperties{ MaximumElasticWorkerCount: to.Ptr[int32](20), - Reserved: to.Ptr(true), + PerSiteScaling: to.Ptr(true), ZoneRedundant: to.Ptr(true), }, }, nil) @@ -90,8 +90,12 @@ func assertPatchedPlan(t *testing.T, stage string, p armappservice.Plan) { t.Errorf("%s: maximumElasticWorkerCount=%v want 20", stage, pp.MaximumElasticWorkerCount) } - if pp.Reserved == nil || !*pp.Reserved { - t.Errorf("%s: reserved=%v want true", stage, pp.Reserved) + if pp.PerSiteScaling == nil || !*pp.PerSiteScaling { + t.Errorf("%s: perSiteScaling=%v want true", stage, pp.PerSiteScaling) + } + + if pp.Reserved == nil || *pp.Reserved { + t.Errorf("%s: reserved=%v want false (unchanged)", stage, pp.Reserved) } if pp.ZoneRedundant == nil || !*pp.ZoneRedundant { @@ -194,3 +198,90 @@ func rawPlanPatch(t *testing.T, ts *httptest.Server, name, body string) (int, [] return resp.StatusCode, []byte(buf.String()) } + +// TestSDKAzureAppServicePlanOSChangeRefused checks both write paths refuse to +// change an existing plan's OS with the 400 BadRequest real Azure returns +// ("You cannot change the OS hosting your app at this time", Azure/bicep#5724): +// a PATCH of reserved or kind, and a re-PUT that omits reserved. +func TestSDKAzureAppServicePlanOSChangeRefused(t *testing.T) { + ts, client := newPlanPatchServer(t) + ctx := context.Background() + + poller, err := client.BeginCreateOrUpdate(ctx, rgName, "linux-plan", armappservice.Plan{ + Kind: to.Ptr("linux"), + Location: to.Ptr("eastus"), + SKU: &armappservice.SKUDescription{Name: to.Ptr("P1v3")}, + Properties: &armappservice.PlanProperties{Reserved: to.Ptr(true)}, + }, nil) + if err != nil { + t.Fatalf("create linux plan: %v", err) + } + + if _, err := poller.PollUntilDone(ctx, &runtimePollerOptions); err != nil { + t.Fatalf("PollUntilDone: %v", err) + } + + _, err = client.Update(ctx, rgName, "linux-plan", armappservice.PlanPatchResource{ + Properties: &armappservice.PlanPatchResourceProperties{Reserved: to.Ptr(false)}, + }, nil) + wantPlanErr(t, "PATCH reserved:false", err, "BadRequest") + + status, out := rawPlanPatch(t, ts, "linux-plan", `{"kind":"app"}`) + if status != http.StatusBadRequest || !strings.Contains(string(out), `"BadRequest"`) { + t.Errorf("PATCH kind:app: status=%d body=%s, want 400 BadRequest", status, out) + } + + _, err = client.BeginCreateOrUpdate(ctx, rgName, "linux-plan", armappservice.Plan{ + Kind: to.Ptr("linux"), + Location: to.Ptr("eastus"), + SKU: &armappservice.SKUDescription{Name: to.Ptr("P1v3")}, + }, nil) + wantPlanErr(t, "re-PUT without reserved", err, "BadRequest") + + got, err := client.Get(ctx, rgName, "linux-plan", nil) + if err != nil { + t.Fatalf("Get: %v", err) + } + + if got.Properties.Reserved == nil || !*got.Properties.Reserved || *got.Kind != "linux" { + t.Errorf("refused OS change mutated the plan: kind=%s reserved=%v", *got.Kind, got.Properties.Reserved) + } +} + +// TestAzureAppServicePlanPatchSKUValidation checks a PATCH with a capacity +// below 1, above the tier maximum, an unknown SKU or a move out of the Elastic +// Premium family is a 400, and leaves the plan unchanged. +func TestAzureAppServicePlanPatchSKUValidation(t *testing.T) { + ts, client := newPlanPatchServer(t) + + for _, body := range []string{ + `{"sku":{"capacity":-5}}`, + `{"sku":{"capacity":0}}`, + `{"sku":{"name":"ZZ9"}}`, + `{"sku":{"name":"S1"}}`, + `{"sku":{"name":"EP2","tier":"Standard"}}`, + } { + status, out := rawPlanPatch(t, ts, "patch-plan", body) + if status != http.StatusBadRequest || !strings.Contains(string(out), `"InvalidParameter"`) { + t.Errorf("PATCH %s: status=%d body=%s, want 400 InvalidParameter", body, status, out) + } + } + + got, err := client.Get(context.Background(), rgName, "patch-plan", nil) + if err != nil { + t.Fatalf("Get: %v", err) + } + + if *got.SKU.Name != "EP1" || *got.SKU.Tier != "ElasticPremium" || *got.SKU.Capacity != 1 { + t.Errorf("sku=%s/%s/%d want EP1/ElasticPremium/1 (unchanged)", *got.SKU.Name, *got.SKU.Tier, *got.SKU.Capacity) + } +} + +func wantPlanErr(t *testing.T, what string, err error, code string) { + t.Helper() + + var respErr *azcore.ResponseError + if !errors.As(err, &respErr) || respErr.StatusCode != http.StatusBadRequest || respErr.ErrorCode != code { + t.Errorf("%s: err=%v, want 400 %s", what, err, code) + } +} From 6710f0e789dd28bdca04ed528c90951e85dd9da9 Mon Sep 17 00:00:00 2001 From: aryanmehrotra Date: Sun, 27 Sep 2026 16:34:13 +0530 Subject: [PATCH 5/5] docs(coverage): regenerate for the AzureDiskPatcher capability go generate picks up the new optional driver interface. The azure/disks wire-op list keeps its existing line and adds Update on its own line with a goconst nolint: wireops.go is a table of SDK operation names and already carries 11 goconst hits on development (min-occurrences is 2), so any first occurrence added to it trips the linter. --- docs/coverage/azure/virtualmachines.md | 8 ++++++++ docs/coverage/coverage.json | 10 ++++++++++ internal/coveragegen/wireops.go | 3 ++- 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/coverage/azure/virtualmachines.md b/docs/coverage/azure/virtualmachines.md index c7ffd2e07..74fc999ff 100644 --- a/docs/coverage/azure/virtualmachines.md +++ b/docs/coverage/azure/virtualmachines.md @@ -66,6 +66,14 @@ AzureDiskDeleteOptioner is an optional Azure-only capability that records a | --- | --- | | `SetDiskDeleteOnTermination` | SetDiskDeleteOnTermination records whether the volume is deleted (true) or | +### AzureDiskPatcher + +AzureDiskPatcher is an optional Azure-only capability for a partial managed + +| Operation | Description | +| --- | --- | +| `PatchVolume` | PatchVolume applies patch to the volume id and returns the stored | + ### AzureDiskUpdater AzureDiskUpdater is an optional Azure-only capability for an in-place managed diff --git a/docs/coverage/coverage.json b/docs/coverage/coverage.json index f3b45ca50..604715f78 100644 --- a/docs/coverage/coverage.json +++ b/docs/coverage/coverage.json @@ -4018,6 +4018,16 @@ } ] }, + { + "name": "AzureDiskPatcher", + "doc": "AzureDiskPatcher is an optional Azure-only capability for a partial managed", + "operations": [ + { + "name": "PatchVolume", + "doc": "PatchVolume applies patch to the volume id and returns the stored" + } + ] + }, { "name": "AzureDiskUpdater", "doc": "AzureDiskUpdater is an optional Azure-only capability for an in-place managed", diff --git a/internal/coveragegen/wireops.go b/internal/coveragegen/wireops.go index 849c2b4ad..554d3cf8e 100644 --- a/internal/coveragegen/wireops.go +++ b/internal/coveragegen/wireops.go @@ -63,7 +63,8 @@ var nativeWireOperations = map[string][]string{ //nolint:gochecknoglobals // gen "GetMetadataEndpoints", "GetToken", }, "azure/disks": { - "CreateOrUpdate", "Delete", "Get", "GrantAccess", "List", "ListByResourceGroup", "RevokeAccess", "Update", + "CreateOrUpdate", "Delete", "Get", "GrantAccess", "List", "ListByResourceGroup", "RevokeAccess", + "Update", //nolint:goconst // SDK operation names are table data; the file already repeats them (pre-existing goconst hits) }, "azure/snapshots": { "CreateOrUpdate", "Delete", "Get", "List", "ListByResourceGroup",