Skip to content

Conversation

@marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Sep 12, 2022

Issue

  • Jira issue: DPE-512
  • Both Patroni REST API (that is used by this charm for some operations, and also by Patroni itself) and the patronictl command line tool (tool used by Patroni for some operations; it uses Patroni REST API under the hood) need to have the communication to and from them being encrypted.

Solution

  • Enable TLS on both Patroni REST API and patronictl command line tool when TLS is enabled on PostgreSQL (what happens after a relations is established with the TLS Certificates Operator).

Context

  • This PR consists on REPLICATING what was already reviewed on TLS on Patroni REST API postgresql-k8s-operator#38 to the PostgreSQL VM charm.
  • When TLS is enabled, the non encrypted endpoint is not available anymore; the opposite happens when TLS is disabled: the encrypted endpoint is not available anymore. So, with the current implementation, even withouth an integration test, the correct implementation of TLS on the API is validated (because the API is used in several events of the charm and any wrong call would make the charm not deploy correctly).
  • Summary of the main changes (you can FOCUS your review on them):
    • tests/integration/helpers.py and tests/integration/test_tls.py: new checks to ensure the API is accepting TLS connections and differently from the k8s charm now we use the TLS CA to verify the server certificate.
  • There is a small difference in the way TLS enabled/disabled is passed to the Patrons object compared to the k8s charm (here peer relation data is used) due to some differences between both charms. This can be updated to be more similar in the future.

Testing

  • The integration test for TLS was updated to check that Patroni REST API has TLS enabled.

Release Notes

  • Add TLS encryption to Patroni REST API and patronictl command line tool.

@marceloneppel marceloneppel changed the base branch from main to tls-integration-test September 13, 2022 02:03
@codecov-commenter
Copy link

Codecov Report

Merging #33 (bdca5d4) into tls-integration-test (abc324f) will increase coverage by 0.04%.
The diff coverage is 35.71%.

@@                   Coverage Diff                    @@
##           tls-integration-test      #33      +/-   ##
========================================================
+ Coverage                 60.62%   60.67%   +0.04%     
========================================================
  Files                         6        6              
  Lines                       795      801       +6     
  Branches                    123      123              
========================================================
+ Hits                        482      486       +4     
- Misses                      285      287       +2     
  Partials                     28       28              
Impacted Files Coverage Δ
src/charm.py 53.63% <0.00%> (-0.13%) ⬇️
src/cluster.py 56.42% <38.46%> (+0.87%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Base automatically changed from tls-integration-test to main September 20, 2022 15:52
Whether TLS is enabled/disabled on Patroni REST API.
"""
unit_address = get_unit_address(ops_test, unit_name)
tls_ca = await get_tls_ca(ops_test, unit_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you return False here if not tls_ca?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll add it.

Copy link
Member Author

@marceloneppel marceloneppel Sep 21, 2022

Choose a reason for hiding this comment

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

Added on 334a50d.

Copy link
Contributor

@MiaAltieri MiaAltieri left a comment

Choose a reason for hiding this comment

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

Looks awesome

planned_units: number of units planned for the cluster
superuser_password: password for the operator user
replication_password: password for the user used in the replication
tls_enabled: whether TLS is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

nit add verify

Copy link
Member Author

Choose a reason for hiding this comment

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

This specific parameter is related to whether TLS is already enabled or not, different from the tests, where we have parameters related to verify whether TLS is enabled.


# The CA bundle file is used to validate the server certificate when
# TLS is enabled, otherwise True is set because it's the default value
# for the verify parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

excellent commenting

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Mia!

@marceloneppel marceloneppel merged commit 5812de2 into main Sep 21, 2022
@marceloneppel marceloneppel deleted the tls-patroni-rest-api branch October 6, 2022 20:21
BON4 pushed a commit to BON4/postgresql-operator that referenced this pull request Apr 23, 2024
* Add workaround for Patroni REST API TLS

* Improve TLS status retrieval

* Readd checks

* Add additional check
github-actions bot added a commit to canonical/test-runners-2-github-x64-postgresql-operator that referenced this pull request May 20, 2024
github-actions bot added a commit to canonical/test-runners-2-is-x64-postgresql-operator that referenced this pull request May 20, 2024
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.

4 participants