enhancement: validate uuid attributes#795
Conversation
8dd8460 to
ccb9000
Compare
lgfa29
left a comment
There was a problem hiding this comment.
I was wondering if it would make sense to implement a custom type, but I don't think it would give us much more than this validation.
|
Hum....something that I just thought about is that the migration path here is not straightforward. Since most We may need a plan modifier to ignore changes from non-UUID to UUID values. Maybe a custom type may make sense after all? We could have a non-UUID and an UUID considered as "semantically equal" to avoid this problem? |
|
From what I researched, Even so, almost all the uses of Only the following resources don't have this behavior, each for their listed reason.
Here's the table my trusty LLM agent helped me confirm.
|
ccb9000 to
d4fa921
Compare
Ah ok. From the docs it sounded like it would do the thing we expect, but I guess it doesn't 😅
Hum....but that's also not a good thing because I think the apply will fail with a |
Exactly, we don't need to worry about the migration path because anyone who was mistakenly using the name instead of the ID would have been shown a replacement in their plan like so. We can safely accept that, outside, the resources and attributes I already flagged, users already are using the UUID. |
36ff80c to
69d6668
Compare
|
Makes sense 👍 But we still need a plan modifier to avoid this right? |
|
I should have clarified that the example code shows how things work today without the changes in this patch. I wanted to show that users who use the name instead of the ID see that replacement and likely would have already filed some issue to get it fixed if they ran into it in production. This makes me believe that we're okay to implement the validation in this patch. When we have validation this behavior won't be allowed and the user will be forced to use a UUID which would match the state that we've already been writing. We don't need the plan modifier for these resources since we're already writing their UUID into state today. What makes you believe we'd need one? |
|
Oh right sorry, I got confused with the order the plan output because I was thinking about the other way. The scenario I'm thinking about is for the cases where we end up writing the name in state instead of ID, like for In this case, I think the order of events would go something like:
The user would have to do statefile surgery to get around this, but that's not ideal. The plan modifier I mentioned is for making the non-UUID to UUID change of |
|
Yeah you're right. I'm not sure what the plan modifier would look like in this case though. The requires replace logic will check for raw string equality so it'll want to replace the resource either way I think. I'll hack at it a bit. |
69d6668 to
17f47db
Compare
|
Okay I think I got it with the latest push. |
|
Yup, it seems to be working now and it doesn't try to replace the resource. I think there are two new problems now:
|
17f47db to
170d98e
Compare
|
After I solved 2 then I get this behavior on 1. I had I see the refresh correctly upgraded my |
|
Yeah, 2 seems to be fixed now, but the problem with 1 is when you set the UUID wrong from the beginning. To provide more details on the test I'm running:
|
|
Here's what I did. I have this configuration. terraform {
required_providers {
oxide = {
source = "oxidecomputer/oxide"
version = "0.20.1"
}
}
}
provider "oxide" {
profile = "colo"
}
resource "oxide_ip_pool_silo_link" "example" {
silo_id = "showcase"
ip_pool_id = "c6f8c195-8cee-40b5-bc95-7eb86edec285"
is_default = false
}
resource "oxide_subnet_pool_silo_link" "example" {
silo_id = "showcase"
subnet_pool_id = "49438a11-9ded-46e2-b2be-7ae9a9fd0109"
is_default = false
}I applied that using the v0.20.1 provider. Now the state shows the expected v0.20.1 values. Now I changed the configuration to use the wrong UUID for both resources and ran the plan again on v0.20.1. This error is expected since v0.20.1 of the provider uses a bogus Now I update the provider to the development version using the changes in this patch and plan again. Both resources show the correct refreshed If I don't apply this and instead change the configuration to use the correct UUID for the silo, I see both resources want to be updated in place as well, not replaced. This also seems correct since the configuration value has changed from the name to the UUID. The only thing I'm unsure about is why the |
|
Ah, the |
Attributes that expect UUIDs now validate whether their values are, in fact, valid UUIDs. This catches cases where names were passed to attributes accidentally worked. Closes SSE-347.
170d98e to
0ea74a4
Compare
I am applying with the wrong UUID, maybe that's the difference between our tests? For reference, this is the config file I'm using: terraform {
required_version = ">= 1.11"
required_providers {
oxide = {
source = "oxidecomputer/oxide"
version = "0.20.1"
}
}
}
provider "oxide" {
host = "http://localhost:12220"
token = trimspace(file("${path.root}/../acctest/oxide-token"))
}
data "oxide_silo" "silo" {
name = "test-suite-silo"
}
resource "oxide_ip_pool" "pool" {
description = "a test ip_pool"
name = "test"
ranges = [
{
first_address = "172.20.15.234"
last_address = "172.20.15.237"
}
]
}
resource "oxide_ip_pool_silo_link" "link" {
silo_id = data.oxide_silo.silo.name
#silo_id = "c7d7c06c-2481-4fae-a42f-59048ac54b5c"
ip_pool_id = oxide_ip_pool.pool.id
is_default = false
}Then I do this:
|
|
Ah I see it now. Without making API calls in the plan modifier I think the play here is to update |
Updated the logic for `oxide_ip_pool_silo_link` and `oxide_subnet_pool_silo_link` to resolve the silo name within the `id` and `silo_id` attributes to the silo ID. This upgrades the state to the now validated UUID.
|
Alright now this does the needful.
|
lgfa29
left a comment
There was a problem hiding this comment.
Double approve, it seems to be covering all the weird upgrade cases I've found 👍 👍
And I'm glad the final fix was rather clean, no plan modifier or weird state handling logic.
It could be useful to have a little blurb in the version upgrade guide about replacing names for UUIDs, but not strictly necessary since I think the error message is clear enough and most *_id fields are probably in the right shape already, so I leave it up to you.
Thanks for fixing this!
Attributes that expect UUIDs now validate whether their values are, in fact, valid UUIDs. This catches cases where names were passed to attributes accidentally worked.
Closes SSE-347.