feat(resourcediscovery): Azure Resource Graph cost-field projection across all supported types (#323) - #327
Conversation
…oss all supported types Emit the properties/sku a real armresourcegraph discoverer prices on — for VM/disk/VMSS, public IP, VNet/subnet, storage account, Cosmos DB, SQL MI/database/server + MySQL/Postgres flex, AKS cluster/agent-pool, Databricks, and App Service plans. Adds SKUTier/SKUCapacity slots to the generic attribute set and fixes two type-map bugs (NIC, public IP emitted wrong Azure types). New driver capabilities: Databases (Azure SQL logical DBs), ScaleSets (VMSS), AppServicePlans (serverfarms), and the optional BucketAttributes/TableAttributes capabilities that enrich storage accounts and Cosmos DB. All additive and nil-safe; AWS/GCP walks are unaffected. Backed by real-SDK ARG round-trip tests plus provider-unit tests. Extends #315/#316; addresses #323.
Merge the four parallel-authored arg_cost_*_test.go files into a single arg_cost_fields_test.go with one shared helper set; assertions unchanged.
…te->discover Extend the Azure SDK-compat handlers to parse the cost fields off the ARM create body onto the driver config (they were accepted-and-dropped), and add create routes for resource types that had none: - virtualmachines: VM priority/licenseType/osType/zones; new VMSS handler - azuresql: MI storageAccountType; SQL Database create/get/list -> Databases - aks: cluster sku.tier; agent-pool scaleSetPriority - functions: new serverfarms (App Service plan) handler - storageaccount, cosmosaccount: new Microsoft.Storage/DocumentDB account handlers Real-SDK create->get round-trip tests per package.
Complete the Microsoft.Storage/storageAccounts and Microsoft.DocumentDB/ databaseAccounts create+get routes so their cost fields (sku/kind/accessTier, capabilities/offerType/enableFreeTier) survive create->discover.
…bscription - disks: parse diskIOPSReadWrite/diskMBpsReadWrite/tier onto VolumeConfig; echo sku.tier. - resourcegraph: stamp the configured subscriptionId on every row instead of parsing inconsistent placeholders from each mock's ARN, so subscription-scoped ARG queries return the emulator's resources.
Wire Microsoft.Network/publicIPAddresses to AllocateAddress with sku + allocation method, so public IPs are creatable over the real SDK and their cost fields surface in Resource Graph.
… match real ARG VMs projected osType as a flat properties.osType; real Azure Resource Graph (and the VMSS projection) nest it under properties.storageProfile.osDisk.osType, so a real discoverer reading that path found it empty. Only Azure VMs set OSType (AWS/GCP mocks leave it blank), so no cross-provider shape leaks.
…ons (unused receiver)
thzgajendra
left a comment
thzgajendra
left a comment
There was a problem hiding this comment.
Review — ARG cost-field projection (in-depth, multi-dimension pass)
Reviewed across correctness, nil-safety, type design, tests, comments/docs, simplification, and architecture-consistency, plus a manual lead pass on the ARG core.
Overall: strong and well-architected. The KQL parser + generic resourceToWire rendering (no per-type branching) are clean, the change is genuinely additive, and both advertised bug fixes are real — the base branch fell through to invalid networking/networkinterface / networking/elasticip type strings; the new portableToAzureTypeMap entries are correct Azure ARG types. The three claimed patterns (NetworkInterfaces optional-capability, relational-DB/K8s walker, and the NodeGroups mirror rule) are applied consistently, and AWS/GCP output is verifiably unaffected (NodeGroupsFromNames shim + zero-value additive fields).
No confirmed correctness blocker. Findings by severity:
Should address
- Test gap (silent-regression risk):
microsoft.sql/servers→properties.versionis projected (providers/azure/azure.go:254) but never asserted at the ARG level —TestARGCostFields_SQLDatabaseonly queries the.../databasesrow. Add amicrosoft.sql/serversround-trip assertingversion. It's also missing from thedocs/services.md§19 table. - Masked test assertion (
arg_cost_fields_test.go:345) — see inline; passes even if the projection breaks. - Doc error: §19 lists VM
osTypeatproperties.osType, but the code nests it atproperties.storageProfile.osDisk.osType(which matches real ARG). Fix the documented path. §19 also under-documents the SQL-MI row (missingsku.name,vCores,tier,licenseType).
Consider
nonEmptyPropsbool / nested-map handling — see inline onazure.go.- Swallowed
BucketAttributes/TableAttributeserrors — see inline onwalkers.go:316. flexTierempty-tier for unlisted families — see inline.- App Service plans bucketed under
ServiceServerless— see inline. - Postgres-flex tests assert fewer fields than MySQL-flex (missing
sku.tier,version) — worth symmetry.
Nits
- Stringly-typed
OSType/Prioritydriver configs could be named enum types with consts (small closed domains). - No compile-time
var _ BucketAttributes = (*Mock)(nil)assertions for the optional capabilities — a typo'd method signature would fail the type-assert silently with no build error.
Nice PR — the field-flow (config → walker → adapter → wire → ARG projection) connects end-to-end for the types I traced, and the units/JSON paths are correct throughout (GB/IOPS/MBps, sku vs properties).
| SKU: db.SKUName, | ||
| SKUTier: db.SKUTier, | ||
| Properties: nonEmptyProps(map[string]any{ | ||
| "zoneRedundant": db.ZoneRedundant, |
There was a problem hiding this comment.
nonEmptyProps (line 442) only strips empty string and int — bool and nested map[string]any fall through its type switch untouched. So zoneRedundant: false and the currentSku map are emitted for every SQL database, including ones created without zone-redundancy or an explicit SKU.
Azure SQL genuinely surfaces properties.zoneRedundant as a bool, so emitting false may actually be faithful to what the real armresourcegraph discoverer returns — in which case the inconsistency is the other way (pruning empty strings/ints that real ARG might include). Either way the helper's doc says "drops zero-valued entries" but its behavior only does so for two of the types it receives.
Worth a quick check against the live discoverer (fidelity is the point of this PR): if defaults should be omitted, add case bool { if !val { delete } } + empty-map handling; if ARG-faithfulness is intended, the string/int pruning of real properties is what needs a second look.
| // Optional capability: providers whose buckets carry storage-account | ||
| // attributes (Azure) project SKU/kind/access-tier for cost discovery. | ||
| if attrer, ok := e.drivers.Storage.(storagedriver.BucketAttributes); ok { | ||
| if a, aErr := attrer.BucketAttributes(ctx, b.Name); aErr == nil { |
There was a problem hiding this comment.
Swallowed error: if BucketAttributes ever returns a non-nil aErr, the sku/kind/accessTier are silently dropped and the storage account is emitted with no priceable SKU — exactly the "cost field silently absent" failure this PR is closing. Today's impl can't error, so it's latent, but the invariant is load-bearing and undiscoverable if a future/alternate impl breaks it. Suggest logging aErr rather than discarding it. Same pattern at line 362 for TableAttributes.
| sku := rowSKU(t, row) | ||
| assert.Equal(t, "premium", sku["name"]) | ||
|
|
||
| if tier, ok := sku["tier"]; ok { |
There was a problem hiding this comment.
This assertion is masked: if Databricks sku.tier projection regresses to absent, ok is false and the inner assert is skipped — the test still passes green. Since the input sets SKUTier: "premium", make it unconditional so a broken projection actually fails:
assert.Equal(t, "premium", rowSKU(t, row)["tier"])| // GeneralPurpose / MemoryOptimized) from the SKU name, which encodes it as a | ||
| // prefix in both the current ("Standard_B1ms") and legacy ("B_Gen5_1") naming. | ||
| // Empty when the name doesn't match a known family. | ||
| func flexTier(skuName string) string { |
There was a problem hiding this comment.
flexTier returns "" for SKU families outside the listed prefixes (e.g. Standard_F* compute-optimized), so sku.tier is silently dropped for those servers and no test catches it. Reasonable as best-effort (a pricing consumer can fall back to sku.name), but worth a short comment noting the mapping is known-incomplete, or a sensible default, so a maintainer doesn't read the empty tier as a bug later.
| } | ||
|
|
||
| r := Resource{ | ||
| Provider: e.provider, Service: ServiceServerless, Type: TypeAppServicePlan, |
There was a problem hiding this comment.
App Service plans are bucketed under ServiceServerless (same discriminator as Functions). ARG type-queries still resolve correctly because Type (TypeAppServicePlan → microsoft.web/serverfarms) disambiguates, so this isn't a functional bug. But serverfarms aren't serverless — following the ServiceRelationalDB precedent, a dedicated service constant for net-new families would be semantically cleaner if anything ever groups by Service.
- nonEmptyProps prunes empty/nested-empty maps and nil (keeps faithful bools); doc aligned to behavior. - Propagate BucketAttributes/TableAttributes errors instead of swallowing them. - Dedicated ServiceAppService discriminator for serverfarms (wire type unchanged). - Compile-time capability assertions for the optional discovery interfaces. - flexTier known-incomplete comment; unmask Databricks sku.tier assertion; add SQL-server version + Postgres-flex symmetry tests; §19 doc corrections.
|
Thanks for the thorough pass — all findings addressed in Should address
Consider
Nits
|
thzgajendra
left a comment
thzgajendra
left a comment
There was a problem hiding this comment.
Re-review — fixes in add1050fd verified ✅
Re-reviewed the fix(resourcediscovery): address ARG cost-field review findings commit against my prior review. Every finding addressed correctly. Built + go vet + tested the affected packages green locally (resourcegraph, resourcediscovery, blobstorage, cosmosdb).
| Finding | Status |
|---|---|
Swallowed BucketAttributes/TableAttributes errors (walkers.go 316/362) |
✅ Now propagate via fmt.Errorf, with a "load-bearing error" comment |
Masked Databricks sku.tier assertion |
✅ Now unconditional |
microsoft.sql/servers → version untested |
✅ New TestARGCostFields_SQLServer + §19 doc row |
| Postgres-flex test asymmetry | ✅ New TestARGCostFields_PostgresFlex (sku.tier/version/storage/HA) |
Doc: VM osType path + SQL-MI under-documented |
✅ §19 corrected; MI sku.name/vCores/tier/licenseType now also asserted in the test |
nonEmptyProps bool / nested-map |
✅ Recursive-map + nil pruning added; false bools intentionally preserved (faithful to real ARG) and now documented — the right call |
App Service plans bucketed under ServiceServerless |
✅ Dedicated ServiceAppService / portableAppService applied across kql.go + handler.go + walkers.go |
flexTier empty tier for unlisted families (nit) |
✅ Documented as intentional best-effort |
| Compile-time capability assertions (nit) | ✅ var _ BucketAttributes/TableAttributes = (*Mock)(nil) added to both mocks |
The one change with regression risk — renaming the App Service plan service discriminator — is sound: walker dispatch is driver-gated with post-filter q.matches, the query-parse and emit sides both use appservice consistently, and TestARGCostFields_AppServicePlan (microsoft.web/serverfarms round-trip) passes.
No new issues found. Clean, thorough turnaround — LGTM from my side (leaving the merge call to you).
2 participants
Summary
Extends the Azure Resource Graph emulation (built in #315/#316) to project the
propertiesandskucost fields a realarmresourcegraphdiscoverer prices on, across every supported Azure resource type. Addresses #323.A downstream project drives its real Azure discoverer against cloudemu's Resource Graph endpoint to E2E-verify cost calculation. Previously only disk size, VM SKU, and SQL-MI storage flowed through. This PR closes the field gap so disk IOPS/throughput, VM/VMSS Spot + Hybrid Benefit, SQL zone-redundancy, AKS Spot node pools, storage redundancy, Cosmos serverless, App Service plan tier, and more all round-trip over the real SDK.
What's projected now
microsoft.compute/virtualmachinesproperties.priority(Spot),licenseType,osType,sku.name,zonesmicrosoft.compute/disksproperties.diskIOPSReadWrite,diskMBpsReadWrite,diskSizeGB,tier,sku.name/sku.tiermicrosoft.compute/virtualmachinescalesets(new)sku.name/sku.capacity, nestedproperties.virtualMachineProfile.{priority,licenseType,storageProfile.osDisk.osType}microsoft.network/publicipaddressessku.name(Basic/Standard),properties.publicIPAllocationMethodmicrosoft.network/virtualnetworks/subnetsproperties.addressSpace.addressPrefixes/addressPrefixmicrosoft.storage/storageaccountssku.name(redundancy),kind,properties.accessTiermicrosoft.documentdb/databaseaccountskind,properties.databaseAccountOfferType,capabilities(serverless),enableFreeTiermicrosoft.sql/managedinstancesproperties.storageSizeInGB,storageAccountTypemicrosoft.sql/servers/databases(new)sku.name,properties.currentSku,zoneRedundantmicrosoft.dbformysql/dbforpostgresql/flexibleserverssku.name/sku.tier,properties.version, nestedstorage.storageSizeGB+highAvailability.modemicrosoft.sql/serversproperties.versionmicrosoft.containerservice/managedclusterssku.tier,properties.powerState.code,kubernetesVersion.../managedclusters/agentpoolssku.name(vmSize),properties.scaleSetPriority(Spot),count,mode/osTypemicrosoft.databricks/workspacessku.name/sku.tier,properties.workspaceId/provisioningStatemicrosoft.web/serverfarms(new)sku.name/sku.tier/sku.capacity,kindHow it's built (per the existing architecture)
SKUTier/SKUCapacityslots to the genericAttributes/Resourceset soresourceToWirecan rendersku.tier/sku.capacity; fixed two type-map correctness bugs (NetworkInterface and ElasticIP emitted the wrong Azure type strings).VolumeConfig.IOPS/Throughput/Tier,InstanceConfig.OSType/Priority/LicenseType/Zones,ManagedInstanceConfig.StorageAccountType,ElasticIPConfig.SKU/AllocationMethod, AKSTier/ScaleSetPriority), and the walkers/adapters project them.NetworkInterfacespattern) for enrichment that doesn't fit the portable config:BucketAttributes(storage account sku/kind/accessTier) andTableAttributes(Cosmos account kind/offer/capabilities/free-tier). S3/GCS/DynamoDB don't implement them and contribute nothing.ScaleSets(VMSS),AppServicePlans(serverfarms), and theDatabasesCRUD capability implemented on Azure SQL for logical databases.NodeGroups []stringcluster projection became[]DiscoveredNodeGroup{Name, Attrs}so agent pools can carry per-pool attributes; the EKS/GKE/AKS adapters were all updated (mirror rule).All additions are additive and nil-safe — AWS/GCP engines skip the Azure-only walkers, and unimplemented optional capabilities are silently absent.
Tests
server/azure/resourcegraph/arg_cost_*_test.go) drive the livearmresourcegraphclient against an httptest server and assert every projectedproperties/skufield per type.go build ./...,go vet ./...,gofmt, fullgo test ./...(225 packages) andgolangci-linton the changed files are all green.Docs
docs/services.md§19 gains the full ARG cost-field projection table and a note on the capability patterns used.