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.

Sign up for GitHub

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

Jump to bottom

b/aws_redshiftserverless_namespace: fix multiple bugs, rewrite to plugin framework #39354

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

b/aws_redshiftserverless_namespace: fix multiple bugs, rewrite to plugin framework #39354

wants to merge 1 commit into from
+893 −363

Conversation

marcinbelczewski
Copy link

@marcinbelczewski marcinbelczewski commented Sep 17, 2024 •
edited
Loading

Description

This PR:

  • fixes several bugs in the implementation of aws_redshiftserverless_namespace (bugs listed below)
  • implements support for several drift fixing scenarios
  • adds missing properties to aws_redshiftserverless_namespace data source
  • due to extensive changes to the source code required, takes the opportunity to rewrite the resource and data source to plugin framework
  • introduces SetOfARNType
  • suppresses (known after apply) message for computed only attributes in places where it could cause confusion
  • is backward compatible - applying namespace in current provider and then updating using the PR code results in no changes for the namespace resource and only addition of two new attributes to the output of namespace data source.

It has to be mentioned that in the PR author's opinion, many of the problems present in the current implementation of aws_redshiftserverless_namespace, are caused by suboptimal design of Amazon Redshift Serverless API. There are still several bugs in the API implementation reported to Amazon but currently with neither a confirmation nor ETA on fixes:

  1. certain updates in managed password mode result in removal and orphaning of admin password secrets
  2. certain updates in managed password mode result in removal of kms key used for secret encryption
  3. associating workgroup with the namespace created with default parameters, results in an update to the namespace and changing the dbName from empty to "dev"
  4. manageAdminPassword property being instrumental in setting up password management yet missing from GetNamespace response payload

Thanks to intensive use of ValidateConfig and ModifyPlan custom implementations, It was possible to work around problems 3 and 4 in the provider code. Problems 1 and 2 can still cause issues and require careful handling of namespace resource.

Relations

Closes #39333 - default "admin" password is sent in API request
Closes #39273 - default "dev" db_name is used in namespace creation
Closes #39269 - removal of 'kms_key_id' from config results in resource replacement
Closes #39268 - passing empty string for to API call removes the role
Closes #39266
Closes #39265 - manage_admin_password = false can coincide with admin_user_password
Closes #39250
Closes #39249 - Update the implementation is similar to aws_redshiftserverless_workgroup
Closes #39248 - empty array is the default for log_exports

References

UpdateNamespace API documentation clarifies the limitation in updating multiple parameters in one request.

Output from Acceptance Testing

