fix: mark outputs as sensitive

[syphernl]

what

• Marks the outputs as sensitive
• Update workflows etc. missed by context.tf updated to v0.24.1, minimum required Terraform version bumped to 0.13.0 when needed, readme updated #119

why

• Otherwise TF 0.14 would give an Error: Output refers to sensitive values when using these outputs to feed into other modules (e.g. terraform-aws-ecs-alb-service-task)
• Keep modules in sync per request of Cloud Posse

references

• https://www.terraform.io/upgrade-guides/0-14.html#sensitive-values-in-plan-output

[syphernl]

Verified that this change actually resolves my problem (I can now properly feed in information from this module into terraform-aws-ecs-alb-service-task.

[Gowiem]

/test all

[Gowiem]

@syphernl we're looking into why this validate-codeowners still is failing. We'll circle back once it's figured.

This Pull Request has been updated, so we're dismissing all reviews.

[Nuru]

/test all

[Nuru]

@osterman This module had never been converted to context.tf and does not really need to be, since it does not call any other modules and takes an explicit ID (container_name). Furthermore, it already has an environment variable declared.

terraform-aws-ecs-container-definition/variables.tf

Lines 84 to 91 in 9aaf88b

	variable "environment" {
	type = list(object({
	name = string
	value = string
	}))
	description = "The environment variables to pass to the container. This is a list of maps. map_environment overrides environment"
	default = []
	}

I started to convert this module to context.tf by renaming environment to containter_environment, but then I thought since that is a breaking change, maybe we should just leave it as is and not use context.tf here since it is, in fact, ignored . What do you think? Maybe we could take a dummy context input just for consistency.

[osterman]

@Nuru I agree: this module should not use context.tf pattern

[Nuru]

/test all

[Nuru]

/test all

[dekimsey]

Just encountered this change, what inputs were being given to this module that required the use of sensitive? I'm having trouble understanding why the change was done. This breaks any diff outputs and I'm trying to figure out what was going on that necessitated this change. In every case I can come up with, there shouldn't be any secrets emitted in these outputs.

[Gowiem]

@dekimsey -- @syphernl specifically mentioned the terraform-aws-ecs-alb-service-task module, which is I'm assuming where he ran into the issue at.

Can you expand on what you mean by it break diff outputs? Keep in mind that you can still get the raw output values that are sensitive by utilizing -json.

I suggest bringing this up in Slack or our forum if you want to surface this with a wider audience and make headway on it.

[dekimsey]

Thanks @Gowiem, I posted on Slack, but I don't think @syphernl is available there.

Well, in essence diffs no longer work since they are being hidden by (sensitive) but the container definition is not sensitive. I used to be able to trivially verify container definition changes, and that's no longer available as a result of this change.

I'm concerned as I believe this change was the result of secrets being passed in improperly by the OP, but I checked and was unable to find the described issue anywhere. What inputs were being used that tripped the sensitive flag? The terraform-aws-ecs-alb-service-task makes no mention or use of any secrets that I can find.

I use this module all over the place without issue. The correct way to pass a secret to a container is via secrets which uses the SecretsManager or SSM Parameter Store. Using those, all we end up is with an ARN to secret reference.

Since the sensitive flag tends to make other things things sensitive, I think we should be very careful of what is marked sensitive.

[syphernl]

Thanks @Gowiem, I posted on Slack, but I don't think @syphernl is available there.

I am on Slack, but under a different name. Can't check it now though since it's my work account and I have the day off today.

I'm concerned as I believe this change was the result of secrets being passed in improperly by the OP, but I checked and was unable to find the described issue anywhere. What inputs were being used that tripped the sensitive flag? The terraform-aws-ecs-alb-service-task makes no mention or use of any secrets that I can find.

I'm not a fan of this approach either but it seemed to be the only way to get the states to work again. None of my inputs are marked as sensitive, yet Terraform seems to desire it that the outputs are set as such.

I use this module all over the place without issue. The correct way to pass a secret to a container is via secrets which uses the SecretsManager or SSM Parameter Store. Using those, all we end up is with an ARN to secret reference.

I'm also passing along secrets via the secrets variable, but also a few environment variables. However, these are not marked as secret so it shouldn't result in the output needing to be marked as sensitive.

[dekimsey]

I'm not a fan of this approach either but it seemed to be the only way to get the states to work again. None of my inputs are marked as sensitive, yet Terraform seems to desire it that the outputs are set as such.
I'm also passing along secrets via the secrets variable, but also a few environment variables. However, these are not marked as secret so it shouldn't result in the output needing to be marked as sensitive.

Well that's just strange, I wonder if there is a bug there with how its passing the sensitive flag. I know there is an open ticket regarding transformations that talks about this a bit, hashicorp/terraform#27337.

[Gowiem]

Calling in @nitrocode as he's the primary maintainer for the module AFAIK.

[nitrocode]

It seems like marking all the outputs sensitive is not the best approach here. The root issue is putting secrets as environment variables.

I'd rather us revert this commit 5d0c6c2 to remove the sensitive argument from each of those outputs.

[dekimsey]

Great!

That leaves us with the original problem to figure out. Something in the inputs was causing the sensitive flags to propagate. @syphernl, when you return to work, would you be able willing to check if your implementation was perhaps tripping on hashicorp/terraform#27337? I'm hoping that explains the issue you were experiencing.

[joshmyers]

Just hit this and is less than helpful being marked as sensitive being unable to see changes to the container_definition

Terraform will perform the following actions:

  # module.ecs_alb_service_task.aws_ecs_task_definition.default[0] will be updated in-place
  ~ resource "aws_ecs_task_definition" "default" {
      # Warning: this attribute value will be marked as sensitive and will
      # not display in UI output after applying this change
      ~ container_definitions    = (sensitive)
        id                       = "badgers-global-build-info-service"
      + ipc_mode                 = ""
      + pid_mode                 = ""
        tags                     = {
            "Environment"       = "global"
            "Name"              = "badgers-global-build-info-service"
            "Namespace"         = "badgers"
        }
        # (9 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

------------------------------------------------------------------------

[syphernl]

That leaves us with the original problem to figure out. Something in the inputs was causing the sensitive flags to propagate. @syphernl, when you return to work, would you be able willing to check if your implementation was perhaps tripping on hashicorp/terraform#27337? I'm hoping that explains the issue you were experiencing.

@dekimsey As soon as I include the secrets parameter within a terraform-aws-ecs-container-definition Terraform starts complaining about the sensitive secrets.
However, all these secrets use this notation (as is required by the module):

    {
      name      = "APP_KEY",
      valueFrom = module.store_write.arn_map[local.app_key_parameter]
    },

@joshmyers @dekimsey Aren't you perhaps not using secrets in your container definitions? Because that would explain why it worked fine for you and not for my usecase.

[nitrocode]

Maybe one way to fix this would be to have both non sensitive and sensitive versions of the outputs.

Could you provide a reproducible terraform code of the issue?

what terraform version and aws provider version are you running?

[syphernl]

@nitrocode

Maybe one way to fix this would be to have both non sensitive and sensitive versions of the outputs.

Ah yeah, I think that may work :)

Could you provide a reproducible terraform code of the issue?

module "demo_container" {
  source          = "git::https://github.com/cloudposse/terraform-aws-ecs-container-definition.git?ref=0.49.2"
  container_name  = "demo-container"
  container_image = "nginxdemos/hello:latest"

  secrets = [
    # App specific
    {
      name      = "APP_KEY",
      valueFrom = module.store_write_demo.arn_map[local.demo_app_key_parameter]
    }
  ]

  environment = [
    {
      name  = "APP_ENV",
      value = "production",
    }
  ]

  port_mappings = [
    {
      containerPort = 80
      hostPort      = 80
      protocol      = "tcp"
    }
  ]

  log_configuration = {
    logDriver = "awslogs"
    options = {
      "awslogs-region"        = var.aws_region
      "awslogs-group"         = join("", aws_cloudwatch_log_group.our_log_group.*.name)
      "awslogs-stream-prefix" = "app"
    }
  }

  healthcheck = {
    command     = ["CMD-SHELL", "curl -f http://localhost:80/health || exit 1"]
    retries     = 5
    timeout     = 5
    interval    = 30
    startPeriod = 30
  }
}

what terraform version and aws provider version are you running?

We're on Terraform 0.14.7 / AWS Provider 3.29.1