Open jtopjian opened 7 years ago
I think this is more of a core bug than specific to OpenStack since it deals with the core schema and resource packages. However, if I'm wrong about this, please let me know :)
Going to tag this with enhancement since I think this is more of a feature request, I don't think this is anything we ever supported. :)
That sounds good to me! I'm in no rush to implement this - I can work around it.
Hi,
Having ConflictsWith
work properly on a TypeList
is something that would also be useful for the Google Cloud Provider.
One example among many: a Google Compute Instance can define its network_interface by referring to a network or a subnetwork. These two options are mutually exclusive. An instance can have multiple network interfaces:
resource "google_compute_instance" "i1" {
name = "test-instance1"
machine_type = "f1-micro"
can_ip_forward = false
zone = "us-east1-c"
boot_disk {
initialize_params {
image = "debian-cloud/debian-8"
}
}
network_interface {
subnetwork = "${google_compute_subnetwork.network1-sub1.self_link}"
// OR
network = "${google_compute_network.network2.self_link}"
// Should fail if both are defined.
}
}
Our workaround is to fail in the Create
method if both are set for a network interface. It means that terraform plan
succeeds but then it fails at terraform apply
. We would love to bring the validation to the planning phase.
Additionally, the schema internal validation on ConflictsWith accepts invalid path and silently do no conflict detection. We had one case in the google provider where the ConflictsWith was misused and the schema validation didn't complain. Fixed this PR
...
Schema: map[string]*schema.Schema{
"node_pool": {
Type: schema.TypeList,
Optional: true,
Computed: true,
ForceNew: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"initial_node_count": {
Type: schema.TypeInt,
Required: true,
// WRONG: This does nothing and the schema validation does not fail!
ConflictsWith: []string{"node_pool.name_prefix"},
ForceNew: true,
},
"name": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
},
"name_prefix": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
},
},
},
},
ConflictsWith: []string{"node_pool.name_prefix"}
does nothing because it only looks if a value is defined based on the ConflictsWith path you pass in.
https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go?utf8=%E2%9C%93#L1175
c.Get("node_pool.name_prefix")
will never be set and therefor will never conflict. network_interface.0.subnetwork...network_interface.x.subnetwork
are set, never the non-indexed version.
The code performing the schema validation is here: https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go?utf8=%E2%9C%93#L590
It should make sure that the index is defined when referencing a nested field in the ConflictsWith or accept it but do proper conflict detection.
Thank you
Hi folks 👋 Non-SDK team member providing some additional information here.
We tightened up the validation of the AtLeastOneOf
/ConflictsWith
/ExactlyOneOf
/RequiredWhen
fields so the references can be checked via unit testing. As of the upcoming v2, the SDK should return errors if you attempt a reference of config_block_attr.child_attr
instead of something like config_block_attr.0.child_attr
. This validation should also be ensuring that the configuration block attributes are Type: TypeList
and MaxItems: 1
, to match the only supported behaviors at the moment.
As for the enhancement request, it seems like the ConflictsWith
fields (and friends) could benefit from supporting relative references so that they could handle TypeList
and TypeSet
attributes, e.g.
Schema: map[string]*schema.Schema{
"config_block_attr": {
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"child_attr1": {
Type: schema.TypeString,
Optional: true,
ConflictsWith: []string{".child_attr2"},
},
"child_attr2": {
Type: schema.TypeString,
Optional: true,
ConflictsWith: []string{".child_attr1"},
},
},
},
},
},
Where the configuration validation could occur at the same parent-children and level of schema. It would probably need to be restricted to this specific setup rather than allowing arbitrary schema decisions. Attributes cannot begin with a period, so this new syntax should not conflict with (😅 ) existing syntax.
This is just a personal design opinion though that the SDK maintainers would need to validate if its possible and review/approve the design.
Hi folks 👋 Non-SDK team member providing some additional information here.
We tightened up the validation of the
AtLeastOneOf
/ConflictsWith
/ExactlyOneOf
/RequiredWhen
fields so the references can be checked via unit testing. As of the upcoming v2, the SDK should return errors if you attempt a reference ofconfig_block_attr.child_attr
instead of something likeconfig_block_attr.0.child_attr
. This validation should also be ensuring that the configuration block attributes areType: TypeList
andMaxItems: 1
, to match the only supported behaviors at the moment.As for the enhancement request, it seems like the
ConflictsWith
fields (and friends) could benefit from supporting relative references so that they could handleTypeList
andTypeSet
attributes, e.g.Schema: map[string]*schema.Schema{ "config_block_attr": { Type: schema.TypeSet, Optional: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "child_attr1": { Type: schema.TypeString, Optional: true, ConflictsWith: []string{".child_attr2"}, }, "child_attr2": { Type: schema.TypeString, Optional: true, ConflictsWith: []string{".child_attr1"}, }, }, }, }, },
Where the configuration validation could occur at the same parent-children and level of schema. It would probably need to be restricted to this specific setup rather than allowing arbitrary schema decisions. Attributes cannot begin with a period, so this new syntax should not conflict with (😅 ) existing syntax.
This is just a personal design opinion though that the SDK maintainers would need to validate if its possible and review/approve the design.
This issue has been around a long time... Is there any possibility of getting relative paths in attribute validation working??
I'm trying to use
ConflictsWith
on aTypeList
and it isn't working. I think I've narrowed down the issue, but I'm not entirely sure.The
openstack_compute_instance_v2
resource has anetwork
attribute ofTypeList
. The list defines several nested attributes that can all be used to create a network.Some of the attributes can't be used together. For example,
port
andfloating_ip
. So I made the following changes:Then, given something simple like:
I would expect an error to be throw, however, one isn't. But, if I change the schema to:
Then I get a validation error. Of course, that is only applicable to the first defined network.
Throwing some debug output into
helper/schema/schema.go
andterraform/resource.go
, I think the following is happening:During the loop for a conflicting key,
s
isnetwork.0.port
butconflicting_key
isnetwork.floating_ip
. Whennetwork.floating_ip
is checked here, since no index is specified, no value is returned.