[ Issue 567 ] Remove tfvars from infrastructure config (#569)

* terraform env variables * Upgrade python 3.12 update * Next version update * template infra changes --------- Co-authored-by: acouch <[email protected]> Co-authored-by: Daphne Gold <[email protected]>
HHS · Oct 12, 2023 · 1bf8744 · 1bf8744
1 parent 88adc4f
commit 1bf8744
Show file tree

Hide file tree

Showing 60 changed files with 1,322 additions and 1,172 deletions.
diff --git a/.dockleconfig b/.dockleconfig
@@ -3,4 +3,4 @@
 # DOCKLE_ACCEPT_FILES="file1,path/to/file2,file3/path,etc"
 # https://github.com/goodwithtech/dockle#accept-suspicious-environment-variables--files--file-extensions
 # The apiflask/settings file is a stub file that apiflask creates, and has no sensitive data in. We are ignoring it since it is unused
-DOCKLE_ACCEPT_FILES=api/.venv/lib/python3.11/site-packages/apiflask/settings.py
+DOCKLE_ACCEPT_FILES=api/.venv/lib/python3.12/site-packages/apiflask/settings.py
diff --git a/.github/actions/configure-aws-credentials/action.yml b/.github/actions/configure-aws-credentials/action.yml
@@ -23,14 +23,14 @@ runs:
         echo "::group::AWS account authentication details"
 
         terraform -chdir=infra/project-config init > /dev/null
-        terraform -chdir=infra/project-config refresh > /dev/null
+        terraform -chdir=infra/project-config apply -refresh-only -auto-approve> /dev/null
         AWS_REGION=$(terraform -chdir=infra/project-config output -raw default_region)
         echo "AWS_REGION=$AWS_REGION"
         GITHUB_ACTIONS_ROLE_NAME=$(terraform -chdir=infra/project-config output -raw github_actions_role_name)
         echo "GITHUB_ACTIONS_ROLE_NAME=$GITHUB_ACTIONS_ROLE_NAME"
 
         terraform -chdir=infra/${{ inputs.app_name }}/app-config init > /dev/null
-        terraform -chdir=infra/${{ inputs.app_name }}/app-config refresh > /dev/null
+        terraform -chdir=infra/${{ inputs.app_name }}/app-config apply -refresh-only -auto-approve> /dev/null
         ACCOUNT_NAME=$(terraform -chdir=infra/${{ inputs.app_name }}/app-config output -json account_names_by_environment | jq -r .${{ inputs.environment }})
         echo "ACCOUNT_NAME=$ACCOUNT_NAME"
 

diff --git a/.grype.yml b/.grype.yml
@@ -16,3 +16,6 @@ ignore:
   - fix-state: not-fixed
   - fix-state: wont-fix
   - fix-state: unknown
+  # https://github.com/anchore/grype/issues/1172
+  - vulnerability: GHSA-xqr8-7jwr-rhp7
+  - vulnerability: GHSA-7fh5-64p2-3v2j
diff --git a/.template-version b/.template-version
@@ -1 +1 @@
-1a5444db0b4d3f85021f729affcf0b014ae3e81c
+fe5c7cd24d3c2c9f15c342826cda0a20af4cd0a5
diff --git a/Makefile b/Makefile
@@ -108,13 +108,12 @@ infra-update-app-database-roles: ## Create or update database roles and schemas
 	@:$(call check_defined, ENVIRONMENT, the name of the application environment e.g. "prod" or "staging")
 	./bin/create-or-update-database-roles.sh $(APP_NAME) $(ENVIRONMENT)
 
-# TODO: update to no longer use tfvars file: https://github.com/navapbc/template-infra/blob/main/docs/decisions/infra/0008-consolidate-infra-config-from-tfvars-files-into-config-module.md
 infra-update-app-service: ## Create or update $APP_NAME's web service module
 	# APP_NAME has a default value defined above, but check anyways in case the default is ever removed
 	@:$(call check_defined, APP_NAME, the name of subdirectory of /infra that holds the application's infrastructure code)
 	@:$(call check_defined, ENVIRONMENT, the name of the application environment e.g. "prod" or "staging")
 	terraform -chdir="infra/$(APP_NAME)/service" init -input=false -reconfigure -backend-config="$(ENVIRONMENT).s3.tfbackend"
-	terraform -chdir="infra/$(APP_NAME)/service" apply -var-file="$(ENVIRONMENT).tfvars"
+	terraform -chdir="infra/$(APP_NAME)/service" apply -var="environment_name=$(ENVIRONMENT)"
 
 # The prerequisite for this rule is obtained by
 # prefixing each module with the string "infra-validate-module-"

diff --git a/api/openapi.generated.yml b/api/openapi.generated.yml
@@ -239,6 +239,9 @@ components:
       properties:
         type:
           description: The name of the role
+          enum:
+          - USER
+          - ADMIN
     User:
       type: object
       properties:

diff --git a/api/poetry.lock b/api/poetry.lock
diff --git a/api/pyproject.toml b/api/pyproject.toml
@@ -21,6 +21,7 @@ marshmallow-dataclass = {extras = ["enum", "union"], version = "^8.5.8"}
 marshmallow = "^3.18.0"
 pip-licenses = "^4.3.2"
 gunicorn = "^21.2.0"
+setuptools = "^68.2.2"
 
 [tool.poetry.group.dev.dependencies]
 black = "^22.6.0"
@@ -35,7 +36,7 @@ coverage = "^6.4.4"
 Faker = "^14.2.0"
 factory-boy = "^3.2.1"
 bandit = "^1.7.4"
-pytest = "^6.0.0"
+pytest = "7.4.2"
 pytest-watch = "^4.2.0"
 pytest-lazy-fixture = "^0.6.3"
 

diff --git a/api/src/db/migrations/env.py b/api/src/db/migrations/env.py
@@ -30,7 +30,7 @@
 
     def include_object(
         object: sqlalchemy.schema.SchemaItem,
-        name: str,
+        name: str | None,
         type_: str,
         reflected: bool,
         compare_to: Any,

diff --git a/api/src/db/migrations/run.py b/api/src/db/migrations/run.py
@@ -1,16 +1,13 @@
 # Convenience script for running alembic migration commands through a pyscript
 # rather than the command line. This allows poetry to package and alias it for
 # running on the production docker image from any directory.
-import itertools
 import logging
 import os
-from typing import Optional
 
 import alembic.command as command
 import alembic.script as script
 import sqlalchemy
 from alembic.config import Config
-from alembic.operations.ops import MigrationScript
 from alembic.runtime import migration
 
 logger = logging.getLogger(__name__)
@@ -46,48 +43,11 @@ def have_all_migrations_run(db_engine: sqlalchemy.engine.Engine) -> None:
                 (
                     "The database schema is not in sync with the migrations."
                     "Please verify that the migrations have been"
-                    f"run up to {expected_heads}; currently at {current_heads}"
+                    f"run up to {expected_heads}\n"
+                    f"currently at {current_heads}"
                 )
             )
 
         logger.info(
             f"The current migration head is up to date, {current_heads} and Alembic is expecting {expected_heads}"
         )
-
-
-def check_model_parity() -> None:
-    revisions: list[MigrationScript] = []
-
-    def process_revision_directives(
-        context: migration.MigrationContext,
-        revision: Optional[str],
-        directives: list[MigrationScript],
-    ) -> None:
-        nonlocal revisions
-        revisions = list(directives)
-        # Prevent actually generating a migration
-        directives[:] = []
-
-    command.revision(
-        config=alembic_cfg,
-        autogenerate=True,
-        process_revision_directives=process_revision_directives,
-    )
-    diff = list(
-        itertools.chain.from_iterable(
-            op.as_diffs() for script in revisions for op in script.upgrade_ops_list
-        )
-    )
-
-    message = (
-        "The application models are not in sync with the migrations. You should generate "
-        "a new automigration or update your local migration file. "
-        "If there are unexpected errors you may need to merge main into your branch."
-    )
-
-    if diff:
-        for line in diff:
-            print("::error title=Missing migration::Missing migration:", line)
-
-        logger.error(message, extra={"issues": str(diff)})
-        raise Exception(message)
diff --git a/api/src/logging/audit.py b/api/src/logging/audit.py
@@ -72,7 +72,9 @@ def handle_audit_event(event_name: str, args: tuple[Any, ...]) -> None:
 def log_audit_event(event_name: str, args: Sequence[Any], arg_names: Sequence[str]) -> None:
     """Log a message but only log recently repeated messages at intervals."""
     extra = {
-        f"audit.args.{arg_name}": arg for arg_name, arg in zip(arg_names, args) if arg_name != "_"
+        f"audit.args.{arg_name}": arg
+        for arg_name, arg in zip(arg_names, args, strict=True)
+        if arg_name != "_"
     }
 
     key = (event_name, repr(args))

diff --git a/api/tests/lib/db_testing.py b/api/tests/lib/db_testing.py
@@ -39,11 +39,11 @@ def _create_schema(conn: db.Connection, schema_name: str):
     """Create a database schema."""
     db_test_user = get_db_config().username
 
-    conn.execute(f"CREATE SCHEMA IF NOT EXISTS {schema_name} AUTHORIZATION {db_test_user};")
+    conn.execute(f"CREATE SCHEMA IF NOT EXISTS {schema_name} AUTHORIZATION {db_test_user}")
     logger.info("create schema %s", schema_name)
 
 
 def _drop_schema(conn: db.Connection, schema_name: str):
     """Drop a database schema."""
-    conn.execute(f"DROP SCHEMA {schema_name} CASCADE;")
+    conn.execute(f"DROP SCHEMA {schema_name} CASCADE")
     logger.info("drop schema %s", schema_name)
diff --git a/api/tests/src/logging/test_audit.py b/api/tests/src/logging/test_audit.py
@@ -141,7 +141,7 @@ def test_audit_hook(
         pass
 
     assert len(caplog.records) == len(expected_records)
-    for record, expected_record in zip(caplog.records, expected_records):
+    for record, expected_record in zip(caplog.records, expected_records, strict=True):
         assert record.levelname == "AUDIT"
         assert_record_match(record, expected_record)
 
@@ -161,7 +161,7 @@ def test_os_kill(init_audit_hook, caplog: pytest.LogCaptureFixture):
     ]
 
     assert len(caplog.records) == len(expected_records)
-    for record, expected_record in zip(caplog.records, expected_records):
+    for record, expected_record in zip(caplog.records, expected_records, strict=True):
         assert record.levelname == "AUDIT"
         assert_record_match(record, expected_record)
 
@@ -232,7 +232,7 @@ def test_repeated_audit_logs(
     ]
 
     assert len(caplog.records) == len(expected_records)
-    for record, expected_record in zip(caplog.records, expected_records):
+    for record, expected_record in zip(caplog.records, expected_records, strict=True):
         assert record.levelname == "AUDIT"
         assert_record_match(record, expected_record)
 

diff --git a/api/tests/src/logging/test_flask_logger.py b/api/tests/src/logging/test_flask_logger.py
@@ -83,7 +83,7 @@ def test_request_lifecycle_logs(
     # then the log message in the after_request handler.
 
     assert len(caplog.records) == len(expected_extras)
-    for record, expected_extra in zip(caplog.records, expected_extras):
+    for record, expected_extra in zip(caplog.records, expected_extras, strict=True):
         assert_dict_contains(record.__dict__, expected_extra)
 
 

diff --git a/bin/configure-monitoring-secret.sh b/bin/configure-monitoring-secret.sh
@@ -17,7 +17,7 @@ ENVIRONMENT=$2
 INTEGRATION_ENDPOINT_URL=$3
 
 terraform -chdir="infra/$APP_NAME/app-config" init > /dev/null
-terraform -chdir="infra/$APP_NAME/app-config" refresh > /dev/null
+terraform -chdir="infra/$APP_NAME/app-config" apply -refresh-only -auto-approve> /dev/null
 
 HAS_INCIDENT_MANAGEMENT_SERVICE=$(terraform -chdir="infra/$APP_NAME/app-config" output -raw has_incident_management_service)
 if [ "$HAS_INCIDENT_MANAGEMENT_SERVICE" = "false" ]; then

diff --git a/bin/publish-release.sh b/bin/publish-release.sh
@@ -16,7 +16,7 @@ echo "  IMAGE_TAG=$IMAGE_TAG"
 
 # Need to init module when running in CD since GitHub actions does a fresh checkout of repo
 terraform -chdir="infra/$APP_NAME/app-config" init > /dev/null
-terraform -chdir="infra/$APP_NAME/app-config" refresh > /dev/null
+terraform -chdir="infra/$APP_NAME/app-config" apply -refresh-only -auto-approve> /dev/null
 IMAGE_REPOSITORY_NAME=$(terraform -chdir="infra/$APP_NAME/app-config" output -raw image_repository_name)
 
 REGION=$(./bin/current-region.sh)

diff --git a/bin/run-command.sh b/bin/run-command.sh
@@ -2,26 +2,50 @@
 # -----------------------------------------------------------------------------
 # Run an application command using the application image
 #
+# Optional parameters:
+#   --environment-variables - a JSON list of environment variables to add to the
+#     the container. Each environment variable is an object with the "name" key
+#     specifying the name of the environment variable and the "value" key
+#     specifying the value of the environment variable.
+#     e.g. '[{ "name" : "DB_USER", "value" : "migrator" }]'
+#   --task-role-arn - the IAM role ARN that the task should assume. Overrides the
+#     task role specified in the task definition.
+#
 # Positional parameters:
-#   APP_NAME (required) – the name of subdirectory of /infra that holds the
+#   APP_NAME (required) - the name of subdirectory of /infra that holds the
 #     application's infrastructure code.
-#   ENVIRONMENT (required) – the name of the application environment (e.g. dev,
+#   ENVIRONMENT (required) - the name of the application environment (e.g. dev,
 #     staging, prod)
-#   COMMAND (required) – a JSON list representing the command to run
+#   COMMAND (required) - a JSON list representing the command to run
 #     e.g. To run the command `db-migrate-up` with no arguments, set
 #     COMMAND='["db-migrate-up"]'
 #     e.g. To run the command `echo "Hello, world"` set
 #     COMMAND='["echo", "Hello, world"]')
 # -----------------------------------------------------------------------------
 set -euo pipefail
 
-# TODO: Add ability to change task IAM Role. Part 3 of multipart update https://github.com/navapbc/template-infra/issues/354#issuecomment-1693973424
-# TODO: Change to keyword arguments. Part 3 of multipart update https://github.com/navapbc/template-infra/issues/354#issuecomment-1693973424
+# Parse optional parameters
+ENVIRONMENT_VARIABLES=""
+TASK_ROLE_ARN=""
+while :; do
+  case $1 in
+    --environment-variables)
+      ENVIRONMENT_VARIABLES=$2
+      shift 2
+      ;;
+    --task-role-arn)
+      TASK_ROLE_ARN=$2
+      shift 2
+      ;;
+    *)
+      break
+      ;;
+  esac
+done
 
 APP_NAME="$1"
 ENVIRONMENT="$2"
 COMMAND="$3"