❯ make testacc TESTS=TestAccRedshiftServerlessNamespace PKG=redshiftserverless
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.6 test ./internal/service/redshiftserverless/... -v -count 1 -parallel 20 -run='TestAccRedshiftServerlessNamespace'  -timeout 360m
=== RUN   TestAccRedshiftServerlessNamespaceDataSource_basic
=== PAUSE TestAccRedshiftServerlessNamespaceDataSource_basic
=== RUN   TestAccRedshiftServerlessNamespaceDataSource_iamRole
=== PAUSE TestAccRedshiftServerlessNamespaceDataSource_iamRole
=== RUN   TestAccRedshiftServerlessNamespaceDataSource_user
=== PAUSE TestAccRedshiftServerlessNamespaceDataSource_user
=== RUN   TestAccRedshiftServerlessNamespaceDataSource_logExports
=== PAUSE TestAccRedshiftServerlessNamespaceDataSource_logExports
=== RUN   TestAccRedshiftServerlessNamespace_emptyPlanWithArrayOutputs
=== PAUSE TestAccRedshiftServerlessNamespace_emptyPlanWithArrayOutputs
=== RUN   TestAccRedshiftServerlessNamespace_basic
=== PAUSE TestAccRedshiftServerlessNamespace_basic
=== RUN   TestAccRedshiftServerlessNamespace_defaultIAMRole
=== PAUSE TestAccRedshiftServerlessNamespace_defaultIAMRole
=== RUN   TestAccRedshiftServerlessNamespace_defaultUserAndCustomPassword
=== PAUSE TestAccRedshiftServerlessNamespace_defaultUserAndCustomPassword
=== RUN   TestAccRedshiftServerlessNamespace_customUserAndUnmanagedPassword
=== PAUSE TestAccRedshiftServerlessNamespace_customUserAndUnmanagedPassword
=== RUN   TestAccRedshiftServerlessNamespace_tags
=== PAUSE TestAccRedshiftServerlessNamespace_tags
=== RUN   TestAccRedshiftServerlessNamespace_disappears
=== PAUSE TestAccRedshiftServerlessNamespace_disappears
=== RUN   TestAccRedshiftServerlessNamespace_withWorkgroup
=== PAUSE TestAccRedshiftServerlessNamespace_withWorkgroup
=== RUN   TestAccRedshiftServerlessNamespace_manageAdminPassword
=== PAUSE TestAccRedshiftServerlessNamespace_manageAdminPassword
=== RUN   TestAccRedshiftServerlessNamespace_replaceOnKmsKeyRemoval
=== PAUSE TestAccRedshiftServerlessNamespace_replaceOnKmsKeyRemoval
=== RUN   TestAccRedshiftServerlessNamespace_iamRolesCleared
=== PAUSE TestAccRedshiftServerlessNamespace_iamRolesCleared
=== RUN   TestAccRedshiftServerlessNamespace_passwordManagementDrift
=== PAUSE TestAccRedshiftServerlessNamespace_passwordManagementDrift
=== RUN   TestAccRedshiftServerlessNamespace_adminUsernameDriftWithManagedPassword
=== PAUSE TestAccRedshiftServerlessNamespace_adminUsernameDriftWithManagedPassword
=== CONT  TestAccRedshiftServerlessNamespaceDataSource_basic
=== CONT  TestAccRedshiftServerlessNamespace_tags
=== CONT  TestAccRedshiftServerlessNamespaceDataSource_user
=== CONT  TestAccRedshiftServerlessNamespace_basic
=== CONT  TestAccRedshiftServerlessNamespace_withWorkgroup
=== CONT  TestAccRedshiftServerlessNamespace_disappears
=== CONT  TestAccRedshiftServerlessNamespaceDataSource_logExports
=== CONT  TestAccRedshiftServerlessNamespace_manageAdminPassword
=== CONT  TestAccRedshiftServerlessNamespace_defaultUserAndCustomPassword
=== CONT  TestAccRedshiftServerlessNamespace_customUserAndUnmanagedPassword
=== CONT  TestAccRedshiftServerlessNamespace_passwordManagementDrift
=== CONT  TestAccRedshiftServerlessNamespace_adminUsernameDriftWithManagedPassword
=== CONT  TestAccRedshiftServerlessNamespace_iamRolesCleared
=== CONT  TestAccRedshiftServerlessNamespace_emptyPlanWithArrayOutputs
=== CONT  TestAccRedshiftServerlessNamespace_defaultIAMRole
=== CONT  TestAccRedshiftServerlessNamespace_replaceOnKmsKeyRemoval
=== CONT  TestAccRedshiftServerlessNamespaceDataSource_iamRole
--- PASS: TestAccRedshiftServerlessNamespace_emptyPlanWithArrayOutputs (24.07s)
--- PASS: TestAccRedshiftServerlessNamespaceDataSource_logExports (26.30s)
--- PASS: TestAccRedshiftServerlessNamespaceDataSource_basic (26.73s)
--- PASS: TestAccRedshiftServerlessNamespace_customUserAndUnmanagedPassword (27.12s)
--- PASS: TestAccRedshiftServerlessNamespace_defaultUserAndCustomPassword (27.12s)
--- PASS: TestAccRedshiftServerlessNamespaceDataSource_user (27.12s)
--- PASS: TestAccRedshiftServerlessNamespace_disappears (27.42s)
--- PASS: TestAccRedshiftServerlessNamespaceDataSource_iamRole (29.11s)
--- PASS: TestAccRedshiftServerlessNamespace_manageAdminPassword (35.85s)
--- PASS: TestAccRedshiftServerlessNamespace_replaceOnKmsKeyRemoval (36.54s)
--- PASS: TestAccRedshiftServerlessNamespace_iamRolesCleared (37.77s)
--- PASS: TestAccRedshiftServerlessNamespace_basic (41.76s)
--- PASS: TestAccRedshiftServerlessNamespace_defaultIAMRole (42.50s)
--- PASS: TestAccRedshiftServerlessNamespace_tags (45.96s)
--- PASS: TestAccRedshiftServerlessNamespace_adminUsernameDriftWithManagedPassword (62.83s)
--- PASS: TestAccRedshiftServerlessNamespace_passwordManagementDrift (62.90s)
--- PASS: TestAccRedshiftServerlessNamespace_withWorkgroup (271.29s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/redshiftserverless	276.830s

@github-actions GitHub Actions
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. sweeper Pertains to changes to or issues with the sweeper. generators Relates to code generators. service/redshiftserverless Issues and PRs that pertain to the redshiftserverless service. needs-triage Waiting for first response or review from a maintainer. labels Sep 17, 2024
@marcinbelczewski marcinbelczewski force-pushed the b-aws_redshiftserverless_namespace branch 8 times, most recently from b493ec1 to e5f366d Compare September 17, 2024 13:15
@marcinbelczewski marcinbelczewski force-pushed the b-aws_redshiftserverless_namespace branch from e5f366d to 19f88d4 Compare September 17, 2024 13:18
@github-actions github-actions bot added the linter Pertains to changes to or issues with the various linters. label Sep 17, 2024
@marcinbelczewski marcinbelczewski marked this pull request as ready for review September 17, 2024 13:43
@marcinbelczewski marcinbelczewski requested a review from a team as a code owner September 17, 2024 13:43
@ewbankkit ewbankkit removed the needs-triage Waiting for first response or review from a maintainer. label Sep 17, 2024
@ewbankkit ewbankkit self-assigned this Sep 17, 2024
@github-actions github-actions bot added the prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. label Sep 17, 2024
@ewbankkit ewbankkit removed their assignment Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
generators Relates to code generators. linter Pertains to changes to or issues with the various linters. prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. service/redshiftserverless Issues and PRs that pertain to the redshiftserverless service. sweeper Pertains to changes to or issues with the sweeper. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: r/aws_redshiftserverless_namespace: in managed password mode, unable to change admin username [Bug]: r/aws_redshiftserverless_namespace: conflict between manage_admin_password and admin_user_password [Bug]: r/aws_redshiftserverless_namespace: unable to switch to managed admin password [Bug]: r/aws_redshiftserverless_namespace: unable to update multiple attributes at the same time [Bug]: r/aws_redshiftserverless_namespace: non-empty plan after apply with log_exports in output
[Bug]: r/aws_redshiftserverless_namespace: unable to remove default IAM role [Bug]: r/aws_redshiftserverless_namespace: removing kms_key_id from config has no effect and can lead to problems [Bug]: r/aws_redshiftserverless_namespace: non-empty plan after creating minimal namespace with workgroup and db_name output [Bug]: r/aws_redshiftserverless_namespace: invalid configuration for resource creation but valid for resource update
2 participants
@marcinbelczewski @ewbankkit