gramsa49 commented 3 months ago

Terraform Core Version

1.6.2

AWS Provider Version

5.44.0

Affected Resource(s)

aws_securitylake_data_lake

Expected Behavior

When changing parameter meta_store_manager_role_arn, the data lake should be updated.

Actual Behavior

When changing parameter meta_store_manager_role_arn, the data lake is recreated (stringplanmodifier.RequiresReplace).

Relevant Error/Panic Output Snippet

n/a

Terraform Configuration Files

resource "aws_securitylake_data_lake" "nonrollup" {
  count                       = length(var.replication) > 0 ? 1 : 0
  provider                    = aws.current
  meta_store_manager_role_arn = var.metastore_role.arn
  configuration {
    region = var.region
    encryption_configuration {
      kms_key_id = aws_kms_key.securitylake.key_id
    }
    lifecycle_configuration {
      expiration {
        days = var.security_lake_retention_days_nonrollup
      }
    }
    replication_configuration {
      role_arn = var.replication_role.arn
      regions = [
        var.security_lake_rollup_region,
      ]
    }
  }
}

Steps to Reproduce

Change the value of var.metastore_role.arn, generate a new plan

Debug Output

  # module.securitylake[0].module.securitylake-ap-northeast-2[0].aws_securitylake_data_lake.nonrollup[0] must be replaced
-/+ resource "aws_securitylake_data_lake" "nonrollup" {
      ~ arn                         = "arn:aws:securitylake:ap-northeast-2:012345678901:data-lake/default" -> (known after apply)
      ~ id                          = "arn:aws:securitylake:ap-northeast-2:012345678901:data-lake/default" -> (known after apply)
      ~ meta_store_manager_role_arn = "arn:aws:iam::012345678901:role/application/AmazonSecurityLakeMetaStoreManager" # forces replacement -> (known after apply) # forces replacement
      ~ s3_bucket_arn               = "arn:aws:s3:::aws-security-data-lake-ap-northeast-2" -> (known after apply)
        # (1 unchanged attribute hidden)

        # (1 unchanged block hidden)
    }

Panic Output

n/a

Important Factoids

The role name used by Security Lake needs to be changed when updating from V1 to V2 because the Lambda functions created by Security Lake are hard coded to use the new role name, AmazonSecurityLakeMetaStoreManagerV2, and does not honor the role arn provided to the security lake configuration.

I believe a new API call, UpdateDataLake, was introduced after this resource was initially authored. See https://docs.aws.amazon.com/security-lake/latest/APIReference/API_UpdateDataLake.html

UpdateDataLake supports updating the Meta Store Manager ARN.

References

https://docs.aws.amazon.com/security-lake/latest/APIReference/API_UpdateDataLake.html

Would you like to implement a fix?

No

github-actions[bot] commented 3 months ago

Community Note

Voting for Prioritization

Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
Please see our prioritization guide for information on how we prioritize.
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.

Volunteering to Work on This Issue

If you are interested in working on this issue, please leave a comment.
If this would be your first contribution, please review the contribution guide.

joelmccoy commented 3 months ago

I would like to take a stab at fixing this.

joelmccoy commented 2 months ago

I took I look at this and this may not be possible to test properly with AWS's current APIs. The updateDataLake does provide an option to change the metaStoreRoleArn... however there is no way in the API to confirm that it has been changed. Even the API response for updateDataLake does not return the new value for metaStoreRoleArn. And the read API call listDataLakes does not return the metaStoreRoleArn :/ .

I could technically remove the requiresReplace for the metaStoreRoleArn field and the updateDataLake should technically update this field. But I have no way of adding another acceptance test for this behavior.

Is it ok to proceed without adding an additional acceptance test because I don't know if there is a way to test this right now due to the limitations in the API?