Snowflake-Labs / terraform-provider-snowflake

Terraform provider for managing Snowflake accounts
https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest
MIT License
513 stars 402 forks source link

Improved documentation for snowflake_system_get_aws_sns_iam_policy #2181

Open chriselion opened 8 months ago

chriselion commented 8 months ago

Is your feature request related to a problem? Please describe.

The documentation for snowflake_system_get_aws_sns_iam_policy (link) doesn't give an example of how to call it (which isn't that bad), and doesn't explain how it should be used in the bigger picture.

Describe the solution you'd like

I assume the primary motivation for snowflake_system_get_aws_sns_iam_policy is to grant access to an SNS topic that is receiving S3 bucket notifications (at least that's what I'm using it for). In this case, snowflake_system_get_aws_sns_iam_policy is a potential foot-gun, because if you pass it blindly to an aws_sns_topic_policy, it will conflict with any aws_sns_topic_policy's that grant the S3 bucket permission to publish on the topic. Instead, you need to combine the two policies with source_policy_documents

Describe alternatives you've considered

An alternative data source that just provides the IAM user ARN (e.g. "arn:aws:iam::123456789001:user/vj4g-a-abcd1234" from here) might be easier to work with in general, since the user can insert than into their own policy JSON.

Additional context The way that I ended up setting up the SNS policy looked like this

resource "aws_sns_topic" "notif_topic" {
  name = var.sns_topic_name
}

# Form the policy that Snowflake will need to subscribe to the topic
data "snowflake_system_get_aws_sns_iam_policy" "snowflake_policy" {
  aws_sns_topic_arn = aws_sns_topic.notif_topic.arn
}

data "aws_iam_policy_document" "notif_topic" {
  # The bucket can publish to the topic
  # This is basically from https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_notification#add-notification-configuration-to-sns-topic
  statement {
    effect = "Allow"

    principals {
      type        = "Service"
      identifiers = ["s3.amazonaws.com"]
    }

    actions   = ["SNS:Publish"]
    resources = [aws_sns_topic.notif_topic.arn]

    condition {
      test     = "ArnLike"
      variable = "aws:SourceArn"
      values   = [data.aws_s3_bucket.snowflake_s3_bucket.arn]
    }
  }

  # Append the snowflake policy
  source_policy_documents = [data.snowflake_system_get_aws_sns_iam_policy.snowflake_policy.aws_sns_topic_policy_json]
}

resource "aws_sns_topic_policy" "bucket_policy" {
  arn    = aws_sns_topic.notif_topic.arn
  policy = data.aws_iam_policy_document.notif_topic.json
}

The source_policy_documents was the tricky part (at least for me).

sfc-gh-asawicki commented 8 months ago

Hey @chriselion. Thanks for creating the issue.

You are right: an example for snowflake_system_get_aws_sns_iam_policy is missing - we will add it.

The behavior and the reasons behind snowflake_system_get_aws_sns_iam_policy are explained in the official docs: https://docs.snowflake.com/en/sql-reference/functions/system_get_aws_sns_iam_policy. We can also add this link to our docs.

Would you like to propose any more additions?

chriselion commented 8 months ago

Thanks @sfc-gh-asawicki - the docs for SYSTEM$GET_AWS_SNS_IAM_POLICY make sense, and it would be great to either link to them or include the same content.

The docs give a good explantion of the "why" but not really the "how" - even with that information, using the results from snowflake_system_get_aws_sns_iam_policy are potentially confusing (although this is more aws_iam_policy_document's fault, not Snowflake's!). A larger example like I gave would probably be helpful.

sfc-gh-jcieslak commented 1 month ago

Hey @chriselion, We adjusted the documentation for snowflake_system_get_aws_sns_iam_policy, mostly added important links that may help users with the understanding how to use the data source. Please review and close or add another comment if you think It should be adjusted in any way.