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

Adds support for custom Security Group rules #8

Merged
merged 8 commits into from
Aug 6, 2021
Merged

Conversation

bradj
Copy link
Contributor

@bradj bradj commented Aug 4, 2021

what

  • Module now handles creating a security group on the users behalf

why

  • More flexible interface that allows the user to either create their own security group or simply supply a list of CIDR's

references

@bradj bradj requested review from a team as code owners August 4, 2021 20:24
@bradj bradj requested a review from a team as a code owner August 4, 2021 20:25
@bradj bradj requested review from jhosteny and RothAndrew August 4, 2021 20:25
main.tf Outdated
Comment on lines 78 to 85
rules = [
{
type = "ingress"
from_port = 22
to_port = 22
protocol = "tcp"
cidr_blocks = var.allowed_cidrs
}
Copy link
Member

Choose a reason for hiding this comment

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

Nono, this goes against the spirit of the module. The objective is to expsoe the rules variable so the caller has unfettered ability to add rules for ingress/egress.

@bradj bradj changed the title Adds allowed_cidrs variable and SG creation Adds support for custom Security Group rules Aug 5, 2021
variables.tf Outdated
from_port = 22
to_port = 22
protocol = "tcp"
cidr_blocks = ["10.0.0.0/16"]
Copy link
Member

Choose a reason for hiding this comment

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

should this be an empty array by default?

Copy link
Contributor Author

@bradj bradj Aug 5, 2021

Choose a reason for hiding this comment

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

@Benbentwo I copied the pattern from this repo.

It also has the cidr_block array populated with 0.0.0.0/0 but I don't like having it open to the world so leaving cidr_block with [] will show an error explaining it needs to be set. Not the greatest UX unfortunately but I think security should take precedence over UX. Unless there's a better way.


variable "security_group_rules" {
type = list(any)
default = [
Copy link
Member

Choose a reason for hiding this comment

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

or this be an empty array by default?

@bradj bradj merged commit 5074c43 into master Aug 6, 2021
@bradj bradj deleted the feat/allow-cidr-list branch August 6, 2021 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants