chore: run 'terraform plan' via github action

[jgravois]

adds a new pull_request driven github action to run terraform plan anytime the contents of the terraform/ directory have changed and disables the pull request triggered ADO pipeline run that did the same thing formerly.

part of #3897

as part of this effort, DevSecOps created a new service principal and federated credentials on our behalf and we worked together to grant the runner needed privileges.

[github-actions]

Coverage report

This PR does not seem to contain any modification to coverable code.

[jgravois]

in this new action i took the route of pinning each dependency to a specific SHA as that is now a security best-practice.

my thinking is that we can/should hold off on expanding this pattern in other workflows until after we see just how seamlessly dependabot is able to suggest updates as additional releases of the dependencies here are tagged.

[jgravois]

setting -lock=false just ensures that the plan run on CI/CD doesn't prevent anyone anywhere else from running a plan simultaneously.

[thekaveman]

Is this correct? I think we have always had locking plans (the default, right?)

I.e. the ADO pipeline wasn't set up to not lock during a plan? And when devs run plan locally, we don't typically/ever? include -lock=false, at least to my knowledge.

We have these docs that speak to addressing lock situations: https://docs.calitp.org/benefits/guides/troubleshooting/#terraform-lock

Have we discussed wanting to turn this off now? I'm fuzzy on all the implications of locking vs. not.

[jgravois]

I'm fuzzy on all the implications of locking vs. not.

this isn't a hill i'd die on by any means, but the documentation you linked to is a great example of why locking is such a PITA.

anytime you might act on a plan and use the information to make changes to a deployed environment. i'd call it a necessary evil. locking is what ensures you can trust that terraform will stick to the plan and that no one else can pull the rug out from underneath you.

in the case of the pipeline and this new github action. i consider the terraform plan to be an ephemeral/throwaway artifact taken at a snapshot in time.

ie: it wouldn't be problematic to rerun the job and see a different output if someone else deployed a change of their own nearly simultaneously and there's no reason why the creation of these plans should block anyone else who happens to be considering deploying a change of their own (imo).

that said, if we keep the default locking behavior, 99% of the time the lock will be assigned and cleared automatically. the other 1% of the time we can either wait for the lock to clear on its own (or nuke it if somebody 💩s the 🛏️).

[thekaveman]

Yeah I agree the linked docs make this seem like an annoying problem that has to be dealt with. I don't recall more than like, once or twice, ever having to actually deal with it.

I'm not wanting to die on this hill either FWIW.

anytime you might act on a plan and use the information to make changes to a deployed environment. i'd call it a necessary evil. locking is what ensures you can trust that terraform will stick to the plan and that no one else can pull the rug out from underneath you.

I will just say that, by definition a PR plan is something we will act on by merging the PR: dev will be get the apply as soon as the merge completes. So these aren't quite as ephemeral in my mind, especially with the path filters... if a plan runs, it signals that an apply will run on merge. Right?

it wouldn't be problematic to rerun the job and see a different output if someone else deployed a change of their own nearly simultaneously and there's no reason why the creation of these plans should block anyone else who happens to be considering deploying a change of their own

Yup agreed.

I guess maybe the issue I'm considering (which, on reflection/writing this out, isn't necessarily related to this PR/change) is: if one assumes their (stale) plan from before another plan/merge with changes was still correct and merged, only to discover their plan/apply now actually fails for some reason. I know you're working on the apply part of this now. Probably more relevant there. Maybe this is another argument for requiring branches to be up to date? I dunno. We can also just try this out for a while and see how it goes... it's not like the infra changes ALL that often.

[jgravois]

a PR plan is something we will act on by merging the PR

i take your point, but most of the time many minutes will pass between the plan associated with the last commit pushed to a PR that touches infrastructure and when that change is actually applied on merge.

regardless, i did a little more research and decided an abundance of caution is appropriate here. i had forgotten that even a plan writes to terraform state files, and not taking a lock opens us up to the (small) possibility of corruption. a32a5cd

https://developer.hashicorp.com/terraform/language/state/locking

[lalver1]

These are all good points, and I agree that out of an abundance of caution we can keep plan's default to lock the state. Since we rarely had any issues with plan in ADO, I expect this behavior to carry on, especially since rarely there have been concurrent changes to the infra. I would suggest though, that we add -lock-timeout=5m like we originally had in the ADO Pipeline. Setting this option is probably a good way to reduce the chance of getting ourselves in the situation described in our docs.

[jgravois]

good call. f57ef83

[lalver1]

In the comment maybe we can say something like

# automatic pull request validation triggers are also now disabled but "manual" runs from a GitHub Actions workflow are still supported
# https://learn.microsoft.com/en-us/azure/devops/pipelines/build/triggers?view=azure-devops&tabs=yaml#pr-triggers

to remind ourselves that this change was focused on turning off the automatic pull request validation automatically ran by the Azure Pipelines GitHub App.

[jgravois]

i dig it 05fc05c

[lalver1]

When this script (that's mostly meant for running terraform from a local machine) runs in this workflow, we might be running redundant commands. Such as getting the subscription via az account list and setting the subscription via az account set, since we already authenticated in the "Log in to azure using federated identity credentials" step. But, maybe that's ok because on the other hand, running the script keeps some of the logic DRY. I don't feel strongly either way so we can keep it like this 👍 Just wanted to note it in case we do come up with other reasons to bypass using init.sh which I might be overlooking now.

[jgravois]

you're not wrong. i've been operating under the assumption that its safe because the federated credential only has access to the subscription that we want it to use, but it is a little overkill.

personally it feels like a good place to start to me, but i'm definitely open to breaking out the explicit commands needed if anyone else feels it would add value.

[lalver1]

I agree, it's a good place to start, especially since we know that the azure/login step does not interfere with init.sh, so running those extra az commands in init.sh doesn't hurt.

[lalver1]

Nice job, this looks good to me @jgravois! I think that if we merge this right now, the terraform plan will fail due to an active lock, probably related to the terraform apply PR testing. That's the only reason why I hesitate to approve it, we'll miss an opportunity to get a clean workflow run since we may need to manually intervene to remove the lock first.

I have to step out for the afternoon but I wanted to mention this so we can look at it in the morning.

[jgravois]

I think that if we merge this right now, the terraform plan will fail due to an active lock, probably related to the terraform apply PR testing.

that was certainly correct for much of this afternoon. 😅 I had a whole slew of hung attempts to terraform apply in #3913 that resulted in locks that needed to be cleared manually. all is well now though. i just confirmed by re-running the job associated with the most recent commit here manually.

https://github.com/cal-itp/benefits/actions/runs/30042334889

[lalver1]

Great job @jgravois!