Add support for custom certificates

[gallois]

The parameters are added to the provider config to support it:

• ca_cert
• client_cert
• client_key

You need to have all three passed for the configuration to be picked up, it will fail otherise. It also overrides the tls parameter (cfg.TLSConfig), if the latter is set.

Tests are not passing (they were passing upstream either), but running it locally works fine.

[gallois]

Tests are actually passing for me (both before and after this change) when I run make acceptance, but how much work would it be to add an acceptance test, that would test this feature works?

Oh, fair enough. I tried with make test only. I'll take a look at adding acceptance tests as well :)

[petoju]

Oh, fair enough. I tried with make test only. I'll take a look at adding acceptance tests as well :)

Good point - I have never noticed. It needs to be eventually fixed.

[gallois]

I'm struggling a bit to write an acceptance test for this one, especially because it requires dealing with the certificates both on the host and the container. And as I mentioned before, I'm not too familiar with how the code should work for that :)

I updated the PR with the changes to address the comments and I'll create a separate one for the acceptance tests

[petoju]

I'm struggling a bit to write an acceptance test for this one, especially because it requires dealing with the certificates both on the host and the container. And as I mentioned before, I'm not too familiar with how the code should work for that :)

I updated the PR with the changes to address the comments and I'll create a separate one for the acceptance tests

That's fine even without acceptance tests if that is complicated - but please at least manually test it.

[petoju]

I have 1 more request: please add the structure to documentation so people can find it without searching the source code.

[gallois]

I did manual tests in two different setups that I have, one with the custom_tls config and another without it. Both worked fine.

With custom_tls

$ TF_LOG=INFO terraform plan
(...)
***: Refreshing state... [id=***]
2024-02-13T22:25:49.385Z [INFO]  provider.terraform-provider-mysql: 2024/02/13 22:25:49 [DEBUG] Using dsn: ***@tcpcheckConnLiveness=false&interpolateParams=true&tls=***&maxAllowedPacket=0: timestamp=2024-02-13T22:25:49.385Z
2024-02-13T22:25:49.385Z [INFO]  provider.terraform-provider-mysql: 2024/02/13 22:25:49 [DEBUG] Using driverName: mysql: timestamp=2024-02-13T22:25:49.385Z
2024-02-13T22:25:49.385Z [INFO]  provider.terraform-provider-mysql: 2024/02/13 22:25:49 [DEBUG] Waiting for state to become: [success]: timestamp=2024-02-13T22:25:49.385Z
2024-02-13T22:25:50.139Z [INFO]  provider.terraform-provider-mysql: 2024/02/13 22:25:50 [DEBUG] Using dsn: ***@tcp(***)/?checkConnLiveness=false&interpolateParams=true&tls=***&maxAllowedPacket=0: timestamp=2024-02-13T22:25:50.139Z
(...)
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # mysql_grant.*** will be created
  + resource "mysql_grant" "***" {
      + database   = "***"
      + grant      = false
      + host       = "***"
      + id         = (known after apply)
      + privileges = [
          + "SELECT",
          + "SHOW VIEW",
        ]
      + table      = "*"
      + tls_option = "NONE"
      + user       = "***"
    }

Plan: 1 to add, 0 to change, 0 to destroy.
(...)

Without custom_tls

$ terraform plan
mysql_user.foo_user_local: Refreshing state... [id=foo_user@localhost]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # mysql_grant.foo_grant_local will be created
  + resource "mysql_grant" "foo_grant_local" {
      + database   = "test_db"
      + grant      = false
      + host       = "localhost"
      + id         = (known after apply)
      + privileges = [
          + "SELECT",
        ]
      + table      = "*"
      + tls_option = "NONE"
      + user       = "foo_user"
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Any other way I can test it?

Another question I have is, do I have to run anything to generate the documentation? I tried make website but it failed locally.

[petoju]

Any other way I can test it?

For security-related features, at least these things should be always tested:

• Whether it works when it's configured and everything is set up properly (you tested this)
• Whether it fails when there is something to protect against. That means - does it fail with another (otherwise correct) CA cert?
• Whether it fails loudly when it's misconfigured (bad / non-existing certificate). Downgrading security because user probably wanted it is not an option. I saw this working correctly.
• Does it work, when security is not configured? (You tested this)

Another question I have is, do I have to run anything to generate the documentation? I tried make website but it failed locally

It will be generated automatically. By make website, you can look at it before publishing it. It shows address where to look at provider's web like http://localhost:4567/docs/providers/mysql

It is really helpful as it shows you didn't write it properly - you need a newline just before starting code block. And the last line of code block contains extra empty line, that's not necessary.

[gallois]

Whether it works when it's configured and everything is set up properly (you tested this)

👍

Whether it fails when there is something to protect against. That means - does it fail with another (otherwise correct) CA cert?

It does. The error is something like:

2024-02-14T13:42:15.222Z [ERROR] vertex "mysql_grant.*** (import id \"***@***@***@*\")" error: Cannot import non-existent remote object
╷
│ Error: Cannot import non-existent remote object
│
│ While attempting to import an existing object to "mysql_grant.***", the provider detected that no object exists with the given id. Only pre-existing objects
│ can be imported; check that the id is correct and that it is associated with the provider's configured region or endpoint, or use "terraform apply" to create a new remote object
│ for this resource.

This is for the case where you are using certificates for another identity in the same endpoint.

Whether it fails loudly when it's misconfigured (bad / non-existing certificate). Downgrading security because user probably wanted it is not an option. I saw this working correctly.

Should fail in different ways, depending on the failure mode, one example

$ terraform plan
╷
│ Error: failed to read CA cert: open : no such file or directory

Does it work, when security is not configured? (You tested this)

👍

By make website, you can look at it before publishing it. It shows address where to look at provider's web like:

Unfortunately, I get

$ make website
make[1]: *** No rule to make target `website-provider'.  Stop.
make: *** [website] Error 2

when I try it

[petoju]

Ah, so for that make website, it was changed on that source URL :-/. I'll try to fix it.

Besides that, I had some time to test this - I added comments to the code (just text + documentation) to reflect what I found. Especially (default) large timeout makes this time-consuming to debug and it would be useful to help users by telling them, how to lower it temporarily.

So my comments are fixed, I'll merge it.

[gallois]

Just pushed changes to address your comments. I'd ask you to just verify that I didn't mess up with the documentation formatting since I couldn't run it locally :) Thanks!

[petoju]

@gallois thanks for your PR! Now, you should be able to run website locally.

Building this with version 3.0.48

[gallois]

Thanks for your patience in reviewing it 🙏