-ENVIRONMENT_VARIABLES=${4:-""}
 
 echo "==============="
 echo "Running command"
@@ -30,7 +54,8 @@ echo "Input parameters"
 echo "  APP_NAME=$APP_NAME"
 echo "  ENVIRONMENT=$ENVIRONMENT"
 echo "  COMMAND=$COMMAND"
-echo "  ENVIRONMENT_VARIABLES=$ENVIRONMENT_VARIABLES"
+echo "  ENVIRONMENT_VARIABLES=${ENVIRONMENT_VARIABLES:-}"
+echo "  TASK_ROLE_ARN=${TASK_ROLE_ARN:-}"
 echo
 
 # Use the same cluster, task definition, and network configuration that the application service uses
@@ -51,16 +76,12 @@ NETWORK_CONFIG=$(aws ecs describe-services --no-cli-pager --cluster "$CLUSTER_NA
 CURRENT_REGION=$(./bin/current-region.sh)
 AWS_USER_ID=$(aws sts get-caller-identity --no-cli-pager --query UserId --output text)
 
-ENVIRONMENT_OVERRIDES=""
-if [ -n "$ENVIRONMENT_VARIABLES" ]; then
-  ENVIRONMENT_OVERRIDES="\"environment\": $ENVIRONMENT_VARIABLES,"
-fi
 CONTAINER_NAME=$(aws ecs describe-task-definition --task-definition "$TASK_DEFINITION_FAMILY" --query "taskDefinition.containerDefinitions[0].name" --output text)
+
 OVERRIDES=$(cat << EOF
 {
   "containerOverrides": [
     {
-      $ENVIRONMENT_OVERRIDES
       "name": "$CONTAINER_NAME",
       "command": $COMMAND
     }
@@ -69,6 +90,14 @@ OVERRIDES=$(cat << EOF
 EOF
 )
 
+if [ -n "$ENVIRONMENT_VARIABLES" ]; then
+  OVERRIDES=$(echo "$OVERRIDES" | jq ".containerOverrides[0].environment |= $ENVIRONMENT_VARIABLES")
+fi
+
+if [ -n "$TASK_ROLE_ARN" ]; then
+  OVERRIDES=$(echo "$OVERRIDES" | jq ".taskRoleArn |= \"$TASK_ROLE_ARN\"")
+fi
+
 TASK_START_TIME=$(date +%s)
 TASK_START_TIME_MILLIS=$((TASK_START_TIME * 1000))
 
@@ -104,8 +133,17 @@ LOG_STREAM="$LOG_STREAM_PREFIX/$CONTAINER_NAME/$ECS_TASK_ID"
 # task that completes quickly can go from PENDING to STOPPED, causing the wait
 # command to error out.
 echo "Waiting for log stream to be created"
+echo "  TASK_ARN=$TASK_ARN"
+echo "  TASK_ID=$ECS_TASK_ID"
 echo "  LOG_STREAM=$LOG_STREAM"
+
+NUM_RETRIES_WAITIN_FOR_LOGS=0
 while true; do
+  NUM_RETRIES_WAITIN_FOR_LOGS=$((NUM_RETRIES_WAITIN_FOR_LOGS+1))
+  if [ $NUM_RETRIES_WAITIN_FOR_LOGS -eq 20 ]; then
+    echo "Timing out task $ECS_TASK_ID waiting for logs"
+    exit 1
+  fi
   IS_LOG_STREAM_CREATED=$(aws logs describe-log-streams --no-cli-pager --log-group-name "$LOG_GROUP" --query "length(logStreams[?logStreamName==\`$LOG_STREAM\`])")
   if [ "$IS_LOG_STREAM_CREATED" == "1" ]; then
     break
@@ -161,7 +199,13 @@ done
 echo "::endgroup::"
 echo
 
-CONTAINER_EXIT_CODE=$(aws ecs describe-tasks --cluster "$CLUSTER_NAME" --tasks "$TASK_ARN" --query "tasks[0].containers[?name=='$CONTAINER_NAME'].exitCode" --output text)
+CONTAINER_EXIT_CODE=$(
+  aws ecs describe-tasks \
+  --cluster "$CLUSTER_NAME" \
+  --tasks "$TASK_ARN" \
+  --query "tasks[0].containers[?name=='$CONTAINER_NAME'].exitCode" \
+  --output text
+)
 
 if [[ "$CONTAINER_EXIT_CODE" == "null" || "$CONTAINER_EXIT_CODE" != "0" ]]; then
   echo "Task failed" >&2

diff --git a/bin/run-database-migrations.sh b/bin/run-database-migrations.sh
@@ -13,8 +13,6 @@
 #     staging, prod)
 # -----------------------------------------------------------------------------
 
-# TODO: Use migrator role instead of general role. Part 3 of multipart update https://github.com/navapbc/template-infra/issues/354#issuecomment-1693973424
-
 set -euo pipefail
 
 APP_NAME="$1"
@@ -32,7 +30,7 @@ echo
 echo "Step 0. Check if app has a database"
 
 terraform -chdir="infra/$APP_NAME/app-config" init > /dev/null
-terraform -chdir="infra/$APP_NAME/app-config" refresh > /dev/null
+terraform -chdir="infra/$APP_NAME/app-config" apply -refresh-only -auto-approve> /dev/null
 HAS_DATABASE=$(terraform -chdir="infra/$APP_NAME/app-config" output -raw has_database)
 if [ "$HAS_DATABASE" = "false" ]; then
   echo "Application does not have a database, no migrations to run"
@@ -41,6 +39,9 @@ fi
 
 DB_MIGRATOR_USER=$(terraform -chdir="infra/$APP_NAME/app-config" output -json environment_configs | jq -r ".$ENVIRONMENT.database_config.migrator_username")
 
+./bin/terraform-init.sh "infra/$APP_NAME/service" "$ENVIRONMENT"
+MIGRATOR_ROLE_ARN=$(terraform -chdir="infra/$APP_NAME/service" output -raw migrator_role_arn)
+
 echo
 echo "::group::Step 1. Update task definition without updating service"
 
@@ -58,4 +59,4 @@ ENVIRONMENT_VARIABLES=$(cat << EOF
 EOF
 )
 
-./bin/run-command.sh "$APP_NAME" "$ENVIRONMENT" "$COMMAND" "$ENVIRONMENT_VARIABLES"
+./bin/run-command.sh --task-role-arn "$MIGRATOR_ROLE_ARN" --environment-variables "$ENVIRONMENT_VARIABLES" "$APP_NAME" "$ENVIRONMENT" "$COMMAND"
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		1a5444db0b4d3f85021f729affcf0b014ae3e81c
		fe5c7cd24d3c2c9f15c342826cda0a20af4cd0a5