identity: add validation to check if a resource identity is null #1513
identity: add validation to check if a resource identity is null #1513
Conversation
helper/schema/resource.go
Outdated
|
|
||
| // AllowNullIdentity toggles whether the managed resource allows identities to be null. Setting this flag to true | ||
| // disables the SDK validation that ensures the identity cannot be null at the end of ApplyResourceChange and ReadResource | ||
| // RPC calls in certain situations. | ||
| AllowNullIdentity bool |
There was a problem hiding this comment.
Added this flag so that we could toggle this behaviour off in case it introduced any unforeseen bugs or behaviours that could break providers - but as it stands no one can think of a specific use case where an identity would need to be completely null, so perhaps this should be removed.
There was a problem hiding this comment.
Continuing our offline conversation, I think I'm on the side of removing this flag and just always applying it. In lieu of a real example of a resource that has an identity that is allowed to be fully null (while still supporting identity), I think any bugs we introduce would require a re-release of the provider anyways, so we could always come back and add a flag if we find a scenario or adjust the validation logic
There was a problem hiding this comment.
If we introduce any bugs with this validation that can be toggled off by this flag I'm not sure I follow why we would still need to re-release the provider, but I'm fine with removing this flag and reassessing on a case by case basis.
There was a problem hiding this comment.
Left some comments! Also one suggestion to perhaps increase our confidence in this change.
Perhaps we can reach out to the AWS team to run these changes (perhaps after we merge to main, before releasing the module) against their acceptance test suite? They have the most resource identity implementations and have a lot of tests that might help us see if there are any edges we missed.
If all those tests are passing that would give us some confidence that we don't need any flags to skip this validation
helper/schema/resource.go
Outdated
|
|
||
| // AllowNullIdentity toggles whether the managed resource allows identities to be null. Setting this flag to true | ||
| // disables the SDK validation that ensures the identity cannot be null at the end of ApplyResourceChange and ReadResource | ||
| // RPC calls in certain situations. | ||
| AllowNullIdentity bool |
There was a problem hiding this comment.
Continuing our offline conversation, I think I'm on the side of removing this flag and just always applying it. In lieu of a real example of a resource that has an identity that is allowed to be fully null (while still supporting identity), I think any bugs we introduce would require a re-release of the provider anyways, so we could always come back and add a flag if we find a scenario or adjust the validation logic
stephybun
force-pushed
the
f/prevent-null-identity-validation
branch
from
e10bfe6 to
7057933
Compare
rainkwan
left a comment
rainkwan
left a comment
There was a problem hiding this comment.
Comments look to be addressed and seems sound to me 🚀
3 participants
Related Issue
Closes #1502
Description
Adds validation to the ApplyResource and ReadResource RPC to check whether a resource identity is fully null i.e. all attributes within the identity are null.
Rollback Plan
Changes to Security Controls
None