Refactor code, add lambda support & terraform module#11
Refactor code, add lambda support & terraform module#11
Conversation
dvogel
left a comment
There was a problem hiding this comment.
Clear improvements here so no reason to hold this back. I left some comments with considerations for follow-up work though.
| if err := check(url, "443", cfg.Days); err != nil { | ||
| msg := fmt.Sprintf("failed host check %s - %s", url, err) | ||
| log.Errorf(msg) | ||
| failures = append(failures, msg) |
There was a problem hiding this comment.
Not really a priority for this PR necessarily, but a general comment on the approach going forward:
Returning these failures up the call stack seems appropriate to me but also incongruent with the log.Errorf(msg) call. It feels to me like that is functionality tied to the main entrypoint and should be moved up there. Alternatively perhaps both main and handle could pass in a list of interfaces that each failure should be passed to. In the main case maybe that would just be a logger while in the handle case it would be a logger and SNS.
There was a problem hiding this comment.
Agreed, I had to rethink how to handle the errors and wasn't sure at first if it should be a script exit code with notifications elsewhere or handled in-app. I like the idea of going through the same flow in either local or lambda case, and just modifying the notification implementation based on interfaces, will try to get that implemented.
| mkdir -p $(BUILD_DIR) | ||
| GOOS=linux GOARCH=amd64 go build -o $(BUILD_DIR)/$(APPNAME) | ||
| build: | ||
| go get && GOOS=linux go build -o $(APPNAME) |
There was a problem hiding this comment.
Does go get not need GOOS=linux? If it is run on macos, does it download macos modules?
There was a problem hiding this comment.
It didn't seem to make a difference, but I will switch it to before the get as that seems more correct.
There was a problem hiding this comment.
From go help get:
In earlier versions of Go, 'go get' was used to build and install packages.
Now, 'go get' is dedicated to adjusting dependencies in go.mod.
I'm not sure, but I don't think bare go get does anything at all? In any event, Go will automatically download dependencies if they're missing. In CI, it can be nice to use go mod download on its own to have a separate step for caching the dependencies, but there's no point in breaking downloading dependencies and building into two commands in one step here.
| key = var.s3_object | ||
| source = var.lambda_local_path | ||
| etag = filemd5(var.lambda_local_path) | ||
| } |
There was a problem hiding this comment.
This could probably be done just as well with a local file upload. Uploading the binary to S3 doesn't seem to accomplish the same type of transparency or reproducibility as uploading javascript or python lambda code to S3 does.
| resource "aws_iam_policy" "lambda-policy" { | ||
| name = "certwatcher-exec" | ||
| path = "/" | ||
| description = "Policy to allow certwatcher to " |
| name = "certwatcher" | ||
| display_name = "certwatcher" | ||
| kms_master_key_id = "alias/aws/sns" | ||
| } |
There was a problem hiding this comment.
Two strategic thoughts, kinda over-lapping:
- With the focus on the sustainability of the internal infrastructure, it might be worth mapping out sooner rather than later how notifications should work. e.g. If there is going to be multiple notification sources then maybe the SNS topic should be defined separately, with a resource policy allowing publication by certwatcher + whatever else needs to send notifications.
- Since CloudWatch metric alarms are likely to be part of the picture, it would make sense to start out with a custom-managed KMS key because CloudWatch doesn't have access to the standard AWS-managed SNS KMS key.
Changes
make localExample:
make lambdawill build and package the zip.Testing
I have only run this locally so far, not via Lambda.
Existing unit tests pass and program appears to be working as expected.
TF module passes format, etc.