Add SSL configuration for RGW#355
Conversation
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
UtkarshBhatthere
left a comment
There was a problem hiding this comment.
one nit, overall LGTM
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
sabaini
left a comment
There was a problem hiding this comment.
Hey @marceloneppel thanks for this welcome contribution!
I'm wondering about two things regarding parametrization.
One is relatively minor thing, aiui with SSL key material present we'd configure both http/https. Ideally we would have a way to turn off non-SSL service if we configure SSL key material -- maybe the logic could be to configure https if SSL key material is provided, and http if there's no key material. And only configure both if both ports are explicitly provided.
The other is around that key material. In your PR the user has to provide file paths to the key material. However due to snap confinement there's a limited number of places the services can actually read data from, and users would see failing services if the ssl files are not in a suitable place.
I'd suggest to check in the CLI part of the code that the file is readable for the snap so users get an early warning, and document the constraints around this.
Alternatively, the code could be changed so that the key material itself is used as a parameter (instead of the file names).
It would also be great to have functional tests for this feature.
Thanks again!
Hi, @sabaini! Thanks for the feedback. I'm going to work on those updates. |
…est-1 Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…est-1 Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Hi @sabaini! I fixed the issues I was facing and updated this PR based on your comments. The code is now turning off the HTTP service if no HTTP port is specified and the SSL material is specified. It's possible to turn on both HTTP and HTTPS services if both the HTTP port and the SSL material are specified. Also, to avoid file permission issues, the new parameters were changed from file paths to the SSL material: sudo microceph enable rgw --ssl-certificate="$(sudo base64 -w0 ~/server.crt)" --ssl-private-key="$(sudo base64 -w0 ~/server.key)"What's missing are the functional tests. I'll add them next week. |
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
sabaini
left a comment
There was a problem hiding this comment.
Thank you @marceloneppel, very nice!
Some commentary / Q inline
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
|
@UtkarshBhatthere think you had requested changes too -- please re-review? |
UtkarshBhatthere
left a comment
There was a problem hiding this comment.
LGTM, 👍🏼 for good testing coverage.
|
@marceloneppel as a programmer how was your experience extending the RGW service capabilities in MC ? Asking specifically about using the service placement interface. Was is easy to understand ? |
Hi, @UtkarshBhatthere! It was easy to understand, specifically the placement interface. The part where I struggled a little bit was related to the RGW configuration, not MC. |
|
Great :D |
Hi @marceloneppel I spent the last two days frustrated trying to find how to add SSL to the rados gateway until I found your PR! Awesome work! A thing I am still confused about, is since you switched from filepath to base64 encoded cert and key, when I update the keys how is the reloading intended to work? I normally set up LetsEncrypt to automatically renew certificates, and would then expect to restart the gateway service with: So for now I would have to disable the rgw and re-enable it again with the updated certs in a deploy-hook on certbot, but that doesn't feel very clean. I would love a way to simply swap out certs for LetsEncrypt compatibility, or if you have ideas on how this can be achieved without disabling the rgw that would be appreciated. |
|
@maximsachs thanks for sharing your feedback on the user experience. I really appreciate it. May I request you to create a GitHub issue that expresses your expected UX ? This helps us keep track of the the task. |
This regression was introduced in canonical#355. Closes: canonical#483
This regression was introduced in canonical#355. Closes: canonical#483
This regression was introduced in canonical#355. Closes: canonical#483 Signed-off-by: Neha Oudin <neha@oudin.red>
This regression was introduced in canonical#355. This change was tested with an update of the current tests. Closes: canonical#483 Signed-off-by: Neha Oudin <neha@oudin.red>
This regression was introduced in canonical#355. This change was tested with an update of the current unit tests. Closes: canonical#483 Signed-off-by: Neha Oudin <neha@oudin.red>
This regression was introduced in canonical#355. This change was tested with an update of the current unit tests. Closes: canonical#483 Signed-off-by: Neha Oudin <neha@oudin.red>
This regression was introduced in canonical#355. This change was tested with an update of the current unit tests. Closes: canonical#483 Signed-off-by: Neha Oudin <neha.oudin@canonical.com>
This regression was introduced in canonical#355. This change was tested with an update of the current unit tests. Closes: canonical#483 Signed-off-by: Neha Oudin <neha.oudin@canonical.com>
# Description In #355 a regression was added where the `--port` rgw option is not respected. This PR fixes this regression, updates the tests in order to ensure that it is respected in all contexts. Fixes #483 ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] CleanCode (Code refactor, test updates, does not introduce functional changes) - [ ] Documentation update (Doc only change) ## How Has This Been Tested? The unit tests were updated to reflect that change and confirm that the option is respected when it is passed. ## Contributor's Checklist Please check that you have: - [x] self-reviewed the code in this PR. - [x] added code comments, particularly in hard-to-understand areas. - [ ] updated the user documentation with corresponding changes. - [x] added tests to verify effectiveness of this change. Signed-off-by: Neha Oudin <neha.oudin@canonical.com>
Description
Some tools, like pgBackRest, can currently only interact with S3-compatible storages if they work with SSL/TLS. This PR adds the possibility of enabling RadosGW with SSL/TLS enabled.
The main idea is to use the PostgreSQL charms with MicroCeph so users can do backups through pgBackRest in bucket without a cloud service subscription.
Type of change
How Has This Been Tested?
To test, I used the following steps:
sudo openssl genrsa -out /var/snap/microceph/common/ca.key 2048 sudo openssl req -x509 -new -nodes -key /var/snap/microceph/common/ca.key -days 1024 -out /var/snap/microceph/common/ca.crt -outform PEM sudo openssl genrsa -out /var/snap/microceph/common/server.key 2048 sudo openssl req -new -key /var/snap/microceph/common/server.key -out /var/snap/microceph/common/server.csr sudo nano /var/snap/microceph/common/extfile.cnf # and put the following content: subjectAltName = DNS:localhost sudo openssl x509 -req -in /var/snap/microceph/common/server.csr -CA /var/snap/microceph/common/ca.crt -CAkey /var/snap/microceph/common/ca.key -CAcreateserial -out /var/snap/microceph/common/server.crt -days 365 -extfile /var/snap/microceph/common/extfile.cnfContributor's Checklist
Please check that you have: