Use existing identity provider in temporary environments

[rocketnova]

Ticket

Resolves #719

Changes

What was added, updated, or removed in this PR.

• Updates /infra/app/service to use existing idp resources for temporary environments
• Re-organizes identity provider modules into a shared folder
• Adds a new existing-identity-provider module as a wrapper for multiple data source calls
• Refactors variable names out of the identity-provider-client module and into app-config to ensure the same name (e.g. ssm name or access policy) is used across modules
• Removes prefix from identity_provider_name

Context for reviewers

Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers.

In temporary environments, such as PR preview environments, we want to use the same Cognito User Pool as the default terraform workspace. That allows QA and UAT to use existing users instead of having to go through an account creation flow on every PR.

This PR checks to see if it's a temporary environment. If it is, it uses data sources to fetch existing resources. If it's not, it creates a new identity provider, just as before.

Testing

Provide evidence that the code works as expected. Explain what was done for testing and the results of the test plan. Include screenshots, GIF demos, shell commands or output to help show the changes working as expected. ProTip: you can drag and drop or paste images into this textbox.

Tested at navapbc/platform-test#128

IAM role for the preview environment has default workspace access policy successfully attached:

ECS task definition for the preview environment service has correct cognito environment variables (I know I redacted the details out, but wanted to give the gist):

ECS task definition for the preview environment service has correct cognito secret arn:

Preview environment successfully applied the updates:

[lorenyu]

Looks great, had a comment about the new pattern of grouping related modules

[lorenyu]

Take a look at the comment I left on infra/modules/identity-provider/README.md, but I think we can have a new pattern that eliminates the need for config variables that are used primarily for dependency management rather than for actual configuration. tldr; I think we can move the definition of these things to something like a modules/identity/interface/ module that can be used by both the module with data sources and the module that actually creates the resources

[coilysiren]

Great layout here 👍🏼 👍🏼 👍🏼

[rocketnova]

@lorenyu @coilysiren Re-requesting review. This PR has been re-architected to incorporate the feedback around module structure. The test can be seen: navapbc/platform-test#128

[lorenyu]

Looks good just left some nits.

Wish we had platform-test-rails up so we can see it in action in the PR environment. Otherwise it's a little bit more of a leap of faith. But I think it's still ok.

[lorenyu]

I don't understand this comment. What does it mean to "choose not to use workspaces"?

[rocketnova]

Oh funny. I copy/pasted this directly from

template-infra/infra/app/app-config/env-config/main.tf

Lines 2 to 5 in 7a9b157

	# The prefix key/value pair is used for Terraform Workspaces, which is useful for projects with multiple infrastructure developers.
	# By default, Terraform creates a workspace named “default.” If a non-default workspace is not created this prefix will equal “default”,
	# if you choose not to use workspaces set this value to "dev"
	prefix = terraform.workspace == "default" ? "" : "${terraform.workspace}-"

[lorenyu]

what was i thinking

[rocketnova]

What if we change the comment here and in /infra/app/app-config/env-config/main.tf to this:

locals {
  # The prefix is used to create uniquely named resources per terraform workspace, which 
  # are needed in CI/CD for preview environments and tests. 
  #
  # To isolate changes during infrastructure development by using manually created 
  # terraform workspaces, see: /docs/infra/develop-and-test-infrastructure-in-isolation-using-workspaces.md.
  prefix = terraform.workspace == "default" ? "" : "${terraform.workspace}-"

  bucket_name = "${local.prefix}${var.project_name}-${var.app_name}-${var.environment}"
}

[lorenyu]

looks great

[rocketnova]

Looks good just left some nits.

No objections to your nits. Let me do a quick test over at platform-test to verify everything is working as expected.

Wish we had platform-test-rails up so we can see it in action in the PR environment. Otherwise it's a little bit more of a leap of faith. But I think it's still ok.

Agree about platform-test-rails. I'll add some screenshots from navapbc/platform-test#128 to demonstrate what it's doing on the test environment. Apologies for failing to remember to attach these to both this PR and the one in platform-test.

[rocketnova]

Oh I did add screenshots to this PR already. Here they are again from the latest platform-test run.

First, here's the dev environment's user pool app-dev:

Then, here's the platform-test CI service creating a client app in the same user pool and the PR preview branch also creating a client app in the same user pool.

[lorenyu]

I see an error in the template CI

I think you may need to set identity_provider_user_pool_id to null if enable_identity_provider is false

[rocketnova]

I see an error in the template CI
I think you may need to set identity_provider_user_pool_id to null if enable_identity_provider is false

Yeah, I'm looking at that error, too. Sadly, I think it's more complicated than that. I think there's a problem with using temporary environments to control whether or not to use an existing user pool.

[rocketnova]

I see an error in the template CI
I think you may need to set identity_provider_user_pool_id to null if enable_identity_provider is false

Yeah, I'm looking at that error, too. Sadly, I think it's more complicated than that. I think there's a problem with using temporary environments to control whether or not to use an existing user pool.

Loren and I chatted about this offline and I fixed the error. I was confusing this issue with a different problem.