feat: add custom tls config support to mysql #184

silphid · 2024-06-12T14:49:59Z

Description of your changes

This PR adds support for custom TLS configuration to mysql implementation. In provider config file, if tls is set to custom, it reads custom TLS configuration from tlsConfig property, reading CA cert and client key/pair from K8s secret(s), and registering that config in mysql driver under the custom key.

Even though the mysql driver allows for multiple tls config key/value pairs, in the context of the provider it didn't appear to make sense to allow user to configure multiple TLS configurations and select only one of them, therefore the tlsConfig property is not a map, but rather a single config entry.

I have:

Read and followed Crossplane's [contribution process].
Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Because e2e tests require a totally different setup with a TLS-enabled mariaDB instance (but with same test cases), the current test script was duplicated and modified to add TLS, making sure that make test-integration runs both the no-tls and tls test scripts. It would be possible to refactor both scripts to combine them together and reduce duplication of setup and test code, however to the cost of readability. Let me know if that is a blocker and I will address it, I just didn't want to introduce more complexity in e2e test script until you confirm that's really what you prefer.

silphid

(left a few comments in-line here to explain some of the decisions)

silphid · 2024-06-12T14:54:09Z

cluster/local/integration_tests_no_tls.sh

+"${UP}" alpha xpkg xp-extract --from-xpkg "${OUTPUT_DIR}"/xpkg/linux_"${SAFEHOSTARCH}"/"${PACKAGE_NAME}"-"${VERSION}".xpkg -o "${CACHE_PATH}/${PACKAGE_NAME}.gz"
+chmod 644 "${CACHE_PATH}/${PACKAGE_NAME}.gz"


This was broken down onto two lines instead of && because that silenced all errors in up alpha xpkg command and continued with rest of script in case of failures.

silphid · 2024-06-12T14:57:07Z

cluster/local/integration_tests_no_tls.sh

 echo_step "installing MariaDB Helm chart into default namespace"
-mariadb_root_pw=$(LC_ALL=C tr -cd "A-Za-z0-9" </dev/urandom | head -c 32)
+# install MariaDB chart
+mariadb_root_pw=$(openssl rand -base64 32)


This is probably a better way to generate a random password, because the previous approach was generating errors:

tr: write error: Broken pipe tr: write error

because head had to truncate its input stream and close its pipe, which tr complained about. It was still working OK, even with the error message, probably because set -o pipefail is not set.

silphid · 2024-06-12T14:59:34Z

cluster/local/integration_tests_no_tls.sh

@@ -265,10 +266,10 @@ echo "${INSTALL_YAML}" | "${KUBECTL}" delete -f -
 timeout=60
 current=0
 step=3
-while [[ $(kubectl get providerrevision.pkg.crossplane.io -o name | wc -l) != "0" ]]; do
+while [[ $(kubectl get providerrevision.pkg.crossplane.io -o name | wc -l | tr -d '[:space:]') != "0" ]]; do


On macOS, the wc -l command is outputting the count with leading/trailing whitespace, which made that check wait infinitely.

probably also depending on PATH and brew packages, because I didn't notice anything on my mac :) But since this works on linux and in CI here I totally approve on the change 👍

silphid · 2024-06-12T15:01:18Z

cluster/local/integration_tests_no_tls.sh

+  current=$((current + step))
+  if [[ $current -ge $timeout ]]; then


This was also not ever timing out (only on macOS?), because doing integer operations/comparisons requires this special syntax.

silphid · 2024-06-12T15:09:03Z

cluster/local/integration_tests_tls.sh

+  init.sql: |
+    CREATE USER 'test'@'%' IDENTIFIED BY '${mariadb_test_pw}' REQUIRE X509;
+    GRANT ALL PRIVILEGES ON *.* TO 'test'@'%' WITH GRANT OPTION;
+    FLUSH PRIVILEGES;


A dedicated test user is required in order to specifically require X509 on it, but not for admin user, which is also used for health probes without TLS.

silphid · 2024-06-12T15:10:38Z

cluster/local/integration_tests_tls.sh

+auth:
+  rootPassword: ${mariadb_root_pw}
+primary:
+  extraFlags: "--ssl --require-secure-transport=ON --ssl-ca=/opt/bitnami/mariadb/certs/ca-cert.pem --ssl-cert=/opt/bitnami/mariadb/certs/server-cert.pem --ssl-key=/opt/bitnami/mariadb/certs/server-key.pem"


Note that --require-secure-transport=ON is not sufficient to require client to also provide its cert, we must also turn on REQUIRE X509 on specific users (see below).

silphid · 2024-06-12T15:13:01Z

cluster/local/integration_tests_tls.sh

+        namespace: default
+        name: mariadb-creds
+        key: client-key.pem
+    insecureSkipVerify: true


insecureSkipVerify is to be used only here in e2e tests, because certs are self-signed, otherwise server would reject its own cert.

silphid · 2024-06-12T15:18:04Z

pkg/controller/mysql/database/reconciler.go

 )

 const (
 	errTrackPCUsage = "cannot track ProviderConfig usage"
 	errGetPC        = "cannot get ProviderConfig"
 	errNoSecretRef  = "ProviderConfig does not reference a credentials Secret"
 	errGetSecret    = "cannot get credentials Secret"
+	errTLSConfig    = "cannot load TLS config"


Used an error constant for consistency here, even though Crossplane contribution guidelines no longer recommend using them.

silphid · 2024-06-19T21:52:44Z

@Duologic @chlunde @iainlane sorry for pinging you directly, I saw you seem to have been active here recently. Would just appreciate your cue on whether you think such a PR is likely to be reviewed on the short term, or if we should rather assume that we'll need to build and use our own fork for the next few months? Thanks! 🙏

chlunde · 2024-06-20T08:35:13Z

apis/mysql/v1alpha1/provider_types.go

 	// +optional
 	TLS *string `json:"tls"`
+


you may consider enforcing this
https://kubernetes.io/docs/reference/using-api/cel/

That would be an interesting exercise, but I have never used CEL, would need to read up on it and learn it, so I'd maybe hope to get away with it! 😅

chlunde · 2024-06-20T08:38:21Z

pkg/controller/mysql/tls/tls.go

+		return err
+	}
+
+	return mysql.RegisterTLSConfig("custom", &tls.Config{


Looks like "custom" is a global key here. I think this must have a unique name per DB if you connect to two databases?

Good catch! 👍 I have now fixed it by making the key suffixed with provider config name, in order to support multiple configs.

chlunde · 2024-06-20T08:40:15Z

pkg/controller/mysql/user/reconciler.go

@@ -115,6 +117,10 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E
 		return nil, errors.Wrap(err, errGetSecret)
 	}

+	if err := tls.LoadConfig(ctx, c.kube, pc.Spec.TLS, pc.Spec.TLSConfig); err != nil {


Question: can and should this be called in newDB instead? It's something you always have to call before newDB? 🤔

We could merge the two, but I see newDB as a non-failing function with no external calls, no error result, etc, and I also perceive those two functions as conceptually different. We would need to refactor newDB considerably, along with corresponding unit tests, as it would slightly affect its scope/role. I tried to be as little intrusive as possible with my changes, but if we commonly decide that they should be merged together, I will tackle that refactoring.

chlunde · 2024-06-20T08:45:31Z

@silphid not sure if we should duplicate the full integration test, because there's also talk about adding PG integration tests.

Two ideas:

Create a common library script with at least functions for "setup cluster + install provider" and "teardown cluster", and reuse that code by importing it as a library using source
Have a main driver script to run integration tests, calls smaller "integration_test_mysql_tls.sh", integration_test_mysql_no_tls.sh, integration_test_postgresql.sh, before cleanup. Each script can assume a working and "fairly clean" cluster. Each script must cleanup the cluster (but keep the provider installed).

@Duologic @Bastichou what do you think?

Duologic · 2024-06-24T20:42:01Z

Thanks for this, I don't have any objections on either proposals, I gladly follow what ya'll think is a good path forward.

Thanks @chlunde for the review, I'm lacking bandwidth to review this but don't mind rubberstamping your approval and get this merged.

@silphid don't forget to sign the DCO ;)

silphid · 2024-06-25T15:47:09Z

@silphid not sure if we should duplicate the full integration test, because there's also talk about adding PG integration tests.

All right, I went ahead and refactored all integration tests into more modular shell functions. I kept them inline for now, as we only have mysql tests, but eventually if we add tests for PG, they would be easy to extract into other files that could be sourced. I'm pretty satisfied with the result, which is IMO much cleaner and readable. Let me know what you think! :)

Signed-off-by: Mathieu Frenette <[email protected]>

silphid · 2024-06-25T18:36:15Z

@chlunde @Duologic I just rebased on latest master, resolved conflicts and signed all commits (also see my above comments/improvements). Anything else I should do before you can run the workflow again?

silphid · 2024-06-28T15:07:20Z

@chlunde I appreciate everyone is busy with their own projects, with little spare time left, but if it was possible to get a quick review on this one, I would be extremely grateful! 🙏☀️ We're hoping to start using those changes via the official release channel, but otherwise the plan is to setup the CI/CD for our fork internally, which I would sincerely prefer to avoid, if possible! 😅

chlunde

Cool, as far as I can see this looks good. I'm not a MySQL user so I can't fully vet this (and not a maintainer of provider-sql).

FYI @Duologic

@silphid when merged to main, there will be a image built even if there is no release, so it is possible to install it in a cluster without duplicating the pipeline.

silphid · 2024-07-09T15:27:01Z

@chlunde @Duologic still says it needs approval from a reviewer with write access.

silphid · 2024-07-29T15:21:08Z

Bump @chlunde @Duologic.

Would so much appreciate your review and ideally unblocking the execution of the checks/workflows! 🙏

silphid · 2024-08-28T15:20:50Z

Last bump @chlunde @Duologic 🤞 before we move on to setting up the build and publishing of our own fork, as a last resort.

silphid commented Jun 12, 2024

View reviewed changes

chlunde reviewed Jun 20, 2024

View reviewed changes

silphid force-pushed the add-custom-tls-config branch from b497c14 to bdd825a Compare June 21, 2024 19:22

silphid force-pushed the add-custom-tls-config branch from 5ec439d to 4d12693 Compare June 25, 2024 14:59

silphid added 3 commits June 25, 2024 13:25

feat: add custom tls config support

4d9c08a

Signed-off-by: Mathieu Frenette <[email protected]>

use provider config name to make tls name unique

eded330

Signed-off-by: Mathieu Frenette <[email protected]>

refactor integration tests into modular functions

6d9c446

Signed-off-by: Mathieu Frenette <[email protected]>

silphid force-pushed the add-custom-tls-config branch from 4d12693 to 6d9c446 Compare June 25, 2024 18:28

silphid requested a review from chlunde June 26, 2024 14:55

chlunde approved these changes Jul 8, 2024

View reviewed changes

silphid force-pushed the add-custom-tls-config branch from 4886268 to 6d9c446 Compare September 4, 2024 15:17

Duologic approved these changes Nov 28, 2024

View reviewed changes

Duologic merged commit 9189b59 into crossplane-contrib:master Nov 28, 2024
1 check passed

Duologic mentioned this pull request Nov 29, 2024

Add RDS CA to Docker image #94

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add custom tls config support to mysql #184

feat: add custom tls config support to mysql #184

silphid commented Jun 12, 2024

silphid left a comment

silphid Jun 12, 2024

silphid Jun 12, 2024

silphid Jun 12, 2024

chlunde Jun 20, 2024

silphid Jun 12, 2024

silphid Jun 12, 2024

silphid Jun 12, 2024

silphid Jun 12, 2024

silphid Jun 12, 2024

silphid commented Jun 19, 2024

chlunde Jun 20, 2024

silphid Jun 25, 2024

chlunde Jun 20, 2024

silphid Jun 25, 2024

chlunde Jun 20, 2024

silphid Jun 25, 2024 •

edited

Loading

chlunde commented Jun 20, 2024

Duologic commented Jun 24, 2024

silphid commented Jun 25, 2024 •

edited

Loading

silphid commented Jun 25, 2024

silphid commented Jun 28, 2024

chlunde left a comment

silphid commented Jul 9, 2024

silphid commented Jul 29, 2024

silphid commented Aug 28, 2024

		"${UP}" alpha xpkg xp-extract --from-xpkg "${OUTPUT_DIR}"/xpkg/linux_"${SAFEHOSTARCH}"/"${PACKAGE_NAME}"-"${VERSION}".xpkg -o "${CACHE_PATH}/${PACKAGE_NAME}.gz"
		chmod 644 "${CACHE_PATH}/${PACKAGE_NAME}.gz"

		current=$((current + step))
		if [[ $current -ge $timeout ]]; then

feat: add custom tls config support to mysql #184

feat: add custom tls config support to mysql #184

Conversation

silphid commented Jun 12, 2024

Description of your changes

How has this code been tested

silphid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silphid commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silphid Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

chlunde commented Jun 20, 2024

Duologic commented Jun 24, 2024

silphid commented Jun 25, 2024 • edited Loading

silphid commented Jun 25, 2024

silphid commented Jun 28, 2024

chlunde left a comment

Choose a reason for hiding this comment

silphid commented Jul 9, 2024

silphid commented Jul 29, 2024

silphid commented Aug 28, 2024

silphid Jun 25, 2024 •

edited

Loading

silphid commented Jun 25, 2024 •

edited

Loading