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

r/s3_bucket_lifecycle_configuration: Rule change generates MalformedXML #23884

Closed
Nuru opened this issue Mar 26, 2022 · 4 comments · Fixed by #23893
Closed

r/s3_bucket_lifecycle_configuration: Rule change generates MalformedXML #23884

Nuru opened this issue Mar 26, 2022 · 4 comments · Fixed by #23893
Labels
bug Addresses a defect in current functionality. service/s3 Issues and PRs that pertain to the s3 service. upstream Addresses functionality related to the cloud provider.

Comments

@Nuru
Copy link

Nuru commented Mar 26, 2022

Linked Issue

This is a follow-up to #23883. In that issue, the apply fails with a provider error. This issue is what happens when that provider error does not get raised and the apply is attempted.

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform CLI and Terraform AWS Provider Version

Terraform v1.1.6
on darwin_amd64

  • provider registry.terraform.io/hashicorp/aws v4.8.0

Affected Resource(s)

  • s3_bucket_lifecycle_configuration

Terraform Configuration Files

main.tf (click to reveal)
variable "trigger" {
  type    = bool
  default = false
}

resource "aws_s3_bucket" "this" {
  bucket = "eg-test-aws-issue-23883"
}

resource "aws_s3_bucket_versioning" "this" {
  bucket = aws_s3_bucket.this.id

  versioning_configuration {
    status = "Enabled"
  }
}

locals {
  lc_rules_list = [
    {
      abort_incomplete_multipart_upload_days = null
      enabled                                = true
      expiration = {
        days                         = 1
        expired_object_delete_marker = null
      }

      # test no filter
      filter_prefix_only = null
      filter_and = {
        prefix                   = null
        object_size_greater_than = null
        tags = {
          temp = "true"
        }
      }

      id = "rule-1"
    },
    {
      abort_incomplete_multipart_upload_days = 1
      enabled                                = true
      expiration = {
        days                         = 1464
        expired_object_delete_marker = null
      }

      # test no filter
      filter_prefix_only = null
      filter_and         = null
      id                 = "rule-2"
    },

    {
      abort_incomplete_multipart_upload_days = 1
      enabled                                = true
      expiration = {
        days                         = null
        expired_object_delete_marker = null
      }

      # test no filter
      filter_prefix_only = null
      filter_and         = null
      id                 = "nofilter"
    },
    {
      abort_incomplete_multipart_upload_days = 1
      enabled                                = true
      expiration = {
        days                         = null
        expired_object_delete_marker = null
      }

      # test prefix only
      filter_prefix_only = {
        prefix = "prefix1"
      }
      filter_and = null
      id         = "prefix1"
    },
    {
      abort_incomplete_multipart_upload_days = null
      enabled                                = true
      expiration = {
        days                         = 1461
        expired_object_delete_marker = false
      }
      # test prefix with other filter
      filter_prefix_only = null
      filter_and = {
        prefix                   = "prefix2"
        object_size_greater_than = 128 * 1024
        tags                     = null
      }
      id = "prefix2"
    },

    {
      abort_incomplete_multipart_upload_days = null
      enabled                                = true
      expiration = {
        days                         = 93
        expired_object_delete_marker = false
      }
      # test filter without prefix
      filter_prefix_only = null
      filter_and = {
        prefix                   = ""
        object_size_greater_than = 256 * 1024
        tags                     = null
      }
      id = "big"
    }
  ]

  lc_rules_map = {
    base    = [local.lc_rules_list[0], local.lc_rules_list[1], local.lc_rules_list[3], local.lc_rules_list[4]]
    trigger = local.lc_rules_list
  }

  lc_rules = local.lc_rules_map[var.trigger ? "trigger" : "base"]
}

resource "aws_s3_bucket_lifecycle_configuration" "default" {
  bucket = aws_s3_bucket.this.id

  dynamic "rule" {
    for_each = local.lc_rules
    content {
      id     = rule.value.id
      status = rule.value.enabled == true ? "Enabled" : "Disabled"

      dynamic "expiration" {
        for_each = rule.value.expiration == null ? [] : [rule.value.expiration]
        content {
          days                         = expiration.value.days
          expired_object_delete_marker = expiration.value.expired_object_delete_marker
        }
      }

      # Filter is always required due to https://github.com/hashicorp/terraform-provider-aws/issues/23299
      dynamic "filter" {
        for_each = rule.value.filter_prefix_only == null && rule.value.filter_and == null ? ["empty"] : []
        content {}
      }

      dynamic "filter" {
        for_each = rule.value.filter_prefix_only == null ? [] : ["prefix"]
        content {
          prefix = rule.value.filter_prefix_only.prefix
        }
      }

      dynamic "filter" {
        for_each = rule.value.filter_and == null ? [] : ["and"]
        content {
          and {
            object_size_greater_than = try(rule.value.filter_and.object_size_greater_than, null)
            object_size_less_than    = try(rule.value.filter_and.object_size_less_than, null)
            prefix                   = rule.value.filter_and.prefix
            tags                     = try(rule.value.filter_and.tags, null)
          }
        }
      }
    }
  }
}

The issue appears to be that when the new list of rules doesn't line up with the old list of rules, and a rule goes from having

filter {
  and {
    prefix = "prefix2"
    object_size_greater_than = 131072
  }
}

to

filter {
  prefix = "prefix1"
}

the provider gets confused.

Debug Output

Note the Terraform plan output (hidden below, click to reveal) correctly indicates it is going to create a rule like this:

rule {
  id = "prefix1"
  status = "Enabled"
  
  filter {
    prefix = "prefix1"
  }
}

but the XML generated for the rule is

<Rule>
    <AbortIncompleteMultipartUpload>
        <DaysAfterInitiation>1</DaysAfterInitiation>
    </AbortIncompleteMultipartUpload>
    <Filter>
        <And>
            <Prefix></Prefix>
        </And>
    </Filter>
    <ID>prefix1</ID>
    <NoncurrentVersionExpiration>
        <NoncurrentDays>30</NoncurrentDays>
        <NewerNoncurrentVersions>2</NewerNoncurrentVersions>
    </NoncurrentVersionExpiration>
    <Status>Enabled</Status>
    <Transition>
        <Days>7</Days>
        <StorageClass>GLACIER</StorageClass>
    </Transition>
</Rule>

Hidden Terraform plan output

Terraform `plan` output (click to reveal)
Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the last "terraform apply":

  # aws_s3_bucket_lifecycle_configuration.default has changed
  ~ resource "aws_s3_bucket_lifecycle_configuration" "default" {
        id     = "eg-test-aws-issue-23883"
        # (1 unchanged attribute hidden)

      ~ rule {
            id     = "rule-2"
            # (1 unchanged attribute hidden)


          ~ filter {
            }
            # (1 unchanged block hidden)
        }
      ~ rule {
          ~ id     = "nofilter" -> "prefix1"
            # (1 unchanged attribute hidden)


          ~ filter {
              + prefix = "prefix1"
            }
            # (1 unchanged block hidden)
        }
      ~ rule {
          ~ id     = "prefix1" -> "prefix2"
            # (1 unchanged attribute hidden)

          ~ expiration {
              ~ days                         = 0 -> 1461
                # (1 unchanged attribute hidden)
            }

          ~ filter {
              - prefix = "prefix1" -> null

              ~ and {
                  ~ object_size_greater_than = 0 -> 131072
                  + prefix                   = "prefix2"
                    tags                     = {}
                    # (1 unchanged attribute hidden)
                }
            }
        }
      - rule {
          - id     = "prefix2" -> null
          - status = "Enabled" -> null

          - expiration {
              - days                         = 1461 -> null
              - expired_object_delete_marker = false -> null
            }

          - filter {
              - and {
                  - object_size_greater_than = 131072 -> null
                  - object_size_less_than    = 0 -> null
                  - prefix                   = "prefix2" -> null
                }
            }
        }
      - rule {
          - id     = "big" -> null
          - status = "Enabled" -> null

          - expiration {
              - days                         = 93 -> null
              - expired_object_delete_marker = false -> null
            }

          - filter {
              - and {
                  - object_size_greater_than = 262144 -> null
                  - object_size_less_than    = 0 -> null
                }
            }
        }
        # (1 unchanged block hidden)
    }


