Skip to content

Conversation

@juliareynolds-nava
Copy link
Contributor

🎫 Ticket

https://jira.cms.gov/browse/PLT-890

🛠 Changes

backend files defining s3 state buckets for greenfield apps

ℹ️ Context

Add backends to the TF in the platform repo (each backend is a 2-line file for each of the vpcs). We expect to have 12 new VPCs.

🧪 Validation

Buckets are created in S3 with tfstate files for each new backend definition file.

@jscott-nava
Copy link
Contributor

AB2D SBX TF plan is failing, looks like aws_iam_account_alias doesn't exist in that account?

Copy link
Contributor

@gfreeman-navapbc gfreeman-navapbc left a comment

Choose a reason for hiding this comment

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

LGTM, but these buckets seem to only exist in the non-prod account atm, just something to be aware of

@gfreeman-navapbc
Copy link
Contributor

LGTM, but these buckets seem to only exist in the non-prod account atm, just something to be aware of

I see now that there's a resource we're creating for these before the data call, disregard

Copy link
Member

@gsf gsf left a comment

Choose a reason for hiding this comment

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

Generally looks good! There is no ab2d-mgmt, bcda-mgmt, or dpc-mgmt, so those backends and related tfstate resources should be removed. Instead, there should be a cdap-mgmt backend and tfstate in the prod account.

bucket = "${data.aws_caller_identity.current.account_id}-bucket-access-logs"
bucket = (var.legacy == true ? "${data.aws_caller_identity.current.account_id}-bucket-access-logs" :
data.aws_iam_account_alias.current.account_alias == "aws-cms-oeda-bcda-non-prod" ? "bucket-access-logs-20250409172631068600000001" :
"bucket-access-logs-tbd")
Copy link
Member

Choose a reason for hiding this comment

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

The "tbd" can be replaced with the suffix for the access logs bucket created in the prod account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

@gsf gsf left a comment

Choose a reason for hiding this comment

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

Let's also switch out the deprecated dynamodb_table for use_lockfile in these new backends (see https://developer.hashicorp.com/terraform/language/backend/s3#state-locking). Those dynamodb tables will also need to be removed, and the tfstate terraform will need to be updated to avoid creating tables in greenfield.

Copy link
Member

@gsf gsf left a comment

Choose a reason for hiding this comment

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

Is there a reason why the aws-params-env-action changes are included in this PR?

Also, we need to keep the legacy ab2d-mgmt and dpc-mgmt backends. And all legacy backends should keep the dynamodb_table.

role-to-assume: arn:aws:iam::${{ secrets.BCDA_ACCOUNT }}:role/delegatedadmin/developer/bcda-mgmt-github-actions
aws-region: ${{ vars.AWS_REGION }}
- run: terraform init -backend-config=../../backends/bcda-mgmt.s3.tfbackend
- run: terraform init -backend-config=../../backends/bcda-test.s3.tfbackend
Copy link
Member

Choose a reason for hiding this comment

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

This should be kept at bcda-mgmt for now to support the legacy accounts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -1 +1 @@
1.5.5
1.10.0
Copy link
Member

Choose a reason for hiding this comment

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

As long as we're updating, we should switch to the latest patch under 1.10, which would be 1.10.5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1 @@
bucket = "ab2d-dev-tfstate"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have a suffix on the bucket name? Also, please run terraform fmt over these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -1,2 +1 @@
bucket = "ab2d-dev-tfstate"
dynamodb_table = "ab2d-dev-tfstate"
Copy link
Member

Choose a reason for hiding this comment

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

The dynamodb_table attribute should be kept for legacy backends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Update your editor configuration to end files with a newline.

@@ -0,0 +1,3 @@
bucket = "ab2d-prod-tfstate-20250411202936776600000001"


Copy link
Member

Choose a reason for hiding this comment

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

Drop these extra newlines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

backend "s3" {
key = "tfstate/terraform.tfstate"
key = "tfstate/terraform.tfstate"
use_lockfile = true
Copy link
Member

Choose a reason for hiding this comment

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

The use_lockfile attribute should be in the backend files for greenfield.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@juliareynolds-nava juliareynolds-nava marked this pull request as draft April 14, 2025 21:17
key = "github-actions/terraform.tfstate"
}
required_version = "~> 1.5.5"
required_version = "~> 1.10.0"
Copy link
Member

Choose a reason for hiding this comment

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

Drop this required_version

Copy link
Member

@gsf gsf left a comment

Choose a reason for hiding this comment

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

Looks good and should be ready to merge once tfstate resources are all created (and extras cleaned up).

# Conflicts:
#	terraform/backends/ab2d-sandbox-gf.s3.tfbackend
#	terraform/backends/bcda-sandbox-gf.s3.tfbackend
#	terraform/backends/dpc-sandbox-gf.s3.tfbackend

terraform {
# Comment out backend block and init without -backend-config for initial creation of resources
# # Comment out backend block and init without -backend-config for initial creation of resources
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# # Comment out backend block and init without -backend-config for initial creation of resources
# Comment out backend block and init without -backend-config for initial creation of resources

Comment on lines 102 to 104
bucket = data.aws_s3_bucket.bucket_access_logs.bucket

target_bucket = data.aws_s3_bucket.bucket_access_logs.bucket
Copy link
Member

Choose a reason for hiding this comment

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

This sets the wrong bucket as the source bucket for logging. Patch incoming.

module "bucket_key" {
source = "../key"
name = "${var.name}-bucket"
name = data.aws_s3_bucket.bucket_access_logs.bucket
Copy link
Member

Choose a reason for hiding this comment

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

The key is for the bucket created in this module, not the access logs bucket. Patch incoming.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think we may run into issues if we start renaming all of the kms keys in legacy.

Copy link
Member

@gsf gsf left a comment

Choose a reason for hiding this comment

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

I think we're good to merge

@juliareynolds-nava juliareynolds-nava merged commit af97746 into main Apr 17, 2025
76 checks passed
@juliareynolds-nava juliareynolds-nava deleted the jreynolds_plt_890_bucket_changes branch April 17, 2025 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants