Automate IAM User Role Policies#3980
Closed
rudransh-shrivastava wants to merge 11 commits intoOWASP:feature/nest-zappa-migrationfrom
Closed
Automate IAM User Role Policies#3980rudransh-shrivastava wants to merge 11 commits intoOWASP:feature/nest-zappa-migrationfrom
rudransh-shrivastava wants to merge 11 commits intoOWASP:feature/nest-zappa-migrationfrom
Conversation
2 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Terraform bootstrap flow with per-environment IAM roles and outputs, refactors backend to per-environment S3/DynamoDB state resources, adds bootstrap Terraform files and docs, and updates the CI workflow to run a staging bootstrap job and pass AWS role-assumption parameters into staging plan/deploy steps. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 17 files
Confidence score: 2/5
- Security risk:
iam:PassRoleis granted unconditionally ininfrastructure/bootstrap/main.tf, which nullifies theiam:PassedToServicecondition and could allow unintended role passing. prevent_destroy = falseon the Terraform state bucket ininfrastructure/backend/main.tfremoves a key safeguard against accidental state deletion, which is a concrete operational risk.- Given the high-severity IAM and state-safety issues, this change carries elevated risk and isn’t a straightforward merge.
- Pay close attention to
infrastructure/bootstrap/main.tfandinfrastructure/backend/main.tf- overly broad IAM permissions and disabled state-bucket protection.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/backend/main.tf">
<violation number="1" location="infrastructure/backend/main.tf:92">
P1: `prevent_destroy = false` on the Terraform state bucket removes the safety net against accidental deletion of state files. If this was changed to facilitate the migration from a single resource to `for_each`, it should be reverted to `true` in a follow-up (or in this PR after state migration). The project README explicitly notes `prevent_destroy` should be `true` by default and only set to `false` before intentional destruction.</violation>
</file>
<file name="infrastructure/bootstrap/main.tf">
<violation number="1" location="infrastructure/bootstrap/main.tf:57">
P2: `CloudWatchEventsManagement` grants `events:*` actions on `resources = ["*"]`, which fully supersedes the scoped `EventBridgeManagement` statement in part_two (CloudWatch Events and EventBridge share the `events:` IAM prefix). This is likely unintended — the events permissions should probably be scoped to project-specific rules as in `EventBridgeManagement`, not open to all resources. Consider removing this wildcard statement and relying on the scoped one, or scoping the resources here.</violation>
<violation number="2" location="infrastructure/bootstrap/main.tf:374">
P1: Security: `iam:PassRole` in the `IAMManagement` statement is granted unconditionally on the same resources as the `IAMPassRole` statement, making the `StringEquals` condition on `iam:PassedToService` completely ineffective. Any principal with this policy can pass these roles to *any* AWS service, not just `ecs-tasks`, `lambda`, and `rds` as intended.
Remove `iam:PassRole` from the `IAMManagement` actions list so that PassRole is only granted through the condition-restricted `IAMPassRole` statement.</violation>
</file>
<file name="infrastructure/bootstrap/README.md">
<violation number="1" location="infrastructure/bootstrap/README.md:63">
P2: The IAM resource ARNs are hardcoded to a specific account ID even though the note says to replace ${AWS_ACCOUNT_ID}. Use the placeholder variable here to keep the example accurate and avoid copying the wrong account ID.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infrastructure/README.md (1)
102-108:⚠️ Potential issue | 🟡 MinorNavigation hint references wrong directory after bootstrap step.
Line 104 says "If you are in
infrastructure/backend" but after completing step 2 the user is ininfrastructure/bootstrap/. Thecdcommandcd ../staging/would be correct from either, but the hint text is misleading.📝 Proposed fix
- - Navigate to the main infrastructure directory. If you are in `infrastructure/backend`, you can use: + - Navigate to the main infrastructure directory. If you are in `infrastructure/bootstrap`, you can use:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/README.md` around lines 102 - 108, The hint text under "Setup Main Infrastructure (staging)" is misleading: update the explanatory sentence that currently reads "If you are in `infrastructure/backend`" to reference the actual prior step location (`infrastructure/bootstrap`) or remove the conditional entirely so it simply shows the correct command `cd ../staging/`; locate the "Setup Main Infrastructure (staging)" section and edit the surrounding sentence accordingly to ensure the `cd ../staging/` hint matches the previous step.infrastructure/backend/main.tf (1)
76-101:⚠️ Potential issue | 🟡 MinorConsider adding a DynamoDB-specific statement to the KMS key policy with
kms:ViaServicecondition.The current KMS key policy allows the root account unrestricted
kms:*permissions, which technically permits DynamoDB encryption operations. However, per the learning from PR#3581, best practice is to include an explicit statement withkms:ViaServicecondition set todynamodb.*.amazonaws.comand required actions:kms:Encrypt,kms:Decrypt,kms:ReEncrypt*,kms:GenerateDataKey*,kms:DescribeKey, andkms:CreateGrant. This restricts key usage to the DynamoDB service specifically.Regarding
prevent_destroy = false: While state-locking tables are sensitive resources, this infrastructure is designated for testing purposes only per learnings, where production-grade protection controls are not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/backend/main.tf` around lines 76 - 101, Add a DynamoDB-specific statement to the KMS key policy used by module.kms so DynamoDB can use the key only via the service: add a new policy statement (referencing the KMS resource created in module.kms) that grants Principal {"Service": "dynamodb.amazonaws.com"} the actions kms:Encrypt, kms:Decrypt, kms:ReEncrypt*, kms:GenerateDataKey*, kms:DescribeKey and kms:CreateGrant and include the Condition {"StringEquals": {"kms:ViaService": "dynamodb.*.amazonaws.com"}}; this ties the key to the aws_dynamodb_table.state_lock usage while keeping the existing broader admin/root permissions intact.
🧹 Nitpick comments (5)
infrastructure/INFO.md (1)
7-42: Minor: Inconsistent indentation in the JSON policy document.The JSON block mixes tabs and spaces (e.g., Line 8 uses tabs while Lines 24-39 use spaces). Consider normalizing to a consistent style for readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/INFO.md` around lines 7 - 42, The JSON policy uses mixed tabs and spaces causing inconsistent indentation; normalize the file to a single indentation style (preferably 2 or 4 spaces) throughout by replacing tab characters with spaces for all objects and arrays (including the top-level object and nested statements with S3StateManagement, DynamoDBStateManagement, and KMSStateManagement) so the keys and nested blocks align consistently for readability..github/workflows/run-ci-cd.yaml (1)
625-690: Bootstrap runsterraform apply -auto-approveon every push to main.Running
terraform apply -auto-approveunconditionally on every merge tomainadds latency to the pipeline even when bootstrap resources haven't changed. While bootstrap resources should be idempotent, consider adding a plan-check step that skips apply when there are no changes, or gate this on changes toinfrastructure/bootstrap/**using a path filter.Additionally, the bootstrap job uses separate
BOOTSTRAP_AWS_ACCESS_KEY_ID/BOOTSTRAP_AWS_SECRET_ACCESS_KEYsecrets (lines 646–648) without role assumption, while all other staging jobs assume thenest-staging-terraformrole. Ensure the bootstrap IAM user has appropriately scoped permissions (least privilege) since it runs with direct credentials.infrastructure/backend/main.tf (1)
233-244: State bucket usesAES256(S3-managed keys) while DynamoDB uses customer-managed KMS.The DynamoDB table (line 99) uses
module.kms.key_arnfor encryption, but the state S3 bucket usesAES256. For consistent encryption posture, consider using the same KMS key for the state bucket. The#trivy:ignoresuppression suggests this was a conscious choice, but it creates an inconsistency worth documenting if intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/backend/main.tf` around lines 233 - 244, The S3 state bucket resource aws_s3_bucket_server_side_encryption_configuration.state currently uses sse_algorithm = "AES256", which is inconsistent with the DynamoDB's customer-managed KMS (module.kms.key_arn); change the configuration to use KMS by setting sse_algorithm = "aws:kms" and adding kms_master_key_id = module.kms.key_arn (for_each still driven by local.state_environments and bucket = aws_s3_bucket.state[each.key].id), and either remove or update the `#trivy`:ignore comment and add a short note in code/comments explaining the intentional use of module.kms.key_arn for consistent encryption if you preserve the suppression.infrastructure/bootstrap/main.tf (2)
199-216: ECRAuth statement grants write actions (PutImage,UploadLayerPart, etc.) onresources = ["*"].Only
ecr:GetAuthorizationTokenrequires*resources. The remaining actions (e.g.,BatchCheckLayerAvailability,PutImage,CompleteLayerUpload) support resource-level restrictions and are already scoped inECRManagement(lines 218–238). The*scope here effectively bypasses that scoping.Consider keeping only
ecr:GetAuthorizationTokenin this statement and leaving the rest inECRManagement.♻️ Proposed fix
statement { sid = "ECRAuth" effect = "Allow" actions = [ - "ecr:BatchCheckLayerAvailability", - "ecr:BatchGetImage", - "ecr:CompleteLayerUpload", - "ecr:DescribeRepositories", "ecr:GetAuthorizationToken", - "ecr:GetDownloadUrlForLayer", - "ecr:GetRepositoryPolicy", - "ecr:InitiateLayerUpload", - "ecr:ListImages", - "ecr:PutImage", - "ecr:UploadLayerPart", ] resources = ["*"] }Then add the removed actions to the scoped
ECRManagementstatement if not already present:statement { sid = "ECRManagement" effect = "Allow" actions = [ + "ecr:BatchCheckLayerAvailability", + "ecr:BatchGetImage", + "ecr:CompleteLayerUpload", "ecr:CreateRepository", "ecr:DeleteLifecyclePolicy", "ecr:DeleteRepository", "ecr:DescribeRepositories", + "ecr:GetDownloadUrlForLayer", "ecr:GetLifecyclePolicy", "ecr:GetRepositoryPolicy", + "ecr:InitiateLayerUpload", + "ecr:ListImages", "ecr:ListTagsForResource", + "ecr:PutImage", "ecr:PutImageScanningConfiguration", "ecr:PutLifecyclePolicy", "ecr:SetRepositoryPolicy", "ecr:TagResource", "ecr:UntagResource", + "ecr:UploadLayerPart", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 199 - 216, The ECRAuth statement currently grants global write permissions; update the statement with sid "ECRAuth" to only include "ecr:GetAuthorizationToken" and keep resources = ["*"], remove all other ECR actions from that statement, and then ensure the removed actions (e.g., "ecr:BatchCheckLayerAvailability", "ecr:BatchGetImage", "ecr:CompleteLayerUpload", "ecr:GetDownloadUrlForLayer", "ecr:InitiateLayerUpload", "ecr:ListImages", "ecr:PutImage", "ecr:UploadLayerPart", etc.) are present in the scoped statement with sid "ECRManagement" (add them there if missing) so write operations are resource-scoped rather than "*".
543-559: SSM resources are overly broad.
arn:aws:ssm:*:${account}:*covers all SSM resource types (parameters, documents, sessions, etc.) across all regions. Consider narrowing to the specific parameter path prefix used by the project, e.g.:resources = [ "arn:aws:ssm:*:${data.aws_caller_identity.current.account_id}:parameter/${var.project_name}-${each.key}-*", "arn:aws:ssm:*:${data.aws_caller_identity.current.account_id}:parameter/${var.project_name}/${each.key}/*", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 543 - 559, The SSM statement with sid "SSMManagement" currently uses an overly broad resource ARN ("arn:aws:ssm:*:${data.aws_caller_identity.current.account_id}:*"); update the resources array in that statement to restrict access to only the parameter ARNs your app uses by replacing the wildcard with parameter-specific ARNs (use the "parameter/" prefix and incorporate project identifiers like var.project_name and each.key or the project's parameter path prefix) so only the intended SSM Parameter resources are allowed instead of all SSM resource types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 412-436: The KMS policy statement with sid "KMSManagement"
currently uses resources = ["*"] which grants management over all keys; tighten
scope by replacing the wildcard with project-scoped ARNs and/or tag-based
conditions: change resources from ["*"] to scoped ARNs (e.g., KMS alias/key ARNs
that include var.project_name like alias/${var.project_name}-*) and/or require a
Condition such as "kms:ResourceAlias" or "kms:ResourceTag/Project" ==
var.project_name so actions in the "KMSManagement" statement only apply to
keys/aliases owned by this project rather than all account keys. Ensure you
reference the same statement block (sid = "KMSManagement") when making the
change.
- Around line 355-410: The IAMManagement statement currently includes
iam:PassRole which overrides the conditional restriction in the IAMPassRole
statement; remove "iam:PassRole" from the actions list in the IAMManagement
statement (the block with sid = "IAMManagement") so that the only Allow for
iam:PassRole comes from the IAMPassRole statement (sid = "IAMPassRole") which
enforces the StringEquals iam:PassedToService condition for
ecs-tasks.amazonaws.com, lambda.amazonaws.com and rds.amazonaws.com; ensure no
other statement duplicates an unconditional iam:PassRole on the same resources.
- Around line 562-583: Add a prerequisite note to infrastructure/README.md under
the "Bootstrap IAM Role" section explaining that the IAM users trusted by the
aws_iam_role "terraform" resource must exist before running terraform apply;
state they must be created manually or via a separate provisioning process,
follow the naming convention ${project_name}-${environment} (e.g., nest-staging,
nest-production) and be created for each environment in local.environments, and
emphasize creation happens prior to running the bootstrap terraform apply so the
assume-role policy (trusting user/${var.project_name}-${each.key}) can succeed.
In `@infrastructure/bootstrap/README.md`:
- Around line 62-65: The policy snippet currently hardcodes the AWS account ID
in the Resource ARNs ("arn:aws:iam::652192963764:role/nest-*-terraform" and
"arn:aws:iam::652192963764:policy/nest-*-terraform"); replace the literal
`652192963764` with the `${AWS_ACCOUNT_ID}` placeholder in both ARNs so the
Resource array consistently uses the configured variable, and ensure any
surrounding README text mentions the `${AWS_ACCOUNT_ID}` substitution where
examples are given.
In `@infrastructure/INFO.md`:
- Around line 33-40: The KMS policy block "KMSStateManagement" only allows
"kms:Decrypt" which is insufficient for S3 PutObject operations using a KMS CMK;
update the policy referenced by the KMSStateManagement block to include
"kms:GenerateDataKey" (add it to the "Action" array alongside "kms:Decrypt") so
Terraform/state writes can obtain data keys for encryption before PutObject.
In `@infrastructure/README.md`:
- Around line 96-100: Update the misleading sentence "Apply the changes to
create the backend resources" in the Bootstrap IAM Role step so it clearly
describes bootstrapping IAM resources; replace that exact line with wording such
as "Apply the changes to create the bootstrap IAM resources" (or similar) in the
same section that contains the "Bootstrap IAM Role" heading to ensure the
description matches the step.
---
Outside diff comments:
In `@infrastructure/backend/main.tf`:
- Around line 76-101: Add a DynamoDB-specific statement to the KMS key policy
used by module.kms so DynamoDB can use the key only via the service: add a new
policy statement (referencing the KMS resource created in module.kms) that
grants Principal {"Service": "dynamodb.amazonaws.com"} the actions kms:Encrypt,
kms:Decrypt, kms:ReEncrypt*, kms:GenerateDataKey*, kms:DescribeKey and
kms:CreateGrant and include the Condition {"StringEquals": {"kms:ViaService":
"dynamodb.*.amazonaws.com"}}; this ties the key to the
aws_dynamodb_table.state_lock usage while keeping the existing broader
admin/root permissions intact.
In `@infrastructure/README.md`:
- Around line 102-108: The hint text under "Setup Main Infrastructure (staging)"
is misleading: update the explanatory sentence that currently reads "If you are
in `infrastructure/backend`" to reference the actual prior step location
(`infrastructure/bootstrap`) or remove the conditional entirely so it simply
shows the correct command `cd ../staging/`; locate the "Setup Main
Infrastructure (staging)" section and edit the surrounding sentence accordingly
to ensure the `cd ../staging/` hint matches the previous step.
---
Nitpick comments:
In `@infrastructure/backend/main.tf`:
- Around line 233-244: The S3 state bucket resource
aws_s3_bucket_server_side_encryption_configuration.state currently uses
sse_algorithm = "AES256", which is inconsistent with the DynamoDB's
customer-managed KMS (module.kms.key_arn); change the configuration to use KMS
by setting sse_algorithm = "aws:kms" and adding kms_master_key_id =
module.kms.key_arn (for_each still driven by local.state_environments and bucket
= aws_s3_bucket.state[each.key].id), and either remove or update the
`#trivy`:ignore comment and add a short note in code/comments explaining the
intentional use of module.kms.key_arn for consistent encryption if you preserve
the suppression.
In `@infrastructure/bootstrap/main.tf`:
- Around line 199-216: The ECRAuth statement currently grants global write
permissions; update the statement with sid "ECRAuth" to only include
"ecr:GetAuthorizationToken" and keep resources = ["*"], remove all other ECR
actions from that statement, and then ensure the removed actions (e.g.,
"ecr:BatchCheckLayerAvailability", "ecr:BatchGetImage",
"ecr:CompleteLayerUpload", "ecr:GetDownloadUrlForLayer",
"ecr:InitiateLayerUpload", "ecr:ListImages", "ecr:PutImage",
"ecr:UploadLayerPart", etc.) are present in the scoped statement with sid
"ECRManagement" (add them there if missing) so write operations are
resource-scoped rather than "*".
- Around line 543-559: The SSM statement with sid "SSMManagement" currently uses
an overly broad resource ARN
("arn:aws:ssm:*:${data.aws_caller_identity.current.account_id}:*"); update the
resources array in that statement to restrict access to only the parameter ARNs
your app uses by replacing the wildcard with parameter-specific ARNs (use the
"parameter/" prefix and incorporate project identifiers like var.project_name
and each.key or the project's parameter path prefix) so only the intended SSM
Parameter resources are allowed instead of all SSM resource types.
In `@infrastructure/INFO.md`:
- Around line 7-42: The JSON policy uses mixed tabs and spaces causing
inconsistent indentation; normalize the file to a single indentation style
(preferably 2 or 4 spaces) throughout by replacing tab characters with spaces
for all objects and arrays (including the top-level object and nested statements
with S3StateManagement, DynamoDBStateManagement, and KMSStateManagement) so the
keys and nested blocks align consistently for readability.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/nest-zappa-migration #3980 +/- ##
=============================================================
Coverage 93.32% 93.32%
=============================================================
Files 513 513
Lines 15827 15827
Branches 2178 2134 -44
=============================================================
Hits 14770 14770
Misses 695 695
Partials 362 362
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/README.md">
<violation number="1" location="infrastructure/README.md:402">
P2: The staging assume-role policy example is not a valid IAM policy document (missing Version/Statement). Users copying this will get a malformed policy error. Wrap the statement in a standard policy document.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
infrastructure/backend/main.tf (1)
114-160: Logs lifecycle is missingnoncurrent_version_expirationnow that versioning is enabled.
aws_s3_bucket_versioning.logs(lines 154–160) newly enables versioning on the logs bucket. However,aws_s3_bucket_lifecycle_configuration.logs(lines 114–128) only hasexpiration.days— on a versioned bucket, that rule creates a delete marker rather than removing the data, so old log versions accumulate indefinitely. By contrast, the state bucket lifecycle (lines 189–191) already includesnoncurrent_version_expiration.♻️ Proposed fix
rule { id = "expire-logs" status = "Enabled" abort_incomplete_multipart_upload { days_after_initiation = var.abort_incomplete_multipart_upload_days } expiration { days = var.expire_log_days } + noncurrent_version_expiration { + noncurrent_days = var.expire_log_days + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/backend/main.tf` around lines 114 - 160, The logs S3 lifecycle only sets expiration on a versioned bucket, which deletes the current object by creating delete markers but leaves prior versions; update aws_s3_bucket_lifecycle_configuration.logs to include a noncurrent_version_expiration block so old object versions are actually removed (e.g., add noncurrent_version_expiration { days = var.expire_log_days } or another appropriate var) alongside the existing expiration/abort_incomplete_multipart_upload so that aws_s3_bucket_versioning.logs does not cause indefinite accumulation of old versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/README.md`:
- Around line 81-88: Remove the blank lines between adjacent blockquote NOTE
callouts so MD028 is resolved: either consolidate the three separate `> [!NOTE]`
blocks into a single `> [!NOTE]` block containing the combined text, or place
the `> [!NOTE]` lines directly adjacent with no empty lines between them (i.e.,
remove the empty lines between the `> [!NOTE]` blocks in the README). Ensure the
final content preserves all guidance (state bucket, defaults, optional region
via `aws_region`) within the single or adjacent NOTE block(s).
- Around line 428-435: The KMS statement with Sid "KMSStateManagement" only
grants kms:Decrypt and must allow DynamoDB to generate and use data keys for
lock table writes; update the policy statement in the "KMSStateManagement" block
to include at minimum kms:GenerateDataKey, kms:Encrypt, kms:Decrypt and
kms:CreateGrant (keeping Effect "Allow" and Resource
"${AWS_BACKEND_KMS_KEY_ARN}") so Terraform/DynamoDB can create/encrypt/decrypt
table keys and create grants for lock acquisition.
- Around line 440-450: Replace the malformed policy snippet with a valid IAM
policy document and fix the grammar: wrap the existing statement (Sid
"STSManagement", Effect "Allow", Action ["sts:AssumeRole","sts:TagSession"],
Resource "arn:aws:iam::${AWS_ACCOUNT_ID}:role/nest-staging-terraform") inside
the required top-level JSON keys ("Version": "2012-10-17" and "Statement": [ ...
]), and update the README sentence to read "staging/ also requires the following
policy to assume its role" (replace "it's" with "its" and add "to").
- Line 14: Remove the broken reference to INFO.md or add the missing INFO.md
file: either delete the line containing "Read `INFO.md` for information related
to policies" from the README, or create an `INFO.md` that duplicates the
"Required Policies" content (the AWS IAM policies documented under the "Required
Policies" section) and ensure the README link points to it; update any
anchor/link text to reference the existing "Required Policies" section if you
choose to remove INFO.md.
---
Duplicate comments:
In `@infrastructure/README.md`:
- Line 96: Replace the misleading sentence "Apply the changes to create the
backend resources" in the README with text that references the bootstrap step
(e.g., "Apply the changes to create the bootstrap resources" or "Apply the
changes to bootstrap the environment") so it correctly describes the bootstrap
step; locate the exact string "Apply the changes to create the backend
resources" and update it accordingly.
---
Nitpick comments:
In `@infrastructure/backend/main.tf`:
- Around line 114-160: The logs S3 lifecycle only sets expiration on a versioned
bucket, which deletes the current object by creating delete markers but leaves
prior versions; update aws_s3_bucket_lifecycle_configuration.logs to include a
noncurrent_version_expiration block so old object versions are actually removed
(e.g., add noncurrent_version_expiration { days = var.expire_log_days } or
another appropriate var) alongside the existing
expiration/abort_incomplete_multipart_upload so that
aws_s3_bucket_versioning.logs does not cause indefinite accumulation of old
versions.
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/README.md">
<violation number="1" location="infrastructure/README.md:442">
P2: The policy example is invalid JSON (extra object wrapper and missing statement object). This will break copy/paste usage and produce an invalid IAM policy.</violation>
</file>
<file name="infrastructure/bootstrap/main.tf">
<violation number="1" location="infrastructure/bootstrap/main.tf:118">
P1: Missing ElastiCache resource ARNs for subnet groups and replication groups. The policy grants actions like `CreateCacheSubnetGroup`, `ModifyReplicationGroup`, etc., but the `resources` block only includes `cluster:*` ARN patterns. ElastiCache subnet group and replication group operations use different ARN resource types (`subnetgroup:` and `replicationgroup:`), so these actions will be denied by IAM.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
infrastructure/bootstrap/main.tf (2)
188-196:EC2ResourceSpecificstatement is dead code — remove it.
ec2:DescribeFlowLogsandec2:DescribeNetworkAclsare already covered by theec2:Describe*wildcard inEC2Management(Line 170) onresources = ["*"]. The more restrictive scoped-resource statement here is fully superseded and has no effect.♻️ Proposed fix
- statement { - sid = "EC2ResourceSpecific" - effect = "Allow" - actions = [ - "ec2:DescribeFlowLogs", - "ec2:DescribeNetworkAcls", - ] - resources = ["arn:aws:ec2:${var.aws_region}:${data.aws_caller_identity.current.account_id}:*"] - } - statement { sid = "ECRAuth"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 188 - 196, Remove the dead IAM policy statement named EC2ResourceSpecific: the statement block that defines sid = "EC2ResourceSpecific" and lists actions ["ec2:DescribeFlowLogs","ec2:DescribeNetworkAcls"] with the scoped resources ARN should be deleted because those actions are already covered by the broader EC2Management statement (sid or block referenced as EC2Management) that grants "ec2:Describe*" on resources = ["*"]; simply delete the EC2ResourceSpecific statement block from the policy to avoid redundant, superseded code.
269-285:ECSOrchestrationbreaks per-environment isolation — add${each.key}to resource ARNs.All four resource patterns use
${var.project_name}-*without the${each.key}segment, meaning every environment role can run tasks, stop tasks, or update services on any environment's cluster or service. Every other service-level statement (ECSClusterManagement,ECSServiceManagement,ECSTaskDefinition) correctly includes${each.key}.♻️ Proposed fix
resources = [ - "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:task-definition/${var.project_name}-*:*", - "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:service/${var.project_name}-*/*", - "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:cluster/${var.project_name}-*", - "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:task/${var.project_name}-*/*" + "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:task-definition/${var.project_name}-${each.key}-*:*", + "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:service/${var.project_name}-${each.key}-*/*", + "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:cluster/${var.project_name}-${each.key}-*", + "arn:aws:ecs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:task/${var.project_name}-${each.key}-*/*" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 269 - 285, The ECSOrchestration IAM policy statement (sid "ECSOrchestration") is too permissive because its resource ARNs use "${var.project_name}-*" without the per-environment discriminator; update the four ARNs in that statement to include "${each.key}" in the name pattern (same style used by ECSClusterManagement/ECSServiceManagement/ECSTaskDefinition) so each generated role only targets its own environment; ensure you insert "${each.key}" in the ARN segment after "${var.project_name}" for task-definition, service, cluster, and task resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 87-120: The ElastiCache ARNs in the IAM statement only include the
cluster: type so ReplicationGroup and CacheSubnetGroup actions
(CreateReplicationGroup, DeleteReplicationGroup, ModifyReplicationGroup,
CreateCacheSubnetGroup, DeleteCacheSubnetGroup, ModifyCacheSubnetGroup) will be
denied; update the resources array used by this statement to also include the
appropriate ElastiCache ARN patterns for replicationgroup and subnetgroup (e.g.,
add entries analogous to
"arn:aws:elasticache:${var.aws_region}:${data.aws_caller_identity.current.account_id}:replicationgroup:${var.project_name}-${each.key}-*"
and
"arn:aws:elasticache:${var.aws_region}:${data.aws_caller_identity.current.account_id}:subnetgroup:${var.project_name}-${each.key}-*")
so the listed ElastiCache actions can succeed.
- Line 117: The CSpell checker is flagging the AWS RDS ARN segment "subgrp" (as
seen in the string "arn:aws:rds:...:subgrp:...") as an unknown word; add the
token subgrp to your CSpell custom dictionary (custom-dict.txt) so the
spellcheck accepts RDS DB subnet group ARNs. Update the dictionary by adding a
single line with subgrp, commit the change, and rerun the spellcheck to verify
the warning is cleared.
In `@infrastructure/README.md`:
- Line 397: Fix the spelling and punctuation in the README line that currently
reads "All users (`nest-staging`, `nest-bootstrap`, etc) require these minumum
policies to use the terraform remote backend:" by changing "minumum" to
"minimum" and adding a period after the parenthesis so it becomes "... etc.)
require these minimum policies to use the terraform remote backend."; update the
exact string in the README to reflect these corrections.
---
Duplicate comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 410-419: The existing IAM statement with sid "KMSManagement"
leaves kms:ListResourceTags scoped to "*"—change this by splitting the
statement: keep a statement (e.g., sid "KMSManagement_CreateAndAliases") that
allows "kms:CreateKey" and "kms:ListAliases" with resources = ["*"], and add a
separate statement (e.g., sid "KMSManagement_ListResourceTags") that grants only
"kms:ListResourceTags" with resources set to the project-specific KMS key ARNs
(use your existing aws_kms_key resources, data sources, or a variable that lists
the key ARNs). Locate the statement with sid "KMSManagement" and replace it with
these two statements so kms:ListResourceTags is no longer a wildcard.
In `@infrastructure/README.md`:
- Around line 439-454: The README's STS policy block and a grammar mistake are
incorrect: fix the malformed JSON for the nest-staging IAM User so it is valid
JSON and a proper IAM policy (remove the extra outer brace, ensure "Statement"
is an array of object(s) with string "Sid", "Effect", array "Action", and string
"Resource", e.g. a single object with "Sid": "STSManagement", "Effect": "Allow",
"Action": ["sts:AssumeRole","sts:TagSession"], "Resource":
"arn:aws:iam::${AWS_ACCOUNT_ID}:role/nest-staging-terraform"), and correct the
wording from "it's own role" to "its own role".
- Line 95: Update the misleading sentence under the "Bootstrap IAM Role" step
that currently reads "Apply the changes to create the backend resources:" so it
accurately describes this step; locate the "Bootstrap IAM Role" heading and
replace that sentence with a precise description such as "Apply the changes to
create the bootstrap IAM role and related resources:" (or similar wording) so
the step reflects creating the IAM role rather than generic backend resources.
- Around line 427-434: The KMSStateManagement statement only allows kms:Decrypt
but needs additional KMS actions for DynamoDB/remote state lock writes; update
the statement with the Sid "KMSStateManagement" to include at minimum
"kms:Encrypt", "kms:GenerateDataKey", "kms:ReEncrypt*", and "kms:DescribeKey"
(keep "kms:Decrypt" as well) and scope them to the same Resource
"${AWS_BACKEND_KMS_KEY_ARN}" so DynamoDB lock and Terraform state operations can
encrypt/decrypt and generate data keys properly.
- Around line 80-87: Remove the blank lines between the consecutive blockquote
note markers so markdownlint MD028 is not triggered: locate the three
consecutive lines beginning with "> [!NOTE]" in infrastructure/README.md and
ensure each "> [!NOTE]" block starts immediately on the next line (no empty line
separating them) so the note blocks are adjacent; if you intentionally need
spacing, combine related notes into a single block or use a different separator
(e.g., a heading) instead of blank lines between the "> [!NOTE]" blocks.
---
Nitpick comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 188-196: Remove the dead IAM policy statement named
EC2ResourceSpecific: the statement block that defines sid =
"EC2ResourceSpecific" and lists actions
["ec2:DescribeFlowLogs","ec2:DescribeNetworkAcls"] with the scoped resources ARN
should be deleted because those actions are already covered by the broader
EC2Management statement (sid or block referenced as EC2Management) that grants
"ec2:Describe*" on resources = ["*"]; simply delete the EC2ResourceSpecific
statement block from the policy to avoid redundant, superseded code.
- Around line 269-285: The ECSOrchestration IAM policy statement (sid
"ECSOrchestration") is too permissive because its resource ARNs use
"${var.project_name}-*" without the per-environment discriminator; update the
four ARNs in that statement to include "${each.key}" in the name pattern (same
style used by ECSClusterManagement/ECSServiceManagement/ECSTaskDefinition) so
each generated role only targets its own environment; ensure you insert
"${each.key}" in the ARN segment after "${var.project_name}" for
task-definition, service, cluster, and task resources.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cspell/custom-dict.txt (1)
171-173:⚠️ Potential issue | 🔴 CriticalAdd
replicationgroupto unblock the CI pipeline.The pipeline is currently failing with
CSpell: Unknown word (replicationgroup)— triggered by the newarn:aws:elasticache:...:replicationgroup:...ARNs added toinfrastructure/bootstrap/main.tf.subgrpwas correctly added (resolving the prior error), butreplicationgroupalso needs to be added.📝 Proposed fix
relativedelta + replicationgroup repositorycontributor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cspell/custom-dict.txt` around lines 171 - 173, Add the missing token "replicationgroup" to the custom dictionary so CSpell recognizes ARNs like "arn:aws:elasticache:...:replicationgroup:..."; update the same list that contains "relativedelta", "repositorycontributor", and "requirepass" by inserting "replicationgroup" (exact spelling) to unblock the CI pipeline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/bootstrap/main.tf`:
- Line 486: The IAM policy currently lists an invalid action
"lambda:GetFunctionUrlConfigs" which has no effect; replace that string with the
correct AWS Lambda action "lambda:GetFunctionUrlConfig" so the policy actually
permits fetching a function URL's configuration. Locate the policy statement
containing "lambda:GetFunctionUrlConfigs" and update the action name to the
singular form.
---
Outside diff comments:
In `@cspell/custom-dict.txt`:
- Around line 171-173: Add the missing token "replicationgroup" to the custom
dictionary so CSpell recognizes ARNs like
"arn:aws:elasticache:...:replicationgroup:..."; update the same list that
contains "relativedelta", "repositorycontributor", and "requirepass" by
inserting "replicationgroup" (exact spelling) to unblock the CI pipeline.
---
Duplicate comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 412-421: The IAM statement with sid "KMSManagement" currently
grants kms:CreateKey, kms:ListAliases, and kms:ListResourceTags to resources =
["*"]; keep kms:CreateKey in a statement with resources = ["*"] (since CreateKey
cannot be resource-scoped) and move kms:ListAliases and kms:ListResourceTags
into a new, separate statement scoped to KMS key ARNs (e.g.,
arn:aws:kms:${var.aws_region}:${data.aws_caller_identity.current.account_id}:key/*
or alias ARNs) to reduce the blast radius; update the policy by splitting the
actions into two statements (one for kms:CreateKey with resources=["*"] and one
for ListAliases/ListResourceTags with the tightened resource ARN pattern).
- Around line 597-608: The trust policy in assume_role_policy references AWS
principal ARN
"arn:aws:iam::${data.aws_caller_identity.current.account_id}:user/${var.project_name}-${each.key}"
but the README Bootstrap IAM Role section doesn't document creating those IAM
users, so role assumption will fail; either update the README to add a
prerequisite step that creates IAM users named "${var.project_name}-${each.key}"
(or documents a script/CloudFormation to create them) and examples for
var.project_name/each.key, or change the trust policy in assume_role_policy to
reference an existing principal type (e.g., IAM roles or a group/role ARN
pattern) that you actually create in this stack (adjust Principal to "AWS =
\"arn:aws:iam::${data.aws_caller_identity.current.account_id}:role/..." or use a
condition), and ensure the README and variables reflect the chosen approach so
operators can successfully assume the role.
In `@infrastructure/README.md`:
- Around line 432-447: The STS assume-role policy snippet uses the wrong Sid and
has a grammar error: replace the misleading "S3StateManagement" Sid with a
clearer "STSManagement" (update the Statement object containing "Action":
["sts:AssumeRole","sts:TagSession"] and "Resource":
"arn:aws:iam::${AWS_ACCOUNT_ID}:role/nest-staging-terraform") and fix the
sentence "to assume it's own role" to the possessive "to assume its own role".
- Around line 420-430: The KMSStateManagement IAM statement only allows
kms:Decrypt which prevents DynamoDB from acquiring/releasing Terraform state
locks; update the statement with the additional required KMS
actions—kms:GenerateDataKey, kms:Encrypt and kms:CreateGrant—so the
"KMSStateManagement" Sid (targeting Resource ${AWS_BACKEND_KMS_KEY_ARN}) permits
Decrypt, GenerateDataKey, Encrypt and CreateGrant for DynamoDB
PutItem/DeleteItem and grant creation during lock operations.
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/staging/README.md">
<violation number="1" location="infrastructure/staging/README.md:17">
P2: Remove the trailing comma in the S3 Resource array so the JSON policy is valid.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
infrastructure/bootstrap/main.tf (2)
190-198:EC2ResourceSpecificis dead code — fully shadowed byEC2Management.
ec2:DescribeFlowLogsandec2:DescribeNetworkAclsare already covered by theec2:Describe*wildcard inEC2Management(line 172) withresources = ["*"], which is more permissive than the specific ARN here. The entireEC2ResourceSpecificstatement can be removed.♻️ Proposed removal
- statement { - sid = "EC2ResourceSpecific" - effect = "Allow" - actions = [ - "ec2:DescribeFlowLogs", - "ec2:DescribeNetworkAcls", - ] - resources = ["arn:aws:ec2:${var.aws_region}:${data.aws_caller_identity.current.account_id}:*"] - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 190 - 198, The statement block labeled EC2ResourceSpecific is redundant because the EC2Management statement already grants ec2:Describe* on resources="*", which subsumes ec2:DescribeFlowLogs and ec2:DescribeNetworkAcls; remove the entire EC2ResourceSpecific statement block from the policy to eliminate dead code, and run a quick plan/apply validation to confirm no other policy depends on that SID and that overall intended permissions remain unchanged.
464-471:lambda:ListVersionsByFunctioninLambdaList(resources = ["*"]) makes the same entry inLambdaManagement(line 492) redundant.Since the
LambdaListstatement already grantslambda:ListVersionsByFunctionon*, the entry at line 492 adds no effective permission. Consider removing it fromLambdaManagementto keep the policy clean.♻️ Proposed change
statement { sid = "LambdaManagement" ... actions = [ ... "lambda:ListFunctionUrlConfigs", - "lambda:ListVersionsByFunction", "lambda:PublishVersion", ... ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 464 - 471, The LambdaList policy statement grants lambda:ListVersionsByFunction on resources = ["*"], making the identical permission in the LambdaManagement statement redundant; remove the duplicate lambda:ListVersionsByFunction entry from the LambdaManagement policy (or collapse/merge the two statements) so the policy contains a single authoritative LambdaList statement granting that action, keeping the resource scoping and other LambdaManagement actions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 488-490: The LambdaManagement IAM statement currently lists
"lambda:ListFunctions" but that action does not support resource-level ARNs;
remove "lambda:ListFunctions" from the LambdaManagement statement and add it to
the LambdaList statement (which already uses resources = ["*"]) so the action is
granted with Resource="*"; update the policy document to ensure LambdaManagement
no longer includes "lambda:ListFunctions" and LambdaList includes
"lambda:ListFunctions".
In `@infrastructure/README.md`:
- Line 13: The link fragment `#required-policies` in the README (the line "Refer
to the respective `README.md` files and [Required Policies](`#required-policies`)
for more information.") is broken; either restore a "Required Policies" heading
(e.g., "## Required Policies") with the referenced content or change the link to
an existing heading or file (for example point to an existing policies section
or to a specific README). Update the referenced text in the same README so the
anchor matches an actual heading (or replace the link with a correct
path/heading) and ensure the heading slug exactly matches `required-policies`
(case/spacing) to satisfy markdownlint MD051.
- Line 391: Remove the stray closing brace character '}' at the end of the
README.md file (it appears after the last content block and is a leftover
HCL/JSON artifact); open the file, delete that lone '}' so the file ends with
the intended Markdown content, and save the file to ensure no literal brace is
rendered in the documentation.
In `@infrastructure/staging/README.md`:
- Around line 39-47: The Sid for the STS statement that contains Action
["sts:AssumeRole","sts:TagSession"] and Resource
"arn:aws:iam::${AWS_ACCOUNT_ID}:role/nest-staging-terraform" is a duplicate
("S3StateManagement"); rename that Sid to a unique, descriptive identifier
(e.g., "STSRoleAssumption") so the statement for role assumption is distinct
from the S3 state management statement.
- Line 3: The README note only mentions ${AWS_ACCOUNT_ID} and
${AWS_BACKEND_KMS_KEY_ARN} but omits ${AWS_REGION},
${TERRAFORM_STATE_BUCKET_NAME}, and ${TERRAFORM_DYNAMODB_TABLE_NAME}; update the
note text so it lists all five placeholders (${AWS_ACCOUNT_ID}, ${AWS_REGION},
${TERRAFORM_STATE_BUCKET_NAME}, ${TERRAFORM_DYNAMODB_TABLE_NAME},
${AWS_BACKEND_KMS_KEY_ARN}) and clarifies they must be replaced with appropriate
values wherever they appear in the JSON block.
---
Nitpick comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 190-198: The statement block labeled EC2ResourceSpecific is
redundant because the EC2Management statement already grants ec2:Describe* on
resources="*", which subsumes ec2:DescribeFlowLogs and ec2:DescribeNetworkAcls;
remove the entire EC2ResourceSpecific statement block from the policy to
eliminate dead code, and run a quick plan/apply validation to confirm no other
policy depends on that SID and that overall intended permissions remain
unchanged.
- Around line 464-471: The LambdaList policy statement grants
lambda:ListVersionsByFunction on resources = ["*"], making the identical
permission in the LambdaManagement statement redundant; remove the duplicate
lambda:ListVersionsByFunction entry from the LambdaManagement policy (or
collapse/merge the two statements) so the policy contains a single authoritative
LambdaList statement granting that action, keeping the resource scoping and
other LambdaManagement actions intact.
|
Collaborator
Author
|
testing CI/CD, will reopen after that. |
rudransh-shrivastava
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Resolves #3976
Added GitHub secrets:
BOOTSTRAP_AWS_ACCESS_KEY_IDBOOTSTRAP_AWS_SECRET_ACCESS_KEYBOOTSTRAP_TF_STATE_BUCKET_NAMEBOOTSTRAP_TF_STATE_DYNAMODB_TABLE_NAMERemoved GitHub secrets:
ZAPPA<UPDATEME>Create
nest-backenduser with the policies defined ininfrastructure/backend/README.md.Backend state needs to be migrated:
Create
nest-bootstrapuser with the policies defined ininfrastructure/bootstrap/README.md.Apply the changes to create a role
nest-staginguser can assume.The
nest-staginguser needs minimum policies to access the remote state.This is defined in
infrastructure/README.mdChecklist
make check-testlocally: all warnings addressed, tests passed