-
Notifications
You must be signed in to change notification settings - Fork 47
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
Enable tiered storage in AWS via IAM policy #86
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
cluster: | ||
cloud_storage_access_key: THISVALUENOTUSED | ||
cloud_storage_bucket: {{ tiered_storage_bucket_name if tiered_storage_bucket_name is defined }} | ||
cloud_storage_enable_remote_read: true | ||
cloud_storage_enable_remote_write: true | ||
Comment on lines
+4
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes all topics tiered-storage-enabled by default if the |
||
cloud_storage_region: {{ aws_region if aws_region is defined }} | ||
cloud_storage_secret_key: THISVALUENOTUSED | ||
cloud_storage_credentials_source: aws_instance_metadata | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to pass this in as a param from terraform otherwise we're assuming AWS only |
||
# cloud_storage_enabled must be after other cloud_storage parameters | ||
cloud_storage_enabled: {{ true if tiered_storage_bucket_name is defined and tiered_storage_bucket_name|d('')|length > 0 else false }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,4 +3,6 @@ | |
custom_config_templates: | ||
- template: configs/defaults.j2 | ||
- template: configs/tls.j2 | ||
condition: "{{ tls | default(False) | bool }}" | ||
condition: "{{ tls | default(False) | bool }}" | ||
- template: configs/tiered_storage.j2 | ||
condition: "{{ tiered_storage_bucket_name is defined | default(False) | bool }}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,10 @@ resource "random_uuid" "cluster" {} | |
resource "time_static" "timestamp" {} | ||
|
||
locals { | ||
uuid = random_uuid.cluster.result | ||
timestamp = time_static.timestamp.rfc3339 | ||
deployment_id = "redpanda-${local.uuid}-${local.timestamp}" | ||
uuid = random_uuid.cluster.result | ||
timestamp = time_static.timestamp.unix | ||
deployment_id = length(var.deployment_prefix) > 0 ? var.deployment_prefix : "redpanda-${substr(local.uuid, 0, 8)}-${local.timestamp}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Terraform complained about names being too long, so I shortened both the UUID and the timestamp. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how safe is it to just grab the first 8 characters off the uuid? I haven't look at how go-uuid works to see if it's one of the time-based uuids. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, why do we use a timestamp here instead of just the larger uuid? |
||
tiered_storage_bucket_name = "${local.deployment_id}-bucket" | ||
|
||
# tags shared by all instances | ||
instance_tags = { | ||
|
@@ -14,15 +15,73 @@ locals { | |
} | ||
} | ||
|
||
resource "aws_iam_policy" "redpanda" { | ||
count = var.tiered_storage_enabled ? 1 : 0 | ||
name = local.deployment_id | ||
path = "/" | ||
policy = jsonencode({ | ||
Version = "2012-10-17" | ||
Statement = [ | ||
{ | ||
"Effect": "Allow", | ||
"Action": [ | ||
"s3:*", | ||
"s3-object-lambda:*", | ||
Comment on lines
+28
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be more limited to list only those needed permissions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed this with the broader team. From that conversation:
So limiting this policy to the specific bucket (as shown in this file below) is the best approach, and avoids a maintenance burden as tiered storage capabilities are expanded. |
||
], | ||
"Resource": [ | ||
"arn:aws:s3:::${local.tiered_storage_bucket_name}/*" | ||
] | ||
}, | ||
] | ||
}) | ||
} | ||
|
||
resource "aws_iam_role" "redpanda" { | ||
count = var.tiered_storage_enabled ? 1 : 0 | ||
name = local.deployment_id | ||
assume_role_policy = jsonencode({ | ||
Version = "2012-10-17" | ||
Statement = [ | ||
{ | ||
Action = "sts:AssumeRole" | ||
Effect = "Allow" | ||
Sid = "" | ||
Principal = { | ||
Service = "ec2.amazonaws.com" | ||
} | ||
}, | ||
] | ||
}) | ||
} | ||
|
||
resource "aws_iam_policy_attachment" "redpanda" { | ||
count = var.tiered_storage_enabled ? 1 : 0 | ||
name = local.deployment_id | ||
roles = [aws_iam_role.redpanda[count.index].name] | ||
policy_arn = aws_iam_policy.redpanda[count.index].arn | ||
} | ||
|
||
resource "aws_iam_instance_profile" "redpanda" { | ||
count = var.tiered_storage_enabled ? 1 : 0 | ||
name = local.deployment_id | ||
role = aws_iam_role.redpanda[count.index].name | ||
} | ||
|
||
vuldin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
resource "aws_instance" "redpanda" { | ||
count = var.nodes | ||
ami = var.distro_ami[var.distro] | ||
instance_type = var.instance_type | ||
key_name = aws_key_pair.ssh.key_name | ||
iam_instance_profile = var.tiered_storage_enabled ? aws_iam_instance_profile.redpanda[0].name : null | ||
vpc_security_group_ids = [aws_security_group.node_sec_group.id] | ||
placement_group = var.ha ? aws_placement_group.redpanda-pg[0].id : null | ||
placement_partition_number = var.ha ? (count.index % aws_placement_group.redpanda-pg[0].partition_count) + 1 : null | ||
tags = local.instance_tags | ||
tags = merge( | ||
local.instance_tags, | ||
{ | ||
Name = "${local.deployment_id}-node-${count.index}", | ||
} | ||
) | ||
|
||
connection { | ||
user = var.distro_ssh_user[var.distro] | ||
|
@@ -53,7 +112,12 @@ resource "aws_instance" "prometheus" { | |
instance_type = var.prometheus_instance_type | ||
key_name = aws_key_pair.ssh.key_name | ||
vpc_security_group_ids = [aws_security_group.node_sec_group.id] | ||
tags = local.instance_tags | ||
tags = merge( | ||
local.instance_tags, | ||
{ | ||
Name = "${local.deployment_id}-prometheus", | ||
} | ||
) | ||
|
||
connection { | ||
user = var.distro_ssh_user[var.distro] | ||
|
@@ -68,7 +132,12 @@ resource "aws_instance" "client" { | |
instance_type = var.client_instance_type | ||
key_name = aws_key_pair.ssh.key_name | ||
vpc_security_group_ids = [aws_security_group.node_sec_group.id] | ||
tags = local.instance_tags | ||
tags = merge( | ||
local.instance_tags, | ||
{ | ||
Name = "${local.deployment_id}-client", | ||
} | ||
) | ||
|
||
connection { | ||
user = var.distro_ssh_user[var.client_distro] | ||
|
@@ -176,20 +245,25 @@ resource "aws_placement_group" "redpanda-pg" { | |
resource "aws_key_pair" "ssh" { | ||
key_name = "${local.deployment_id}-key" | ||
public_key = file(var.public_key_path) | ||
tags = local.instance_tags | ||
} | ||
|
||
resource "local_file" "hosts_ini" { | ||
content = templatefile("${path.module}/../templates/hosts_ini.tpl", | ||
{ | ||
redpanda_public_ips = aws_instance.redpanda.*.public_ip | ||
redpanda_private_ips = aws_instance.redpanda.*.private_ip | ||
monitor_public_ip = var.enable_monitoring ? aws_instance.prometheus[0].public_ip : "" | ||
monitor_private_ip = var.enable_monitoring ? aws_instance.prometheus[0].private_ip : "" | ||
ssh_user = var.distro_ssh_user[var.distro] | ||
enable_monitoring = var.enable_monitoring | ||
client_public_ips = aws_instance.client.*.public_ip | ||
client_private_ips = aws_instance.client.*.private_ip | ||
rack = aws_instance.redpanda.*.placement_partition_number | ||
aws_region = var.aws_region | ||
client_count = var.clients | ||
client_public_ips = aws_instance.client.*.public_ip | ||
client_private_ips = aws_instance.client.*.private_ip | ||
enable_monitoring = var.enable_monitoring | ||
monitor_public_ip = var.enable_monitoring ? aws_instance.prometheus[0].public_ip : "" | ||
monitor_private_ip = var.enable_monitoring ? aws_instance.prometheus[0].private_ip : "" | ||
rack = aws_instance.redpanda.*.placement_partition_number | ||
redpanda_public_ips = aws_instance.redpanda.*.public_ip | ||
redpanda_private_ips = aws_instance.redpanda.*.private_ip | ||
ssh_user = var.distro_ssh_user[var.distro] | ||
tiered_storage_bucket_name = local.tiered_storage_bucket_name | ||
tiered_storage_enabled = var.tiered_storage_enabled | ||
} | ||
) | ||
filename = "${path.module}/../hosts.ini" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
resource "aws_s3_bucket" "tiered_storage" { | ||
count = var.tiered_storage_enabled ? 1 : 0 | ||
bucket = local.tiered_storage_bucket_name | ||
tags = local.instance_tags | ||
} | ||
|
||
resource "aws_s3_bucket_acl" "tiered_storage" { | ||
count = var.tiered_storage_enabled ? 1 : 0 | ||
bucket = aws_s3_bucket.tiered_storage[count.index].id | ||
acl = "private" | ||
} | ||
|
||
resource "aws_s3_bucket_versioning" "tiered_storage" { | ||
count = var.tiered_storage_enabled ? 1 : 0 | ||
bucket = aws_s3_bucket.tiered_storage[count.index].id | ||
versioning_configuration { | ||
status = "Disabled" | ||
} | ||
} | ||
Comment on lines
+13
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get a comment added on here on why we disable versioning (and maybe what happens, good or bad, if the user overrides this)? |
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 you help me understand why this is here if it's not used? Are we expecting users to override this with their own storage key in the TF config?
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, this is just a placeholder. Unfortunately Redpanda requires this to be something even when we are using
aws_instance_metadata
(IAM permissions applied to the EC2 instance).