Task definition is broken with pre-created secrets

[Vlaaaaaaad]

Hi,

Looks like something regarding the secrets is broken. We discovered this during our upgrade to 0.12 and the v2.1.0 of this module.

The following config used to work:

module "atlantis" {
  source  = "terraform-aws-modules/atlantis/aws"
  version = "~> 2.1.0"

  name = "atlantis"

  # VPC
  cidr            = "10.20.0.0/16"
  azs             = ["us-east-1a", "us-east-1b", "us-east-1c"]
  private_subnets = ["10.20.1.0/24", "10.20.2.0/24", "10.20.3.0/24"]
  public_subnets  = ["10.20.101.0/24", "10.20.102.0/24", "10.20.103.0/24"]

  # DNS (without trailing dot)
  route53_zone_name = "example.com"
  # Atlantis
  atlantis_github_user       = "atlantis-bot"
  atlantis_repo_whitelist    = ["github.com/terraform-aws-modules/*"]
}

provider "aws" {
  region = "us-east-1"
}

Note that that's the same code from the example, without atlantis_github_user_token defined. We don't want to have that in code so we put it in SSM.
This module is smart enough that it can take it from SSM if it's not in the Terrafrom config. That's what was happening in v1.9.0.

In the v2.1.0 this leads to the following in the JSON task definition:

     "secrets": [
        {
          "valueFrom": "unknown_secret_name_value",
          "name": "unknown_secret_name_key"
        },
        {
          "valueFrom": "/atlantis/webhook/secret",
          "name": "unknown_secret_webhook_key"
        }
      ],

This is obviously wrong and leads to the Task definition failing cause it cannot find those SSM secrets.

I think in https://github.com/terraform-aws-modules/terraform-aws-atlantis/blob/master/main.tf#L17-L23 there's something bad. That breaks the definitions in https://github.com/terraform-aws-modules/terraform-aws-atlantis/blob/master/main.tf#L67-L81. Maybe instead of the token there should've been user as I define the user, but the token is always empty.

Am I missing something?

[mbravorus]

+1

Complex unparenthesised conditionals added as part of Terraform 0.12 compatibility commit make completely no sense when trying to invoke the module in an existing infra. Secrets don't get populated, IAM policy which should allow the task to access SSM secrets doesn't get applied.

I suspect that the configuration will still work in the "standalone" mode where you have a standalone copy of the repo and create terraform.tfvars from the sample file, so that secret(s) get populated from there. For all other cases everything is broken, unless you hardcode your token into the module invocation, which is bad. Pre-created secrets are not supported at all in such configuration.

This needs to be fixed, and at the very least explicitly mentioned in documentation.

[darrylb-github]

I encountered a similar problem as I was expecting it to read an existing value in var.atlantis_github_user_token_ssm_parameter_name, but it does not and just sets it to unknown causing errors.

You should be able to work around this though without having to hardcode your token. For me I do the following in my terraform that calls this module:

data "aws_ssm_parameter" "atlantis_user_token" {
  name = "/atlantis/github/user/initial_token" # note `initial_token` vs default `token`
}
module "atlantis" {
  ...
  atlantis_github_user = "myuser"
  atlantis_github_user_token = data.aws_ssm_parameter.atlantis_user_token.value
  atlantis_gitlab_user_token_ssm_parameter_name
  ...
}

Note that you cannot use the same param path though as atlantis or it will error with ParameterAlreadyExists when the module tries to write the secret. To prevent this the module could be updated to add the overwrite option to the aws_ssm_parameter resources here. That way it would allow you to read and write from the same path, but this feels like a minor detail.

[anthonyagresta]

Those ternary operators definitely aren't working correctly when using SSM.

It seems like the logic for local.has_secrets is missing a reference to the SSM settings at all.

It might be good to have an additional "switch" variable to tell the module to use SSM instead of the regular token variables, so that the logic to coalesce whichever secret it should be using is less complicated.

[github-actions]

This issue has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this issue will be closed in 10 days

[antonbabenko]

This issue has been resolved in version 4.0.0 🎉

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.