Closed rtyley closed 2 years ago
There are a couple of things that crossed my mind when reviewing this code:
- this approach requires a fair amount of scaffolding (there's more code)
Yep, there is quite a lot of new code in the cdk
folder - 8 new files, with only one of them (cdk/lib/facia-scala-client-testing.ts
) really containing interesting stuff. It's hard to see how to get rid of the other 7 while still allowing devs to execute that code locally, but I'm not an expert, perhaps cleverer people than me can figure that out?!
One alternative is to just to store the cloudformation YAML, which is just 1 file and about 50 lines long (longer than the 32 lines of facia-scala-client-testing.ts
tho'). About 6 lines of that YAML is interesting, the rest is AWS boilerplate and a set of obscure magical values like ThumbprintList: 6938fd4d98bab03faadb97b34396831e3780aea1
that GU CDK spares us from... I'm happy enough with the choice of GU-CDK for this, but it would be lovely if all there was to look at was those 6 lines of config!
- it looks like we must manually deploy the test stack after changes – although, speaking pragmatically, it's unlikely this'll happen too often (there are more moving parts)
Yep, Akash and I did talk about making the cloudformation riff-raff deployable - it would have to be a new artifact of course - and decided in the end, as you say, because the changes would come very infrequently, it's probably ok to make this manually deployable for now.
Did you consider Localstack as one alternative? It might result in less code and fewer moving parts: there's an example of a config in one of our projects here – sadly this runs in TC at the moment, but examples of using this in GHA look fairly concise (the link's just as an illustration, I haven't given that a try, although looking at the repo the action has appeared to trigger successfully).
(One wrinkle – we'd need to make sure the tests pointed our AWS clients at the appropriate URL, which might be a pain. Another wrinkle – Localstack has a paid-for tier for some services, and although well used, obviously runs the risk of imperfectly replicating AWS services. These objections might be enough to sink it.)
That's an interesting option, I wasn't aware of Localstack! The imperfection of replicating AWS services is always a tricky one. In some cases, if a test suite is really slow to execute because of AWS speed (eg, it's always really slow to create new DynamoDB tables) I think services like AWS DynamoDB Local, or Localstack, can be a definite preferable choice. Here, the speed isn't really a problem, and I guess the only downside here is the price we're paying in the GU-CDK boilerplate, and the added complexity of a new dedicated cloudformation stack. I think it is a price worth paying for CI - and I guess we could say that the cloudformation stack kind of nicely documents & proves the permissions required to use facia-scala-client
!
Running the tests for this project requires read access to
s3://facia-tool-store/DEV/
, so we need to provide the GitHub Action with AWS credentials for a AWS role that allows that.We're using https://github.com/aws-actions/configure-aws-credentials to grant the credentials, and https://github.com/guardian/cdk to create the AWS Role - the new cloudformation stack is here.
Specific IAM permissions required
Even though all the FAPI client does, in terms of S3 API calls, is call
getObject
, we need more than thes3:GetObject
permission. We also needs3:ListBucket
because FAPI sometimes has to request objects that don't exist ...and withouts3:ListBucket
, S3 will throw aAccessDenied
error even tho' you possess thes3:GetObject permission
: https://stackoverflow.com/a/56027548/438886Abusing the repositories field
Note that I seem to be having to abuse the
repositories
field a bit (is this field badly named?) in order to get thisrepo:guardian/facia-scala-client:*
value:...which is apparently the format required:
https://github.com/aws-actions/configure-aws-credentials/issues/306#issuecomment-979849934
You can see what happens if you leave the
:*
off here:https://github.com/guardian/facia-scala-client/runs/7110152225?check_suite_focus=true#step:3:6
...you get a
Error: Not authorized to perform sts:AssumeRoleWithWebIdentity
from theaws-actions/configure-aws-credentials
GitHub Action.Co-authored-by: @akash1810