Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update policy document creation when Statement is nil #9

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

anGie44
Copy link

@anGie44 anGie44 commented Mar 31, 2022

Relates hashicorp/terraform-provider-aws#22944

Description

When used in the terraform-provider-aws, a policy string can technically be provided with the null Statement block when generated from the aws_iam_policy_document data source; thus, to represent this in the policy (though not valid by the upstream IAM API when sent in a method like CreatePolicyVersion), we should map the policyDocument.Statements to nil as well. Doing so will prevent terraform plan or refresh errors like
Error: while setting policy (), encountered: Error parsing policy: Unknown error parsing statement
which occur when trying to update and compare a policy already applied to a new one that has no Statement configured e.g.

variable "ssm_param_path" {
  type        = list(string)
  default     = []
}

data "aws_iam_policy_document" "maybe_empty" {
  dynamic "statement" {
    for_each = toset(var.ssm_param_path)
    content {
      actions   = ["ssm:GetParameter*"]
      resources = ["arn:aws:ssm:eu-central-1:123456789012:parameter${statement.key}"]
    }
  }
}

resource "aws_iam_policy" "tf_go_spang" {
  name        = "tf-go-spang"
  policy      = data.aws_iam_policy_document.maybe_empty.json
}

# Within the IAM policy resource, the policy argument gets evaluated as the following during Read:

{
  "Version": "2012-10-17",
  "Statement": null
}

Alternative approaches:

  • we could add validation to the policy argument in the aws_iam_policy resource so that it'll fail before trying to compare the configured value and that in state 🤔

Output of unit test:

--- PASS: TestIntermediatePolicyDocument (0.00s)

@anGie44 anGie44 force-pushed the b-iam-policy-document-creation branch from 8ff1deb to 143e507 Compare March 31, 2022 01:34
@anGie44 anGie44 requested a review from YakDriver March 31, 2022 01:39
@anGie44 anGie44 marked this pull request as draft May 23, 2022 19:34
@YakDriver YakDriver force-pushed the b-iam-policy-document-creation branch from 143e507 to 8ab7995 Compare July 8, 2022 15:55
@YakDriver YakDriver marked this pull request as ready for review July 8, 2022 15:59
@YakDriver YakDriver self-assigned this Jul 8, 2022
Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anGie44 LGTM! Sorry for not reviewing sooner!

