Allow configuration of default resources under VCNs #374

alexng-canuck · 2017-11-29T21:51:09Z

This addresses issue #322. Creating a VCN typically generates a
default DHCP option, route table, and security list resource. Allow
configuration of these default resources in the same way that they
are currently configured for non-default resources. Default
resources are identified via a new "default_id" field that marks
them as such.

To support this, we make following changes:

Change Create, Get, Delete request handling for default resources
Only allow compartment_id/vcn_id or default_id, but not both
Add unit tests for default resources
Add example TF file for configuring default VCN resources

One issue we may want to fix after this change:

If we try to remove an updated default resource, nothing will happen to it and it retains its updated settings. The user may expect this to revert it to default settings. We can consider implementing this logic instead.

briangustafson · 2017-11-30T02:44:17Z

provider/core_route_table_resource.go

 func (s *RouteTableResourceCrud) Delete() (e error) {
+	if s.D.Get("default_id") != "" {
+		// We can't actually delete a default resource.
+		// Instead, remove all the route rules first and mark it as deleted.


This is a good point. Should the fake delete operation reset the resource to its default state? I'll have to think this over a bit more. Is there any reason this is done for route rules but not for security lists and DHCP options?

For route rules, it's necessary to clear them to avoid dangling references to gateways. If we don't clear them, then "terraform destroy" will fake delete the route table and then try to delete the gateway. This would fail because the route rules are still referring to the gateway.

I'm leaning towards resetting the resource to its default state. For security list and DHCP options, it's a little more complicated because the default settings rely on values from the parent VCN resource.

One way we could handle this is to read and store the default state of the resource on Create, before we apply any updated settings. Then when we need to reset, just apply the stored state of the resource.

alexng-canuck · 2017-11-30T23:23:09Z

Implemented logic for storing default config state at Create time and then restoring it upon Delete. Still missing tests for this logic, which would be a good idea to add.

alexng-canuck · 2017-12-02T00:47:22Z

Latest commits address some feedback from @briangustafson

We rename the default_id attribute to reflect its role in modifying but not creating/deleting default resources
Targeted deletion of a default resource now leaves the resource in an indeterminate state. Update the docs to reflect this behavior as well as how to generally configure default resources.

briangustafson · 2017-12-04T21:39:05Z

provider/core_dhcp_options_resource.go

 func (s *DHCPOptionsResourceCrud) Create() (e error) {
+	// If we are creating a default resource, then don't have to
+	// actually create it. Just set the ID and update it.
+	if defaultId := s.D.Get("manage_default_resource_id").(string); defaultId != "" {


nit, but normally GetOk should be used to validate whether a property is set. The result should really be the same, but doesn't have to depend on the default value.

if vnicID, ok := s.D.GetOk("vnic_id"); ok {
opts.VnicID = vnicID.(string)
}

I will address this. Should be easy to fix.

briangustafson · 2017-12-04T21:47:18Z

provider/core_dhcp_options_resource.go

+		// we need to assume that the parent resource will remove it
+		// and notify terraform of it. Otherwise, terraform will
+		// see that the resource is still available and error out
+		if s.D.Get("manage_default_resource_id") != "" &&


What happens if I have a default resource in my config, do a targeted delete, and then apply again? Does that work correctly?

Yes, when you target delete a default resource, it is removed from the Terraform state but still exists in OCI. If you then try to add it to the config again, this will result in a Terraform Create request that's converted to an Update API call by the provider.

briangustafson · 2017-12-04T21:51:38Z

provider/core_security_list_resource_test.go

 					resource.TestCheckResourceAttr(s.ResourceName, "egress_security_rules.#", "0"),
 					resource.TestCheckResourceAttr(s.ResourceName, "ingress_security_rules.#", "0"),
+					resource.TestCheckResourceAttr(s.DefaultResourceName, "egress_security_rules.#", "0"),
+					resource.TestCheckResourceAttr(s.DefaultResourceName, "ingress_security_rules.#", "0"),


How about testing that a default resource can be applied, then removed from the config and applied, then added back and applied?

This is a good idea for a test case. I will add this.

briangustafson

Looks good - just a couple nitpicky things and a question.

alexng-canuck · 2017-12-04T23:34:49Z

Addressed latest round of feedback:

Use GetOk() instead of Get()
Add tests for removing/re-adding default resources.

parweza · 2017-12-05T22:22:35Z

docs/examples/networking/vcn_default/vcn.tf

+  cidr_block = "10.0.0.0/16"
+  dns_label = "vcn1"
+  compartment_id = "${var.compartment_ocid}"
+  display_name = "vcn1"


nitpick: I like the naming convention of prepending the name with TF. It easily lets find the resources created from TF example

parweza · 2017-12-05T22:24:50Z

docs/resources/core/dhcp_option.md


-* `compartment_id` - (Required) The OCID of the compartment.
-* `vcn_id` - (Required) The OCID of the VCN.
+* `compartment_id` - (Required) The OCID of the compartment. Do not specify with `manage_default_resource_id`.


isnt this optional then?

You raise a good point. We need a way to say that you either must specify a compartment/vcn id or you must specify a default resource id. I will figure out a way to explain this.

parweza · 2017-12-05T22:25:06Z

docs/resources/core/dhcp_option.md

-* `compartment_id` - (Required) The OCID of the compartment.
-* `vcn_id` - (Required) The OCID of the VCN.
+* `compartment_id` - (Required) The OCID of the compartment. Do not specify with `manage_default_resource_id`.
+* `vcn_id` - (Required) The OCID of the VCN. Do not specify with `manage_default_resource_id`.


same as above

parweza · 2017-12-05T22:28:37Z

provider/core_dhcp_options_resource.go

 func (s *DHCPOptionsResourceCrud) Create() (e error) {
+	// If we are creating a default resource, then don't have to
+	// actually create it. Just set the ID and update it.
+	if defaultId, ok := s.D.GetOk("manage_default_resource_id"); ok {


I dot see a validation that makes sure either (compartmentID, vcnId ) or defaultId is present.

If both aren't present, the service API will fail with a message that the comparmentId/vcnId isn't valid.

That being said, I will add some validation logic here to catch this earlier.

parweza · 2017-12-05T22:31:57Z

provider/core_route_table_resource_test.go


+var defaultRouteTable = `
+resource "oci_core_route_table" "default" {
+	manage_default_resource_id = "${oci_core_virtual_network.t.default_route_table_id}"


what is the expected behavior if user removes the manage default resource id after creating the resources

This should fail with the same error as above.

alexng-canuck · 2017-12-06T19:31:25Z

Discovered some edge cases that require hacks for making sure that the manage_default_resource_id and compartment_id/vcn_id don't conflict. Rather than chasing down this rabbit hole of unknown edge cases; I've decided to let Terraform do most of the schema validation for us before hand.

In order to do this, I will create a new resource schema for default resources. Almost all of the logic remains the same; while having a new schema for default resources will let terraform validate the schema values for us properly. This simplifies our logic for handling default resource ID.

alexng-canuck · 2017-12-06T21:12:05Z

Latest commits address edge cases and issues from @parweza feedback.

Default resources now have their own resource schema but use the same underlying logic for CRUD operations. This avoids edge cases resulting from having two required parameter sets in compartment_id/vcn_id and manage_default_resource_id. This avoids the need for parameter validation logic as Terraform now does it for us via the default resource schema.

briangustafson · 2017-12-07T00:33:47Z

docs/resources/core/default_route_table.md

@@ -0,0 +1,42 @@
+# oci\_core\_default\_route\_table


Not blocking, but I like the idea of combining the default and regular docs for each resource better. That is, just add a line to each of the 3 resources about the default variant with pointer to the Managing%20Default%20Resources.md. Users looking to use the defaults will likely look at the regular resource first, before realizing that there's an entirely separate resource, and it cuts down on duplication.

Also, if we use separate docs then these need to be added to https://github.com/oracle/terraform-provider-oci/blob/master/docs/Table%20of%20Contents.md.

Valid feedback. I added the default resource docs for the sake of completeness, but I see that it's probably overkill and not easily discoverable. Will address this.

alexng-canuck · 2017-12-07T17:49:30Z

Addressed documentation feedback from @briangustafson.

alexng-canuck · 2017-12-08T00:56:25Z

Addressed feedback from @briangustafson, @rcohenma, and @codycushing regarding how to handle removal of default resources.

This addresses issue #322. Creating a VCN typically generates a default DHCP option, route table, and security list resource. Allow configuration of these default resources in the same way that they are currently configured for non-default resources. Default resources are identified via a new oci_core_default_<resource> type. To support this, we make following changes: - Introduce new oci_core_defaut_<resource> schema - Change Create, Get, Delete handling for default resources - Add unit tests for default resources - Add example TF file for configuring default VCN resources - Add docs for handling default resources

alexng-canuck requested review from AlaaShaker, briangustafson, codycushing, kohashim, parweza, rcohenma and rohanbhattacharjee November 29, 2017 21:51

alexng-canuck added the bug label Nov 29, 2017

briangustafson reviewed Nov 30, 2017

View reviewed changes

alexng-canuck force-pushed the default_vcn_resource branch 2 times, most recently from e758f36 to 69bf6b3 Compare December 2, 2017 01:03

briangustafson reviewed Dec 4, 2017

View reviewed changes

briangustafson approved these changes Dec 5, 2017

View reviewed changes

parweza reviewed Dec 5, 2017

View reviewed changes

alexng-canuck force-pushed the default_vcn_resource branch 2 times, most recently from 54cc365 to abaf7ed Compare December 6, 2017 23:42

briangustafson approved these changes Dec 7, 2017

View reviewed changes

briangustafson approved these changes Dec 11, 2017

View reviewed changes

alexng-canuck force-pushed the default_vcn_resource branch from 510e08c to f6108c9 Compare December 11, 2017 22:49

alexng-canuck merged commit babab56 into master Dec 11, 2017

alexng-canuck deleted the default_vcn_resource branch December 11, 2017 22:54

Allow configuration of default resources under VCNs #374

Allow configuration of default resources under VCNs #374

Uh oh!

Conversation

alexng-canuck commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexng-canuck Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexng-canuck commented Nov 30, 2017

Uh oh!

alexng-canuck commented Dec 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briangustafson left a comment

Choose a reason for hiding this comment

Uh oh!

alexng-canuck commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexng-canuck commented Dec 6, 2017

Uh oh!

alexng-canuck commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexng-canuck commented Dec 7, 2017

Uh oh!

alexng-canuck commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexng-canuck commented Nov 29, 2017 •

edited

Loading

alexng-canuck Nov 30, 2017 •

edited

Loading