fix: Legacy role grantID to work with new grant functionality

[daniepett]

These changes will allow legacy grantID for role grants (Just role name), to work with the new grantID structure/functionality.

Implementing these changes requires less stringent testing on the IDs, but gives flexibility that allows legacy roleGrantIDs to be using the same function, therefore limiting code duplication.

• acceptance tests
• unit tests

References

• Closes Error: 5 or 6 fields allowed (using snowflake_role_grants) #922

[ChrisIsidora]

@alldoami can this be reviewed, merged and released asap? Thanks in advance

[ChrisIsidora]

@daniepett Does this change comes with the caveat that grants applied OUTSIDE Terraform will not be revoked. If so is there a reason why this behavior is changed? This would mean I can never achieve a desired state with this new behavior

[daniepett]

@ChrisIsidora The behaviour was changed to allow similar grants to be created in different resources. Resource A won't know if the grant is being applied through Resource B (Or outside Terraform), and therefore Terraform won't revoke it. It will however check that all the roles specified in that resource has the right grants applied.

[ChrisIsidora]

@daniepett For this case I don't understand why you would want to have role to role grant distributed in different places. One of the point of IAC and terraform is reaching a desired state after applying a resource. Changing this behavior means terraform doesn't have control anymore over said resource, while the whole point is that this should be the case. When a resource is managed by terraform every change that is made outside of terraform will be corrected according to desired state you apply via terraform. This change to the grants means that if someone manually add a grant to a specific role that is managed by terraform then at the next apply terraform won't revoke that grant, while I do expect it to revoke that grant because it's not the desired state that I have in terraform.

[daniepett]

I do agree, but for implementation of this functionality it was a trade off. It was clearly stated in the original PR that this would be the case, I left it up to maintainers to make that call. It can be reverted if need be.

[ChrisIsidora]

@daniepett Furthermore this changes the behavior of what people expect that this resource will do and also for people like me that have been using these resources for ages and expect it to work a specific way. If the community wants this new behavior i would rather see it be implemented with a feature flag / switch rather than a full behavior change without introducing new resources

[ChrisIsidora]

I do agree, but for implementation of this functionality it was a trade off. It was clearly stated in the original PR that this would be the case, I left it up to maintainers to make that call. It can be reverted if need be.

@alldoami @edulop91 @robbruce What do you think?

[daniepett]

@ChrisIsidora I've pushed some changes as an example of a feature flag implementation. Is there a better way to implement this than on a resource level?

[ChrisIsidora]

@ChrisIsidora I've pushed some changes as an example of a feature flag implementation. Is there a better way to implement this than on a resource level?

@daniepett I think it's a good implementation. Normally this is implemented a different way but in this case I think it's fine. Usually for these partial/multiple cases you see that some providers chose to introduce a new resources with only that specific functionality. A fair example is for instance azurerm_keyvault and azurerm_keyvault_access_policy. Access policy is also defined on azurerm_keyvault.

Thanks for the changes :-)

[daniepett]

@ChrisIsidora It will potentially force replacement of a few grant resources as some of the grant resources don't have update functions and the config for them has changed. Should hopefully be a compromise everyone can live with.

Thanks for the feedback, do appreciate that :)

[alldoami]

@daniepett I want to say we should error on reverting as this is causing issues. Sorry we didn't do enough testing to see whether or not this would cause issues. Is that possible to do until we have more ways to test this?

[ChrisIsidora]

@alldoami I think with the new changes and the feature flag set by default to false it should have the same way of working as before. So i think now it should be ok. I can do a test on my side when you release.

[robbruce]

@daniepett @alldoami @ChrisIsidora - Can a StateMigration function be added? With different schema versions? So that the the provider know's how to migrate the old id format to the new id format? I'm not sure if can be used with the id, I've never done it myself.

https://www.terraform.io/plugin/sdkv2/resources/state-migration

[alldoami]

/ok-to-test sha=4a19ffd

[github-actions]

Integration tests failure for 4a19ffd

[ChrisIsidora]

@daniepett Can you fix the tests?

[daniepett]

Hi @ChrisIsidora, I don't have time until later today. Seems to be all import steps that are failing, so need to figure out the best way to deal with these. Feel free to have a go at them.

[daniepett]

@ChrisIsidora @alldoami I've ignored the feature flag for imports, won't affect functionality. It can't be read from current state, so better to use the default false once it's imported.

[ChrisIsidora]

Can we have a ok-to-test @alldoami?

[alldoami]

/ok-to-test sha=0ff1779

[github-actions]

Integration tests success for 0ff1779

[ChrisIsidora]

@alldoami Can we release a new version? Thanks in advance