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.

Sign up for GitHub

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

Jump to bottom

Fix -add support to allow attaching security lists in the subnet module - #128

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix -add support to allow attaching security lists in the subnet module - #128

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
237cdf0
Issue #115
syedthameem85 Nov 20, 2023
978a28a
Issue-#106
syedthameem85 Nov 20, 2023
5db4261
Update terraform.tfvars.example
syedthameem85 Nov 23, 2023
4ec1b7c
Merge branch 'main' into Issue-#106
syedthameem85 Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion modules/subnet/subnet.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ resource "oci_core_subnet" "vcn_subnet" {
cidr_block = each.value.cidr_block
compartment_id = var.compartment_id
vcn_id = var.vcn_id
availability_domain = lookup(each.value, "availability_domain", null)

hyder marked this conversation as resolved.
Show resolved Hide resolved


defined_tags = var.defined_tags
Expand All @@ -23,7 +25,8 @@ resource "oci_core_subnet" "vcn_subnet" {
#prohibit_internet_ingress = var.enable_ipv6 && lookup(each.value,"type","public") == "public" ? each.value.prohibit_internet_ingress : false
prohibit_public_ip_on_vnic = lookup(each.value, "type", "public") == "public" ? false : true
route_table_id = lookup(each.value, "type", "public") == "public" ? var.ig_route_id : var.nat_route_id
security_list_ids = null
security_list_ids = lookup(each.value, "security_list_ids", null)
Copy link
Contributor

Choose a reason for hiding this comment

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

The module creates a VCN and optionally some subnets. In these subnets, you want to attach security lists which can only be created after the VCN is created.

But I don't see any security lists being created. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes , the interpretation of this request from my side was , the security lists will be added if a VCN exists . If a VCN does not exist, the user will not be able to add the security list . In terraform.tfvars.example line 106 will be relevant to the user's specific implementation if a VCN exists . Please let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

We are not checking if a VCN exists here. This module is for creating a VCN. The ocids will only be available after the VCN is created

Copy link
Member Author

@syedthameem85 syedthameem85 Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@hyder - As this module is for creating VCN and the security lists do not exist before VCN creation is done, so it cannot handle attaching security lists to subnet that just got created as part of VCN creation. So, i think, this request can be rejected. Please comment if this issue /request can be rejected



lifecycle {
ignore_changes = [defined_tags, dns_label, freeform_tags]
Expand Down
2 changes: 2 additions & 0 deletions terraform.tfvars.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,6 @@ attached_drg_id = null
#subnets = {
# sub1 = {name = "subnet1",cidr_block = "10.0.4.0/24"}
# sub2 = {cidr_block="10.0.5.0/24",type="private"}
# sub3 = {cidr_block="10.0.6.0/24",availability_domain="Uocm:PHX-AD-1"}
Copy link
Contributor

Choose a reason for hiding this comment

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

see my comment about looking these values up in your other PR

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been fixed via other PR.

Copy link

Choose a reason for hiding this comment

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

What is the other PR?

# sub4 = {cidr_block="10.0.7.0/24",security_list_ids =["ocid1.securitylist.oc1.iad.xxxxx", "ocid1.securitylist.oc1.iad.xxxxxx"]}
#}