-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide initial functionality #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I have one question before approving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Powerful work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions/thoughts on my first pass. I think I have suggestions in variables.tf
corresponding to all suggestions in README.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your continued work on this. I have for your consideration some suggestions around the validation you added for the lambda_schedule_interval
variable, correctly defining the rate provided to the schedule_expression
argument, and then some miscellaneous capitalization/language suggestions.
I just added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick reminder to enable these ignore directives:
publish-egress-ip-terraform/.github/dependabot.yml
Lines 24 to 36 in d1c0a29
- package-ecosystem: "terraform" | |
directory: "/" | |
schedule: | |
interval: "weekly" | |
# ignore: | |
# - dependency-name: "hashicorp/aws" | |
- package-ecosystem: "terraform" | |
directory: "/examples/basic_usage" | |
schedule: | |
interval: "weekly" | |
# ignore: | |
# - dependency-name: "hashicorp/aws" |
The ZIP file is created with http://github.com/cisagov/publish-egress-ip-lambda Also: * Define the CloudWatch log group used by the Lambda * Schedule the Lambda function to run every X minutes using a CloudWatch event (configured via var.lambda_schedule_interval)
These are no longer needed now that we have moved to version 4.x of the Terraform AWS provider.
Using the current default runtime (nodejs14.x) of this module results in this error: "The runtime parameter of nodejs14.x is no longer supported for creating or updating AWS Lambda functions. We recommend you use the new runtime (nodejs20.x) while creating or updating functions." Using the AWS-recommended runtime of nodejs20.x results in this error: "expected runtime to be one of [nodejs nodejs4.3 nodejs6.10 nodejs8.10 nodejs10.x nodejs12.x nodejs14.x nodejs16.x java8 java8.al2 java11 python2.7 python3.6 python3.7 python3.8 python3.9 dotnetcore1.0 dotnetcore2.0 dotnetcore2.1 dotnetcore3.1 dotnet6 nodejs4.3-edge go1.x ruby2.5 ruby2.7 provided provided.al2 nodejs18.x python3.10 java17], got nodejs20.x" As best I can tell, this is because the Terraform AWS provider that we are currently pinned to (~> 4.9) does not support nodejs20.x. Given all of the info above, I chose to set the runtime to nodejs18.x and everything is working.
…ss bucket Things have changed a bit with how AWS does public bucket access since this code was initially created, so we must change with the times.
10 minutes was not long enough to iterate through all of our AWS accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one suggested language tweak, but nothing that should hold up this PR.
Co-authored-by: Shane Frasier <jeremy.frasier@gwe.cisa.dhs.gov>
…Front to read the bucket contents This is a more secure bucket configuration, since there is no direct public access to the bucket. Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty solid. Some minor items for your consideration. I didn't want to go wild with the formatting changes until we codify it in the development guide but left the two suggestions I'd already done.
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
This is a recently agreed upon change to how our team should format Terraform. Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Note that some of these changes were overlooked at the end of PR #6.
🗣 Description
This PR implements the initial functionality for this repository, namely Terraform to do the following:
cisagov/publish-egress-ip-lambda
, and it's execution role/policy💭 Motivation and context
We need a mechanism to publish egress IP info from a subset of our AWS accounts and this Terraform will allow us to deploy the infrastructure to do that.
This is part of the work for:
This PR is related to the following:
This PR resolves #5 (via f22ab6d).
🧪 Testing
I successfully applied all of this Terraform and confirmed that the Lambda was deployed, scheduled, and worked as expected. I also confirmed that I could cleanly destroy all of this Terraform.
✅ Pre-approval checklist
in code comments.
to reflect the changes in this PR.
✅ Pre-merge checklist
✅ Post-merge checklist