diff --git a/provider/pkg/provider/provider.go b/provider/pkg/provider/provider.go index 58fe4ee33503..2e9ebb5147c4 100644 --- a/provider/pkg/provider/provider.go +++ b/provider/pkg/provider/provider.go @@ -1172,50 +1172,6 @@ Verbose logs contain more information, see https://www.pulumi.com/docs/support/t return response } -// Properties pointing to sub-resources that can be maintained as separate resources might not be -// present in the inputs because the user wants to manage them as standalone resources. However, -// such a property might be required by Azure even if it's not annotated as such in the spec, e.g., -// Key Vault's accessPolicies. Therefore, we set these properties to their default value here, -// an empty array. For more details, see section "Sub-resources" in CONTRIBUTING.md. -// -// During create, no sub-resources can exist yet so there's no danger of overwriting existing values. -// -// The `input` param is used to determine the unset sub-resource properties. They are then reset in -// the `output` parameter which is modified in-place. -// -// Implementation note: we should make it possible to write custom resources that call code from -// the default implementation as needed. This would allow us to cleanly implement special logic -// like for Key Vault into custom resources without duplicating much code. In the Key Vault case, -// the custom Read() would look like -// -// provider.azureCanCreate(ctx, id, &res) -// setUnsetSubresourcePropertiesToDefaults(res, bodyParams) // custom -// k.azureCreateOrUpdate -// ... -func (k *azureNativeProvider) setUnsetSubresourcePropertiesToDefaults(res resources.AzureAPIResource, - input, output map[string]interface{}, outputIsInApiShape bool) { - unset := k.findUnsetPropertiesToMaintain(&res, input, outputIsInApiShape) - - for _, p := range unset { - cur := output - for _, pathEl := range p.path[:len(p.path)-1] { - curObj, ok := cur[pathEl] - if !ok { - newContainer := map[string]any{} - cur[pathEl] = newContainer - cur = newContainer - continue - } - cur, ok = curObj.(map[string]any) - if !ok { - break - } - } - - cur[p.path[len(p.path)-1]] = []any{} - } -} - type propertyPath struct { path []string propertyName string @@ -1334,11 +1290,9 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (* return nil, err } - var outputsWithoutIgnores = outputs - plainOldState := oldState.Mappable() if oldState.HasValue(customresources.OriginalStateKey) { - outputsWithoutIgnores[customresources.OriginalStateKey] = plainOldState[customresources.OriginalStateKey] + outputs[customresources.OriginalStateKey] = plainOldState[customresources.OriginalStateKey] } if inputs == nil { @@ -1369,7 +1323,7 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (* oldInputProjection := k.converter.SdkOutputsToSdkInputs(res.PutParameters, plainOldState) // 3a. Remove sub-resource properties from new outputs which weren't set in the old inputs. // If the user didn't specify them inline originally, we don't want to push them into the inputs now. - outputsWithoutIgnores = k.resetUnsetSubResourceProperties(ctx, urn, outputs, inputs, res) + outputsWithoutIgnores := k.removeUnsetSubResourceProperties(ctx, urn, outputs, inputs, res) // 3b. Project new outputs to their corresponding input shape (exclude read-only properties). newInputProjection := k.converter.SdkOutputsToSdkInputs(res.PutParameters, outputsWithoutIgnores) // 4. Calculate the difference between two projections. This should give us actual significant changes @@ -1382,7 +1336,7 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (* } // Store both outputs and inputs into the state. - obj := checkpointObject(res, inputs, outputsWithoutIgnores) + obj := checkpointObject(res, inputs, outputs) // Serialize and return RPC outputs. checkpoint, err := plugin.MarshalProperties( @@ -1419,8 +1373,14 @@ func removeDefaults(res resources.AzureAPIResource, plainOldState, previousInput // removeUnsetSubResourceProperties resets sub-resource properties in the outputs if they weren't set in the old inputs. // If the user didn't specify them inline originally, we don't want to push them into the inputs now. // For more details, see section "Sub-resources" in CONTRIBUTING.md. -func (k *azureNativeProvider) resetUnsetSubResourceProperties(ctx context.Context, urn resource.URN, sdkResponse map[string]any, +func (k *azureNativeProvider) removeUnsetSubResourceProperties(ctx context.Context, urn resource.URN, sdkResponse map[string]any, oldInputs resource.PropertyMap, res *resources.AzureAPIResource) map[string]any { + propertiesToRemove := k.findUnsetPropertiesToMaintain(res, oldInputs.Mappable(), false) + + if len(propertiesToRemove) == 0 { + return sdkResponse + } + // Take a deep copy so we don't modify the original which is also used later for diffing. copy := deepcopy.Copy(sdkResponse) result, ok := copy.(map[string]interface{}) @@ -1432,11 +1392,35 @@ Verbose logs contain more information, see https://www.pulumi.com/docs/support/t return sdkResponse } - k.setUnsetSubresourcePropertiesToDefaults(*res, oldInputs.Mappable(), result, false) - + for _, prop := range propertiesToRemove { + deleteFromMap(result, prop.path) + } return result } +func deleteFromMap(m map[string]interface{}, path []string) bool { + container := m + for i, key := range path { + if i == len(path)-1 { + _, found := container[key] + if found { + delete(container, key) + } + return found + } + + value, ok := container[key] + if !ok { + return false + } + container, ok = value.(map[string]interface{}) + if !ok { + return false + } + } + return false +} + // Update updates an existing resource with new values. func (k *azureNativeProvider) Update(ctx context.Context, req *rpc.UpdateRequest) (*rpc.UpdateResponse, error) { // Use the global context to handle provider shutdown. diff --git a/provider/pkg/provider/provider_e2e_test.go b/provider/pkg/provider/provider_e2e_test.go index f3ccea63cf21..e72f56e107c5 100644 --- a/provider/pkg/provider/provider_e2e_test.go +++ b/provider/pkg/provider/provider_e2e_test.go @@ -22,8 +22,10 @@ import ( "github.com/pulumi/providertest/pulumitest" "github.com/pulumi/providertest/pulumitest/assertpreview" "github.com/pulumi/providertest/pulumitest/assertrefresh" + "github.com/pulumi/providertest/pulumitest/changesummary" "github.com/pulumi/providertest/pulumitest/opttest" + "github.com/pulumi/pulumi/sdk/v3/go/common/apitype" pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go" ) @@ -67,6 +69,31 @@ func TestRequiredContainers(t *testing.T) { assertrefresh.HasNoChanges(t, pt.Refresh(t)) } +func TestSubResources(t *testing.T) { + t.Parallel() + pt := newPulumiTest(t, "subresources") + + // deploy an NSG with an "external" security rule, and an NSG with an inline security rule. + up := pt.Up(t) + assert.Len(t, up.Outputs["external-nsg-security-rules"].Value, 0) + assert.Len(t, up.Outputs["inline-nsg-security-rules"].Value, 1) + inlineRule := up.Outputs["inline-nsg-security-rules"].Value.([]any)[0].(map[string]any) + assert.Equal(t, "inline", inlineRule["name"]) + + // update a tag on the NSGs, and then check that the external security rules are now available as outputs. + pt.SetConfig(t, "subresources:revision", "2") + up = pt.Up(t) + upSummary := changesummary.FromStringIntMap(*up.Summary.ResourceChanges) + assert.Equal(t, 2, upSummary[apitype.OpUpdate]) + assert.Len(t, up.Outputs["inline-nsg-security-rules"].Value, 1) + assert.Len(t, up.Outputs["external-nsg-security-rules"].Value, 1) + externalRule := up.Outputs["external-nsg-security-rules"].Value.([]any)[0].(map[string]any) + assert.Equal(t, "external", externalRule["name"]) + + // check that the state is stable after a refresh. + assertrefresh.HasNoChanges(t, pt.Refresh(t)) +} + func TestParallelSubnetCreation(t *testing.T) { t.Parallel() if !util.EnableAzcoreBackend() { diff --git a/provider/pkg/provider/provider_test.go b/provider/pkg/provider/provider_test.go index ea5f15938f47..713f208ab0dc 100644 --- a/provider/pkg/provider/provider_test.go +++ b/provider/pkg/provider/provider_test.go @@ -159,7 +159,7 @@ func TestMappableOldStatePreservesDefaultsThatWereNotInputs(t *testing.T) { assert.Contains(t, m, "networkRuleSet") } -func TestResetUnsetSubResourceProperties(t *testing.T) { +func TestRemoveUnsetSubResourceProperties(t *testing.T) { ctx := context.Background() res, provider := setUpResourceWithRefAndProviderWithTypeLookup("") @@ -168,7 +168,7 @@ func TestResetUnsetSubResourceProperties(t *testing.T) { empty := &resources.AzureAPIResource{} oldInputs := resource.PropertyMap{} sdkResponse := map[string]any{} - actual := provider.resetUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, empty) + actual := provider.removeUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, empty) expected := map[string]any{} assert.Equal(t, expected, actual) }) @@ -182,11 +182,9 @@ func TestResetUnsetSubResourceProperties(t *testing.T) { }, }, } - actual := provider.resetUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res) + actual := provider.removeUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res) expected := map[string]any{ - "properties": map[string]any{ - "accessPolicies": []any{}, - }, + "properties": map[string]any{}, } assert.Equal(t, expected, actual) }) @@ -202,12 +200,40 @@ func TestResetUnsetSubResourceProperties(t *testing.T) { "accessPolicies": []any{}, }, } - actual := provider.resetUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res) + actual := provider.removeUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res) expected := sdkResponse assert.Equal(t, expected, actual) }) } +func TestDeleteFromMap(t *testing.T) { + m := map[string]any{ + "a": "scalar", + "b": map[string]any{ + "b.a": "scalar", + "b.b": map[string]any{ + "b.b.a": map[string]any{}, + }, + }, + } + + deleted := deleteFromMap(m, []string{"a"}) + assert.True(t, deleted) + assert.NotContains(t, m, "a") + assert.Contains(t, m, "b") + + deleted = deleteFromMap(m, []string{"b", "b.b", "b.b.a"}) + assert.True(t, deleted) + assert.Contains(t, m, "b") + b := m["b"].(map[string]any) + assert.Contains(t, b, "b.a") + assert.Contains(t, b, "b.b") + bb := b["b.b"].(map[string]any) + assert.NotContains(t, bb, "b.b.a") + + assert.False(t, deleteFromMap(m, []string{"b", "notfound"})) +} + // Helper to avoid repeating the same setup code in multiple tests. Returns a resource with a // "properties" property of type azure-native:keyvault:VaultProperties, which the returned provider // will return when asked to look up that type. @@ -275,66 +301,6 @@ func setUpResourceWithRefAndProviderWithTypeLookup(version string) (*resources.A return &res, &provider } -func TestSetUnsetSubresourcePropertiesToDefaults(t *testing.T) { - res, provider := setUpResourceWithRefAndProviderWithTypeLookup("") - - t.Run("unchanged", func(t *testing.T) { - body := map[string]any{ - "container": map[string]any{ - "properties": map[string]any{ - "accessPolicies": []any{}, - }, - }, - } - provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, true) - assert.Equal(t, map[string]any{ - "container": map[string]any{ - "properties": map[string]any{ - "accessPolicies": []any{}, - }, - }, - }, body) - }) - - t.Run("simple missing", func(t *testing.T) { - body := map[string]any{ - "container": map[string]any{ - "properties": map[string]any{}, - }, - } - provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, true) - assert.Equal(t, map[string]any{ - "container": map[string]any{ - "properties": map[string]any{ - "accessPolicies": []any{}, - }, - }, - }, body) - }) - - t.Run("nested missing", func(t *testing.T) { - body := map[string]any{} - provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, true) - assert.Equal(t, map[string]any{ - "container": map[string]any{ - "properties": map[string]any{ - "accessPolicies": []any{}, - }, - }, - }, body) - }) - - t.Run("nested missing in SDK shape", func(t *testing.T) { - body := map[string]any{} - provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, false) - assert.Equal(t, map[string]any{ - "properties": map[string]any{ - "accessPolicies": []any{}, - }, - }, body) - }) -} - func TestInvokeResponseToOutputs(t *testing.T) { invoke := resources.AzureAPIInvoke{ APIVersion: "v20241101", diff --git a/provider/pkg/provider/test-programs/subresources/Pulumi.yaml b/provider/pkg/provider/test-programs/subresources/Pulumi.yaml new file mode 100644 index 000000000000..dd4074109b35 --- /dev/null +++ b/provider/pkg/provider/test-programs/subresources/Pulumi.yaml @@ -0,0 +1,55 @@ +name: subresources +runtime: yaml +description: Tests for subresources +config: + revision: + type: string + default: "1" +resources: + rg: + type: azure-native:resources:ResourceGroup + + external-nsg: + type: azure-native:network:NetworkSecurityGroup + properties: + resourceGroupName: ${rg.name} + tags: + revision: ${revision} + + external-rule: + type: azure-native:network:SecurityRule + properties: + resourceGroupName: ${rg.name} + networkSecurityGroupName: ${external-nsg.name} + securityRuleName: external + priority: 120 + direction: Inbound + access: Deny + protocol: Tcp + sourcePortRange: "*" + destinationPortRange: "80" + sourceAddressPrefix: "*" + destinationAddressPrefix: "*" + description: Denies inbound HTTP traffic + + inline-nsg: + type: azure-native:network:NetworkSecurityGroup + properties: + resourceGroupName: ${rg.name} + tags: + revision: ${revision} + securityRules: + - name: inline + priority: 120 + direction: Inbound + access: Deny + protocol: Tcp + sourcePortRange: "*" + destinationPortRange: "80" + sourceAddressPrefix: "*" + destinationAddressPrefix: "*" + description: Denies inbound HTTP traffic + +outputs: + external-nsg-security-rules: ${external-nsg.securityRules} + inline-nsg-security-rules: ${inline-nsg.securityRules} \ No newline at end of file