-
Notifications
You must be signed in to change notification settings - Fork 2
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: Allow array of Org IDs #97
base: main
Are you sure you want to change the base?
Conversation
description = "A external ID that correspond to your Organization within StreamNative Cloud, used for all STS assume role calls to the IAM roles created by the module. This will be the organization ID in the StreamNative console, e.g. \"o-xhopj\"." | ||
type = string | ||
} | ||
|
||
variable "external_ids" { | ||
default = [] |
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.
It should not have default value, users have to provide the Organization id list as the external ids
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.
How should we handle this, I thought the idea would be to allow either the external_id OR the external_ids value to be set. If we remove the default = []
it will break existing modules when they update because it will expect a defined value.
@@ -52,10 +52,17 @@ variable "eks_cluster_pattern" { | |||
} | |||
|
|||
variable "external_id" { | |||
default = "" |
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.
It should not have default value, users have to provide the Organization id as the external id
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.
Similar to above, do we want the user to have to specify both external_id
and external_ids
? Personally I think the best change would be to just have the external_ids array and when customers upgrade have them update the tf, that way we're not maintaining 2 variables that are related
@@ -32,12 +32,13 @@ locals { | |||
allowed_iam_policies = join(", ", formatlist("\"%s\"", distinct(concat(local.additional_iam_policy_arns, local.default_allowed_iam_policies)))) | |||
arn_like_vpcs = formatlist("\"arn:%s:ec2:%s:%s:vpc/%s\"", local.aws_partition, var.region, local.account_id, var.vpc_allowed_ids) | |||
arn_like_vpcs_str = format("[%s]", join(",", local.arn_like_vpcs)) | |||
assume_conditions = concat(local.external_id, local.source_identity, local.principal_check, local.vendor_federation) | |||
support_assume_conditions = concat(local.external_id, local.source_identity) | |||
assume_conditions = length(var.external_ids) != 0 ? concat(local.external_ids, local.source_identity, local.principal_check, local.vendor_federation) : concat(local.external_id, local.source_identity, local.principal_check, local.vendor_federation) |
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 should append the external_id to the external_ids, so existing user won't be break.
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.
If the external_ids is an empty array it will default to the existing behaviour, so the TF will show no changes.
If we append the external_id to the end of the array does that mean we also want to update the policy from StringEquals
to ForAllValues:StringEquals
?
Current limit is 62 Orgs |
Set org id as a list to allow multiple orgs in the same AWS account
Result of tf plan on an existing apply
Apply results