-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Support permission init for control plane management volume #104
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming this module volume-access
would be better to maintain consistency with format of vendor-access
.
modules/aws/sn-volume-access/files/sn_volume_s3_bucket.json.tpl
Outdated
Show resolved
Hide resolved
modules/aws/sn-volume-access/files/sn_volume_s3_bucket.json.tpl
Outdated
Show resolved
Hide resolved
} | ||
|
||
module "sn_managed_cloud" { | ||
source = "../../modules/aws/volume-access" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change it after this pr approve and merged
modules/aws/volume-access/main.tf
Outdated
} | ||
|
||
resource "aws_iam_role" "access_bucket_role" { | ||
name = "sn-${var.external_id}-${var.bucket}-${var.path}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need handle the situation that the S3 path can contain multi path separator, e.g, s3://test-snowflake/it_iceberg/data/streaming_ingest/GBQMaKNHsA/a1/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And be noted that the IAM Role name length is restricted to up to 64 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks
variable "bucket" { | ||
description = "User bucket name" | ||
type = string | ||
} | ||
|
||
variable "path" { | ||
description = "S3 bucket path" | ||
type = string | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make it as an array, so user just need to append new bucket to the array rather than define a new terraform module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
No description provided.