PROJQUAY-11215: feat: add TLS support for operator-managed PostgreSQL - #1277
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-component PostgreSQL TLS override support to the Quay operator: defines API types and CRD validation, implements secret validation and context wiring during reconciliation, generates self-signed ECDSA certificates, integrates TLS material into Kustomize inflation and managed secrets, injects TLS volumes/mounts/init-containers via middleware, and validates behavior across four comprehensive e2e test suites (basic, lifecycle, mixed, secretref, service-CA). ChangesPostgreSQL TLS Override
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
2bbfdb8 to
57dd677
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/middleware/middleware.go (1)
502-508: 💤 Low valueConsider simplifying the shell escaping for maintainability.
The nested quote escaping pattern
'\"'\"'in the printf command is difficult to reason about and maintain. While e2e tests confirm this works correctly, consider refactoring to use a heredoc or simpler escaping pattern.Alternative approach
initContainer := corev1.Container{ Name: "postgres-tls-init", Image: dep.Spec.Template.Spec.Containers[0].Image, Command: []string{ - "sh", "-c", - "if [ -f /var/lib/pgsql/data/userdata/postgresql.conf ]; then " + - "grep -q '^ssl = on' /var/lib/pgsql/data/userdata/postgresql.conf || " + - "printf '\\nssl = on\\nssl_cert_file = '\"'\"'" + tlsCertsMountPath + "/tls.crt'\"'\"'\\nssl_key_file = '\"'\"'" + tlsCertsMountPath + "/tls.key'\"'\"'\\n' >> /var/lib/pgsql/data/userdata/postgresql.conf; " + - "fi", + "sh", "-c", + fmt.Sprintf(` +if [ -f /var/lib/pgsql/data/userdata/postgresql.conf ]; then + grep -q '^ssl = on' /var/lib/pgsql/data/userdata/postgresql.conf || \ + cat >> /var/lib/pgsql/data/userdata/postgresql.conf <<'EOF' + +ssl = on +ssl_cert_file = '%s/tls.crt' +ssl_key_file = '%s/tls.key' +EOF +fi`, tlsCertsMountPath, tlsCertsMountPath), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/middleware/middleware.go` around lines 502 - 508, The shell command constructing the postgresql.conf append is hard to read due to repeated `'\"'\"'` escaping; update the Command in middleware.go that builds this sh -c script (the Command slice containing the long printf and tlsCertsMountPath usage) to use a simpler heredoc or simpler quoting approach: replace the printf with a cat >> /var/lib/pgsql/data/userdata/postgresql.conf <<'EOF' ... EOF pattern (or an equivalent single-quote heredoc) so the ssl lines and tlsCertsMountPath are injected cleanly without nested quote escapes; ensure you still guard with the existing if [ -f ... ] check and preserve the same appended content (ssl = on, ssl_cert_file, ssl_key_file) and variable interpolation semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apis/quay/v1/quayregistry_types.go`:
- Line 161: Update the kubebuilder XValidation rule that guards
overrides.tls.secretRef so that it also ensures secretRef.name is non-empty;
replace or augment the existing rule (the rule currently referencing
has(self.overrides.tls.secretRef) and self.overrides.tls.enabled) with a
condition that allows no secretRef or requires both tls.enabled to be true and
overrides.tls.secretRef.name != "" (e.g. !has(self.overrides) ||
!has(self.overrides.tls) || !has(self.overrides.tls.secretRef) ||
(self.overrides.tls.enabled && self.overrides.tls.secretRef.name != "")),
ensuring the validation fails fast when secretRef.name is empty.
In `@controllers/quay/features_test.go`:
- Around line 578-590: Replace the PEM-formatted private key literals used in
the test fixture with non-sensitive placeholders and update the corresponding
expectations: remove the fake PEM blocks assigned to "POSTGRES_TLS_KEY" and
"CLAIRPOSTGRES_TLS_KEY" and set them to simple innocuous strings (e.g.,
"fake-key" or "placeholder-key"); likewise update expectPostgresTLSKey and
expectClairPgTLSKey to match the new placeholder values so the test asserts
against the harmless string instead of PEM content. Ensure you modify the
variables in controllers/quay/features_test.go where "POSTGRES_TLS_KEY",
"CLAIRPOSTGRES_TLS_KEY", expectPostgresTLSKey, and expectClairPgTLSKey are
defined.
In `@controllers/quay/features.go`:
- Around line 737-765: The TLS secret validation currently only checks presence
of ca.crt and basic parsing of tls.crt/key; update the validation (around
secret.Data checks and the tls.X509KeyPair/pem.Decode/x509.ParseCertificate
code) to also parse ca.crt as PEM and X.509 (use pem.Decode then
x509.ParseCertificate or x509.ParseCertificates), build an x509.CertPool/Roots
from the CA bytes, and then verify the server certificate chain against that CA
and the expected DNS name(s) using cert.Verify with x509.VerifyOptions (set
Roots to the CA pool and DNSName to the appropriate host); for
Postgres/ClairPostgres when validating a DB secret derive the expected
hostname(s) (e.g. "<quay-name>-quay-database" and "<quay-name>-clair-postgres")
and return a clear error if the CA is malformed or the certificate does not
verify for the expected hostname(s).
In `@pkg/kustomize/kustomize.go`:
- Around line 784-805: The code currently removes sslmode/sslrootcert in the
else branch even when GetTLSOverrideForComponent(quay, v1.ComponentPostgres)
returns nil; change the logic so you only strip TLS params when an explicit
override exists and is disabled. Concretely, keep the existing branch that
applies verify-full when override != nil && override.Enabled, but replace the
else with an explicit check (override != nil && !override.Enabled) to remove
sslmode/sslrootcert; if override == nil do nothing (leave ctx.DbUri and
parsedUserConfig["DB_URI"] unchanged). Ensure you reference
GetTLSOverrideForComponent, ComponentPostgres, ctx.DbUri, and parsedUserConfig
when making the change.
In `@pkg/middleware/middleware.go`:
- Around line 526-527: Summary: The sed cleanup only removes directives at
column 0 and misses lines with leading whitespace; update the sed expressions in
the shell command string in middleware.go to match and delete lines with
optional leading whitespace (and optionally spaces around the '=' in the ssl
line). Locate the shell command string that contains "if [ -f
/var/lib/pgsql/data/userdata/postgresql.conf ] && grep -q '^ssl = on' ..." and
replace the deletion patterns (/^ssl = on$/d;/^ssl_cert_file/d;/^ssl_key_file/d)
with patterns that accept leading whitespace (e.g., use POSIX character class or
\s equivalent such as '^[[:space:]]*ssl = ' and '^[[:space:]]*ssl_cert_file' /
'^[[:space:]]*ssl_key_file') and enable extended regex if needed so the sed -i
invocation reliably deletes those lines regardless of indentation.
- Around line 623-625: The current idempotency check uses
strings.Contains(cm.Data[key], "ssl = on") which misses variants like "ssl=on"
or extra spaces; replace it with a whitespace-insensitive match (e.g., use a
regexp like `\bssl\s*=\s*on\b` and call regexp.MatchString or a precompiled
regexp's MatchString against cm.Data[key]) so the check correctly detects
existing SSL directives before returning and avoids appending duplicates;
reference the existing usage of cm.Data[key] and the strings.Contains call when
making the change.
---
Nitpick comments:
In `@pkg/middleware/middleware.go`:
- Around line 502-508: The shell command constructing the postgresql.conf append
is hard to read due to repeated `'\"'\"'` escaping; update the Command in
middleware.go that builds this sh -c script (the Command slice containing the
long printf and tlsCertsMountPath usage) to use a simpler heredoc or simpler
quoting approach: replace the printf with a cat >>
/var/lib/pgsql/data/userdata/postgresql.conf <<'EOF' ... EOF pattern (or an
equivalent single-quote heredoc) so the ssl lines and tlsCertsMountPath are
injected cleanly without nested quote escapes; ensure you still guard with the
existing if [ -f ... ] check and preserve the same appended content (ssl = on,
ssl_cert_file, ssl_key_file) and variable interpolation semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Enterprise
Run ID: 64e5b62b-e902-4ba9-b015-5f646c55ed91
📒 Files selected for processing (34)
apis/quay/v1/quayregistry_types.goapis/quay/v1/quayregistry_types_test.goapis/quay/v1/zz_generated.deepcopy.gobundle/manifests/quayregistries.crd.yamlconfig/crd/bases/quay.redhat.com_quayregistries.yamlcontrollers/quay/features.gocontrollers/quay/features_test.gocontrollers/quay/quayregistry_controller.gocontrollers/quay/quayregistry_controller_test.gopkg/context/context.gopkg/kustomize/certs.gopkg/kustomize/certs_test.gopkg/kustomize/kustomize.gopkg/kustomize/kustomize_test.gopkg/kustomize/secrets.gopkg/kustomize/secrets_test.gopkg/middleware/middleware.gopkg/middleware/middleware_test.gotest/chainsaw/Makefiletest/chainsaw/postgres_tls/00-assert-status.yamltest/chainsaw/postgres_tls/00-create-quay-registry.yamltest/chainsaw/postgres_tls/chainsaw-test.yamltest/chainsaw/postgres_tls_lifecycle/00-assert-status.yamltest/chainsaw/postgres_tls_lifecycle/00-create-quay-registry.yamltest/chainsaw/postgres_tls_lifecycle/01-enable-tls.yamltest/chainsaw/postgres_tls_lifecycle/02-disable-tls.yamltest/chainsaw/postgres_tls_lifecycle/chainsaw-test.yamltest/chainsaw/postgres_tls_mixed/00-assert-status.yamltest/chainsaw/postgres_tls_mixed/00-create-quay-registry.yamltest/chainsaw/postgres_tls_mixed/chainsaw-test.yamltest/chainsaw/postgres_tls_secretref/00-assert-status.yamltest/chainsaw/postgres_tls_secretref/00-create-quay-registry.yamltest/chainsaw/postgres_tls_secretref/01-create-invalid-registry.yamltest/chainsaw/postgres_tls_secretref/chainsaw-test.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
quay/quay(manual)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1277 +/- ##
==========================================
+ Coverage 62.13% 65.35% +3.22%
==========================================
Files 25 26 +1
Lines 3784 4327 +543
==========================================
+ Hits 2351 2828 +477
- Misses 1433 1499 +66
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@bcaton85 pls review this critical bug when using unmanaged postgres database with TLS Cert. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/middleware/middleware_test.go (2)
905-952: ⚡ Quick winAdd assertion for FSGroup=26.
The implementation sets
FSGroup=26in the pod security context to allow PostgreSQL (UID 26) to read the private key, as mentioned in the commit message. The test should verify this critical configuration.✅ Suggested assertion
Add after line 920 or before the volume checks:
assert.Equal(t, "quay.io/sclorg/postgresql-13-c9s:latest", dep.Spec.Template.Spec.InitContainers[0].Image) + + // Verify FSGroup is set for PostgreSQL to read private key + assert.NotNil(t, dep.Spec.Template.Spec.SecurityContext) + assert.NotNil(t, dep.Spec.Template.Spec.SecurityContext.FSGroup) + assert.Equal(t, int64(26), *dep.Spec.Template.Spec.SecurityContext.FSGroup) // Check projected volume added with correct mode🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/middleware/middleware_test.go` around lines 905 - 952, The test "injects init container and volume when TLS enabled" is missing an assertion that the pod security context FSGroup is set to 26; update the test after calling applyPostgresTLS(quay, dep, v1.ComponentPostgres) (or before the volume checks) to inspect dep.Spec.Template.Spec.SecurityContext (or PodSecurityContext) and assert that SecurityContext.FSGroup (or PodSecurityContext.FSGroup) is non-nil and equals int64(26), referencing the dep variable and the applyPostgresTLS function to locate the code to modify.
1076-1087: ⚡ Quick winAdd assertion for FSGroup=26 in integration test.
Since
Process()internally callsapplyPostgresTLS, which setsFSGroup=26, this integration test should verify that the pod security context is correctly configured.✅ Suggested assertion
Add after line 1078 or before the volume check:
assert.Equal(t, "postgres-tls-init", processed.Spec.Template.Spec.InitContainers[0].Name) + + // Verify FSGroup is set for PostgreSQL to read private key + assert.NotNil(t, processed.Spec.Template.Spec.SecurityContext) + assert.NotNil(t, processed.Spec.Template.Spec.SecurityContext.FSGroup) + assert.Equal(t, int64(26), *processed.Spec.Template.Spec.SecurityContext.FSGroup) // Projected volume added🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/middleware/middleware_test.go` around lines 1076 - 1087, The integration test currently checks init containers and volumes but misses asserting that the pod security context FSGroup is set by applyPostgresTLS; update the test in middleware_test.go (the test that calls Process()) to fetch processed.Spec.Template.Spec.SecurityContext and assert that SecurityContext.FSGroup != nil and equals 26 (or use pointer comparison to 26), ensuring Process()'s call to applyPostgresTLS results in FSGroup=26 being applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/middleware/middleware_test.go`:
- Around line 905-952: The test "injects init container and volume when TLS
enabled" is missing an assertion that the pod security context FSGroup is set to
26; update the test after calling applyPostgresTLS(quay, dep,
v1.ComponentPostgres) (or before the volume checks) to inspect
dep.Spec.Template.Spec.SecurityContext (or PodSecurityContext) and assert that
SecurityContext.FSGroup (or PodSecurityContext.FSGroup) is non-nil and equals
int64(26), referencing the dep variable and the applyPostgresTLS function to
locate the code to modify.
- Around line 1076-1087: The integration test currently checks init containers
and volumes but misses asserting that the pod security context FSGroup is set by
applyPostgresTLS; update the test in middleware_test.go (the test that calls
Process()) to fetch processed.Spec.Template.Spec.SecurityContext and assert that
SecurityContext.FSGroup != nil and equals 26 (or use pointer comparison to 26),
ensuring Process()'s call to applyPostgresTLS results in FSGroup=26 being
applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Enterprise
Run ID: a7062542-99d4-4d3a-81a1-163a2d87cae8
📒 Files selected for processing (2)
pkg/middleware/middleware.gopkg/middleware/middleware_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
quay/quay(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/middleware/middleware.go
bcaton85
changed the title
|
@bcaton85: This pull request references [Jira Issue PROJQUAY-11215](https://redhat.atlassian.net/browse/PROJQUAY-11215), which is invalid:
Comment DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/chainsaw/postgres_tls_service_ca/chainsaw-test.yaml (1)
141-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert
SSL_COUNTinstead of only logging it.Lines 141-145 compute and print SSL remote connection count, but the step never fails when count is
0, so the “verify-pg-stat-ssl” check can pass without proving client TLS usage.Suggested fix
SSL_COUNT=$(kubectl exec -n $NAMESPACE "${PG_POD}" -- \ bash -c "psql -t -c \"SELECT count(*) FROM pg_stat_ssl s JOIN pg_stat_activity a ON s.pid = a.pid WHERE s.ssl = true AND a.client_addr IS NOT NULL;\"" \ 2>/dev/null | tr -d ' \n') - echo "SSL remote connections: ${SSL_COUNT:-0}" + if ! [[ "${SSL_COUNT}" =~ ^[0-9]+$ ]]; then + echo "FAIL: invalid SSL connection count: '${SSL_COUNT}'" + exit 1 + fi + if [ "${SSL_COUNT}" -lt 1 ]; then + echo "FAIL: expected at least one SSL-encrypted remote connection" + exit 1 + fi + echo "PASS: SSL remote connections: ${SSL_COUNT}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/chainsaw/postgres_tls_service_ca/chainsaw-test.yaml` around lines 141 - 145, The script computes SSL_COUNT but only echoes it; update the block around SSL_COUNT (the kubectl exec "psql ... SELECT count(*) ..." call and the echo "SSL remote connections: ${SSL_COUNT:-0}") to assert the value is >0: after assigning SSL_COUNT, add a conditional that treats ${SSL_COUNT:-0} == 0 as a failure (echo a clear error like "No SSL remote connections detected" and exit 1) so the verify-pg-stat-ssl step fails when no client TLS connections are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/chainsaw/postgres_tls_service_ca/chainsaw-test.yaml`:
- Around line 124-133: The script checks SHOW ssl by kubectl exec into the
Postgres pod (PG_POD) immediately after waiting for quay-app rollout; insert an
explicit readiness gate for Postgres before the kubectl exec: after computing
PG_POD, wait for that pod to reach the Ready condition (use kubectl wait
--for=condition=ready on the PG_POD with an appropriate timeout) or wait for the
Postgres deployment/StatefulSet to finish rollout, then proceed to run the psql
SHOW ssl check; update the block around PG_POD, kubectl wait, and the kubectl
exec to ensure the pod is ready.
---
Outside diff comments:
In `@test/chainsaw/postgres_tls_service_ca/chainsaw-test.yaml`:
- Around line 141-145: The script computes SSL_COUNT but only echoes it; update
the block around SSL_COUNT (the kubectl exec "psql ... SELECT count(*) ..." call
and the echo "SSL remote connections: ${SSL_COUNT:-0}") to assert the value is
>0: after assigning SSL_COUNT, add a conditional that treats ${SSL_COUNT:-0} ==
0 as a failure (echo a clear error like "No SSL remote connections detected" and
exit 1) so the verify-pg-stat-ssl step fails when no client TLS connections are
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2ed38e34-2ac8-44e9-8a5b-7db0684a8946
📒 Files selected for processing (14)
controllers/quay/features.gocontrollers/quay/features_test.gocontrollers/quay/quayregistry_controller.gopkg/context/context.gopkg/kustomize/kustomize.gopkg/kustomize/kustomize_test.gopkg/kustomize/secrets.gopkg/kustomize/secrets_test.gopkg/middleware/middleware.gopkg/middleware/middleware_test.gotest/chainsaw/Makefiletest/chainsaw/postgres_tls_service_ca/00-assert-status.yamltest/chainsaw/postgres_tls_service_ca/00-create-quay-registry.yamltest/chainsaw/postgres_tls_service_ca/chainsaw-test.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
quay/quay(manual)
✅ Files skipped from review due to trivial changes (1)
- test/chainsaw/postgres_tls_service_ca/00-assert-status.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/context/context.go
- pkg/kustomize/secrets.go
- pkg/kustomize/secrets_test.go
- test/chainsaw/Makefile
- controllers/quay/quayregistry_controller.go
- pkg/kustomize/kustomize_test.go
- pkg/kustomize/kustomize.go
| kubectl rollout status deployment/pgsca-quay-app -n $NAMESPACE --timeout=300s | ||
| echo "quay-app rollout complete" | ||
|
|
||
| PG_POD=$(kubectl get pods -n $NAMESPACE -l quay-component=postgres \ | ||
| -o jsonpath='{.items[0].metadata.name}') | ||
| echo "Postgres pod: ${PG_POD}" | ||
|
|
||
| echo "Checking SHOW ssl..." | ||
| SSL_STATUS=$(kubectl exec -n $NAMESPACE "${PG_POD}" -- \ | ||
| bash -c "psql -t -c 'SHOW ssl;'" 2>/dev/null | tr -d ' \n') |
There was a problem hiding this comment.
Wait for postgres rollout before running psql checks.
Line 124 waits for pgsca-quay-app, but Lines 127-133 immediately exec into a postgres pod without an explicit postgres readiness gate. This can introduce intermittent failures in slower clusters.
Suggested fix
- kubectl rollout status deployment/pgsca-quay-app -n $NAMESPACE --timeout=300s
+ kubectl rollout status deployment/pgsca-quay-database -n $NAMESPACE --timeout=300s
+ kubectl rollout status deployment/pgsca-quay-app -n $NAMESPACE --timeout=300s
echo "quay-app rollout complete"
PG_POD=$(kubectl get pods -n $NAMESPACE -l quay-component=postgres \
-o jsonpath='{.items[0].metadata.name}')
+ if [ -z "${PG_POD}" ]; then
+ echo "FAIL: unable to locate postgres pod"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl rollout status deployment/pgsca-quay-app -n $NAMESPACE --timeout=300s | |
| echo "quay-app rollout complete" | |
| PG_POD=$(kubectl get pods -n $NAMESPACE -l quay-component=postgres \ | |
| -o jsonpath='{.items[0].metadata.name}') | |
| echo "Postgres pod: ${PG_POD}" | |
| echo "Checking SHOW ssl..." | |
| SSL_STATUS=$(kubectl exec -n $NAMESPACE "${PG_POD}" -- \ | |
| bash -c "psql -t -c 'SHOW ssl;'" 2>/dev/null | tr -d ' \n') | |
| kubectl rollout status deployment/pgsca-quay-database -n $NAMESPACE --timeout=300s | |
| kubectl rollout status deployment/pgsca-quay-app -n $NAMESPACE --timeout=300s | |
| echo "quay-app rollout complete" | |
| PG_POD=$(kubectl get pods -n $NAMESPACE -l quay-component=postgres \ | |
| -o jsonpath='{.items[0].metadata.name}') | |
| if [ -z "${PG_POD}" ]; then | |
| echo "FAIL: unable to locate postgres pod" | |
| exit 1 | |
| fi | |
| echo "Postgres pod: ${PG_POD}" | |
| echo "Checking SHOW ssl..." | |
| SSL_STATUS=$(kubectl exec -n $NAMESPACE "${PG_POD}" -- \ | |
| bash -c "psql -t -c 'SHOW ssl;'" 2>/dev/null | tr -d ' \n') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/chainsaw/postgres_tls_service_ca/chainsaw-test.yaml` around lines 124 -
133, The script checks SHOW ssl by kubectl exec into the Postgres pod (PG_POD)
immediately after waiting for quay-app rollout; insert an explicit readiness
gate for Postgres before the kubectl exec: after computing PG_POD, wait for that
pod to reach the Ready condition (use kubectl wait --for=condition=ready on the
PG_POD with an appropriate timeout) or wait for the Postgres
deployment/StatefulSet to finish rollout, then proceed to run the psql SHOW ssl
check; update the block around PG_POD, kubectl wait, and the kubectl exec to
ensure the pod is ready.
|
/retest |
1 similar comment
|
/retest |
|
🤖 Finished Review · ✅ Success · Started 5:15 PM UTC · Completed 5:31 PM UTC |
ReviewFindingsMedium
Low
Info
|
fullsend-ai-review
Bot
added
the
requires-manual-review
Add TLS configuration support to the QuayRegistry CRD for managed PostgreSQL components. The TLSOverride struct provides an opt-in `enabled` field and an optional `secretRef` for user-provided certificates (including cert-manager integration). Restricted to postgres and clairpostgres components via CEL validation rules. Existing deployments are unaffected as the field defaults to nil. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add self-signed certificate generation for managed PostgreSQL TLS using ECDSA P-256 with 10-year validity. Generated certificates include SANs for all in-cluster DNS names and are persisted in the managed keys Secret to survive reconcile loops. Each component (postgres, clairpostgres) gets its own CA so TLS can be enabled, disabled, and rotated independently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wire TLS into database connection strings when enabled: - Quay DB_URI: merge sslmode=verify-full and sslrootcert via net/url.Parse, preserving existing query params and passwords. Strip TLS params on disable for clean downgrade. - Clair connstring: conditional sslmode=verify-full with sslrootcert, replacing the hardcoded sslmode=disable. - Generate postgresql-ca and clairpostgres-ca Secrets for both self-signed and secretRef modes so projected volumes on quay-app, mirror, and upgrade jobs can mount the CA cert. - Move cert generation before secret list construction so certs are available for both managed keys persistence and CA Secret generation on first reconcile after TLS is enabled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…UAY-11215) Add TLS support to PostgreSQL and Clair deployments via middleware: - applyPostgresTLS: mount TLS certs via projected volume with mode 0600, add init container to patch postgresql.conf on existing PVCs. - applyPostgresTLSCleanup: strip SSL directives from postgresql.conf when TLS is disabled (downgrade path). - applyPostgresConfSampleTLS: inject SSL directives into the postgres-conf-sample ConfigMap so fresh PVCs get SSL from initdb. - applyClairDBTLS: mount CA cert volume on clair-app for sslrootcert verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…1215) Add secretRef validation during reconciliation before applying any configuration changes to deployments: - Verify referenced Secret exists and contains required keys (ca.crt, tls.crt, tls.key) in PEM format. - Validate cert/key pair matches and cert is not expired. - Set RolloutBlocked condition with specific error messages on validation failure, preserving the existing deployment state. - Extend Secret watch to trigger reconciliation when secretRef Secrets change, enabling cert-manager rotation workflows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 4 Chainsaw E2E test suites covering all PostgreSQL TLS scenarios: - postgres_tls: self-signed TLS on both postgres and clairpostgres — verifies init containers, volumes, secrets, certificate SANs/chain, clair-db-tls volume, and SHOW ssl. - postgres_tls_mixed: TLS on postgres only — verifies clairpostgres has no TLS volume and uses sslmode=disable. - postgres_tls_secretref: user-provided certs via secretRef and invalid secretRef — verifies RolloutBlocked with ConfigInvalid. - postgres_tls_lifecycle: enable/disable TLS migration on a live deployment — verifies SSL activates after enable and clean downgrade after disable. Excluded from non-destructive runs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… (PROJQUAY-11215) The else branch unconditionally stripped sslmode and sslrootcert from DB_URI, including unmanaged postgres where users provide their own connection string with TLS params. Guard the stripping with both an override != nil check and a ComponentIsManaged check so unmanaged postgres DB_URIs are left untouched. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add CEL validation rule and Go-level check to reject empty tls.secretRef.name at admission time rather than failing later during reconciliation when the secret lookup fails. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…1215) Use non-PEM placeholder strings for TLS key test fixtures to avoid tripping leak scanners. The test validates passthrough, not PEM format. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…JQUAY-11215) Parse ca.crt through pem.Decode and x509.ParseCertificate to catch malformed CA certificates at validation time instead of failing at PostgreSQL connection time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-11215) Update the sed cleanup init container and ConfigMap idempotency check to handle optional leading whitespace in postgresql.conf SSL directives using POSIX character classes and a compiled regex respectively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ROJQUAY-11215) The projected volume for TLS certs was mounted with mode 0600 owned by root, which works on OpenShift (SCC injects fsGroup) but fails on KinD where the postgres process runs as UID 26 with no group ownership. Change defaultMode to 0640 and explicitly set fsGroup=26 on the pod security context so PostgreSQL can read the key file via group permissions. PostgreSQL accepts root-owned key files with mode 0640 when the database user is in the files group. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…AY-11215) When running on OpenShift with tls.enabled and no secretRef, the operator now annotates the postgres Service with service.beta.openshift.io/serving- cert-secret-name instead of generating self-signed certificates. The existing cluster-service-ca ConfigMap provides the CA trust chain. Falls back to self-signed certs on vanilla Kubernetes. Per-component (postgres, clairpostgres) with independent detection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On OpenShift the restricted SCC rejects explicit fsGroup: 26, but the SCC auto-assigns an fsGroup from the namespace range making cert files group-readable. On vanilla Kubernetes (KinD) there is no SCC so fsGroup must be set explicitly. Gate the fsGroup assignment on SupportsRoutes being false. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On OpenShift the operator uses service CA instead of self-signed certs, so the CA comes from the cluster-service-ca ConfigMap rather than operator-generated postgresql-ca secrets. Update the assert-tls-secrets, assert-cert-sans-and-chain, and assert-clair-db-tls steps to detect OpenShift and verify the correct CA source on each platform. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…JQUAY-11215) Remove manual serial number generation for CA and server certificates — Go 1.23+ auto-generates RFC 5280-compliant serials when SerialNumber is nil. Fix trailing colon in DB_URI host when user-provided URI omits port by defaulting to 5432. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace `grep -q` with `grep >/dev/null` in platform detection pipes. Under `set -euo pipefail`, `grep -q` exits immediately on match, sending SIGPIPE to the still-writing `kubectl api-resources` process. The SIGPIPE exit code (141) becomes the pipeline result, causing the test to incorrectly fall into the non-OpenShift branch on OpenShift clusters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…JQUAY-11215) On OpenShift the CA is provided via a cluster-service-ca ConfigMap, not a postgresql-ca secret. The mixed test unconditionally checked for the secret, failing on OpenShift. Add platform detection to check the ConfigMap on OpenShift and the secret on KinD. Apply the same fix to the lifecycle test for consistency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c4be77f to
1a2a88c
Compare
|
@bcaton85: An error was encountered updating to the MODIFIED state for bug PROJQUAY-11215 on the Jira server at https://redhat.atlassian.net. No known errors were detected, please see the full error message for details. Full error message.
No transition status with name `MODIFIED` could be found. Please select from the following list: [New Planning In Progress Closed]
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
🤖 Finished Retro · ✅ Success · Started 5:11 PM UTC · Completed 5:20 PM UTC |
Retro: PR #1277 — PostgreSQL TLS supportTimeline: PR opened June 10 by bcaton85 (+4165/−15, 37 files). CodeRabbit reviewed immediately (June 10–12, 3 rounds). Human reviewer jbpratt reviewed June 15, approved June 16. Fullsend review agent ran June 17 — after human approval — posted 1 medium + 7 low + 5 info findings and applied What went well: The human review process worked effectively. CodeRabbit caught early issues (secretRef validation, PEM handling), the human reviewer caught practical bugs (trailing colon in DB_URI, unnecessary serial number generation), and the author addressed feedback promptly across 20 commits. Review agent observations: The fullsend review ran late (after human approval), so its findings didn't influence the review process. It applied Existing issue coverage: The key improvement opportunities identified here are already tracked:
No new proposals filed — existing issues adequately cover the identified gaps. |
5 participants
Adds opt-in TLS encryption for operator-managed PostgreSQL instances used by Quay Registry and Clair, enabling customers in regulated industries to meet encryption-in-transit requirements without provisioning external database infrastructure.
What this enables
spec:
components:
- kind: postgres
managed: true
overrides:
tls:
enabled: true
- kind: clairpostgres
managed: true
overrides:
tls:
enabled: true
Certificate options
rotation.
Commits
Test plan
Unit tests
E2E tests (Chainsaw, all passing)
Manual verification on OpenShift
Enhancement: quay/enhancements#41