feat: add custom tls config support to mysql#184
Conversation
silphid
left a comment
There was a problem hiding this comment.
(left a few comments in-line here to explain some of the decisions)
| "${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" |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
On macOS, the wc -l command is outputting the count with leading/trailing whitespace, which made that check wait infinitely.
There was a problem hiding this comment.
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 👍
| current=$((current + step)) | ||
| if [[ $current -ge $timeout ]]; then |
There was a problem hiding this comment.
This was also not ever timing out (only on macOS?), because doing integer operations/comparisons requires this special syntax.
| init.sql: | | ||
| CREATE USER 'test'@'%' IDENTIFIED BY '${mariadb_test_pw}' REQUIRE X509; | ||
| GRANT ALL PRIVILEGES ON *.* TO 'test'@'%' WITH GRANT OPTION; | ||
| FLUSH PRIVILEGES; |
There was a problem hiding this comment.
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.
| 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" |
There was a problem hiding this comment.
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).
| namespace: default | ||
| name: mariadb-creds | ||
| key: client-key.pem | ||
| insecureSkipVerify: true |
There was a problem hiding this comment.
insecureSkipVerify is to be used only here in e2e tests, because certs are self-signed, otherwise server would reject its own cert.
| errGetPC = "cannot get ProviderConfig" | ||
| errNoSecretRef = "ProviderConfig does not reference a credentials Secret" | ||
| errGetSecret = "cannot get credentials Secret" | ||
| errTLSConfig = "cannot load TLS config" |
There was a problem hiding this comment.
Used an error constant for consistency here, even though Crossplane contribution guidelines no longer recommend using them.
|
@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! 🙏 |
| // +kubebuilder:validation:Enum="true";skip-verify;preferred;custom | ||
| // +optional | ||
| TLS *string `json:"tls"` | ||
|
|
There was a problem hiding this comment.
you may consider enforcing this
https://kubernetes.io/docs/reference/using-api/cel/
There was a problem hiding this comment.
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! 😅
| return err | ||
| } | ||
|
|
||
| return mysql.RegisterTLSConfig("custom", &tls.Config{ |
There was a problem hiding this comment.
Looks like "custom" is a global key here. I think this must have a unique name per DB if you connect to two databases?
There was a problem hiding this comment.
Good catch! 👍 I have now fixed it by making the key suffixed with provider config name, in order to support multiple configs.
| return nil, errors.Wrap(err, errGetSecret) | ||
| } | ||
|
|
||
| if err := tls.LoadConfig(ctx, c.kube, pc.Spec.TLS, pc.Spec.TLSConfig); err != nil { |
There was a problem hiding this comment.
Question: can and should this be called in newDB instead? It's something you always have to call before newDB? 🤔
There was a problem hiding this comment.
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.
|
@silphid not sure if we should duplicate the full integration test, because there's also talk about adding PG integration tests. Two ideas:
@Duologic @Bastichou what do you think? |
b497c14 to
bdd825a
Compare
5ec439d to
4d12693
Compare
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 <silphid@users.noreply.github.com>
Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com>
Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com>
4d12693 to
6d9c446
Compare
|
@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
left a comment
There was a problem hiding this comment.
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.
4886268 to
6d9c446
Compare
* Refresh build system - update to latest Go, Up, crossplane-runtime etc (crossplane-contrib#182) * go get github.com/crossplane/crossplane-tools@master && go mod tidy && make generate Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Add latest crossplane-runtime (including managementPolicies) go get github.com/crossplane/crossplane-runtime@latest && go mod tidy && make generate Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Update controller-runtime API after breaking changes upstream Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Bump build/ and up version to be compatible with newer docker Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Avoid deprecated k8s.io/utils/pointer Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Bump Go to 1.21, golangci-linter to 1.54 (match build/) Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Integration test: Set registry to xpkg.upbound.io/ Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Integration test: OS for Kind is always Linux Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> --------- Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * PostgreSQL CREATE SCHEMA support (crossplane-contrib#181) * Schema controller for PostgreSQL This adds support for CREATE SCHEMA (not any DDL like Schema hero). This is needed to run third party solutions that require the schema to exist, like grafana operator and temporal operator. Co-authored-by: Lars Haugan <456305+larhauga@users.noreply.github.com> Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Generate files for PostgreSQL Schema Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Expand PostgreSQL config example to include secret and ssl mode, docker info Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Avoid new(string) and use nicer ptr.To values in test Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> --------- Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> Co-authored-by: Lars Haugan <456305+larhauga@users.noreply.github.com> * chore: add provider install documentation (crossplane-contrib#174) Signed-off-by: Bastien CERIANI <bastien.ceriani@gmail.com> * Give MySQL sql_log_bin at DSN level to take parameter into account (crossplane-contrib#171) Signed-off-by: Florian Gaillot <fgaillot@qosenergy.com> * change the host with endpoint to make it worked Signed-off-by: Batuhan Apaydin <batuhan.apaydin@chainguard.dev> * ci: fix upload-artifact ref * allow to define login db (crossplane-contrib#192) Signed-off-by: bakito <github@bakito.ch> * add support for mssql schema in grants (crossplane-contrib#193) Signed-off-by: bakito <github@bakito.ch> * optional binlog parameters in mysql connection string to allow the use of DB default setting (crossplane-contrib#201) Signed-off-by: Marius Leahu <Marius.Leahu@swisscom.com> * fix: remove unneeded flush privileges call for mysql (crossplane-contrib#202) Signed-off-by: Marius Ziemke <marius@ziemke.net> Co-authored-by: Marius Ziemke <marius@ziemke.net> * feat: add custom tls config support to mysql (crossplane-contrib#184) * feat: add custom tls config support Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> * use provider config name to make tls name unique Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> * refactor integration tests into modular functions Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> --------- Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> Co-authored-by: Mathieu Frenette <silphid@users.noreply.github.com> * refactor(mysql): consistent username/host parameters (crossplane-contrib#205) Signed-off-by: Duologic <jeroen@simplistic.be> * chore: update build/, Makefile and integration test script (crossplane-contrib#204) Signed-off-by: Duologic <jeroen@simplistic.be> * feat(postgresql): add revoke public logic to grant and schema resources (crossplane-contrib#207) - added a field called revokePublicOnDb in grant resource; if true, it will revoke from public access to DB; This usually a DBA requirement. - added a field called revokePublicOnSchema in schema resource; if true, it will revoke from public access to a schema; This usually a DBA requirement. Signed-off-by: oliver.zokra <oliver.zokra@kyriba.com> * test(postgresql): implement postgresdb integration tests (crossplane-contrib#208) Signed-off-by: oliver.zokra <oliver.zokra@kyriba.com> * update helm repo before intalling mariadb (crossplane-contrib#209) Signed-off-by: oliver.zokra <oliver.zokra@kyriba.com> * chore: add chlunde to maintainers list (crossplane-contrib#225) Signed-off-by: Duologic <jeroen@simplistic.be> * bump github actions upload-action to latest release due to deprecation Signed-off-by: Lars Haugan <lars.haugan@sparebank1.no> * chore: bump golangci version (crossplane-contrib#216) * chore: bump golangci Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: bump golangci on github actions workflow Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: bump golangci up to v2, bump up all action dependencies Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: even more actions pinned Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> --------- Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: Update Go to 1.23.9 (crossplane-contrib#230) Go 1.23.9 is a security update, and we also need to update Go to get dependabot security updates out (dependabot does not run 'go mod tidy'). Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * Bump golang.org/x/crypto from 0.21.0 to 0.35.0 (crossplane-contrib#229) Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.21.0 to 0.35.0. - [Commits](golang/crypto@v0.21.0...v0.35.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.35.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/net from 0.25.0 to 0.38.0 (crossplane-contrib#231) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.25.0 to 0.38.0. - [Commits](golang/net@v0.25.0...v0.38.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-version: 0.38.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump google.golang.org/protobuf from 1.31.0 to 1.33.0 (crossplane-contrib#232) Bumps google.golang.org/protobuf from 1.31.0 to 1.33.0. --- updated-dependencies: - dependency-name: google.golang.org/protobuf dependency-version: 1.33.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix: enable management policies (crossplane-contrib#215) * fix: enable management policies Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * fix: linter Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: offload lint changes to crossplane-contrib#216 Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: revert changes as per peer review Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> --------- Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * Move negz to emeritus status (crossplane-contrib#226) As requested on slack Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * test: add coverage of managementPolicies to e2e (crossplane-contrib#233) Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> * chore: remove duplicate entry of golangci version (crossplane-contrib#236) * chore: remove duplicate entry of golangci version Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: bump up versions for crossplane 1.20 Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * chore: restore changes Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> --------- Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> * Refactor * Revert to previous usertype contants --------- Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no> Signed-off-by: Bastien CERIANI <bastien.ceriani@gmail.com> Signed-off-by: Florian Gaillot <fgaillot@qosenergy.com> Signed-off-by: Batuhan Apaydin <batuhan.apaydin@chainguard.dev> Signed-off-by: bakito <github@bakito.ch> Signed-off-by: Marius Leahu <Marius.Leahu@swisscom.com> Signed-off-by: Marius Ziemke <marius@ziemke.net> Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> Signed-off-by: Duologic <jeroen@simplistic.be> Signed-off-by: oliver.zokra <oliver.zokra@kyriba.com> Signed-off-by: Lars Haugan <lars.haugan@sparebank1.no> Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Carl Henrik Lunde <chlunde@ifi.uio.no> Co-authored-by: Lars Haugan <456305+larhauga@users.noreply.github.com> Co-authored-by: Bastien Cer <bastien.ceriani@gmail.com> Co-authored-by: fgaillot-qosenergy <67331418+fgaillot-qosenergy@users.noreply.github.com> Co-authored-by: Batuhan Apaydin <batuhan.apaydin@chainguard.dev> Co-authored-by: Hasan Turken <turkenh@gmail.com> Co-authored-by: Duologic <jeroen@simplistic.be> Co-authored-by: Marc Brugger <github@bakito.ch> Co-authored-by: Marius Leahu <11443917+mleahu@users.noreply.github.com> Co-authored-by: Marius Ziemke <marius@ziemke.net> Co-authored-by: Mathieu Frenette <1917993+silphid@users.noreply.github.com> Co-authored-by: Mathieu Frenette <silphid@users.noreply.github.com> Co-authored-by: olikyr <oliver.zokra@kyriba.com> Co-authored-by: Lars Haugan <lars.haugan@sparebank1.no> Co-authored-by: J. Fernández <7312236+fernandezcuesta@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat: add custom tls config support Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> * use provider config name to make tls name unique Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> * refactor integration tests into modular functions Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> --------- Signed-off-by: Mathieu Frenette <silphid@users.noreply.github.com> Co-authored-by: Mathieu Frenette <silphid@users.noreply.github.com> Signed-off-by: Joaquín Fernández Campo <xocasdashdash@users.noreply.github.com>
Description of your changes
This PR adds support for custom TLS configuration to mysql implementation. In provider config file, if
tlsis set tocustom, it reads custom TLS configuration fromtlsConfigproperty, reading CA cert and client key/pair from K8s secret(s), and registering that config in mysql driver under thecustomkey.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
tlsConfigproperty is not a map, but rather a single config entry.I have:
make reviewableto 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-integrationruns 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.