Skip to content
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

feature: Add security group as input #67

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

fatbasstard
Copy link
Contributor

@fatbasstard fatbasstard commented Dec 15, 2023

This PR adds the capability to pass a security group instead always creating one with the Lambda itself.

Reasoning
Currently, for every Lambda it's own security group is created (when running in a VPC), with just a simple Egress. Downside it that this can cause limits being hit:

AWS is not happy with updating these limits BTW...

The change
By allowing to pass along the security group, it's possible to reuse SG's (e.g. between similar Lambda's) and prevent the limitation issue.

Update
Passing a single SG as a string causes new deployments (where the SG and the Lambda are deployed in the same TF run) to break ("unable to determine..." thing).

Changed into an array (which is the actual settings of a Lambda) so we can use length which works for Terraform.

Kept the "old" output to return the generated ID, added a new for all the ID's.

With this it's fully backwards compatible and just adds a feature (to pass your own SG's)

@github-actions github-actions bot added the feature New feature or request label Dec 15, 2023
Copy link
Contributor

github-actions bot commented Dec 15, 2023

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Lint 📖success

Terraform Validation 🤖success

Validation Output

Success! The configuration is valid.


@fatbasstard fatbasstard force-pushed the fvb/securitygroup-refactor branch from 5324407 to 366b8b5 Compare December 15, 2023 10:03
@github-actions github-actions bot added the bug Something isn't working label Dec 15, 2023
@fatbasstard fatbasstard force-pushed the fvb/securitygroup-refactor branch from af3b2ba to 60ecab4 Compare December 15, 2023 15:19
Copy link
Contributor

@stefanwb stefanwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! In the current state of the PR if you specify both a security_group_id and security_group_egress_rules TF will fail on resource "aws_vpc_security_group_egress_rule" "default".

Also the variable security_group_name_prefix should not be set when security_group_id is set.

Could you add validation to security_group_id and check if security_group_egress_rules or security_group_name_prefix are set?

@stefanwb stefanwb marked this pull request as draft December 22, 2023 15:58
@fatbasstard fatbasstard force-pushed the fvb/securitygroup-refactor branch 2 times, most recently from a8bc5db to 8ab1e0f Compare March 7, 2024 13:25
@fatbasstard fatbasstard dismissed stefanwb’s stale review March 7, 2024 13:55

Some changes made

@fatbasstard fatbasstard marked this pull request as ready for review March 7, 2024 13:55
@fatbasstard fatbasstard force-pushed the fvb/securitygroup-refactor branch from 0e1656b to 46710ed Compare March 7, 2024 13:58
Copy link
Contributor

@stefanwb stefanwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change requested

outputs.tf Outdated Show resolved Hide resolved
@fatbasstard fatbasstard force-pushed the fvb/securitygroup-refactor branch from 74a9e58 to 0784ae5 Compare March 7, 2024 15:16
@fatbasstard fatbasstard force-pushed the fvb/securitygroup-refactor branch from a3898c1 to 50719e0 Compare March 7, 2024 18:27
@fatbasstard fatbasstard merged commit be9af50 into master Mar 7, 2024
@fatbasstard fatbasstard deleted the fvb/securitygroup-refactor branch March 7, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants