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

Optional+Computed attribute of type string can't be force zeroed #282

Open
magodo opened this issue Dec 17, 2019 · 5 comments
Open

Optional+Computed attribute of type string can't be force zeroed #282

magodo opened this issue Dec 17, 2019 · 5 comments
Labels
enhancement New feature or request subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.

Comments

@magodo
Copy link

magodo commented Dec 17, 2019

SDK version

v1.4.0

Relevant provider source code

I just write a dummy provider to better illustrate my question.

Below code is the schema definition, there defined 3 different attributes:

  • name: primitive type of string
  • age: primitive type of int
  • aliases: aggregate type of list
Schema: map[string]*schema.Schema{
	"name": &schema.Schema{
		Type:     schema.TypeString,
		Optional: true,
		Computed: true,
	},
	"age": &schema.Schema{
		Type:     schema.TypeInt,
		Optional: true,
		Computed: true,
	},
	"aliases": &schema.Schema{
		Type:     schema.TypeList,
		Optional: true,
		Computed: true,
		Elem: &schema.Schema{
			Type: schema.TypeString,
		},
	},
},

Terraform Configuration Files

resource "example_server" "my-server" {
  name = "foo"
  age = 10
  aliases = ["a", "b"]
}

Steps to Reproduce

  1. terraform apply with the config i attached above
  2. modify the config to be as below (force zero each attribute):
resource "example_server" "my-server" {
 name = ""
 age = 0
 aliases = []
}
  1. terraform plan

Expected Behavior

  ~ resource "example_server" "my-server" {
      ~ age     = 10 -> 0
      ~ aliases = [
          - "a",
          - "b",
        ]
        id      = "1.2.3.5"
        ~ name    = ""
    }

Actual Behavior

  ~ resource "example_server" "my-server" {
      ~ age     = 10 -> 0
      ~ aliases = [
          - "a",
          - "b",
        ]
        id      = "1.2.3.5"
        name    = "foo"
    }

The name attribute doesn't honour the force zero operartion.

@magodo magodo added the bug Something isn't working label Dec 17, 2019
@radeksimko
Copy link
Member

Hi @magodo
Firstly thank you for reproducing this with a dummy provider and attaching such simple example!

I just relabelled the issue as the SDK/Terraform does what it was designed for, although maybe not what you expect. The borders between bug and enhancement are a bit blurry here, but I will do my best to explain my reasoning below.

As name is marked as Computed, any value will be interpreted as satisfactory.
Since it is also Optional, empty string is as satisfactory as any non-empty value (including the last value stored in state before the field was removed from config).

This is all aligned with design decisions of the SDK, but may not be aligned with design of some APIs. I vaguely remember AWS land has resources/APIs where such field is expected to be zero-ed and not be sent to the API in following requests, but I can't remember exact examples.

Generally it is sensible for us to change the SDK and alter the design to reflect behaviour of APIs in the wild.

To do that though it would be incredibly helpful to have more concrete examples, ideally with links to API docs. We'd like to know why do we need to zero the field and what would happen if it doesn't get zero-ed and also what value do we expect to get from the API after we wipe it and whether it's the same as default value set after initial creation.

i.e. does the field default to a static value (in which case it probably shouldn't be Computed in the first place), or dynamic value? Is that dynamic value predictable? i.e. would it help if there was a hypothetical DefaultFunc which had access to meta and potentially some parts of the config?

@radeksimko radeksimko added enhancement New feature or request and removed bug Something isn't working labels Jan 3, 2020
@magodo
Copy link
Author

magodo commented Jan 4, 2020

Hi @radeksimko
Thank you for the clarification!

I have an live example in azure land, in resource virtual network, there is a property named subnet, which is O+C. There might be a scenario that a user firstly use in-line configuration for one subnet, i.e. set subnet inside virtual network resource. Later, user changes mind and wants to delete the subnet by forcing zero it.

The expected behavior I expected is that, if the property is explicitly set to any string (incl. empty string), it should be reflected in diff. Otherwise, if it was totally omitted, then respect the Computed attribute, i.e. to be whatever current state is.

@radeksimko
Copy link
Member

The expected behavior I expected is that, if the property is explicitly set to any string (incl. empty string), it should be reflected in diff. Otherwise, if it was totally omitted, then respect the Computed attribute, i.e. to be whatever current state is.

There is more generic problem related to the fact that the SDK or Terraform doesn't differentiate between empty string and null. While 0.12 did introduce null and you may see null sometimes in diffs, this is mostly just visual representation in the CLI which may represent both "zero" value and undefined. This came as cost of retaining backward compatibility of providers/SDK with 0.12. There is ongoing work aimed at deprecating the old protocol (v4), Terraform 0.11 and older, which will allow providers to speak strictly protocol v5 ("Terraform 0.12") and therefore differentiate between undefined and zero.

This issue is tracked in #102

While resolving that will make it behave as you described, I still think there's more work to do. Computed is IMO overused today in many providers and this Azure resource you linked to may be one of the examples. To be clear I'm not blaming providers for the overuse as they don't really have better solution available.

I suspect subnet is Computed merely because subnets can be also defined as first-class resources, in which case azurerm_virtual_network would keep showing diff (extra subnets). This is because resources are entirely decoupled from each other today, at the benefit of performance (parallelisation of operations).

If we had just single way of defining subnets - e.g. deprecated subnet as field of azurerm_virtual_network and make everyone use just azurerm_subnet then this problem wouldn't exist. Deprecation is not always easy or possible though.

aws_security_group & aws_security_group_rule is affected by the exact same problem where rules can be managed inline or first-class.

I'm hoping we could address this by somehow allowing providers to couple the schema and lifecycle of certain resources (which helps reusability) and make Terraform understand which one is the point of truth without having to mark fields Computed. This is tracked in #57


The two issues above are the ones I'd suggest following, but I will keep this one open for now as I imagine there may be some other examples which may require different solution.

@katbyte
Copy link
Contributor

katbyte commented Jun 3, 2020

@radeksimko - this has come up yet again in AzureRM (actually multiple times the last couple weeks) for a property where the API sets a default if not specified (that changes based on a few factors so a default value wouldn't work, and if we did add one it would depend on multiple properties excluding a DefaultFunc), the user has then set it to something different, and then wants to clear the property by setting it to empty and there is currently no way to enable this senario with the current SDK/terraform behaviour.

Has there be any further discussion/progress on this issue and will it be address in v2 of the sdk? There are multiple situations in AzureRM where we need to use optional+computed and there is no alternative.

@paultyng
Copy link
Contributor

paultyng commented Jun 3, 2020

I don't think for backwards compatibility we will be able to do this in the existing SDK methods, but I do think we can probably introduce some new methods or functionality to achieve this, potentially as part of implementing #261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.
Projects
None yet
Development

No branches or pull requests

6 participants