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

provider/openstack: Resolve issues with Port Fixed IPs #13056

Merged
merged 2 commits into from
Mar 27, 2017

Conversation

jtopjian
Copy link
Contributor

@jtopjian jtopjian commented Mar 24, 2017

This commit has two parts:

  1. It adds a new computed-only attribute called all_fixed_ips which contain all of the fixed IPs returned by the OpenStack Networking API in the order the API sent them.

  2. It changes the fixed_ip attribute back to a TypeList, reverting the change made in provider/openstack: Change Port fixed_ip to a Set #12613. The change made in provider/openstack: Change Port fixed_ip to a Set #12613 resolved the reported issue, but created other state issues in different use-cases.

Fixes #13052
Supersedes #12807

@stack72 Let me know if you would like to discuss this one in more detail.

I'm pretty sure moving from TypeSet to TypeList is safe to do, but maybe I am wrong about that.

The main change being made (making fixed_ip non-compute) will affect users who were relying on referencing a DHCP-allocated IP by openstack_networking_port_v2.foo.fixed_ip.0.ip_address. They will now need to use openstack_networking_port_v2.foo.all_fixed_ips.0. This should probably be noted in the CHANGELOG.

Users who specified a static IP can return to referencing openstack_networking_port_v2.foo.fixed_ip.0.ip_address as they did before, since it's not relying on a returned API value. The most notable reason for doing this is to make implicit ordering between resources.

Users who reported issues with fixed_ip being re-ordered upon a refresh are still in the clear since fixed_ip is no longer being set by Terraform.

Unless I'm mistaken, with the list of scenarios in #13052, there are just too many varied cases that can be cleanly applied in fixed_ip alone. Notably, creating a unique hash with TypeSet is not possible when a user wants multiple DHCP-based Fixed IPs since the only required piece of data is a subnet_id, and if there are multiple entries with the same subnet_id, Terraform will hash them all the same and aggregate them into 1 requested IP. Further, if ip_address is also used in the hash, then DHCP IPs will have a new generated hash after apply finishes. Therefore, something like all_fixed_ips is needed.

Perhaps a better long-term design is to create a dedicated openstack_networking_port_fixedip_v2 resource and deprecate the use of fixed_ip and all_fixed_ips, but, again, the multiple-DHCP use-case is probably going to make that difficult.

/cc @sebastien-prudhomme

@jtopjian
Copy link
Contributor Author

jtopjian commented Mar 24, 2017

Also, it seems #12613 broke some acceptance tests in a few other Network resource tests. No crashes, but just invalid references to resource attribute names. This PR resolves those issues. I've run the entire suite of openstack_network_* tests and they all pass with this PR in place.

…urce

This commit adds the `all_fixed_ips` attribute to the
openstack_networking_port_v2 resource. This contains all of the port's
Fixed IPs returned by the Networking v2 API.
This commit reverts the changes made in a8c4e81. This
re-enables the ability to reference IP addresses using the
numerical-element notation.

This commit also makes fixed_ip a non-computed field, meaning
Terraform will no longer set fixed_ip with what was returned
by the API. This resolves the original issue about ordering.

The last use-case is for fixed_ips that received an IP address
via DHCP. Because fixed_ip is no longer computed, the DHCP IP
will not be set. The workaround for this use-case is to use the
new all_fixed_ips attribute.

In effect, users should use fixed_ip only as a way of inputting
data into Terraform and use all_fixed_ips as the output returned
by the API. If use-cases exist where fixed_ip can be used as an
output, that's a bonus feature.
@jtopjian
Copy link
Contributor Author

jtopjian commented Mar 25, 2017

I've just tested a configuration state with the current TypeSet and moving back to TypeList. This change will trigger an update call to the OpenStack Network API, but that is safe to do. The currently configured Fixed IPs are specified in the request, a port update is done, and the same Fixed IPs (in alphabetical order) are sent back down, and now placed in all_fixed_ips. This works for both statically defined and DHCP-requested Fixed IPs.

The end result is that fixed_ip is back to the list-based ordering, as defined in the configuration and all_fixed_ips have the order returned by the API.

Detected changes to the fixed_ip attribute do not force a new resource to be made, so in no situation should a port be destroyed from this.

I've given this PR a lot of thought and I'm still convinced it's the right way to go. Having fixed_ip defined as a computed TypeList before #12613 was incorrect, but it appeared correct since the most common port configuration will only ever have one Fixed IP.

Upon report that Fixed IPs were being re-ordered in the state, changing to TypeSet seemed correct, but I failed to take into account multiple DHCP-requested IPs (technical bug) and the commonality of looking for an IP address at openstack_networking_port_v2.foo.fixed_ip.0.ip_address (usability bug).

Therefore, making fixed_ip a non-computed TypeList resumes normal behavior, resolves ordering issues, and enables users to find the DHCP IPs at all_fixed_ips.

@stack72 stack72 added the bug label Mar 27, 2017
@stack72
Copy link
Contributor

stack72 commented Mar 27, 2017

LGTM - the reasoning seems sane

@ghost
Copy link

ghost commented Apr 14, 2020

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provider/openstack: Issues with Networking Ports and Fixed IPs
2 participants