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

Importer normalize #14891

Closed
wants to merge 2 commits into from
Closed

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jan 11, 2022

Normalize the ID using the importer function to conform to what is
defined in the ID formatter.

This is to mitigate cases like users import resource ID with lowercase
'g' for the resource group. In this case, the current validation in the
importer raise no error, which results into the incorrect cased ID leak
into terraform state. This will cause the other referencing resource get
unexpected plan diff (see #14854).

This commit adds the helper function for both untyped and typed sdk. The
users are expected to pass in a wrapper function around the parse
function generated via the resource id tool. While this wrapper might be
eliminated once we have generic in Go.

Together with this PR comes with two real world examples (one for typed sdk and one for the untyped sdk). If this makes sense, then we can move on refactoring for the remaing resources.

Normalize the ID using the importer function to conform to what is
defined in the ID formatter.

This is to mitigate cases like users import resource ID with lowercase
'g' for the resource group. In this case, the current validation in the
importer raise no error, which results into the incorrect cased ID leak
into terraform state. This will cause the other referencing resource get
unexpected plan diff (see hashicorp#14854).

This commit adds the helper function for both untyped and typed sdk. The
users are expected to pass in a wrapper function around the parse
function generated via the resource id tool. While this wrapper might be
eliminated once we have generic in Go.
@tombuildsstuff
Copy link
Contributor

hey @magodo

Thanks for this PR.

Whilst this is one approach we could look to take, we're intentionally not doing so and validating that the Resource ID is parsed in the correct casing so that this is predictable at import (and usage) time.

As you've mentioned there's a bug in the older Resource ID parser logic (which are generated via the tool in this repository) where the casing on the resourceGroups segment isn't being validated correctly - however the new Resource ID parsers do, so we should probably look to instead fix this via fixing the old Resource ID parser to check the casing on the Resource Groups segment (or to update the Resource ID generator to use the newer Resource ID parsers, which isn't something we're planning to do at this point in time, in favour of moving those out of this repository in the near future).

Whilst we can look to make the parser case-sensitive for this segment - we'd need to ensure this is still insensitive for the Resource Group ID resource at this time, since this is the only resource which has this segment in lower-case (resourcegroups) rather than camelCase (resourceGroups) - so this'll require a state migration for that resource (which we're planning to do once all of the remaining resources have been switched to using the generated Resource ID Parsers, fwiw).

Where we could go through and update the existing Resource ID parsers to use the new approach, give we're not far off replacing the existing ones with the new approach - I think it's probably best waiting until the new ones are rolled out everywhere, rather than amending the older Resource ID parsers in this case, WDYT?

Thanks!

@tombuildsstuff tombuildsstuff self-assigned this Jan 11, 2022
@magodo
Copy link
Collaborator Author

magodo commented Jan 11, 2022

@tombuildsstuff That makes a lot of sense, thank you for letting me know the details!

@magodo
Copy link
Collaborator Author

magodo commented Jan 11, 2022

One thing worth mentioning is that we can validate case sensitively for importing, while we still need the case insensitive one for parsing id from response

@tombuildsstuff
Copy link
Contributor

@magodo

One thing worth mentioning is that we can validate case sensitively for importing, while we still need the case insensitive one for parsing id from response

Indeed, the new Resource ID Parsers generate an insensitive parser by default, which makes that easier.

Since this'll be fixed by both the new Resource ID Parsers (and the Resource Group ID issue fixed by updating this ID with a state migration), whilst I'd like to thank you for this contribution I'm going to close this PR for the moment - but we'll be switching over to use the new Resource ID parsers in the not-too-distant future which should solve this 👍

Thanks!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants