Mark google_sql_database.{charset,collation} as computed instead of having defaults

[ubschmidt2]

This change is required to avoid the following scenario:
When upgrading from a previous version of the Google provider, TF will change
the charset/collation of existing (TF-managed) databases to utf8/utf8_general_ci
(if the user hasn't added different config values before running TF apply),
potentially overriding any non-default settings that the user my have applied
through the Cloud SQL admin API. This violates POLA.

This is an addendum to PR #183 and issue #179.

[ubschmidt2]

PLKM if this makes sense. I'm not sure how this could be properly tested for in an acceptance test.

[rileykarson]

Right now, if we try to update the charset of a Postgres database, it fails to update silently while reporting that the operation was a success. As a result, the config also does not converge as Read will read the old value. This isn't ideal, and I'd like to be able to error out if we try to update these values on Postgres, while keeping Update working for MySQL databases.

We don't have the information in the database object returned from the API to tell what the type of the database is; we'll need to get it in some other way.

My best idea so far is to pull the google_sql_database_instance in Create, and to perform a d.Set() against a new computed field we add to this resource, database_type, stripping version information so we just set mysql or postgres. Then, in Update, we check if the database_type is postgres, and if charset or collation have any changes using HasChange(); if so, we return an error.

We could also set the fields ForceNew and file a bug upstream that we get no error, deferring Update support until either both can do it or we can bubble up an API error for Postgres.

@danawillow, @paddycarver: What are your thoughts on this kind of approach?

[rileykarson]

Let's use version 5.7's docs instead of 5.6, since that's the most up to date version of MySQL that we support.

[ubschmidt2]

Done.

[paddycarver]

My 2 cents on the update problem:

• Filing an upstream bug is definitely where I'd start with this.
• We should similarly file a bug for (what sounds like?) our inability to tell whether it's MySQL or Postgres
• My concern with the Create workaround is how it will work with import. If you can successfully create a database outside of Terraform (e.g., through the console) and then can import it, and the type still gets set correctly, I'm happy.
• Silly question, but there's no way for the database to change type, right? Otherwise, Create is really the wrong spot for it, as we need to detect drift somehow.

I think if we can't get a workaround working without too much hacky behaviour, not allowing it for MySQL is better than silently failing in Postgres.

[rileykarson]

This is all the context I have on what's going on with these values in Postgres right now, + with the Cloud SQL documentation for it. Feel free to correct anything I got wrong; I don't know what I want to do to fix how these fields work for it yet.

There exist two side by side api docs; MySQL and Postgres that are essentially the same, and describe the exact same API client; they are written for interacting with MySQL databases and not Postgres.

This becomes really clear when looking at Creating and Managing PostgreSQL Databases, where it's specified that when creating a database in Cloud Console;

In the New database dialog, specify the name of the database, and optionally the character set and collation.

This operation is impossible as you can only specify these values [charset and collation] for a MySQL database in the Cloud Console UI; they are automatically always UTF8 and en_US.UTF8 for Postgres.

Why becomes clear when Create-ing a database in a Postgres instance with custom values; even though charset becomes the proper Postgres value, ENCODING, and collation becomes LC_COLLATE, it still doesn't work. The issue is that the creation script is:

CREATE DATABASE "db-name" WITH OWNER cloudsqlsuperuser ENCODING `UTF8` LC_COLLATE 'en_GB.UTF8';

Because we didn't explicitly specify a "template" database, Postgres implicitly uses template1, a template that has it's charset and collation set to the same as the system defaults. These are UTF8 and en_US.UTF8. There exists another default template, template0, which has no charset/collation, and would work. But since we used template1 and don't match the template, the CREATE fails. The error on the Go side has no text so users won't know what's wrong if they encounter this error, so we would need to make an error ourselves.

Read works as expected, reflecting the correct value from each type of database, as does Delete.

Update for Postgres does nothing; the Operation reports success, and no changes are made. So we will never converge if the value is changed if the fields are not ForceNew. There should be an API-side error here, because changing these values for Postgres has a different meaning than for MySQL. (see #260 for context on that)

[ubschmidt2]

PTAL. I think it would be worthwhile to merge this PR. It's an improvement for MySQL, but doesn't address the larger PostgreSQL issues yet.

[rileykarson]

Would you mind adding a note that Postgres doesn't support a charset other than UTF8 and a collation other than en_US.UTF8 at Create?

Otherwise this LGTM - we don't have a strong case for preferring Default over Computed, especially when there can be two values like in this case.

[ubschmidt2]

Apologies for the delay. Now back on this. I've added a small note about the restriction for PostgreSQL databases.

[ubschmidt2]

Sent upstream FR for supporting user-specified charset and collation values for Cloud SQL PostgreSQL databases.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!