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

Extends CloudSql with replicaConfiguration #409

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mcbenjemaa
Copy link

@mcbenjemaa mcbenjemaa commented Dec 23, 2021

Signed-off-by: Mohamed Chiheb mc.benjemaa@gmail.com

Description of your changes

In order to be able to provision CloudSql failOver replicas, provider GCP CloudSql instance must include replicaConfiguration

In this PR:

  • I've added support for replicaConfiguration

  • Added support for replicaConfiguration.mysqlReplicaConfiguration with ability to hide sensitive data (Explained below)

Failover replica

To create a failover replica you must have a master replica either created manually or provisioned using crossplane.

Specify those values:

  forProvider:
    masterInstanceName: <projectId>:<masterIntanceName>
    replicaConfiguration:
      failoverTarget: true

If you, provisioned the master instance using crossplane, you need to update the master:

  forProvider:
     failoverReplica:
        name: "<replicaName>"

Sensitive Data

I have the feeling that the fields under replicaConfiguration.mysqlReplicaConfiguration are sensitive:

Thus, I made this concept of fetching those fields from a provided secret:

example:

  forProvider:
    replicaConfiguration:
      failoverTarget: false
      mysqlReplicaConfiguration:
        secretRef:         # secretRef that contain data
          name: cloudsql-replica-creds
          namespace: default
        usernameKey: "username"  # a reference key from the specified secret above
        passwordKey: "password"    # a reference key from the specified secret above

what do you think about this config?

WIP

Still WIP

TBD

Fixes # ?

related #388

I have:

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

How has this code been tested

Tested on a GCP account.

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

// SecretRef: Kubernetes Secret containing all credentials for MySqlReplicaConfiguration
// +optional
SecretRef *xpv1.SecretReference `json:"secretRef,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of putting all configurations in a secret and receiving which parameter represented by which key, I would suggest to have separate fields for sensitive ones. For example:

PasswordSecretRef *v1.SecretKeySelector `json:"passwordSecretRef,omitempty" tf:"-"`

This would still allow us to keep all information in a single secret but would be more explicit and flexible, for example, client cert key can be in secret A but ca cert can be in secret B.

And we can keep non sensitive ones, e.g. username, simply in spec. For public keys, I am a bit unsure though, since even it is not sensitive, I believe would be hard to maintain in the spec.

Copy link
Author

Choose a reason for hiding this comment

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

Definitely that's great.
But, I'm wondering about controller implementation.

This will end up checking each secretRef provided and fetch it accordingly,
thinking about, in each reconciliation loop, the controller will fetch those secrets as user provided.
Isn't a bit noisy... (for controller perspective, as this will be implemented in the Observe())

Copy link
Contributor

@turkenh turkenh Jan 24, 2022

Choose a reason for hiding this comment

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

Yeah, that's a good point.

However, controller runtime's caching would help with this and we won't (really) fetch those secrets (or that secret, if all put into 1 with different keys) in each reconcile from API Server.


// SslCipher: A list of permissible ciphers to use for SSL encryption.
// +optional
SslCipher *string `json:"sslCipher,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SslCipher *string `json:"sslCipher,omitempty"`
SSLCipher *string `json:"sslCipher,omitempty"`

// CaCertificateKey: key in the secret representing PEM representation of the trusted CA's x509
// certificate.
// +optional
CaCertificateKey *string `json:"caCertificateKey,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CaCertificateKey *string `json:"caCertificateKey,omitempty"`
CACertificateKey *string `json:"caCertificateKey,omitempty"`

// connection and is stored by MySQL in a file named **master.info** in
// the data directory.
// +optional
MysqlReplicaConfiguration *MySqlReplicaConfiguration `json:"mysqlReplicaConfiguration,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a possibility that other database types could be supported in the future? Asking because on terraform side no dedicated parameter for Mysql, rather there is only a note that replica_configuration is only valid for mysql:
https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#ca_certificate

// connection and is stored by MySQL in a file named **master.info** in
// the data directory.
// +optional
MysqlReplicaConfiguration *MySqlReplicaConfiguration `json:"mysqlReplicaConfiguration,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MysqlReplicaConfiguration *MySqlReplicaConfiguration `json:"mysqlReplicaConfiguration,omitempty"`
MySQLReplicaConfiguration *MySQLReplicaConfiguration `json:"mySQLReplicaConfiguration,omitempty"`

Comment on lines +15 to +20
mysqlReplicaConfiguration:
secretRef:
name: cloudsql-replica-creds
namespace: default
usernameKey: "username"
passwordKey: "password"
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in the previous comment, by convention, the following API is preferable since it is more explicit and flexible:

Suggested change
mysqlReplicaConfiguration:
secretRef:
name: cloudsql-replica-creds
namespace: default
usernameKey: "username"
passwordKey: "password"
mysqlReplicaConfiguration:
username: admin
passwordSecretRef:
name: cloudsql-replica-creds
namespace: default
key: password
caCertificateSecretRef:
name: cloudsql-replica-certs
namespace: default
key: ca.crt
clientCertificateSecretRef:
name: cloudsql-replica-certs
namespace: default
key: tls.crt
clientKeyCertificateSecretRef:
name: cloudsql-replica-certs
namespace: default
key: tls.key

@mcbenjemaa
Copy link
Author

sorry for the delay! I will try to adapt the changes soon.

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.

None yet

2 participants