Email alert notification receipts

[gpeng]

This work is part of the ongoing efforts to improve travel advice alert email monitoring and will allow us to reconcile a 'received' email with a published edition in Travel Advice Publisher (where we'll add checks that can be run by Jenkins -> Icinga)

This project sets up an S3 bucket and a lambda function that parses files and renames them with a prefix (currently only travel-advice-alerts) and the govuk_request_id parsed from the email body.

An SES domain and RuleSet and an S3 event source are also required but can't currently be configured via Terraform. (details in the README)

I've not been able to test this via the rake tasks from the repo root as I've not got access to the state files, environments etc so I'm not sure that the paths work from there.

The lambda code is deployed from a zip file which I've checked in and included in this PR. I'm not sure this is the best approach as it's 'unreviewable'. I'd welcome any better ideas. Zipping the file as part of the rake task to apply maybe?

Apologies for the one big commit. Code was moved from a local repo. Happy to break up if it will help with reviews.

UPDATE: Added an aws_s3_bucket_notification to trigger the lambda. This is only supported in Terraform 0.6.15

[surminus]

This isn't required because it is supplied as part of common variables

[surminus]

I think currently the way the Rakefile deploys stuff is to copy everything from resource directories with the *.tf filename extension into a temporary directory and then deploy, so this may miss out the zip/py files, so we'll need to tweak that. I think it's also worth discussing whether we want "extra" files within the resource/ dir.

Currently if an environment isn't defined within the resources/ dir then it'll assume that you want it deployed to all environments.

[deanwilson]

The file paths that use path.module will all be incorrect I think, as this isn't a module. You can use relative ones although I suspect something like path.cwd might also work.

[deanwilson]

This depends on #47 being merged.

[boffbowsh]

Could give this a proper ID I think?

[gpeng]

Fixed in ca7c3c1. Seems that using a uuid is the way to go although I haven't used an ID at all in the other policies. I'm not sure if there's any benefit to using them. As it happens this policy confuses terraform apply into thinking it has changed every time it is run which I hoped the change you suggested might fix but didn't unfortunately. I think it's line 27 not being specific enough but haven't been able to work out a more 'fully qualified' version that works.

[deanwilson]

In terms of the lambda build I've been looking at something like this rather than building the zip manually.

resource "null_resource" "build_poller_zip" {
    provisioner "local-exec" {
        command = "zip -j py-poller.zip files/url-poller.py"
    }
}

resource "aws_lambda_function"
    depends_on = [ "null_resource.build_poller_zip"]

Would you be willing to give this a go on this PR?

[surminus]

We're going to add some code that works with the additional files, but the structure needs to be rejigged:

• *.tf files should live in <project>/resources
• templates can go in <project>/templates
• any other files should go in <project>/files

The README can stay in the project root. If you remove the references to the module.path then our rake tasks should hopefully do the right thing.

[radeksimko]

FYI: source_code_hash expects a hash of the ZIP file, so you'll keep getting unnecessary diffs in plan with the current approach as the hash will never match with what the API returns.

[gpeng]

Thanks for your comment. I was using the zip file path originally but in an effort to try and avoid having zipped files in the repo I've moved its creation into a null_resource. As a result the base64sha256 function raises a no such file error if I use the zip file. Is there a better way that I can trigger this resource to be updated when the .py source changes?

[radeksimko]

Is there a better way that I can trigger this resource to be updated when the .py source changes?

Before Terraform gets to plan/apply stage, it (by default) performs refresh (effectively Read from CRUD for each resource in all *.tf files) so it can figure out what may have changed and how does it affect plan/apply. After refresh is done for all resources, it performs plan/apply.

As a result the base64sha256 function raises a no such file error if I use the zip file.

The base64sha256() function is executed at the time of refresh since it has static value (static path to zip file) hence Terraform won't mark it as "computed". All provisioners are always executed at the time of apply, not during refresh. That's why you're hitting this.

To mark it as "computed" you would probably need to have a 1st class support for "zipping" in terraform which would be returning the shasum that you would reference in aws_lambda_function. source_code_hash. There are some (good) reasons why Terraform doesn't support this (yet):
hashicorp/terraform#3858

The only way to get around this would be possibly executing terraform twice (+ removing any unnecessary triggers & depends_on):

terraform apply -target=null_resource.build_rename_email_files_with_request_id
terraform plan/apply

that way terraform won't perform any validation/checks on any resources except the targeted one.
Obviously the null_resource's provisioner would be executed twice (in the second run too), but Terraform would at least get a chance to prepare the right plan and apply it correctly.

Also keep in mind that any local-exec provisioner makes your Terraform files platform-specific - i.e. the requirement suddenly isn't just terraform binary, but also platform-specific zip utility having the same interface (e.g. supporting -j flag). This is also why I think that building ZIP files via CI is a better solution.

[gpeng]

@surminus I've reorganised the files as you suggested. I think it will need a slight change to #47 as per my comments there

@deanwilson I've used that null_resource approach you suggested with a couple of tweaks to get it running. Good shout 👍

[radeksimko]

I'm not entirely sure how you do deal with other artefacts, like ruby gems in GDS, but my friendly recommendation would be to treat the ZIP files for Lambda functions the same way as other artefacts - i.e. have their own repo with CI (Jenkins/Travis/Circle) that is running tests (if you have any) and most importantly generating ZIP file and uploading it into a versioned S3 bucket. That build in your CI can spit out an S3 VersionId of the file it has just uploaded.

Then you can just bump the s3_object_version in *.tf and actually release the ZIP file. It may look like too many steps, but it's codebase like any other and IMO should be treated the same way.

There is also a separate thread about proper versioning of Lambda Functions, which isn't well supported in Terraform yet.

[radeksimko]

0.6.15 was released just a few hours ago 😉
https://www.terraform.io/downloads.html

[gpeng]

Thanks for all your comments @radeksimko . Really helpful.

[radeksimko]

There is an ongoing effort to release Terraform 0.7 soon which will contain something that might help you to make the workflow cleaner/more automated - Data Sources.

Effectively you'd be able to do something like this:

data "aws_s3_object" "lambda_zip_file" {
  bucket = "my-lambda-functions"
  path = "lambda.zip"
}
...
resource "aws_lambda_function" "rename_email_files_with_request_id" {
  s3_object_version = "${data.aws_s3_object.lambda_zip_file.latest_version}"

That way you wouldn't need that extra step of bumping Version ID manually, but you'd just let Terraform to pull down the latest from S3 at the time of refresh.

The syntax and actual implementation is WIP though - this is just to give you an idea.

[gpeng]

I've updated the directory structure and reverted the null_resource commit. Once we've settled on where the zipped lambdas are going to live I'll add support for that.

[surminus]

@gpeng as suggested in #47 (comment) I think what I stated above was incorrect.

[gpeng]

Thanks... I've laid it out as per #47 so I think it should be good to go now. Just need to update to pull the lambda zip from s3 once that is setup.

[surminus]

We have the config in for buckets to store the lambda zips from (#50) although not sure if they're deployed yet (easy to do). I guess the next thing to do is https://trello.com/c/eukUJfOU/84-write-jenkins-deployment-job-for-lambda-apps

[surminus]

@gpeng, we've just had a chat about this. To deploy this the process would be:

1. Dev team add a jenkins.sh script which zips up the code for the lambda function
2. Jenkins uploads the zip to an S3 bucket
3. Jenkins kicks off this Terraform job to deploy the function

We are prioritising steps 2 & 3 along with setting up the email endpoints.

[gpeng]

Ok... Sounds good. So would we have a convention for where zipped lambdas live within a typical app and Jenkins would upload any that it finds in the 'standard' location /lambdas/<whatevs.zip>?

[surminus]

That sounds like a good plan.

[gpeng]

Would that mean that we'd have to have a separate update/install lambda Terraform job?

[surminus]

I think this is fine as it is for the time-being and don't think we should have a separate job. This is dependent on #47 which I think @deanwilson has thoughts on, though I am happy with it in this way and don't really mind either way. I think this terraform project looks fine to run on each deployment (though if you want to do the work to split it up feel free).

For this part, will the source change as the lambda.zip will be stored in another S3 bucket that were created here?

[gpeng]

Yes... the lambda source needs to change there. I think we'll have to keep the lambda source within this repo too now as I think about it as we need to be able to get the hash of it here unless we upload the source and the zip as part of the Jenkins job that pulls the lambdas in. That might be a better idea.

I think we also need to add support for the 3 environments too don't we for both the lambda source and the bucket where the emails land? Might need some pointers on that...

[gpeng]

@surminus I've updated this to get the lambda from S3 now as discussed. We need to pass in the S3 object version as per the last commit message. I think this should be pretty much good to go now. Just need to work out which repo to put the lambda source in.

[surminus]

I'm going to write the deployment job today. The last thing to decide as you suggest is where the source goes, I'd suggest either a new repo (govuk-lambda-app-deployment?) or the govuk-app-deployment repo? We could put it in this repo but ideally I think this should be separate from Terraform code. Either way it should be easy enough to update the deployment job.

[surminus]

This isn't required because it's set by default.

[alexmuller]

Does this need to go in the Gemfile?

[surminus]

The dependency is pulled in by awspec so I don't think it also needs to be included in the Gemfile.

[alexmuller]

Yep sorry, you're right!

[alexmuller]

I'm not sure I understand the template change in 48ad577. I think it's duplicating the templates but I'm not sure why we can't use a relative path of projects/email_alert_notifications/resources/templates/foo.json. Going to have a play with this now.

[surminus]

@alexmuller you're right, we could just have a full relative path in the Terraform code. I thought it was easier to read in it's own directory, but I don't really mind either way!

[alexmuller]

Maybe something like 1cfb375, is that ok? I think that's what @deanwilson had in mind with the directory structure but I might be wrong.

[surminus]

Fine with me!

[alexmuller]

I know hardly anything about how this SES/Lambda interaction works but the code seems like it probably makes sense. Let's run it in integration and see what happens.

[boffbowsh]

@alexmuller 👋 I know these things. Shout if I can help.

[alexmuller]

There's a bit of a loop here where this Terraform code creates an S3 bucket but the Rakefile relies on that bucket existing.

[alexmuller]

@boffbowsh: Thanks!

[surminus]

I didn't think the Rakefile should rely on the bucket existing that the Terraform code creates? It relies upon a different bucket where we upload the Lambda app, then creates a new bucket for a different function (I think).

[alexmuller]

@surminus: Ah yep sorry, you're right. I have no idea how any of this works.