% go test -v ./...
=== RUN   TestPolicyEquivalence
=== RUN   TestPolicyEquivalence/Invalid_policy_JSON
=== RUN   TestPolicyEquivalence/Identical_policy_text
=== RUN   TestPolicyEquivalence/Action_block_as_single_item_array_versus_string
=== RUN   TestPolicyEquivalence/Action_block_as_single_item_array_versus_string,_different_action
=== RUN   TestPolicyEquivalence/NotAction_block_and_ActionBlock,_mixed_string_versus_array
=== RUN   TestPolicyEquivalence/NotAction_block_on_one_side
=== RUN   TestPolicyEquivalence/Principal_in_single_item_array_versus_string
=== RUN   TestPolicyEquivalence/Different_principal_in_single_item_array_versus_string
=== RUN   TestPolicyEquivalence/String_principal
=== RUN   TestPolicyEquivalence/String_NotPrincipal
=== RUN   TestPolicyEquivalence/Different_NotPrincipal_in_single_item_array_versus_string
=== RUN   TestPolicyEquivalence/Different_Effect
=== RUN   TestPolicyEquivalence/Different_Version
=== RUN   TestPolicyEquivalence/Same_Condition
=== RUN   TestPolicyEquivalence/Different_Condition
=== RUN   TestPolicyEquivalence/Condition_in_single_string_instead_of_array
=== RUN   TestPolicyEquivalence/Multiple_Condition_Blocks_in_one_policy
=== RUN   TestPolicyEquivalence/Multiple_Condition_Blocks,_same_in_both_policies
=== RUN   TestPolicyEquivalence/Multiple_Statements,_Equivalent
=== RUN   TestPolicyEquivalence/Multiple_Statements,_missing_one_from_policy_2
=== RUN   TestPolicyEquivalence/Casing_of_Effect
=== RUN   TestPolicyEquivalence/Single_Statement_vs_[]Statement
=== RUN   TestPolicyEquivalence/Empty_Principal_set
=== RUN   TestPolicyEquivalence/Empty_Principals_sets_of_different_types_have_the_same_effect
=== RUN   TestPolicyEquivalence/Empty_Principal_and_missing_Principal_have_the_same_effect
=== RUN   TestPolicyEquivalence/Principal_with_empty_sets_and_missing_Principal_have_the_same_effect
=== RUN   TestPolicyEquivalence/Principal_with_string_root_IAM_user_matches_account_ID
=== RUN   TestPolicyEquivalence/Principal_with_map_string_root_IAM_user_matches_account_ID
=== RUN   TestPolicyEquivalence/Principal_with_map_array_single_root_IAM_user_matches_account_ID
=== RUN   TestPolicyEquivalence/Principal_with_map_array_multiple_root_IAM_user_matches_account_ID
=== RUN   TestPolicyEquivalence/Missing_Statement
=== RUN   TestPolicyEquivalence/Incorrect_Statement_type
=== RUN   TestPolicyEquivalence/Incorrect_single_Resource_type
=== RUN   TestPolicyEquivalence/Incorrect_multiple_Resource_type
=== RUN   TestPolicyEquivalence/Principal_order_not_important
=== RUN   TestPolicyEquivalence/Differences_matter
=== RUN   TestPolicyEquivalence/Boolean_condition_with_and_without_quotes_on_single_value
=== RUN   TestPolicyEquivalence/Numeric_condition_with_and_without_quotes_on_single_value
=== RUN   TestPolicyEquivalence/Numeric_condition_with_and_without_quotes_on_array_of_values
=== RUN   TestPolicyEquivalence/Condition_containing_empty_array
=== RUN   TestPolicyEquivalence/Different_empty_lists
=== RUN   TestPolicyEquivalence/One-length_lists
=== RUN   TestPolicyEquivalence/Equivalent_assume-role_policies_1
=== RUN   TestPolicyEquivalence/Equivalent_assume-role_policies_2
=== RUN   TestPolicyEquivalence/Not_equivalent_assume-role_policies
=== RUN   TestPolicyEquivalence/Equivalence_of_emptiness
--- PASS: TestPolicyEquivalence (0.00s)
    --- PASS: TestPolicyEquivalence/Invalid_policy_JSON (0.00s)
    --- PASS: TestPolicyEquivalence/Identical_policy_text (0.00s)
    --- PASS: TestPolicyEquivalence/Action_block_as_single_item_array_versus_string (0.00s)
    --- PASS: TestPolicyEquivalence/Action_block_as_single_item_array_versus_string,_different_action (0.00s)
    --- PASS: TestPolicyEquivalence/NotAction_block_and_ActionBlock,_mixed_string_versus_array (0.00s)
    --- PASS: TestPolicyEquivalence/NotAction_block_on_one_side (0.00s)
    --- PASS: TestPolicyEquivalence/Principal_in_single_item_array_versus_string (0.00s)
    --- PASS: TestPolicyEquivalence/Different_principal_in_single_item_array_versus_string (0.00s)
    --- PASS: TestPolicyEquivalence/String_principal (0.00s)
    --- PASS: TestPolicyEquivalence/String_NotPrincipal (0.00s)
    --- PASS: TestPolicyEquivalence/Different_NotPrincipal_in_single_item_array_versus_string (0.00s)
    --- PASS: TestPolicyEquivalence/Different_Effect (0.00s)
    --- PASS: TestPolicyEquivalence/Different_Version (0.00s)
    --- PASS: TestPolicyEquivalence/Same_Condition (0.00s)
    --- PASS: TestPolicyEquivalence/Different_Condition (0.00s)
    --- PASS: TestPolicyEquivalence/Condition_in_single_string_instead_of_array (0.00s)
    --- PASS: TestPolicyEquivalence/Multiple_Condition_Blocks_in_one_policy (0.00s)
    --- PASS: TestPolicyEquivalence/Multiple_Condition_Blocks,_same_in_both_policies (0.00s)
    --- PASS: TestPolicyEquivalence/Multiple_Statements,_Equivalent (0.00s)
    --- PASS: TestPolicyEquivalence/Multiple_Statements,_missing_one_from_policy_2 (0.00s)
    --- PASS: TestPolicyEquivalence/Casing_of_Effect (0.00s)
    --- PASS: TestPolicyEquivalence/Single_Statement_vs_[]Statement (0.00s)
    --- PASS: TestPolicyEquivalence/Empty_Principal_set (0.00s)
    --- PASS: TestPolicyEquivalence/Empty_Principals_sets_of_different_types_have_the_same_effect (0.00s)
    --- PASS: TestPolicyEquivalence/Empty_Principal_and_missing_Principal_have_the_same_effect (0.00s)
    --- PASS: TestPolicyEquivalence/Principal_with_empty_sets_and_missing_Principal_have_the_same_effect (0.00s)
    --- PASS: TestPolicyEquivalence/Principal_with_string_root_IAM_user_matches_account_ID (0.00s)
    --- PASS: TestPolicyEquivalence/Principal_with_map_string_root_IAM_user_matches_account_ID (0.00s)
    --- PASS: TestPolicyEquivalence/Principal_with_map_array_single_root_IAM_user_matches_account_ID (0.00s)
    --- PASS: TestPolicyEquivalence/Principal_with_map_array_multiple_root_IAM_user_matches_account_ID (0.00s)
    --- PASS: TestPolicyEquivalence/Missing_Statement (0.00s)
    --- PASS: TestPolicyEquivalence/Incorrect_Statement_type (0.00s)
    --- PASS: TestPolicyEquivalence/Incorrect_single_Resource_type (0.00s)
    --- PASS: TestPolicyEquivalence/Incorrect_multiple_Resource_type (0.00s)
    --- PASS: TestPolicyEquivalence/Principal_order_not_important (0.00s)
    --- PASS: TestPolicyEquivalence/Differences_matter (0.00s)
    --- PASS: TestPolicyEquivalence/Boolean_condition_with_and_without_quotes_on_single_value (0.00s)
    --- PASS: TestPolicyEquivalence/Numeric_condition_with_and_without_quotes_on_single_value (0.00s)
    --- PASS: TestPolicyEquivalence/Numeric_condition_with_and_without_quotes_on_array_of_values (0.00s)
    --- PASS: TestPolicyEquivalence/Condition_containing_empty_array (0.00s)
    --- PASS: TestPolicyEquivalence/Different_empty_lists (0.00s)
    --- PASS: TestPolicyEquivalence/One-length_lists (0.00s)
    --- PASS: TestPolicyEquivalence/Equivalent_assume-role_policies_1 (0.00s)
    --- PASS: TestPolicyEquivalence/Equivalent_assume-role_policies_2 (0.00s)
    --- PASS: TestPolicyEquivalence/Not_equivalent_assume-role_policies (0.00s)
    --- PASS: TestPolicyEquivalence/Equivalence_of_emptiness (0.00s)
=== RUN   TestStringValueSlicesEqualIgnoreOrder
--- PASS: TestStringValueSlicesEqualIgnoreOrder (0.00s)
=== RUN   TestIntermediatePolicyDocument
=== RUN   TestIntermediatePolicyDocument/invalid_policy_with_null_string_statement
=== RUN   TestIntermediatePolicyDocument/policy_with_version_and_nil_statement
=== RUN   TestIntermediatePolicyDocument/basic_policy
--- PASS: TestIntermediatePolicyDocument (0.00s)
    --- PASS: TestIntermediatePolicyDocument/invalid_policy_with_null_string_statement (0.00s)
    --- PASS: TestIntermediatePolicyDocument/policy_with_version_and_nil_statement (0.00s)
    --- PASS: TestIntermediatePolicyDocument/basic_policy (0.00s)
PASS
ok  	github.com/hashicorp/awspolicyequivalence	0.022s

@YakDriver YakDriver merged commit b090cdc into main Jul 8, 2022
@YakDriver YakDriver deleted the b-iam-policy-document-creation branch July 8, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants