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

Helper function for TLS support in database access #357

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

dciabrin
Copy link
Contributor

@dciabrin dciabrin commented Oct 5, 2023

New function to generate the mysql flags read by oslo.db to connect to a mysql database via TLS.
This commit also returns a fqdn for the database hostname, as it is required to validate the mysql database's certificate.

@dciabrin dciabrin requested review from Deydra71 and stuggi October 5, 2023 08:54
"ssl-key=/etc/pki/tls/private/tls.key")
}
// Client uses a CA certificate that will get merged
// into the pod's CA bundle by the pod's init container
Copy link
Contributor

Choose a reason for hiding this comment

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

not by the init container, but via kolla_start openstack-k8s-operators/tcib#82

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's copied during kolla_start, but the idea was that this file was merged with 'update-ca-trust' during the init container.

https://github.com/openstack-k8s-operators/keystone-operator/blob/1e32653ddc594eda8428fc0335608b7befbb1a14/templates/keystoneapi/bin/init.sh#L44

Copy link
Contributor

Choose a reason for hiding this comment

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

but we don't have to do this in the init container as you can see in the tcib change. the deployment get the cacert mounted and kolla does it already.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the plan is to get rid of any init container where possible. for client cert we then should mount the cert and use the kolla config to copy it to the right place. the CA update is handled when we get new images from the above PR. if you need test images, I have built some at quay.io/mschuppe/...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I didn't know that, thanks

// a common directory for all services
if t.Service.SecretName != "" {
conn = append(conn,
"ssl-cert=/etc/pki/tls/certs/tls.crt",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this for when client certs are used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, it might be useful in the future if we enable client cert-verification in mariadb.
The idea is that this name is generic, and the real certificate for the service is copied by kolla_start into this generic location.
https://github.com/openstack-k8s-operators/keystone-operator/blob/1e32653ddc594eda8428fc0335608b7befbb1a14/templates/keystoneapi/config/keystone-api-config.json#L35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might want to use a similar copy mechanism in kolla_start, and agree on a cert name to use here, rather than updating all service's kolla config to copy them manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we could do that and have pre-defined names for client/server cert

New function to generate the mysql flags read by oslo.db to
connect to a mysql database via TLS.
Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

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

/lgtm

@stuggi stuggi merged commit 7fd4e4c into openstack-k8s-operators:main Oct 11, 2023
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.

2 participants