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.

Sign up for GitHub

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

DT-1395: Sharing staging S3 DAP bucket with Funding Service Find team #467

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+39 −1
Open

DT-1395: Sharing staging S3 DAP bucket with Funding Service Find team #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e3815c
DT-1395: Sharing staging S3 DAP bucket with Funding Service Find team
keithkennedy Jan 13, 2025
4dbf076
DT-1395: Sharing staging S3 DAP bucket with Funding Service Find team
keithkennedy Jan 13, 2025
c610b47
DT-1395: Formatting fixes
keithkennedy Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions terraform/modules/marklogic/dap_export_s3.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@ data "aws_iam_policy_document" "allow_access_from_dap" {
"${module.dap_export_bucket.bucket_arn}/latest/*",
]
}
statement {
sid = "AllowExternalBucketAccess"
effect = "Allow"
principals {
type = "CanonicalUser"
identifiers = var.dap_external_canonical_users
}
actions = [
"s3:GetObject"
]
resources = [
module.dap_export_bucket.bucket_arn,
"${module.dap_export_bucket.bucket_arn}/latest/*",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth checking with Hugh if this needs to be restricted further to a specific folder/objects etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes, I will discuss this with @hugh-emerson
I think here we would want to grant access to the latest folder as this will have info for form-data and testing-center dirs which would be required.

]
}
}

resource "aws_s3_bucket_lifecycle_configuration" "dap_export" {
Expand Down
4 changes: 4 additions & 0 deletions terraform/modules/marklogic/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ variable "dap_external_role_arns" {
type = list(string)
}

variable "dap_external_canonical_users" {
type = list(string)
}

variable "marklogic_ami_version" {
type = string

Expand Down
3 changes: 2 additions & 1 deletion terraform/production/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,10 @@ module "marklogic" {
alarms_sns_topic_arn = module.notifications.alarms_sns_topic_arn
data_disk_usage_alarm_threshold_percent = 55
dap_external_role_arns = var.dap_external_role_arns
dap_external_canonical_users = var.dap_external_canonical_users
dap_job_notification_emails = concat(
local.all_notifications_email_addresses,
["deltastatsupport@levellingup.gov.uk"]
["deltastatsupport@communities.gov.uk"]
)
backup_replication_bucket = module.backup_replication_bucket.bucket
ebs_backup_role_arn = module.ebs_backup.role_arn
Expand Down
5 changes: 5 additions & 0 deletions terraform/production/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,8 @@ variable "dap_external_role_arns" {
# "DSQSS" is DAP's staging/test server. Added here for MSD-54917, informed they exist in the same environment.
default = ["arn:aws:iam::062321884391:role/DSQL1", "arn:aws:iam::062321884391:role/DSQSS"]
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth checking if this is required in production if the aim is to implement only for staging

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to leave these fields empty as if the access is a success, then it would follow we'd need it in production.
I believe I need the empty variable as dap_export_s3.tf references the variable and is used across each env.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think for safety we can limit to staging for now, as this policy grants access from our Dev and Test accounts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this is empty here default = [] and only terraform/staging/variables.tf has the specified user account identifiers.

variable "dap_external_canonical_users" {
type = list(string)
default = []
}
1 change: 1 addition & 0 deletions terraform/staging/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ module "marklogic" {
alarms_sns_topic_arn = module.notifications.alarms_sns_topic_arn
data_disk_usage_alarm_threshold_percent = 70
dap_external_role_arns = var.dap_external_role_arns
dap_external_canonical_users = var.dap_external_canonical_users
dap_job_notification_emails = local.all_notifications_email_addresses
backup_replication_bucket = module.backup_replication_bucket.bucket
ebs_backup_role_arn = module.ebs_backup.role_arn
Expand Down
6 changes: 6 additions & 0 deletions terraform/staging/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,9 @@ variable "dap_external_role_arns" {
type = list(string)
default = ["arn:aws:iam::062321884391:role/DSQSS"]
}

variable "dap_external_canonical_users" {
type = list(string)
description = "Funding service accounts that we wish to have access to staging DAP S3 bucket"
default = ["4a20e1ecba266786127536b068cbbf222b344a2e21024029f1a778f98e8667c0", "5544757b63b565e6774e61121ba15cfa98206f1629455df924f60d942a861d56"]
}
Copy link
Collaborator

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 practice to use 'AWS = "arn:aws:iam::OTHER_ACCOUNT_ID:root"'.

Alternatively to avoid hardcoding completely I believe you can set up a role with the correct permissions etc which can then be assumed by FIND to access the bucket, but that's a whole different approach to the way you've implemented it and I'm not sure how sensitive the stuff inside the bucket is to warrant extra levels of security.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YD-MHCLG I'm open to whichever approach you'd prefer from your side. We've used the CanonicalUser approach successfully in the past for our own internal x-account needs.

1 change: 1 addition & 0 deletions terraform/test/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ module "marklogic" {
alarms_sns_topic_arn = module.notifications.alarms_sns_topic_arn
data_disk_usage_alarm_threshold_percent = 70
dap_external_role_arns = var.dap_external_role_arns
dap_external_canonical_users = var.dap_external_canonical_users
dap_job_notification_emails = local.all_notifications_email_addresses
backup_replication_bucket = module.backup_replication_bucket.bucket
ebs_backup_role_arn = module.ebs_backup.role_arn
Expand Down
5 changes: 5 additions & 0 deletions terraform/test/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,8 @@ variable "dap_external_role_arns" {
type = list(string)
default = ["arn:aws:iam::062321884391:role/DSQSS"]
}

variable "dap_external_canonical_users" {
type = list(string)
default = []
}
Loading