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

Allow DBA password to be updated via Terraform #46

Merged
merged 5 commits into from
Apr 8, 2024

Conversation

adriansuarez
Copy link
Collaborator

This change allows the DBA password to be updated declaratively via Terraform. If a change is detected to the dba_password attribute, then we first attempt to update the password using the POST /dbaPassword sub-resource. If that fails for any reason, then we throw an error. We specifically detect the case where the failure is due to the sub-resource not being supported and display a message telling the user to revert the value of the password.

This is a change in behavior, since previously we would ignore changes to the DBA password and display a warning, unless the environment variable NUODB_CP_ALLOW_DESTRUCTIVE_REPLACE=true was set, which caused the database to be deleted and recreated. The previous behavior does not seem desirable, and a support ticket was already filed about it by a customer, so it seems okay to change it.

This change also includes:

  • Updates the OpenAPI spec to target the latest development version of the Control Plane REST server. This will have to be set to the published version before releasing 1.1.0.
  • Includes some refactoring to the way the schema is built to allow the specific resources that use GenericResource to supply the Terraform schema attributes, rather than allowing the GenericResource to convert the OpenAPI schema to the Terraform schema attributes. This is useful in case we want to include special handling of certain attributes that is different from the behavior associated with the various OpenAPI extensions (e.g. x-immutable).
  • The GenericResource is updated to supply both the plan and the state, to allow implementer of the ResourceState interface to perform special update logic for certain attribute changes.

This change allows the DBA password to be updated declaratively via
Terraform. If a change is detected to the `dba_password` attribute, then
we first attempt to update the password using the `POST /dbaPassword`
sub-resource. If that fails for any reason, then we throw an error. We
specifically detect the case where the failure is due to the
sub-resource not being supported and display a message telling the user
to revert the value of the password.

This is a change in behavior, since previously we would ignore changes
to the DBA password and display a warning, unless the environment
variable `NUODB_CP_ALLOW_DESTRUCTIVE_REPLACE=true` was set, which caused
the database to be deleted and recreated. The previous behavior does not
seem desirable, and a support ticket was already filed about it by a
customer, so it seems okay to change it.

This change also includes:

- Updates the OpenAPI spec to target the latest development version of
  the Control Plane REST server. This will have to be set to the
  published version before releasing 1.1.0.
- Includes some refactoring to the way the schema is built to allow the
  specific resources that use GenericResource to supply the Terraform
  schema attributes, rather than allowing the GenericResource to convert
  the OpenAPI schema to the Terraform schema attributes. This is useful
  in case we want to include special handling of certain attributes that
  is different from the behavior associated with the various OpenAPI
  extensions (e.g. x-immutable).
- The GenericResource is updated to supply both the plan and the state,
  to allow implementer of the ResourceState interface to perform special
  update logic for certain attribute changes.
Comment on lines +64 to +65
return fmt.Errorf("DBA password for database %s/%s/%s has not been updated",
state.Organization, state.Project, state.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the logic, but it would be nice to include in the error details that the most likely cause of the failure is that there was an external change that set the database password to not match the old or new password and to resolve it, manually set the password to the new one.

Copy link
Collaborator Author

@adriansuarez adriansuarez Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is the case where an error was not returned by the REST service. If it did, we would have returned the error to the user.

If we're here, then the response would have been "202 Accepted", which indicates that password rotation is still in progress. Password rotation could be in progress indefinitely if the database is in Stopped state, or if there is some issue that is preventing the operator from doing reconciliation (but not an authentication failure, since that would cause the operator to blow away target key).

Copy link
Collaborator

@sivanov-nuodb sivanov-nuodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@adriansuarez adriansuarez merged commit 910a7ce into main Apr 8, 2024
4 checks passed
@adriansuarez adriansuarez deleted the asuarez/update-dba-password branch April 8, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants