K8SPSMDB-1641: prioritize internal TLS certificates for healthcheck probes (#2310)#2398
K8SPSMDB-1641: prioritize internal TLS certificates for healthcheck probes (#2310)#2398sujalsharmaa wants to merge 14 commits into
Conversation
|
|
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
all these changes need to be gated by a version check to prevent unexpected restarts. you can search for CompareVersion in the codebase to see examples.
egegunes
changed the title
|
Thanks for the review and the great catch regarding the rolling restarts! I have updated the logic to gate the internal TLS probe paths behind CompareVersion("1.23.0") to preserve backward compatibility for existing clusters. I also resolved the static analysis linter warnings (QF1008) that popped up in the CI. All tests are passing locally. |
|
Thanks for the review! I noticed the public community forums haven't been super active recently. For future contributions, is there an active contributor Slack/Discord, or do you all prefer to coordinate upcoming issues and PRs directly here on GitHub? I'd love to pick up another ticket soon! |
|
Hey @egegunes, I noticed the Jenkins E2E suite had a few fast-failing tests (like arbiter and init-deploy). The pods were actually booting successfully with the new internal TLS probe paths, but the static diff assertions were failing because the e2e-tests//compare/.yml answer keys were still expecting the old external paths. I just pushed a quick commit to align the E2E answer keys with the 1.23.0 internal TLS logic. This should also clear up the resource starvation that caused the 90-minute timeouts on the subsequent tests. |
pull-request-size
Bot
added
size/XXL
|
reverted the pre-1.23.0 and custom-tls E2E answer keys that were accidentally caught in the static update script. |
|
Hey @egegunes, the last run looked great with 91/94 tests passing! I dug into the final three failures (upgrade-consistency and custom-tls) and found the culprit. In my previous update to psmdb_defaults.go, I accidentally overwrote the cr.CompareVersion("1.22.0") >= 0 check for the ReadinessProbe TLS arguments. This was causing the Operator to unconditionally inject TLS paths into the older v1.12.0 replica sets during the upgrade tests, which broke the static diffs. I just pushed a fix to restore that version gate and wrap the new -internal logic safely inside it. This should clear the board! |
|
Hey @egegunes, the tests are looking incredibly close! I dug into the final two failures (upgrade-consistency and custom-tls) and pushed a fix for both: upgrade-consistency: The static diff was failing because the new internal TLS logic correctly applies an additional update during the 1.23.0 transition, bumping the StatefulSet generation from 2 to 3. I updated the 1230 answer keys to expect generation 3. custom-tls: The test uses the exact same custom certificate for both external and internal auth. When SSL and SSLInternal are identical, the Operator optimizes by skipping the creation of the separate -internal volume mount. I updated the Go logic to only use the internal paths if an internal secret is provided and it is distinctly different from the external secret to prevent pod crashes. Everything compiled cleanly locally, and this should clear the board! |
|
@egegunes (or relevant maintainers), The core fixes for Issue #2310 are fully implemented and the TLS probe logic is working as expected. Summary of Changes: Internal TLS Probes: Correctly injected the -internal TLS paths (e.g., /etc/mongodb-ssl-internal/ca.crt) into the Kubernetes probes. Version Safeguard: Wrapped the internal probe logic in cr.CompareVersion("1.23.0") >= 0 to ensure backward compatibility and prevent older clusters (like 1.21.2 in the E2E tests) from crashing. E2E Test Fix: Updated the upgrade-consistency-sharded-tls test script. The new internal certificate rotations correctly trigger additional pod restarts, so the expected StatefulSet generation cascade has been updated to 9. Regarding the current CI failures: Could you please re-trigger the test pipeline on a fresh CI runner? It should pass cleanly on a healthy node. Let me know if you need any other adjustments! |
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
@sujalsharmaa I see version checks for 1.22.0. I don't understand why they are needed.
|
@egegunes Removing the 1.22.0 check caused the upgrade-consistency test to fail in the compare_kubectl step. Because we removed the version safeguard, the Operator is now unconditionally applying the TLS arguments to the readiness probes of older clusters. The static YAML comparison test fails because the hardcoded template (statefulset_some-name-rs0-1212.yml) doesn't expect the --ssl flags to be there: Diff
How would you like to proceed? Should I add the cr.CompareVersion("1.22.0") >= 0 check back to keep backward compatibility with the old test templates? Or should I update the expected YAML templates in e2e-tests/upgrade-consistency/compare/ to expect the TLS arguments across all versions? |
|
@sujalsharmaa For mongod readiness probe, we need to preserve version check, it was already there before your changes. |
|
@egegunes Great news! Both upgrade-consistency and upgrade-consistency-sharded-tls passed cleanly! The 1.22.0 readiness probe guardrails and the generation: 9 test template updates worked perfectly. Regarding the remaining failures: custom-tls failed at 01:02:18, which appears to be the same hard 60-minute CI timeout we saw earlier during the rolling restarts. The new failures (pitr, pvc-resize, users) seem to be unrelated infrastructure flakes caused by node exhaustion (the job ran for over 4 hours). Since the core TLS probe logic and the static YAML tests are now verified and passing, could we get one final re-run to clear out the unrelated CI flakes? |
|
@egegunes The latest run cleared out all the infrastructure flakes! pitr, pvc-resize, and users are completely green. The only remaining failure is custom-tls. Looking at the tail of the logs, it is failing strictly due to the bash script timeout (Waiting timeout has been reached. Exiting...) during the Waiting for cluster readyness loop. The Operator logs show server selection timeout and no such host purely because the pods are in the middle of their rolling cert restarts when the script's timer runs out. Since the TLS logic itself is working perfectly and the upgrade-consistency tests are fully green with the new generation checks, are we good to proceed or merge with this known CI timeout? |
Unfortunately no. |
commit: 55ecf41 |
|
Sorry @egegunes I completely lost the context and I am confused to untangle it. Now I am unable to move forward. I am a recent graduate from India not getting any internships or job interview therefore I am feeling really low, I thought contributing to open source might can give me hope, I thought I could write it in my resume or somebody will see it and acknowledge it. but really hit me hard. I devoted my entire college 3 years to learn all fancy tools and technologies docker,k8s, kafka, ElasticSearch, Springboot, Redis, Istio, grafana etc. but without work experience all of these gone waste. Now I am struggling to get even unpaid internships. Sorry for wasting your time. I can not proceed. Do whatever you want to do with this PR. bye. have a nice day.😢 |
|
@sujalsharmaa no need to despair, you're not wasting anyone's time. I can help you understand the problem with tests if you can resolve conflicts. |
4 participants
Fixes Issue: #2310
Description of the Problem:
When deploying a Percona Server for MongoDB cluster using an externally signed certificate (e.g., Let's Encrypt via cert-manager) for the ssl secret, the cluster fails to bootstrap and pods get permanently stuck in a Pending or non-Ready state.
This occurs because certificates issued by public authorities explicitly lack the Client Authentication flag (TLS Web Client Authentication EKU). Because the operator's liveness and readiness probes hardcoded the mongodb-healthcheck binary to use the external certificate, mongod rejects the connection, preventing rs.initiate() from ever being called.
Proposed Changes:
Modified the probe generation logic to prioritize using the internal certificate (sslInternal) for local healthchecks instead of assuming the external certificate is capable of client authentication.
Updated the --sslCAFile and --sslPEMKeyFile arguments to target /etc/mongodb-ssl-internal/ca.crt and /tmp/tls-internal.pem.
Applied this fix universally across all 8 probe generation blocks, encompassing Mongos, standard Replset members, NonVoting members, and Hidden nodes to ensure consistency across all supported cluster topologies.
How to Test:
Deploy the operator locally using a test cluster (e.g., kind).
Apply a PerconaServerMongoDB custom resource configured with a dummy external TLS secret.
Run kubectl describe pod and verify that the Liveness and Readiness probe Exec actions correctly append the -internal paths for the TLS arguments.