wellcomecollection / identity

Identity services for Wellcome Collection users
MIT License
0 stars 2 forks source link

Reduce the repetition in the identity Terraform #366

Closed alexwlchan closed 1 year ago

alexwlchan commented 1 year ago

While working on #365, I found some of the Terraform very verbose and difficult to follow.

It's extremely repetitive, so I've tried to use for_each (and maybe later modules) to consolidate it.

I'm using the new Terraform moved blocks to handle the state management, which I've checked in as part of the commit history (although then deleted in a subsequent commit). If we decide to revert this, we can flip all this blocks to remove this change.

Reviewing suggestions

There are very few changes that affected the deployed infrastructure; those that did are listed in the comments below.

The two interesting files are:

A lot of their repetitive bits have been pushed down into a new route module, which should make it much simpler to add my /resend_verification route.

alexwlchan commented 1 year ago

One thing that's becoming very clear is that we're inconsistent about what error codes we handle in our API Gateway method responses. I don't want to fix this as part of this PR – this is deliberately trying to be a no-op on the existing state, and once I start changing things that's much harder to verify.

alexwlchan commented 1 year ago

Okay, so trying to modularise all this code is revealing places where it's inconsistent. I am going to make a small number of changes rather than shoehorn in workarounds we'll likely remove later.

  # module.api_gw_resource_users_registration.aws_api_gateway_method_response.put_success["200"] will be updated in-place
  # (moved from aws_api_gateway_method_response.users_userid_registration_put_200)
  ~ resource "aws_api_gateway_method_response" "put_success" {
        id                  = "agmr-m7u12kxh7k-lb784z-PUT-200"
      ~ response_models     = {
          - "application/json" = "Empty" -> null
        }
        # (5 unchanged attributes hidden)
    }

  # module.api_gw_resource_users_userid_password.aws_api_gateway_method_response.put_success["200"] will be updated in-place
  # (moved from aws_api_gateway_method_response.users_userid_password_put_200)
  ~ resource "aws_api_gateway_method_response" "put_success" {
        id                  = "agmr-m7u12kxh7k-29vc7a-PUT-200"
      ~ response_models     = {
          - "application/json" = "Empty" -> null
        }
        # (5 unchanged attributes hidden)
    }

These are the 200 OK PUT method response for /users/:user_id/registration and /users/:user_id/password.

I don't want to declare it as empty for all successful PUT responses in case some of them do return content; this seems like an insignificant change.

alexwlchan commented 1 year ago

Also PUT for /users/:user_id/deletion-request

  # module.api_gw_resource_users_userid_deletion-request.aws_api_gateway_method_response.put_success["200"] will be updated in-place
  # (moved from aws_api_gateway_method_response.users_userid_deletion-request_put_200)
  ~ resource "aws_api_gateway_method_response" "put_success" {
        id                  = "agmr-m7u12kxh7k-b4o6f5-PUT-200"
      ~ response_models     = {
          - "application/json" = "Empty" -> null
        }
        # (5 unchanged attributes hidden)
    }

  # module.api_gw_resource_users_userid_deletion-request.aws_api_gateway_method_response.put_success["304"] will be updated in-place
  # (moved from aws_api_gateway_method_response.users_userid_deletion-request_put_304)
  ~ resource "aws_api_gateway_method_response" "put_success" {
        id                  = "agmr-m7u12kxh7k-b4o6f5-PUT-304"
      ~ response_models     = {
          - "application/json" = "Empty" -> null
        }
        # (5 unchanged attributes hidden)
    }
alexwlchan commented 1 year ago

And one case where we're adding a request parameter:

  # module.api_gw_resource_users_userid_validate.aws_api_gateway_method.post[0] will be updated in-place
  ~ resource "aws_api_gateway_method" "post" {
        id                   = "agm-m7u12kxh7k-gtfo21-POST"
      ~ request_parameters   = {
          + "method.request.path.userId" = true
        }
        # (9 unchanged attributes hidden)
    }

This is the POST for /users/:user_id/validate, because we also need those request parameters on /users/:user_id/item-requests, and there doesn't seem to be much harm in adding them – vs adding a specific exclusion that's probably not that exciting.

alexwlchan commented 1 year ago

@jamieparkinson I seem to have broken linting – any ideas how? It's not immediately obvious to me what's special about the commit where it broke.

jamieparkinson commented 1 year ago

Ah, this is my mistake - the commit where you broke it is the first one with multiple terraform files, I think? So we're running into https://github.com/hashicorp/terraform/issues/19442. There are a couple of workarounds in that ticket - sorry about this!

alexwlchan commented 1 year ago

Linting should be fixed; Terraform formatting is quick enough that we'll just re-run it every time.

alexwlchan commented 1 year ago

I'm happy for it to be merged and applied so we can see what happens.

In theory, nothing – I've already "applied" the changes so I could delete the moved blocks, but 99% of it was Terraform moving stuff around in its state file. The few extra request parameters aside, I don't think it changes API Gateway at all.