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.

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

Updates made on NSIDC Fork #193

Merged
merged 9 commits into from
Jun 17, 2024
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# CHANGELOG

## Unreleased
* Added separate urs_tea_client_id and urs_tea_client_password that can be specified if these are different from the non-tea versions of the variables.
* Added optional ecs_include_docker_cleanup_cronjob variable, defaulting to False.
* Fixed the value of the output report_granules_sns_topic_arn to point to module.cumulus.report_granules_sns_topic_arn instead of report_executions_sns_topic_arn.
* Updated aws_s3_object.bucket_map_yaml so we only deploy this TEA bucket map when we don't provide a bucket_map_key from the daac module.

## v18.2.0.0

* Upgrade to [Cumulus v18.2.0](https://github.com/nasa/cumulus/releases/tag/v18.2.0)
Expand Down
3 changes: 3 additions & 0 deletions cumulus/locals.tf
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,7 @@ locals {
default_tags = {
Deployment = local.prefix
}

urs_tea_client_id = var.urs_tea_client_id != null ? var.urs_tea_client_id : var.urs_client_id
urs_tea_client_password = var.urs_tea_client_password != null ? var.urs_tea_client_id : var.urs_client_password
}
2 changes: 2 additions & 0 deletions cumulus/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ module "cumulus" {
execution_limit = var.thottled_queue_execution_limit
}]

ecs_include_docker_cleanup_cronjob = var.ecs_include_docker_cleanup_cronjob

tags = local.default_tags
}

Expand Down
2 changes: 1 addition & 1 deletion cumulus/outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ output "report_executions_sns_topic_arn" {
value = module.cumulus.report_executions_sns_topic_arn
}
output "report_granules_sns_topic_arn" {
value = module.cumulus.report_executions_sns_topic_arn
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch!

value = module.cumulus.report_granules_sns_topic_arn
}
output "report_pdrs_sns_topic_arn" {
value = module.cumulus.report_pdrs_sns_topic_arn
Expand Down
8 changes: 5 additions & 3 deletions cumulus/thin-egress.tf
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module "thin_egress_app" {
source = "s3::https://s3.amazonaws.com/asf.public.code/thin-egress-app/tea-terraform-build.1.3.5.zip"

auth_base_url = var.urs_url
bucket_map_file = local.bucket_map_key == null ? aws_s3_object.bucket_map_yaml.id : local.bucket_map_key
bucket_map_file = local.bucket_map_key == null ? aws_s3_object.bucket_map_yaml[0].id : local.bucket_map_key
bucketname_prefix = ""
config_bucket = local.system_bucket
cookie_domain = var.thin_egress_cookie_domain
Expand Down Expand Up @@ -35,12 +35,14 @@ resource "aws_secretsmanager_secret" "thin_egress_urs_creds" {
resource "aws_secretsmanager_secret_version" "thin_egress_urs_creds" {
secret_id = aws_secretsmanager_secret.thin_egress_urs_creds.id
secret_string = jsonencode({
UrsId = var.urs_client_id
UrsAuth = base64encode("${var.urs_client_id}:${var.urs_client_password}")
UrsId = local.urs_tea_client_id
UrsAuth = base64encode("${local.urs_tea_client_id}:${local.urs_tea_client_password}")
})
}

resource "aws_s3_object" "bucket_map_yaml" {
# If bucket_map_key is set, we already have a bucket map on S3 and don't need to create one
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe clarify that it's coming from the daac module instead something like this:

Suggested change
# If bucket_map_key is set, we already have a bucket map on S3 and don't need to create one
# If bucket_map_key is set, the daac module already created one and we can skip creation here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call - updated on the next commit.

count = local.bucket_map_key == null ? 1 : 0
Copy link
Contributor

Choose a reason for hiding this comment

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

At ASF we just changed the key that the daac module uploads the bucket map to so that it doesn't conflict with the core bucket map and then we just ignore the core bucket map. But it's probably less confusing for core to just skip creating that extraneous bucket map all together.

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 see - this makes sense and is good to know!

bucket = local.system_bucket
key = "${local.prefix}/thin-egress-app/${local.prefix}-bucket_map.yaml"
content = templatefile("./thin-egress-app/bucket_map.yaml.tmpl", {
Expand Down
16 changes: 16 additions & 0 deletions cumulus/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,16 @@ variable "urs_client_password" {

# Optional

variable "urs_tea_client_id" {
type = string
default = null
}

variable "urs_tea_client_password" {
type = string
default = null
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain to me the use case for having different URS clients for TEA and cumulus?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question! This update predates my time at NSIDC, but I believe this is because the dashboard (corresponding to the urs_client_id/password) and the TEA (corresponding to the urs_tea_client_id/password) are two separate EDL applications that have different IDs and passwords.


Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these down to where the other TEA variables are?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a description saying that they will default to the non-tea version of the variables?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call - done in the next commit.

variable "api_gateway_stage" {
type = string
default = "dev"
Expand Down Expand Up @@ -356,6 +366,12 @@ variable "ecs_cluster_instance_docker_volume_size" {
default = 50
}

variable "ecs_include_docker_cleanup_cronjob" {
description = "*Experimental* flag to configure a cron to run fstrim on all active container root filesystems"
type = bool
default = false
}

variable "bucket_map" {
type = map(object({ name = string, type = string }))
default = {}
Expand Down
Loading