From f9d0b01afe45d6e35923e84e8ffc69e358353e5f Mon Sep 17 00:00:00 2001 From: Florian Stadler Date: Tue, 11 Jun 2024 10:40:39 +0200 Subject: [PATCH] Fix deletion of inline role policies (#4045) In https://github.com/pulumi/pulumi-aws/pull/3587 we introduced auto-naming for the inline role policies and added filtering of the inputs within a transformation. This didn't take the special case of removing all inline policies into consideration, which requires an empty object as the delete marker. Fixes #4031 --- examples/examples_nodejs_test.go | 6 ++- examples/examples_py_test.go | 55 ++++++++++++++++++++++++- examples/regress-4031/Pulumi.yaml | 10 +++++ examples/regress-4031/__main__.py | 46 +++++++++++++++++++++ examples/regress-4031/requirements.txt | 2 + examples/regress-4031/step1/__main__.py | 23 +++++++++++ provider/resources.go | 10 ++++- 7 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 examples/regress-4031/Pulumi.yaml create mode 100644 examples/regress-4031/__main__.py create mode 100644 examples/regress-4031/requirements.txt create mode 100644 examples/regress-4031/step1/__main__.py diff --git a/examples/examples_nodejs_test.go b/examples/examples_nodejs_test.go index 7813037db1c..f2c5c5331fd 100644 --- a/examples/examples_nodejs_test.go +++ b/examples/examples_nodejs_test.go @@ -643,7 +643,11 @@ func TestRoleInlinePolicyAutoName(t *testing.T) { policyEmpty := res.Outputs["inlinePolicyEmpty"] - require.Equal(t, policyEmpty.Value, []interface{}{}) + // Check that the delete marker is present + require.Len(t, policyEmpty.Value, 1) + deleteMarker := policyEmpty.Value.([]interface{})[0] + require.Empty(t, deleteMarker.(map[string]interface{})["policy"]) + require.Regexp(t, regexp.MustCompile("testrole-*"), inlinePolicy.Name) require.JSONEq(t, `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Action": "s3:GetObject", "Resource": "*" }]}`, inlinePolicy.Policy) } diff --git a/examples/examples_py_test.go b/examples/examples_py_test.go index cad8b7af53b..a30dc421155 100644 --- a/examples/examples_py_test.go +++ b/examples/examples_py_test.go @@ -5,9 +5,13 @@ package examples import ( + "context" + "os" "path/filepath" "testing" + "github.com/aws/aws-sdk-go-v2/config" + iamsdk "github.com/aws/aws-sdk-go-v2/service/iam" "github.com/pulumi/pulumi/pkg/v3/testing/integration" "github.com/stretchr/testify/assert" ) @@ -70,13 +74,49 @@ func TestSecretManagerPy(t *testing.T) { func TestRegress3905(t *testing.T) { test := getPythonBaseOptions(t). With(integration.ProgramTestOptions{ - Dir: filepath.Join(getCwd(t), "regress-3905"), + Dir: filepath.Join(getCwd(t), "regress-3905"), ExpectRefreshChanges: true, // JobDefinition.retry_strategy is suffering from a perma diff if the dict is empty. This is caused by the upstream provider ignoring empty object types }) integration.ProgramTest(t, &test) } +func TestRegress4031(t *testing.T) { + iamClient := configureIAMClient(t) + test := getPythonBaseOptions(t). + With(integration.ProgramTestOptions{ + Dir: filepath.Join(getCwd(t), "regress-4031"), + ExtraRuntimeValidation: func(t *testing.T, stack integration.RuntimeValidationStackInfo) { + roleName := stack.Outputs["roleName"].(string) + rolePolicies, err := iamClient.ListRolePolicies(context.TODO(), &iamsdk.ListRolePoliciesInput{ + RoleName: &roleName, + }) + + assert.NoError(t, err, "failed to get policies of role %s", roleName) + // Should have two inline policies called "bucket1" and "bucket2" + assert.Contains(t, rolePolicies.PolicyNames, "bucket1") + assert.Contains(t, rolePolicies.PolicyNames, "bucket2") + }, + EditDirs: []integration.EditDir{ + { + Dir: filepath.Join(getCwd(t), "regress-4031", "step1"), + Additive: true, + ExtraRuntimeValidation: func(t *testing.T, stack integration.RuntimeValidationStackInfo) { + roleName := stack.Outputs["roleName"].(string) + rolePolicies, err := iamClient.ListRolePolicies(context.TODO(), &iamsdk.ListRolePoliciesInput{ + RoleName: &roleName, + }) + + assert.NoError(t, err, "failed to get policies of role %s", roleName) + assert.Empty(t, rolePolicies.PolicyNames) + }, + }, + }, + }) + + integration.ProgramTest(t, &test) +} + func getPythonBaseOptions(t *testing.T) integration.ProgramTestOptions { envRegion := getEnvRegion(t) pythonBase := integration.ProgramTestOptions{ @@ -90,3 +130,16 @@ func getPythonBaseOptions(t *testing.T) integration.ProgramTestOptions { return pythonBase } + +func configureIAMClient(t *testing.T) *iamsdk.Client { + loadOpts := []func(*config.LoadOptions) error{} + if p, ok := os.LookupEnv("AWS_PROFILE"); ok { + loadOpts = append(loadOpts, config.WithSharedConfigProfile(p)) + } + if r, ok := os.LookupEnv("AWS_REGION"); ok { + loadOpts = append(loadOpts, config.WithRegion(r)) + } + cfg, err := config.LoadDefaultConfig(context.TODO(), loadOpts...) + assert.NoErrorf(t, err, "failed to load AWS config") + return iamsdk.NewFromConfig(cfg) +} diff --git a/examples/regress-4031/Pulumi.yaml b/examples/regress-4031/Pulumi.yaml new file mode 100644 index 00000000000..ebd6c48b22a --- /dev/null +++ b/examples/regress-4031/Pulumi.yaml @@ -0,0 +1,10 @@ +name: regress-4031 +runtime: + name: python + options: + virtualenv: venv +description: A minimal AWS Python Pulumi program to reproduce regression 4031 +config: + pulumi:tags: + value: + pulumi:template: "" diff --git a/examples/regress-4031/__main__.py b/examples/regress-4031/__main__.py new file mode 100644 index 00000000000..d165930557c --- /dev/null +++ b/examples/regress-4031/__main__.py @@ -0,0 +1,46 @@ +import pulumi +import pulumi_aws + + +role = pulumi_aws.iam.Role( + "test-role", + assume_role_policy=pulumi_aws.iam.get_policy_document( + statements=[ + pulumi_aws.iam.GetPolicyDocumentStatementArgs( + actions=["sts:AssumeRole"], + principals=[ + pulumi_aws.iam.GetPolicyDocumentStatementPrincipalArgs( + type="Service", + identifiers=["ecs-tasks.amazonaws.com"], + ), + ], + ), + ], + ).json, + inline_policies=[ + pulumi_aws.iam.RoleInlinePolicyArgs( + name="bucket1", + policy=pulumi_aws.iam.get_policy_document( + statements=[ + pulumi_aws.iam.GetPolicyDocumentStatementArgs( + actions=["s3:GetObject"], + resources=["arn:aws:s3:::bucket1/*"], + ), + ], + ).json, + ), + pulumi_aws.iam.RoleInlinePolicyArgs( + name="bucket2", + policy=pulumi_aws.iam.get_policy_document( + statements=[ + pulumi_aws.iam.GetPolicyDocumentStatementArgs( + actions=["s3:GetObject"], + resources=["arn:aws:s3:::bucket2/*"], + ), + ], + ).json, + ), + ], +) + +pulumi.export("roleName", role.name) diff --git a/examples/regress-4031/requirements.txt b/examples/regress-4031/requirements.txt new file mode 100644 index 00000000000..5f626e8c2af --- /dev/null +++ b/examples/regress-4031/requirements.txt @@ -0,0 +1,2 @@ +pulumi>=3.0.0,<4.0.0 +pulumi-aws>=6.0.0,<7.0.0 diff --git a/examples/regress-4031/step1/__main__.py b/examples/regress-4031/step1/__main__.py new file mode 100644 index 00000000000..8298b1242dc --- /dev/null +++ b/examples/regress-4031/step1/__main__.py @@ -0,0 +1,23 @@ +import pulumi +import pulumi_aws + + +role = pulumi_aws.iam.Role( + "test-role", + assume_role_policy=pulumi_aws.iam.get_policy_document( + statements=[ + pulumi_aws.iam.GetPolicyDocumentStatementArgs( + actions=["sts:AssumeRole"], + principals=[ + pulumi_aws.iam.GetPolicyDocumentStatementPrincipalArgs( + type="Service", + identifiers=["ecs-tasks.amazonaws.com"], + ), + ], + ), + ], + ).json, + inline_policies=[pulumi_aws.iam.RoleInlinePolicyArgs()] +) + +pulumi.export("roleName", role.name) diff --git a/provider/resources.go b/provider/resources.go index 2764be046f9..9b048751ea5 100644 --- a/provider/resources.go +++ b/provider/resources.go @@ -2495,7 +2495,9 @@ func ProviderFromMeta(metaInfo *tfbridge.MetadataInfo) *tfbridge.ProviderInfo { // inlinePolicies: [] // or a list with empty objects // inlinePolicies: [{}] - // In both cases the role will be created _without_ any inline policies attached. + // If an empty list is provided for inline policies then the provider will not manage any inline + // policies in this resource. + // Providing a list with an empty object will cause the provider to remove all inline policies // If a policy is provided, then both the `policy` and the `name` fields are required. // If one is provided and the other is not, then no error will be thrown and no inline policy // will be created. @@ -2505,8 +2507,14 @@ func ProviderFromMeta(metaInfo *tfbridge.MetadataInfo) *tfbridge.ProviderInfo { for _, value := range pv.ArrayValue() { if value.IsObject() { policy := value.ObjectValue() + // Filter out policies with no policy document. This can happens due to the auto-naming + // of the inline policy. It is not valid and will end up with no policy being created in AWS, but will show + // a perpetual diff which is confusing to users. + // An absent policy document is the one exception because it is the delete marker. if policy.HasValue("policy") && policy["policy"].IsString() && policy["policy"].StringValue() != "" { inlinePolicy = append(inlinePolicy, value) + } else if !policy.HasValue("policy") || policy["policy"].IsNull() { + inlinePolicy = append(inlinePolicy, value) } } }