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

Security list diff output change in v2.1.14+ is not innocuous #557

Closed
darkfibre opened this issue Jul 17, 2018 · 5 comments
Closed

Security list diff output change in v2.1.14+ is not innocuous #557

darkfibre opened this issue Jul 17, 2018 · 5 comments

Comments

@darkfibre
Copy link

darkfibre commented Jul 17, 2018

Terraform Version

Terraform v0.11.7

OCI Provider Version

provider.oci v2.1.15

Description:

Terraform plan output, before and after upgrade to v2.1.15.

Before-

  ~ oci_core_security_list.example-seclist
      egress_security_rules.0.source:      "10.2.0.0/21" => "10.2.0.0/22"

After- (removed 1000+ lines of unrelated items, to just focus on the difficulty of assessing a change, even when you already know what it is)

      egress_security_rules.1096359265.destination:                            "" => 10.2.0.0/22"
      egress_security_rules.1096359265.destination_type:                       "" => <computed>
      egress_security_rules.1096359265.icmp_options.#:                         "0" => "0"
      egress_security_rules.1096359265.protocol:                               "" => "6"
      egress_security_rules.1096359265.stateless:                              "" => <computed>
      egress_security_rules.1096359265.tcp_options.#:                          "0" => "0"
      egress_security_rules.1096359265.udp_options.#:                          "0" => "0"
      egress_security_rules.3792673736.destination:                            "10.2.0.0/21" => ""
      egress_security_rules.3792673736.icmp_options.#:                         "0" => "0"
      egress_security_rules.3792673736.protocol:                               "6" => ""
      egress_security_rules.3792673736.tcp_options.#:                          "0" => "0"
      egress_security_rules.3792673736.udp_options.#:                          "0" => "0"

Nobody is going to figure out that 1096359265 and 3792673736 are actually the same security list entry. This renders Terraform borderline unusable for managing security list entries. The probability of someone correctly assessing the impact of a change by examining the output of terraform plan is near-zero.

@darkfibre darkfibre changed the title Security list diff output change in v2.1.15 is not innocuous Security list diff output change in v2.1.14+ is not innocuous Jul 17, 2018
@briangustafson
Copy link
Member

Hi @darkfibre - This is caused by a switch in 2.1.14 from representing security list rules as ordered lists to unordered sets. That change was required in order to support Service Gateways. Also, security lists are inherently unordered, and this prevents diffs in the case of changes in order.

The diff that you're seeing is a result of how Terraform handles sets. This handling is not ideal, as discussed at hashicorp/terraform#15180. Until improvements are made in the core Terraform code, there is a good suggestion for an awk command to help parse the output at hashicorp/terraform#15180 (comment).

@darkfibre
Copy link
Author

Yes, my “is not innocuous” is a reference to the release note entry about this change.

While the inability to do denys in security lists makes order irrelevant to the traffic, the UI and terraform (up until now) cared about order.

If a change was reflected in the diff, its position in the security list was reflected in the diff output. This is no longer the case, worse is that the awk command you reference does nothing add clarity since the diff output shows the removal of a rule, and then the addition of a separate rule instead of the single attribute change that is actually happening. Adding colors or bolding text (which seems to be the idea on the terraform ticket) won’t fix that.

@codycushing
Copy link
Contributor

@darkfibre is this ticket a recommendation to replace the word "innocuous" in the changelog with something more portentous? Or are you advocating for making every attempt not to use sets given the resulting output behavior?

The problem with assuming ordered list behavior is that it's not part of the API contract services have made. It is assumed, and reasonably so, because service behavior has never behaved otherwise, that resources will always remain ordered, but that is not a safe assumption (#328), so we have been advised (#476) to use sets where there is not an explicit order field or service contract.

We are currently evaluating if Terraform .12 mitigates the console output issue sufficiently enough to continue with sets elsewhere in the provider.

@darkfibre
Copy link
Author

I think at this point I'm advocating for avoiding sets until the output becomes more usable. Normally in the networking world the order of ACL entries really does matter, so the aspect of this that seems amiss is that there isn't an API commitment to ordering of ingress and egress rules.

@rcohenma
Copy link
Contributor

We had to move from Lists to Sets in Security lists and Route tables because of other issues we were seeing with the List implementation when we added the Service Gateway feature.
We will try to avoid converting other lists into sets.
Thanks for your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants