Updates made on NSIDC Fork #193

mikedorfman · 2024-05-29T15:37:42Z

We're working to get from our fork back to the asfadmin/CIRRUS-core baseline. These are a few of the smaller changes we had made on our fork. These changes should not result in changed default behavior. The changes include:

Adding urs_tea_client_id and urs_tea_client_password to specify alternative client id and password for use in TEA. If not specified, this defaults to urs_client_password/id variables.
Adding optional ecs_include_docker_cleanup_cronjob, which defaults to False
Fixing the value of report_granules_sns_topic_arn, updating it from module.cumulus.report_executions_sns_topic_arn to module.cumulus.report_granules_sns_topic_arn.
Adding {DAR = "YES"} to the list of tags passed into TEA for use in the buckets created by this module
Updated TEA bucket map so we do not redeploy the bucket map file if the file name is specified.

…ing the correct Cumulus output resource

…these are different from their Cumulus counterparts

… a bucket_map_key present

lindsleycj

Other than the DAR question, I'm fine with this PR

lindsleycj · 2024-05-31T16:03:20Z

cumulus/thin-egress.tf

@@ -23,7 +23,7 @@ module "thin_egress_app" {
  urs_auth_creds_secret_name         = aws_secretsmanager_secret.thin_egress_urs_creds.name
  use_cors                           = var.use_cors
  vpc_subnet_ids                     = data.aws_subnets.subnet_ids.ids
-  tags                               = local.default_tags
+  tags                               = merge(local.default_tags, {DAR = "YES"})


Does TEA create a bucket? I'm not sure why this is needed.

Yes, the TEA terraform module creates a bucket for the lambda source code zip, dependency layer zip, and CloudFormation.

We are in the process of releasing a new version of TEA that adds the DAR tag to the bucket: https://github.com/asfadmin/thin-egress-app/releases/tag/tea-release.2.0.1

I think it would be better to update the version of TEA rather than pass the tags in like this, because doing it this way will also tag the CloudFormation stack with the DAR tag meaning it will tag every resource (lambdas, queues, API gateways, etc) with the DAR tag.

Good to know - agreed, it is better to update in TEA than to include this tag with everything in the TEA stack. I'll remove this from the PR given that this work is ongoing.

Updated ChangeLog

reweeden

Most of it looks good, I just don't understand why we might need 2 different URS clients.

reweeden · 2024-05-31T17:11:06Z

cumulus/outputs.tf

@@ -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


Great catch!

reweeden · 2024-05-31T17:13:23Z

cumulus/variables.tf

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


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

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

Good call - done in the next commit.

reweeden · 2024-05-31T17:24:15Z

cumulus/variables.tf

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


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

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.

reweeden · 2024-05-31T17:30:12Z

cumulus/thin-egress.tf

  })
 }

 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


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

Good call - updated on the next commit.

reweeden · 2024-05-31T17:31:37Z

cumulus/thin-egress.tf

  })
 }

 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
+  count = local.bucket_map_key == null ? 1 : 0


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.

I see - this makes sense and is good to know!

Updated comment for the bucket map resorce to be clearer

mikedorfman added 4 commits May 29, 2024 07:36

Fixed bug where report_granules_sns_topic_arn output was not referenc…

ce00426

…ing the correct Cumulus output resource

Added ecs_include_docker_cleanup_cronjob variable, defaulting to false

c6aefc5

Added DAR="YES" to thin-egress module

2d701be

Added optional urs_tea_client_id and urs_tea_client_password in case …

eb4e829

…these are different from their Cumulus counterparts

mikedorfman changed the title ~~Several changes~~ Updates made on NSIDC Fork May 29, 2024

Added the bucket map deployment as optional, not deploying if there's…

5297e5d

… a bucket_map_key present

mikedorfman requested review from lindsleycj, reweeden, mattp0 and mckadesorensen May 31, 2024 15:43

lindsleycj approved these changes May 31, 2024

View reviewed changes

Removed DAR = "YES" tag from TEA stack

129d284

Updated ChangeLog

reweeden reviewed May 31, 2024

View reviewed changes

mikedorfman and others added 2 commits May 31, 2024 12:27

Moved urs_tea variables to the TEA section

ae98488

Updated comment for the bucket map resorce to be clearer

Merge branch 'master' into MDD/NDCUM-821

efbf367

mattp0 approved these changes Jun 6, 2024

View reviewed changes

mckadesorensen approved these changes Jun 11, 2024

View reviewed changes

Fixed typo in locals.tf

67607f9

mikedorfman merged commit 832167e into master Jun 17, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates made on NSIDC Fork #193

Updates made on NSIDC Fork #193

mikedorfman commented May 29, 2024 •

edited

Loading

lindsleycj left a comment

lindsleycj May 31, 2024

reweeden May 31, 2024

mikedorfman May 31, 2024

reweeden left a comment

reweeden May 31, 2024

mckadesorensen Jun 11, 2024

reweeden May 31, 2024

reweeden May 31, 2024

mikedorfman May 31, 2024

reweeden May 31, 2024

mikedorfman May 31, 2024

reweeden May 31, 2024

mikedorfman May 31, 2024

reweeden May 31, 2024

mikedorfman May 31, 2024

	# 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

Updates made on NSIDC Fork #193

Updates made on NSIDC Fork #193

Conversation

mikedorfman commented May 29, 2024 • edited Loading

lindsleycj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reweeden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedorfman commented May 29, 2024 •

edited

Loading