🤖🤖🤖 r/aws_cognito_user_pool: Fix InvalidParameterException for a partially specified admin_create_user_config.invite_message_template - #49207
Open
timoguin wants to merge 3 commits into
Conversation
`expandAdminCreateUserConfigType` reads `admin_create_user_config` from a Terraform Plugin SDKv2 nested block map, in which every declared attribute is always present holding its zero value. The `ok` presence checks therefore never fail, and an unset `email_message`, `email_subject`, or `sms_message` was marshalled as `""`. Cognito requires a minimum length for each of these (6, 1, and 6 respectively), so `CreateUserPool` and `UpdateUserPool` fail with `InvalidParameterException` whenever the block is partially specified, even though all three attributes are Optional. An omitted field is accepted; only a present-and-empty one is not. Guard each assignment on a non-empty value, matching `expandVerificationMessageTemplateType` in the same file, which already does this for the sibling `verification_message_template` block. `""` is not a legal value for any of the three, so nothing expressible is discarded, and `flattenAdminCreateUserConfigType` already skips nil fields, so an omitted field reads back as `""` and matches the configuration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mplate` Every existing test sets all three `invite_message_template` attributes, so the partially specified case had no coverage. Add `TestExpandAdminCreateUserConfigType`, covering email-only, SMS-only, empty and absent templates, plus acceptance tests for email-only and SMS-only invite templates with `ImportState`/`ImportStateVerify` steps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
✅ Thank you for correcting the previously detected issues! The maintainers appreciate your efforts to make the review process as smooth as possible. |
Contributor
Community GuidelinesThis comment is added to every new Pull Request to provide quick reference to how the Terraform AWS Provider is maintained. Please review the information below, and thank you for contributing to the community that keeps the provider thriving! 🚀 Voting for Prioritization
Pull Request Authors
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the library.
Changes to Security Controls
No.
Description
admin_create_user_config.invite_message_templatecannot be partially specified today. All three attributes are documentedOptional, but setting any one of them requires setting all three.expandAdminCreateUserConfigTypereads the block from a Plugin SDKv2 nested block map, in which every declared attribute is always present holding its zero value. Theif v, ok := tfMap[...]; okpresence checks therefore never fail, so an unset attribute is marshalled asaws.String(""):Cognito enforces a minimum length on each field (
emailMessage6,emailSubject1,sMSMessage6), soCreateUserPool/UpdateUserPoolfail:The defect is symmetric — an SMS-only template fails on the two email fields the same way.
This guards each assignment on a non-empty value, matching
expandVerificationMessageTemplateTypein the same file, which already does exactly this for the siblingverification_message_templateblock.Why this is safe:
us-east-2:CreateUserPoolwithSMSMessageabsent fromInviteMessageTemplatesucceeds and stores an email-only template, on a pool with nosms_configuration.""is not a legal value for any of the three. The provider's own validators (validUserPoolInviteTemplateEmailMessage,validUserPoolTemplateEmailSubject,validUserPoolInviteTemplateSMSMessage) enforce the same minimum lengths as the API, so no configuration's intent is "send the empty string" and the guard discards nothing expressible.flattenAdminCreateUserConfigTypealready skips nil fields, so an omitted field reads back as the SDKv2 zero value""and matches the configuration. Covered by theImportState/ImportStateVerifysteps in the new tests.sms_messagefrom a configuration that had one will not clear it in Cognito. That is pre-existing and unavoidable — sending""cannot clear it either, since the API rejects""; clearing requires removing the wholeinvite_message_templateblock. Theverification_message_templatesibling behaves identically today.A plan-time validator cannot address this: an SDKv2
ValidateFuncdoes not run on an unset attribute, so the minimum-length validators never fire on this path. Making the attributesRequiredwas also considered and rejected — it would break working configurations that set all three, and would encode an API quirk in the provider's interface rather than fixing the marshalling.Every existing test sets all three attributes, which is why this survived so long. Added:
TestExpandAdminCreateUserConfigType— a unit test over the map shape SDKv2 actually supplies, covering all-fields, email-only, SMS-only, empty and absent templates. This fails onmainwithout AWS credentials.TestAccCognitoIDPUserPool_withAdminCreateUserEmailOnlyInviteMessageTemplateand..._withAdminCreateUserSMSOnlyInviteMessageTemplate, each with anImportState/ImportStateVerifystep.The two acceptance tests are deliberately separate rather than two steps of one test: transitioning between email-only and SMS-only cannot clear the previously set fields (see above), so a second step would report a non-empty plan for reasons unrelated to this change.
AI usage disclosure
Per
docs/ai-usage.md, this PR was prepared with the assistance of an LLM agent (Claude Code), hence🤖🤖🤖in the title.expandVerificationMessageTemplateType— and wrote the guard, the unit test, and the two acceptance tests plus their config builders.us-east-2both before (reproducing theInvalidParameterExceptionabove) and after the change, as well asmake build,make test, andmake ci-quick.Relations
Closes #49203
Relates #13060
References
CreateUserPool—MessageTemplateType(minimum lengths forEmailMessage,EmailSubject,SMSMessage)Output from Acceptance Testing
Before the change (the reported failure, reproduced):
After the change, including the previously passing all-fields tests:
Unit test: