-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Implement ingress and egress with prefix lists #258
feat: Implement ingress and egress with prefix lists #258
Conversation
4749952
to
bca1ce6
Compare
This PR has been automatically marked as stale because it has been open 30 days |
63c3cdb
to
1dc33e2
Compare
Thanks, bot. It is still a desired PR. |
1dc33e2
to
7cc8ea3
Compare
This PR has been automatically marked as stale because it has been open 30 days |
Thanks, bot. It is still a desired PR. |
This PR has been automatically marked as stale because it has been open 30 days |
Thanks, bot. It is still a desired PR. |
bcac86f
to
3c0981f
Compare
This PR has been automatically marked as stale because it has been open 30 days |
Thanks, bot. It is still a desired PR. |
8c27973
to
ac51765
Compare
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.
LGTM
Hello ! @antonbabenko is it something that may block the merge ? Best regards |
This PR has been automatically marked as stale because it has been open 30 days |
Thanks, bot. It is still a desired PR. |
@andyshinn , can you fix all conflicts? Maybe after that your PR will accepted |
…ids and egress_with_prefix_list_ids
ac51765
to
ff9164f
Compare
Rebased to fix conflicts. |
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.
Looks rather good, but there are still a few things to do.
PS: Apologies that it took ages to review this PR.
security_group_id = local.this_sg_id | ||
type = "ingress" | ||
|
||
prefix_list_ids = var.ingress_prefix_list_ids |
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.
prefix_list_ids = var.ingress_prefix_list_ids | |
prefix_list_ids = var.ingress_with_prefix_list_ids |
Or something along those lines, right?
Please verify in other resources, too.
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'm a little confused here. The ingress_with_prefix_list_ids
is the list of custom rules to apply, not the list of prefix IDs. I was just following the template of the other ones which create a resource per rule (ingress_with_prefix_list_ids
and then set the argument to the resource ID to apply to (ingress_prefix_list_ids
). Let me know if I have confused something and if you could clarify.
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.
You're right. I misunderstood the code.
What is missing for this PR to be merged? |
I will fix the feedback items soon. I am busy last few weeks. |
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.
LGTM
security_group_id = local.this_sg_id | ||
type = "ingress" | ||
|
||
prefix_list_ids = var.ingress_prefix_list_ids |
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.
You're right. I misunderstood the code.
Thank you @andyshinn ! |
## [5.1.0](v5.0.0...v5.1.0) (2023-06-06) ### Features * Implement ingress and egress with prefix lists ([#258](#258)) ([2e1cbcb](2e1cbcb))
This PR is included in version 5.1.0 🎉 |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
This is a reopening of pull request #226 which didn't get any movement. I am opening after forking and rebasing to see if we can get some movement on it as I would like the feature.
This feature aims at allowing the module to provision SG Rules by specifying explicitly the ingress or ingress rules with prefix lists. It also allows to unset a new variable enable_prefix_lists_cross_over which drives whether there should be cross over flow openings between the inputs (self, cidr blocks, or security groups) and the prefix lists.
Motivation and Context
I am creating security groups with prefix lists as ingress and egress targets.
Breaking Changes
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull requestI am still going through the testing parts (trouble getting pre-commit to run). But I am applying this to projects using my fork.