chore: fixing mariadb template when pop to pod tls is enabled #960
Conversation
Signed-off-by: Nelesh Singla <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds a PodToPodTLS conditional that toggles TLS-related settings: enables Changes
Sequence Diagram(s)sequenceDiagram
participant Kube as Kubelet
participant Pod as MariaDB Pod
participant Container as MariaDB Container
participant MySQL as MySQL Server (inside container)
participant Secrets as Secret/ConfigMap
Kube->>Pod: Start pod (with PodToPodTLS=true)
Pod->>Secrets: Mount TLS secret & configMap
Pod->>Container: Start container
Kube->>Container: Run readinessProbe (TLS path)
Container->>MySQL: mysql client connects via TLS using mounted certs
MySQL-->>Container: SELECT 1 result
Container-->>Kube: Probe succeeds/fails
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
A new image has been built to help with testing out this PR: To use this image run the following: cd $(mktemp -d)
git clone [email protected]:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/960/head
git checkout -b pullrequest ba061bd76b0519a519ce5fdf41c3c44250629137
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-960"More instructions here on how to deploy and test a Data Science Pipelines Application. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/internal/mariadb/default/tls-config.yaml.tmpl (1)
14-16: Logic is correct; consider trimming template whitespace for cleaner output.The conditional correctly enforces
require_secure_transport = onwhen pod-to-pod TLS is enabled. However, the current template will render extra blank lines (with trailing whitespace) in the generated config regardless of the condition value.Using the
-modifier in Go templates trims adjacent whitespace for cleaner output:✨ Suggested improvement for cleaner rendered output
ssl_key = /.mariadb/certs/tls.key - {{ if .PodToPodTLS }} +{{- if .PodToPodTLS }} require_secure_transport = on - {{ end }} +{{- end }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/internal/mariadb/default/tls-config.yaml.tmpl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: functest
- GitHub Check: dspo-byoargo-tests
- GitHub Check: dspo-tests
- GitHub Check: precommit
- GitHub Check: unittest
Signed-off-by: Nelesh Singla <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @config/internal/mariadb/default/deployment.yaml.tmpl:
- Around line 41-50: The livenessProbe currently uses an unconditional tcpSocket
(port 3306) which will fail when .PodToPodTLS is true and MariaDB enforces
require_secure_transport; modify the livenessProbe block in deployment.yaml.tmpl
to mirror the readinessProbe conditional: wrap the probe in an {{ if
.PodToPodTLS }} / {{ else }} / {{ end }} and when PodToPodTLS is true use an
exec probe running the same MYSQL_PWD=$MYSQL_PASSWORD mysql -u $MYSQL_USER -D
$MYSQL_DATABASE -e 'SELECT 1' (omitting -h to use the Unix socket), otherwise
keep the existing tcpSocket: port: 3306 behavior so unencrypted TCP is only used
when TLS is disabled.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/internal/mariadb/default/deployment.yaml.tmpl
🔇 Additional comments (2)
config/internal/mariadb/default/deployment.yaml.tmpl (2)
98-104: LGTM!The TLS volume mounts are correctly structured. Using
subPathfor the config file mount is appropriate to avoid overwriting the existing/etc/my.cnf.d/directory contents.
109-121: LGTM! Volume definitions correctly reference the TLS secret and configMap with appropriate naming conventions. The MariaDB TLS secret is automatically provisioned by OpenShift via the Service annotation, and the configMap is created by the corresponding template. Server-side TLS only requires the certificate and key pair.
| {{ if .PodToPodTLS }} | ||
| - >- | ||
| MYSQL_PWD=$MYSQL_PASSWORD mysql -u $MYSQL_USER -D | ||
| $MYSQL_DATABASE -e 'SELECT 1' | ||
| {{ else }} | ||
| - >- | ||
| MYSQL_PWD=$MYSQL_PASSWORD mysql -h 127.0.0.1 -u $MYSQL_USER -D | ||
| $MYSQL_DATABASE -e 'SELECT 1' | ||
| {{ end }} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the specific lines
if [ -f "config/internal/mariadb/default/deployment.yaml.tmpl" ]; then
echo "=== File exists. Showing lines 35-55 for context ==="
sed -n '35,55p' config/internal/mariadb/default/deployment.yaml.tmpl | cat -n
else
echo "File not found at config/internal/mariadb/default/deployment.yaml.tmpl"
# Try to find it
find . -name "*deployment.yaml.tmpl" -type f 2>/dev/null | head -20
fiRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 966
🏁 Script executed:
# Search for other readiness probe configurations in MariaDB templates
echo "=== Searching for readinessProbe in MariaDB files ==="
fd -e yaml.tmpl -e yaml . config/internal/mariadb 2>/dev/null | while read f; do
if grep -l "readinessProbe" "$f" 2>/dev/null; then
echo "Found in: $f"
grep -n -A 10 "readinessProbe" "$f"
fi
doneRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 633
🏁 Script executed:
# Search for require_secure_transport to understand TLS enforcement
echo "=== Searching for require_secure_transport ==="
rg -n 'require_secure_transport' --type yaml --type tmplRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 182
🏁 Script executed:
# Check the volumeMounts and volumes sections mentioned in the comment (lines 98-104 and 109-121)
echo "=== Checking volumeMounts and volumes in deployment.yaml.tmpl ==="
if [ -f "config/internal/mariadb/default/deployment.yaml.tmpl" ]; then
echo "=== volumeMounts section (lines 95-110) ==="
sed -n '95,110p' config/internal/mariadb/default/deployment.yaml.tmpl | cat -n
echo ""
echo "=== volumes section (lines 105-125) ==="
sed -n '105,125p' config/internal/mariadb/default/deployment.yaml.tmpl | cat -n
fiRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 1586
🏁 Script executed:
# Look for TLS configuration files
echo "=== Searching for TLS config files ==="
fd 'tls-config\|tls.conf\|my.cnf' config/internal/mariadb 2>/dev/null | head -20Repository: opendatahub-io/data-science-pipelines-operator
Length of output: 128
🏁 Script executed:
# Check if there are MySQL/MariaDB config templates with require_secure_transport
echo "=== Searching for MariaDB config templates ==="
fd -e tmpl -e conf -e cnf . config/internal/mariadb 2>/dev/nullRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 505
🏁 Script executed:
# Examine the TLS configuration template
echo "=== Content of tls-config.yaml.tmpl ==="
cat config/internal/mariadb/default/tls-config.yaml.tmplRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 522
🏁 Script executed:
# Search for any mention of require_secure_transport or ssl-related configs
echo "=== Searching for TLS/SSL enforcement in all files ==="
rg -n 'require_secure_transport|ssl|tls' config/internal/mariadb --type yaml -iRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 144
🏁 Script executed:
# Check if there are any init containers or other setup scripts that configure require_secure_transport
echo "=== Checking for init containers or setup scripts ==="
sed -n '1,200p' config/internal/mariadb/default/deployment.yaml.tmpl | grep -A 20 -B 5 'initContainer\|mysql\|mariadb'Repository: opendatahub-io/data-science-pipelines-operator
Length of output: 3096
🏁 Script executed:
# Look for any references to socket paths or connection methods in the entire deployment template
echo "=== Checking full readiness and liveliness probe sections ==="
grep -n -B 5 -A 15 'readinessProbe\|livenessProbe' config/internal/mariadb/default/deployment.yaml.tmplRepository: opendatahub-io/data-science-pipelines-operator
Length of output: 1634
TLS-enabled liveness probe will fail with require_secure_transport = on.
The readiness probe correctly uses a Unix socket connection (by omitting -h 127.0.0.1) to bypass require_secure_transport enforcement. However, the livenessProbe at line 56-63 is unconditionally TCP-based (tcpSocket: port: 3306) and does not have a TLS-enabled variant. When PodToPodTLS=true, the MariaDB server enforces require_secure_transport = on, which will reject the unencrypted TCP liveness probe and cause the probe to fail indefinitely.
The liveness probe must be made conditional (similar to the readiness probe) to use either a Unix socket connection or TLS-enabled TCP when PodToPodTLS=true.
🤖 Prompt for AI Agents
In @config/internal/mariadb/default/deployment.yaml.tmpl around lines 41 - 50,
The livenessProbe currently uses an unconditional tcpSocket (port 3306) which
will fail when .PodToPodTLS is true and MariaDB enforces
require_secure_transport; modify the livenessProbe block in deployment.yaml.tmpl
to mirror the readinessProbe conditional: wrap the probe in an {{ if
.PodToPodTLS }} / {{ else }} / {{ end }} and when PodToPodTLS is true use an
exec probe running the same MYSQL_PWD=$MYSQL_PASSWORD mysql -u $MYSQL_USER -D
$MYSQL_DATABASE -e 'SELECT 1' (omitting -h to use the Unix socket), otherwise
keep the existing tcpSocket: port: 3306 behavior so unencrypted TCP is only used
when TLS is disabled.
|
Change to PR detected. A new PR build was completed. |
2 participants
The issue resolved by this Pull Request:
Resolves #<issue_number>
Description of your changes:
Testing instructions
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.