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

Improved documentation for snowflake_system_get_aws_sns_iam_policy #2181

Closed
chriselion opened this issue Nov 9, 2023 · 4 comments
Closed
Assignees
Labels
category:data_source category:documentation data_source:system_get_aws_sns_iam_policy Issue connected to the snowflake_system_get_aws_sns_iam_policy data source docs Used to mark issues with documentation remark/questions

Comments

@chriselion
Copy link

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).

@chriselion chriselion added the feature-request Used to mark issues with provider's missing functionalities label Nov 9, 2023
@sfc-gh-asawicki
Copy link
Collaborator

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
Copy link
Author

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-asawicki sfc-gh-asawicki added docs Used to mark issues with documentation remark/questions and removed feature-request Used to mark issues with provider's missing functionalities labels Feb 8, 2024
@sfc-gh-jcieslak sfc-gh-jcieslak self-assigned this May 7, 2024
sfc-gh-jcieslak added a commit that referenced this issue May 7, 2024
Add missing documentation that should resolve
- #1087 - added descriptions for the `SNOWFLAKE_IAM_USER` and
`AWS_EXTERNAL_ID` fields in stage
- #2181 - added simple example and custom description with links for the
snowflake_system_get_aws_sns_iam_policy to show how it could be used
with AWS
- Add missing parts for the Issue creating guide + FAQ
- Pull out the SDK error to common package + usage in places where
errors were compared with string instead of predefined error
@sfc-gh-jcieslak sfc-gh-jcieslak added data_source:system_get_aws_sns_iam_policy Issue connected to the snowflake_system_get_aws_sns_iam_policy data source category:data_source category:documentation labels May 20, 2024
@sfc-gh-jcieslak
Copy link
Collaborator

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.

@sfc-gh-jcieslak
Copy link
Collaborator

Closing due to long inactivity and the fact that the documentation was adjusted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:data_source category:documentation data_source:system_get_aws_sns_iam_policy Issue connected to the snowflake_system_get_aws_sns_iam_policy data source docs Used to mark issues with documentation remark/questions
Projects
None yet
Development

No branches or pull requests

3 participants