-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add resources to support request locking mechanism #71
Conversation
examples/complete/variables.tf
Outdated
variable "dynamodb_table_name" { | ||
type = string | ||
description = "Either the name of the user-created DynamoDB table, or a custom name for the default table" | ||
default = "geff-request-locking-table" |
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.
let's use the prefix and lock the name up and not take it as an input.
examples/complete/variables.tf
Outdated
@@ -103,6 +103,24 @@ variable "arn_format" { | |||
default = "aws" | |||
} | |||
|
|||
variable "use_custom_dynamodb_table" { |
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.
Is the usage of dynamodb table optional?
examples/complete/variables.tf
Outdated
default = "geff-request-locking-table" | ||
} | ||
|
||
variable "custom_dynamodb_table_arn" { |
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.
Are we make the the dynamodb an optional resource where it is created outside and input into the module or created within the module?
examples/complete/variables.tf
Outdated
variable "user_managed_dynamodb_table_name" { | ||
type = string | ||
description = "Name of the user-managed custom DynamoDB table." | ||
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.
I think it's better to use "null" for the default value instead of empty string.
variables.tf
Outdated
variable "user_managed_dynamodb_table_name" { | ||
type = string | ||
description = "Name of the user-managed DynamoDB table." | ||
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.
Same thing, I think it's better to use "null" for the default value instead of empty string.
dynamodb.tf
Outdated
} | ||
|
||
ttl { | ||
attribute_name = "TimeToExist" |
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.
ttl
dynamodb.tf
Outdated
} | ||
|
||
tags = { | ||
name = "${local.geff_prefix}_batch_locking_table" |
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.
@sfc-gh-bkou do you know of a tagging strategy that might be helpful here?
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.
Yeah, I think we should have general tagging on the provider and then overwrite/add at the resource level. I started putting general tags in the provider for Anecdotes. But I think we should agree on the format and what information we want to capture. For a module like this, we can look into having an input map variable for users to pass in multiple tags.
provider "aws" {
region = var.aws_region
default_tags {
tags = {
environment = "prod"
owner = "compliance"
project = "anecdotes"
}
}
}
But the existing resource naming in this module is using hyphen instead of underscore. For this PR, keeping it consistent with the existing resource naming should be fine for now until we agree on the general tagging name and approach.
resource "aws_api_gateway_rest_api" "ef_to_lambda" {
name = "${local.geff_prefix}-api-gateway"
endpoint_configuration {
types = [
"REGIONAL",
]
}
}
dynamodb.tf
Outdated
@@ -0,0 +1,30 @@ | |||
resource "aws_dynamodb_table" "geff_batch_locking_table" { | |||
count = var.create_dynamodb_table ? 1 : 0 | |||
name = "${local.geff_prefix}_batch_locking_table" |
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 user_managed_dynamodb_table_name
is provided along with create_table, name this table user_managed_dynamodb_table_name
dynamodb.tf
Outdated
} | ||
|
||
data "aws_dynamodb_table" "user_managed_table" { | ||
count = var.user_managed_dynamodb_table_name != null ? 1 : 0 |
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 user_managed_dynamodb_table_name
and create_dynamodb_table
are both provided, don't create this resource
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.
getting there, but still seeing a couple of places where we can make things more clear
dynamodb.tf
Outdated
} | ||
|
||
locals { | ||
dynamodb_table_name = var.user_managed_dynamodb_table_name != null ? (var.create_dynamodb_table ? aws_dynamodb_table.geff_batch_locking_table[0].name : data.aws_dynamodb_table.user_managed_table[0].name) : (var.create_dynamodb_table ? aws_dynamodb_table.geff_batch_locking_table[0].name : null) |
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.
I think you repeat var.create_dynamodb_table ? aws_dynamodb_table.geff_batch_locking_table[0].name
in both cases of the inner tertiaries, and repeat identical logic varying only .name
, so you can instead do something like
locals {
dynamodb_table = (
var.create_dynamodb_table ? aws_dynamodb_table.geff_batch_locking_table[0]:
var.user_managed_dynamodb_table_name ? data.aws_dynamodb_table.user_managed_table[0]:
null
)
}
and then use local.dynamodb_table.name
/ local.dynamodb_table.arn
in the rest of the code. does that make sense?
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.
Makes sense 👍
examples/complete/variables.tf
Outdated
default = false | ||
} | ||
|
||
variable "user_managed_dynamodb_table_name" { |
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.
I don't think it makes sense to call this "user managed" anymore when it might be created by the other variable being set -- could we call it batch_locking_table_name
and set the default to "${local.geff_prefix}_batch_locking_table"
?
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.
But if this has a default value, we wouldn't be able to do the above condition right, unless the user specifies something like below?
batch_locking_table_name = null
create_dynamodb_table = false
locals {
dynamodb_table = (
var.create_dynamodb_table ? aws_dynamodb_table.geff_batch_locking_table[0]:
var. batch_locking_table_name ? data.aws_dynamodb_table.user_managed_table[0]:
null
)
}
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.
yes, and that seems like the ideal default, since the perspective of most users is ignorance about the 30s limitation of API Gateway having an impact on potential workloads, and all the need to know is that this creates a dynamodb table needed for some kind of batch synchronization mechanisms and go on with their installation. the few users that want to give it a custom name or not have a backend db or make a custom should be allowed to customize it, but most users should get it without having to worry about what it is.
on that thought, we might want to call it something more general, to support other features (e.g. rate limiting) in the future, without having to rename the table, maybe _batch_sync
?
examples/complete/variables.tf
Outdated
variable "create_dynamodb_table" { | ||
type = bool | ||
description = "Boolean for if a DynamoDB table is to be created for batch locking." | ||
default = false |
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.
default to true
dynamodb.tf
Outdated
} | ||
|
||
data "aws_dynamodb_table" "user_managed_table" { | ||
count = var.user_managed_dynamodb_table_name != null && !var.create_dynamodb_table ? 1 : 0 |
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.
does the first part do anything now? afaict, the predicate could be !var.create_dynamodb_table
?
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.
I think we'd have to check for both of these, for a case where the user sets create_dynamodb_table
to be false, and batch_locking_table_name
to be null.
iam.tf
Outdated
# 4. Policy for the DynamoDB table to be used as a backend for batch locking | ||
# ----------------------------------------------------------------------------- | ||
resource "aws_iam_policy" "dynamodb_table_policy" { | ||
count = var.create_dynamodb_table || var.user_managed_dynamodb_table_name != null ? 1 : 0 |
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.
can this be simplified to local.dynamodb_table
? it seems that would make it a clear connection to the Resource
value below
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 can, but the target needs to be specified on each apply.
The "count" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the count depends on.
So this seemed simpler than that.
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.
not sure if I follow, how does not being able to predict how many instances will be created apply to using a local
variable in the count?
that said, no strong feeling -- the invariant that local.dynamodb_table_arn
exists if one of the others is non-null seems relatively clear from the code above.
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.
The explanation I got for this was that tf needs the count
value during planning to know exactly how many resources are to be created, and that it's unable to do that when a local
is used in such a condition since it derives its value from attributes of other resources. It is then unable to determine the value of count
until the resources it depends on, have been created in an apply
.
iam.tf
Outdated
} | ||
|
||
resource "aws_iam_role_policy_attachment" "dynamodb_table_policy_attachment" { | ||
count = var.create_dynamodb_table || var.user_managed_dynamodb_table_name != null ? 1 : 0 |
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.
can this be simplified to local.dynamodb_table
?
variables.tf
Outdated
default = false | ||
} | ||
|
||
variable "user_managed_dynamodb_table_name" { |
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.
no longer user-managed
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 seems like it might work, but there's a bit more clarifying cleanup that will help me be sure, could you make those and Slack me the tf plan, or put it in the description if there isn't anything private there?
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.
looks like there are a few changes pending here. let's make sure the default behavior is adjusted to be on, for the user that is not aware of the technical details of API Gateways.
examples/complete/variables.tf
Outdated
default = false | ||
} | ||
|
||
variable "user_managed_dynamodb_table_name" { |
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.
yes, and that seems like the ideal default, since the perspective of most users is ignorance about the 30s limitation of API Gateway having an impact on potential workloads, and all the need to know is that this creates a dynamodb table needed for some kind of batch synchronization mechanisms and go on with their installation. the few users that want to give it a custom name or not have a backend db or make a custom should be allowed to customize it, but most users should get it without having to worry about what it is.
on that thought, we might want to call it something more general, to support other features (e.g. rate limiting) in the future, without having to rename the table, maybe _batch_sync
?
iam.tf
Outdated
# 4. Policy for the DynamoDB table to be used as a backend for batch locking | ||
# ----------------------------------------------------------------------------- | ||
resource "aws_iam_policy" "dynamodb_table_policy" { | ||
count = var.create_dynamodb_table || var.user_managed_dynamodb_table_name != null ? 1 : 0 |
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.
not sure if I follow, how does not being able to predict how many instances will be created apply to using a local
variable in the count?
that said, no strong feeling -- the invariant that local.dynamodb_table_arn
exists if one of the others is non-null seems relatively clear from the code above.
@sfc-gh-afedorov terraform plan when
and there are no dynamodb resources present.
|
terraform plan when
and there are no dynamodb resources present.
|
terraform plan when
and "rlm-custom-dynamodb-table" is already present, and is used above. (this also errors as expected, if
|
terraform plan when
and there are no dynamodb resources present.
|
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.
lgtm, but please test before merging
examples/complete/versions.tf
Outdated
@@ -9,7 +9,7 @@ terraform { | |||
|
|||
snowflake = { | |||
source = "Snowflake-Labs/snowflake" | |||
version = "~> 0.57.0" | |||
version = ">= 0.64.0" |
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.
"~> 0.64.0" This might be better? It only upgrades to latest minor version and allows us to specify explicitly when you want to upgrade major versions.
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.
Makes sense 👍
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.
minor comments.
cdcdbbd
to
9eb3af2
Compare
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.
lgtm
Adding resources required for geff/pull/59, such as the DynamoDB table and related policies.