-
Notifications
You must be signed in to change notification settings - Fork 16
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Loading status checks…
Don't send writeOnly properties if they're also createOnly (#1448)
Fixes #1435 Ideally we'd have an integration test around this case to avoid future regression but currently the VPC-based test is very unstable, so has been disabled, but included for documentation of the issues. - Deletion of the `aws-native:ec2:IpamPoolCidr` regularly fails with `"GeneralServiceException": Error occurred during operation 'The CIDR has one or more allocations.'` even after the VPC is deleted for serveral minutes. - Sometimes Update of the `aws-native:ec2:IpamPool` fails with `"NotStabilized": IpamPoolCidrFailureReason(Message=The CIDR has one or more allocations.` - The refresh of the `aws-native:ec2:IpamPool` fails due to a diff in the `provisionedCidrs` property. - It's also not ideal that we have to create the IPAM manually to avoid the test failing if run in parallel as only 1 IPAM is allowed to be created in each region. Extracted the patch generation code instead and wrote a unit around it. Also replicated the fix into the untyped ExtensionResource implementation and tested using the same test suite.
- v1.18.0
- v1.17.0
- v1.16.0
- v1.15.0
- v1.14.0
- v1.13.0
- v1.12.0
- v1.11.0
- v1.10.0
- v1.9.0
- v1.8.0
- v1.7.0
- v1.6.0
- v1.5.0
- v1.4.0
- v1.3.0
- v1.2.0
- v1.1.0
- v1.0.2
- v1.0.1
- v1.0.0
- v0.126.0
- v0.125.0
- v0.124.0
- v0.123.0
- v0.122.0
- v0.121.0
- v0.120.0
- v0.119.0
- v0.118.0
- v0.117.1
- v0.117.0
- v0.116.0
- v0.115.0
- v0.114.0
- v0.113.0
- v0.112.0
- v0.111.0
- v0.110.0
- v0.109.0
- v0.108.4
- v0.108.3
- v0.108.2
- v0.108.1
- v0.108.0
- v0.107.0
- v0.106.0
- v0.105.0
- v0.104.0
- v0.103.0
- v0.102.0
- v0.101.0
- v0.100.0
- sdk/v1.18.0
- sdk/v1.17.0
- sdk/v1.16.0
- sdk/v1.15.0
- sdk/v1.14.0
- sdk/v1.13.0
- sdk/v1.12.0
- sdk/v1.11.0
- sdk/v1.10.0
- sdk/v1.9.0
- sdk/v1.8.0
- sdk/v1.7.0
- sdk/v1.6.0
- sdk/v1.5.0
- sdk/v1.4.0
- sdk/v1.3.0
- sdk/v1.2.0
- sdk/v1.1.0
- sdk/v1.0.2
- sdk/v1.0.1
- sdk/v1.0.0
- sdk/v0.126.0
- sdk/v0.125.0
- sdk/v0.124.0
- sdk/v0.123.0
- sdk/v0.122.0
- sdk/v0.121.0
- sdk/v0.120.0
- sdk/v0.119.0
- sdk/v0.118.0
- sdk/v0.117.1
- sdk/v0.117.0
- sdk/v0.116.0
- sdk/v0.115.0
- sdk/v0.114.0
- sdk/v0.113.0
- sdk/v0.112.0
- sdk/v0.111.0
- sdk/v0.110.0
- sdk/v0.109.0
- sdk/v0.108.4
- sdk/v0.108.3
- sdk/v0.108.2
- sdk/v0.108.1
- sdk/v0.108.0
- sdk/v0.107.0
- sdk/v0.106.0
- sdk/v0.105.0
- sdk/v0.104.0
- sdk/v0.103.0
- sdk/v0.102.0
- sdk/v0.101.0
- sdk/v0.100.0
- provider/v1.11.0
1 parent
5bb7e7c
commit 210092f
Showing
16 changed files
with
402 additions
and
105 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
*.pyc | ||
venv/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
name: simple-py | ||
runtime: python | ||
description: Test updating a VPC which has properties which are both create-only and write-only |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# Copyright 2021, Pulumi Corporation. All rights reserved. | ||
|
||
import pulumi | ||
from pulumi_aws_native import ec2 | ||
from pulumi_command import local | ||
|
||
aws_native_config = pulumi.Config("aws-native") | ||
|
||
# Only 1 IPAM can exist per region, so this must be created manually as a pre-requisite and is therefore looked up here. | ||
ipamId = local.run( | ||
command="aws ec2 describe-ipams --query 'Ipams[0].IpamId' --output text").stdout | ||
|
||
ipam = ec2.get_ipam(ipam_id=ipamId) | ||
|
||
ipamPool = ec2.IpamPool("ipamPool", address_family="ipv4", | ||
ipam_scope_id=ipam.private_default_scope_id, | ||
locale=aws_native_config.require("region")) | ||
|
||
ipamPoolCidr = ec2.IpamPoolCidr( | ||
"ipamPoolCidr", ipam_pool_id=ipamPool.id, cidr="10.0.0.0/16") | ||
|
||
vpc = ec2.Vpc("vpc", | ||
# enable_dns_hostnames=True, | ||
ipv4_ipam_pool_id=ipamPool.id, | ||
ipv4_netmask_length=20, | ||
opts=pulumi.ResourceOptions(depends_on=[ipamPoolCidr])) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
pulumi>=3.5.1,<4.0.0 | ||
pulumi-command>=0.9.2,<2.0.0 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# Copyright 2021, Pulumi Corporation. All rights reserved. | ||
|
||
import pulumi | ||
from pulumi_aws_native import ec2 | ||
from pulumi_command import local | ||
|
||
aws_native_config = pulumi.Config("aws-native") | ||
|
||
ipamId = local.run( | ||
command="aws ec2 describe-ipams --query 'Ipams[0].IpamId' --output text").stdout | ||
|
||
ipam = ec2.get_ipam(ipam_id=ipamId) | ||
|
||
ipamPool = ec2.IpamPool("ipamPool", address_family="ipv4", | ||
ipam_scope_id=ipam.private_default_scope_id, | ||
locale=aws_native_config.require("region")) | ||
|
||
ipamPoolCidr = ec2.IpamPoolCidr( | ||
"ipamPoolCidr", ipam_pool_id=ipamPool.id, cidr="10.0.0.0/16") | ||
|
||
vpc = ec2.Vpc("vpc", | ||
enable_dns_hostnames=True, | ||
ipv4_ipam_pool_id=ipamPool.id, | ||
ipv4_netmask_length=20, | ||
opts=pulumi.ResourceOptions(depends_on=[ipamPoolCidr])) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
package resources | ||
|
||
import ( | ||
"fmt" | ||
"slices" | ||
"strings" | ||
|
||
"github.com/mattbaird/jsonpatch" | ||
"github.com/pulumi/pulumi-aws-native/provider/pkg/metadata" | ||
"github.com/pulumi/pulumi-aws-native/provider/pkg/naming" | ||
"github.com/pulumi/pulumi/pkg/v3/codegen" | ||
"github.com/pulumi/pulumi/sdk/v3/go/common/resource" | ||
"github.com/wI2L/jsondiff" | ||
) | ||
|
||
func CalcPatch(oldInputs resource.PropertyMap, newInputs resource.PropertyMap, spec metadata.CloudAPIResource, types map[string]metadata.CloudAPIType) ([]jsonpatch.JsonPatchOperation, error) { | ||
diff := oldInputs.Diff(newInputs) | ||
|
||
// Write-only properties can't even be read internally within the CloudControl service so they must be included in | ||
// patch requests as adds to ensure the updated model validates. | ||
// If a property is both write-only and create-only, we should not include it in the patch request | ||
// because create-only properties can't be updated and even doing an add of the same value is rejected. | ||
createOnlyProps := codegen.NewStringSet(spec.CreateOnly...) | ||
writeOnlyProps := codegen.NewStringSet(spec.WriteOnly...) | ||
mustSendProps := writeOnlyProps.Subtract(createOnlyProps) | ||
for _, writeOnlyPropName := range mustSendProps.SortedValues() { | ||
propKey := resource.PropertyKey(writeOnlyPropName) | ||
if _, ok := diff.Sames[propKey]; ok { | ||
delete(diff.Sames, propKey) | ||
diff.Adds[propKey] = newInputs[propKey] | ||
} | ||
} | ||
|
||
return naming.DiffToPatch(&spec, types, diff) | ||
} | ||
|
||
func CalculateUntypedPatch(typedOldInputs ExtensionResourceInputs, typedInputs ExtensionResourceInputs) ([]jsonpatch.JsonPatchOperation, error) { | ||
jsonDiffPatch, err := jsondiff.Compare(typedOldInputs.Properties, typedInputs.Properties) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to compare properties: %w", err) | ||
} | ||
|
||
// Write-only properties can't even be read internally within the CloudControl service so they must be included in | ||
// patch requests as adds to ensure the updated model validates. | ||
// If a property is both write-only and create-only, we should not include it in the patch request | ||
// because create-only properties can't be updated and even doing an add of the same value is rejected. | ||
createOnlyProps := codegen.NewStringSet(typedInputs.CreateOnly...) | ||
for _, writeOnlyPropName := range typedInputs.WriteOnly { | ||
if createOnlyProps.Has(writeOnlyPropName) { | ||
continue | ||
} | ||
newValue, ok := typedInputs.Properties[writeOnlyPropName] | ||
if !ok { | ||
continue | ||
} | ||
propPath := "/" + writeOnlyPropName | ||
hasPatch := false | ||
for _, op := range jsonDiffPatch { | ||
if op.Path == propPath { | ||
hasPatch = true | ||
break | ||
} | ||
} | ||
if !hasPatch { | ||
jsonDiffPatch = append(jsonDiffPatch, jsondiff.Operation{ | ||
Type: "add", | ||
Path: propPath, | ||
Value: newValue, | ||
}) | ||
} | ||
} | ||
|
||
jsonPatch := make([]jsonpatch.JsonPatchOperation, 0, len(jsonDiffPatch)) | ||
for _, op := range jsonDiffPatch { | ||
jsonPatch = append(jsonPatch, jsonpatch.JsonPatchOperation{ | ||
Operation: op.Type, | ||
Path: op.Path, | ||
Value: op.Value, | ||
}) | ||
} | ||
slices.SortStableFunc(jsonPatch, func(a, b jsonpatch.JsonPatchOperation) int { | ||
return strings.Compare(a.Path, b.Path) | ||
}) | ||
return jsonPatch, nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
package resources | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/mattbaird/jsonpatch" | ||
"github.com/pulumi/pulumi-aws-native/provider/pkg/metadata" | ||
"github.com/pulumi/pulumi-aws-native/provider/pkg/naming" | ||
"github.com/pulumi/pulumi/pkg/v3/codegen/schema" | ||
"github.com/pulumi/pulumi/sdk/v3/go/common/resource" | ||
"github.com/pulumi/pulumi/sdk/v3/go/common/slice" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestCalcPatch(t *testing.T) { | ||
type args struct { | ||
oldInputs resource.PropertyMap | ||
newInputs resource.PropertyMap | ||
spec metadata.CloudAPIResource | ||
types map[string]metadata.CloudAPIType | ||
} | ||
type implementation func(t *testing.T, args args) []jsonpatch.JsonPatchOperation | ||
typed := func(t *testing.T, args args) []jsonpatch.JsonPatchOperation { | ||
t.Helper() | ||
patch, err := CalcPatch(args.oldInputs, args.newInputs, args.spec, args.types) | ||
assert.NoError(t, err) | ||
return patch | ||
} | ||
untyped := func(t *testing.T, args args) []jsonpatch.JsonPatchOperation { | ||
t.Helper() | ||
// Convert all keys to upper camel case to match the behavior of the SdkToCfn function. | ||
keysToUpperCamel := func(s string) (string, bool) { | ||
return naming.ToUpperCamel(s), true | ||
} | ||
patch, err := CalculateUntypedPatch(ExtensionResourceInputs{ | ||
Properties: args.oldInputs.MapRepl(keysToUpperCamel, nil), | ||
}, ExtensionResourceInputs{ | ||
Properties: args.newInputs.MapRepl(keysToUpperCamel, nil), | ||
CreateOnly: slice.Map(args.spec.CreateOnly, naming.ToUpperCamel), | ||
WriteOnly: slice.Map(args.spec.WriteOnly, naming.ToUpperCamel), | ||
}) | ||
assert.NoError(t, err) | ||
return patch | ||
} | ||
|
||
for name, run := range map[string]implementation{ | ||
"Typed": typed, | ||
"Untyped": untyped, | ||
} { | ||
t.Run(name, func(t *testing.T) { | ||
t.Run("no diff", func(t *testing.T) { | ||
patch := run(t, args{ | ||
oldInputs: resource.PropertyMap{ | ||
"prop1": resource.NewStringProperty("value1"), | ||
}, | ||
newInputs: resource.PropertyMap{ | ||
"prop1": resource.NewStringProperty("value1"), | ||
}, | ||
spec: metadata.CloudAPIResource{ | ||
Inputs: map[string]schema.PropertySpec{ | ||
"prop1": {TypeSpec: schema.TypeSpec{Type: "string"}}, | ||
}, | ||
}}) | ||
assert.Empty(t, patch) | ||
}) | ||
t.Run("always sends write-only properties", func(t *testing.T) { | ||
expected := []jsonpatch.JsonPatchOperation{ | ||
{Operation: "add", Path: "/Prop1", Value: "1"}, | ||
{Operation: "replace", Path: "/Prop2", Value: "2b"}, | ||
} | ||
patch := run(t, args{ | ||
oldInputs: resource.PropertyMap{ | ||
"prop1": resource.NewStringProperty("1"), | ||
"prop2": resource.NewStringProperty("2a"), | ||
}, | ||
newInputs: resource.PropertyMap{ | ||
"prop1": resource.NewStringProperty("1"), | ||
"prop2": resource.NewStringProperty("2b"), | ||
}, | ||
spec: metadata.CloudAPIResource{ | ||
Inputs: map[string]schema.PropertySpec{ | ||
"prop1": {TypeSpec: schema.TypeSpec{Type: "string"}}, | ||
"prop2": {TypeSpec: schema.TypeSpec{Type: "string"}}, | ||
}, | ||
WriteOnly: []string{"prop1"}, | ||
}}) | ||
assert.Equal(t, expected, patch) | ||
}) | ||
t.Run("don't send write-only, create-only properties", func(t *testing.T) { | ||
expected := []jsonpatch.JsonPatchOperation{ | ||
{Operation: "replace", Path: "/Prop2", Value: "2b"}, | ||
} | ||
patch := run(t, args{ | ||
oldInputs: resource.PropertyMap{ | ||
"prop1": resource.NewStringProperty("1"), | ||
"prop2": resource.NewStringProperty("2a"), | ||
}, | ||
newInputs: resource.PropertyMap{ | ||
"prop1": resource.NewStringProperty("1"), | ||
"prop2": resource.NewStringProperty("2b"), | ||
}, | ||
spec: metadata.CloudAPIResource{ | ||
Inputs: map[string]schema.PropertySpec{ | ||
"prop1": {TypeSpec: schema.TypeSpec{Type: "string"}}, | ||
"prop2": {TypeSpec: schema.TypeSpec{Type: "string"}}, | ||
}, | ||
WriteOnly: []string{"prop1"}, | ||
CreateOnly: []string{"prop1"}, | ||
}}) | ||
assert.Equal(t, expected, patch) | ||
}) | ||
}) | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.