Unless you have made equivalent changes to your configuration, or ignored the relevant attributes using ignore_changes, the following plan may
include actions to undo or respond to these changes.

──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # aws_s3_bucket_lifecycle_configuration.default will be updated in-place
  ~ resource "aws_s3_bucket_lifecycle_configuration" "default" {
        id     = "eg-test-aws-issue-23883"
        # (1 unchanged attribute hidden)

      ~ rule {
          ~ id     = "prefix1" -> "nofilter"
            # (1 unchanged attribute hidden)


          ~ filter {
              - prefix = "prefix1" -> null
            }
            # (1 unchanged block hidden)
        }
      ~ rule {
          ~ id     = "prefix2" -> "prefix1"
            # (1 unchanged attribute hidden)

          ~ expiration {
              ~ days                         = 1461 -> 0
                # (1 unchanged attribute hidden)
            }

          ~ filter {
              + prefix = "prefix1"

              ~ and {
                  - object_size_greater_than = 131072 -> null
                  - prefix                   = "prefix2" -> null
                    tags                     = {}
                    # (1 unchanged attribute hidden)
                }
            }
        }
      + rule {
          + id     = "prefix2"
          + status = "Enabled"

          + expiration {
              + days                         = 1461
              + expired_object_delete_marker = false
            }

          + filter {
              + and {
                  + object_size_greater_than = 131072
                  + prefix                   = "prefix2"
                }
            }
        }
      + rule {
          + id     = "big"
          + status = "Enabled"

          + expiration {
              + days                         = 93
              + expired_object_delete_marker = false
            }

          + filter {
              + and {
                  + object_size_greater_than = 262144
                }
            }
        }
        # (2 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Actual Behavior

On terraform apply

│ Error: error updating S3 Bucket Lifecycle Configuration (eg-test-s3-lifecycle-test-nuru): MalformedXML: The XML you provided was not well-formed or did not validate against our published schema

Steps to Reproduce

  1. terraform init
  2. terraform apply -auto-approve
  3. terraform apply -auto-approve -var trigger=true

Note: if step 3 triggers #23883 then run it again to produce this issue.

References

This complex setup of the filter configuration is made necessary by:

This is the same configuration that triggers

@github-actions github-actions bot added needs-triage Waiting for first response or review from a maintainer. service/s3 Issues and PRs that pertain to the s3 service. labels Mar 26, 2022
@anGie44 anGie44 added bug Addresses a defect in current functionality. upstream Addresses functionality related to the cloud provider. and removed needs-triage Waiting for first response or review from a maintainer. labels Mar 27, 2022
@anGie44
Copy link
Contributor

anGie44 commented Mar 27, 2022

Relates hashicorp/terraform-plugin-sdk#743

Noting here that there is a DiffSuppressFunc applied to rule.filter but it seems to be interfering with nested fields like rule.filter.and when it is removed in a Terraform configuration. In this issue's example, we have a change that removes the filter.and block but Terraform shows that it needs to be updated in-place rather than be removed.

@Nuru
Copy link
Author

Nuru commented Mar 28, 2022

@anGie44 Given that the SDK issue you are referring to is almost 1 year old and has had no progress of any kind since it was opened, and appears to be related to hashicorp/terraform-plugin-sdk#477 which is almost 2 years old and still open, I would not expect a change in how DiffSuppressFunc works and instead say that, de facto, the provider is using it incorrectly.

@anGie44
Copy link
Contributor

anGie44 commented Mar 28, 2022

I agree @Nuru 👍 To workaround the DiffSuppressFunc 's default functionality which doesn't entirely work with the way we use the verify.SuppressMissingOptionalConfigurationBlock method in the filter parameter, I've added arg specific handling as we only want to suppress diffs in the case users don't specify both filter and the root-level prefix parameter.

The linked PR has a test that attempts to replicate the update you've suggested, from

filter {
  and {
    prefix = "prefix2"
    object_size_greater_than = 131072
  }
}

to

filter {
  prefix = "prefix1"
}

@github-actions
Copy link

github-actions bot commented May 5, 2022

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/s3 Issues and PRs that pertain to the s3 service. upstream Addresses functionality related to the cloud provider.
Projects
None yet
2 participants