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

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

Conversation

syedthameem85
Copy link
Member

@syedthameem85 syedthameem85 commented Nov 20, 2023

Resolves - [https://github.com//issues/106]

Proposed change

How has these changes been tested?

Automated testing

If you're running automated testing for this module, we would love to hear from you, and potentially integrate it to the module standard workflow.

Manual testing

If no automated testing is run, please ensure that at least the three steps below are passing without any error.

  • Running terraform apply on each example provided with this module provisions the intended resource(s) without any errors.
  • Modifying module's Input Variables after initial provisioning behaves as intended, i.e: any updateable properties are ameneded without recreation of the resource(s).
  • Running terraform destroy on each example provided with this module destroys all the resources created by this module and only the resources created by this module.

Checklist before submitting your PR

  • My code follows the style guidelines of this project
  • these changes provision new resources:
    • I have updated the README introduction section (README.adoc)
    • I have updated the README introduction section (README.md)
  • these changes adds any new variables:
  • I have updated the changelog to include an entry for these changes
  • I have updated all provided examples, including each README file and all applicable code blocks
  • these changes generates no new warnings
  • Any dependent changes have been merged and published in upstream modules

Note: If you are not an Oracle employee, to contribute to an Oracle-sponsored open-source project, you need to sign the Oracle Contributor Agreement (OCA).

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 20, 2023
@@ -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.

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?

Copy link
Contributor

@hyder hyder left a comment

Choose a reason for hiding this comment

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

Please make it easier for end users to use the API.

modules/subnet/subnet.tf Outdated Show resolved Hide resolved
@@ -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

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

@syedthameem85
Copy link
Member Author

syedthameem85 commented Nov 30, 2023

@hyder - This PR is the solution to the issue - #106 - Add support to allow attaching Security lists in the subnet module . I believe this request can be rejected see my comments above

@syedthameem85 syedthameem85 changed the title Issue #106- add support to allow attaching security lists in the subnet module Fix for Issue -106- add support to allow attaching security lists in the subnet module - Issue #106 Dec 15, 2023
@syedthameem85 syedthameem85 changed the title Fix for Issue -106- add support to allow attaching security lists in the subnet module - Issue #106 Resolves #106 -add support to allow attaching security lists in the subnet module - Feb 21, 2024
@syedthameem85 syedthameem85 changed the title Resolves #106 -add support to allow attaching security lists in the subnet module - Resolves Issue #106 -add support to allow attaching security lists in the subnet module - Feb 21, 2024
@syedthameem85 syedthameem85 changed the title Resolves Issue #106 -add support to allow attaching security lists in the subnet module - Fix -add support to allow attaching security lists in the subnet module - Feb 28, 2024
@chrisgilmerproj
Copy link

Can this get merged into the main repo? Folks ought to be able to first create the module using the default security list. Then they can create security lists outside of the module and attach them in a second update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants