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

Multiple security_list_ids in baremetal_core_subnet Cause Subnet Resource to Add/Detroy Every Time #44

Closed
beamanm opened this issue Mar 27, 2017 · 9 comments · Fixed by #59
Assignees
Labels
bug listVsSet Customer issues arising from diffs due to ambiguity between list and set usage

Comments

@beamanm
Copy link

beamanm commented Mar 27, 2017

Multiple security_list_ids in baremetal_core_subnet Cause Subnet Resource to Add/Detroy Every Time, even with no changes. If I reorder the security_list_ids to what terraform is expecting (ie swap the pair) this seems to resolve the issue. There doesn't seem to be a pattern, but for longer seclists (2+), they pretty much guarantees the subnets/compute instances will rebuild on every apply. Also, what is the seclist number limit for resources?

@beamanm
Copy link
Author

beamanm commented Mar 28, 2017

Example snippet:

resource "baremetal_core_subnet" "baston-subnet" {
availability_domain = "${var.ADs[0]}"
cidr_block = "${var.CIDR-baston-subnet}"
display_name = "Bastion Subnet"
compartment_id = "${var.compartment_services_ocid}"
vcn_id = "${baremetal_core_virtual_network.sandbox-vcn-1.id}"
route_table_id = "${baremetal_core_route_table.sandbox-route-1.id}"
security_list_ids = [
"${baremetal_core_security_list.SEC-tools-all-Internet.id}",
"${baremetal_core_security_list.VCN-wide-EXTERNAL-security-list.id}",
"${baremetal_core_security_list.VCN-wide-INTERNAL-security-list.id}",
]
}

@beamanm
Copy link
Author

beamanm commented Mar 28, 2017

One other note, we are currently on Terraform v0.8.8, not sure if we should be on a newer version.

@srinatar
Copy link

srinatar commented Mar 28, 2017 via email

@mikejihbe
Copy link
Contributor

mikejihbe commented Apr 2, 2017

Just FYI, I've reproduced this issue. @craigcarl-oracle there is no facility immediately apparent to me in terraform to handle list ordering issues like this. I think ideally the security_list_ids attribute is always ordered consistently in API responses (both from creation (POST) and fetching (GET).

There is a diff suppression function, but it receives each key individually, e.g.
security_list_ids.0
security_list_ids.1
security_list_ids.2
And the order is inconsistent. We can access the entire state from that function, so we could see if the ids are already represented in the state and suppress the diff in that case, but that is pretty hacky IMO.

@craigcarl-oracle
Copy link
Contributor

@mikejihbe It's a fairly serious issue, it can lead to data loss when an instance gets terminated unexpectedly. We're going to need a fix in the provider even if it's temporary and hacky. I'm still not completely clear on what dimension TF needs the list of SLs ordered in, is it creation date? Ascending or descending?

I think there are two different situations we're going to need to deal with -

  1. No changes to the SLs attached to a subnets (as reported here).
  2. SL(s) attached/detached from a subnet.

I don't think your proposed fix will deal with the second situation, if we always suppress the diff wouldn't that prevent changes to the SLs attached to a subnets?

Can we can get the order SLs were created in from the state and use that to order the list of SLs appropriately?

I'll file a ticket against the API once we've determined exactly what needs to change.

@craigcarl-oracle craigcarl-oracle added P0 and removed P1 labels Apr 2, 2017
@mikejihbe
Copy link
Contributor

It doesn't matter what the order is as long as it's consistent. You're right, this change will not cause adding or removing security lists to not destroy and re-create the subnet. That is an API limitation though, there is no security_list_ids parameter in the update api

@craigcarl-oracle
Copy link
Contributor

@mikejihbe We're not supporting the addition/removal of SLs yet, ignore that case.

Can we internally add

lifecycle {
     ignore_changes = ["security_list_ids"]
}

to every baremetal_core_subnets resource? That wouldn't require understanding the state, would it solve the problem?

@mikejihbe
Copy link
Contributor

It looks like the lifecycle attributes are only presented in the parsing, so there are actually no schema attributes we could update to influence the lifecycle in this fashion. I tried for a few hours to get DiffSuppressFunc to do what we want, but it's not working. I also looked for but couldn't find an example of a list being "internally" suppressed like we want here.

@mikejihbe
Copy link
Contributor

I had another thought here. The problem may not be unstable sorting on the API, the problem may be that stable sorting from the API is different than the order presented in the TF statefile by the user. We could probably mitigate this problem by altering our GET function to order the subnet ids in the same order as what exists in the tf state (if any) if possible.

josephholsten added a commit to MustWin/terraform-provider-baremetal that referenced this issue Apr 7, 2017
`baremetal_core_subnet.security_list_ids` with multiple items causes
destroy/add on every `terraform apply`, even with no changes. API state
does not respect item ordering, so config and API state do not match
(except when items happen to have exactly the same sorting as used by
API state). Because local and remote state do not match, terraform will
attempt to change remote state to match on every `apply`.

Change `security_list_ids` from `TypeList` to `TypeSet`, so terraform
only considers membership, not ordering.

Fixes oracle#44
craigcarl-oracle added a commit that referenced this issue Apr 13, 2017
Closes #50, closes #44, closes #49, closes #34, closes #12
@kilokahn kilokahn added the listVsSet Customer issues arising from diffs due to ambiguity between list and set usage label Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug listVsSet Customer issues arising from diffs due to ambiguity between list and set usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants