fix: use shared system trust store sds secret - #9357
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9357 +/- ##
==========================================
+ Coverage 75.70% 75.74% +0.03%
==========================================
Files 254 254
Lines 42110 42190 +80
==========================================
+ Hits 31880 31956 +76
- Misses 8085 8086 +1
- Partials 2145 2148 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
177ceb3 to
9305124
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9305124dfa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR updates the xDS translation pipeline to avoid generating a per-backend SDS secret when upstream TLS validation uses the system trust store, instead having all such clusters reference a single shared SDS secret (system_ca_certificates) to reduce per-policy inotify watchers.
Changes:
- Switch upstream TLS validation to reference a shared SDS secret when
UseSystemTrustStoreis enabled (no more per-cluster secret emission). - Add global resource emission for the shared system trust store SDS secret.
- Update/extend golden testdata to reflect shared-secret behavior (including a new multi-backend system-truststore case).
Required fixes (blocking):
internal/gatewayapi/globalresources.go:scanXdsIRonly detectsUseSystemTrustStoreon HTTP/TCP route destinations. Other xDS IR destinations (notably AccessLog OpenTelemetry/ALS and Tracing) can also carrytls.useSystemTrustStore: true; with this PR they will referencesystem_ca_certificatesbut won’t trigger global secret emission, producing invalid xDS (clusters referencing a missing secret).release-notes/current/performance_improvements/9357-system-trust-store-single-sds-secret.md: This modifies existing generated xDS resource names/content (secrets), which can break existing EnvoyPatchPolicies/extension servers that patch the previous per-policy secret names; it should be called out as a breaking change (and typically placed underrelease-notes/current/breaking_changes/).
Reviewed changes
Copilot reviewed 22 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| release-notes/current/performance_improvements/9357-system-trust-store-single-sds-secret.md | Adds release note for shared system trust store SDS secret (needs breaking-change callout). |
| internal/xds/translator/translator.go | Updates secret emission + validation-context secret naming for system trust store. |
| internal/xds/translator/globalresources.go | Introduces shared secret name constant and emits shared system trust store secret as a global resource. |
| internal/ir/xds.go | Adds GlobalResources.UseSystemTrustStore flag to drive shared secret emission. |
| internal/gatewayapi/globalresources.go | Sets GlobalResources.UseSystemTrustStore based on IR scan (scan currently incomplete). |
| internal/xds/translator/testdata/in/xds-ir/websocket-backend-force-http1-upstream.yaml | Sets globalResources.useSystemTrustStore: true in IR input. |
| internal/xds/translator/testdata/in/xds-ir/tcproute-mtls.yaml | Sets globalResources.useSystemTrustStore: true in IR input. |
| internal/xds/translator/testdata/in/xds-ir/httproute-with-tls-and-http.yaml | Sets globalResources.useSystemTrustStore: true in IR input. |
| internal/xds/translator/testdata/in/xds-ir/http-route-with-tls-system-truststore.yaml | Sets globalResources.useSystemTrustStore: true in IR input. |
| internal/xds/translator/testdata/in/xds-ir/http-route-multiple-system-truststore.yaml | Adds new IR input case with multiple backends using system trust store. |
| internal/xds/translator/testdata/in/xds-ir/http-route-dynamic-resolver-with-host-rewriting.yaml | Sets globalResources.useSystemTrustStore: true in IR input. |
| internal/xds/translator/testdata/in/xds-ir/backend-tls-settings.yaml | Sets globalResources.useSystemTrustStore: true in IR input. |
| internal/xds/translator/testdata/out/xds-ir/websocket-backend-force-http1-upstream.secrets.yaml | Updates expected secrets to single system_ca_certificates entry. |
| internal/xds/translator/testdata/out/xds-ir/websocket-backend-force-http1-upstream.clusters.yaml | Updates expected cluster secret refs to system_ca_certificates. |
| internal/xds/translator/testdata/out/xds-ir/tcproute-mtls.secrets.yaml | Updates expected secrets to include shared system trust store secret. |
| internal/xds/translator/testdata/out/xds-ir/tcproute-mtls.clusters.yaml | Updates expected cluster secret refs to system_ca_certificates. |
| internal/xds/translator/testdata/out/xds-ir/httproute-with-tls-and-http.secrets.yaml | Updates expected secrets to shared system trust store secret. |
| internal/xds/translator/testdata/out/xds-ir/httproute-with-tls-and-http.clusters.yaml | Updates expected cluster secret refs to system_ca_certificates. |
| internal/xds/translator/testdata/out/xds-ir/http-route-with-tls-system-truststore.secrets.yaml | Updates expected secrets to shared system trust store secret. |
| internal/xds/translator/testdata/out/xds-ir/http-route-with-tls-system-truststore.clusters.yaml | Updates expected cluster secret refs to system_ca_certificates. |
| internal/xds/translator/testdata/out/xds-ir/http-route-multiple-system-truststore.secrets.yaml | New expected secrets output for multi-backend shared secret case. |
| internal/xds/translator/testdata/out/xds-ir/http-route-multiple-system-truststore.routes.yaml | New expected routes output for multi-backend case. |
| internal/xds/translator/testdata/out/xds-ir/http-route-multiple-system-truststore.listeners.yaml | New expected listeners output for multi-backend case. |
| internal/xds/translator/testdata/out/xds-ir/http-route-multiple-system-truststore.endpoints.yaml | New expected endpoints output for multi-backend case. |
| internal/xds/translator/testdata/out/xds-ir/http-route-multiple-system-truststore.clusters.yaml | New expected clusters output for multi-backend shared secret case. |
| internal/xds/translator/testdata/out/xds-ir/http-route-dynamic-resolver-with-host-rewriting.secrets.yaml | Updates expected secrets to shared system trust store secret. |
| internal/xds/translator/testdata/out/xds-ir/http-route-dynamic-resolver-with-host-rewriting.clusters.yaml | Updates expected cluster secret refs to system_ca_certificates. |
| internal/xds/translator/testdata/out/xds-ir/backend-tls-settings.secrets.yaml | Updates expected secrets to dedupe system trust store into one shared secret. |
| internal/xds/translator/testdata/out/xds-ir/backend-tls-settings.clusters.yaml | Updates expected cluster secret refs to system_ca_certificates. |
| internal/xds/translator/testdata/out/extension-xds-ir/http-route-extension-translate-error.secrets.yaml | Adds expected shared system trust store secret output in extension error case. |
| internal/gatewayapi/testdata/tcproute-with-backendtlspolicy.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/httproute-with-tls-and-http.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/httproute-rule-with-non-service-backends-and-websocket-app-protocols.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/httproute-dynamic-resolver.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/httproute-dynamic-resolver-host-rewriting.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/gateway-tls-frontend-backend.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/envoyproxy-tls-settings.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/backendtlspolicy-system-truststore.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/backendtlspolicy-serviceimport-target.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
| internal/gatewayapi/testdata/backend-tls-settings.out.yaml | Updates expected xDS IR to include globalResources.useSystemTrustStore: true. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Guy Daich <guy.daich@sap.com>
9305124 to
7d1c60c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9fa8c423c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Guy Daich <guy.daich@sap.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab78b3b040
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7549fd70ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebdb88fe8f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Heal system_ca_certificates if it was tampered with by the extension post-translation | ||
| // hook. EnvoyPatchPolicy tampering is caught per-policy inside processJSONPatches. | ||
| // Healing keeps the snapshot publishable; the logger carries the forensic detail. | ||
| t.ensureSystemTrustStoreSecret(tCtx) |
There was a problem hiding this comment.
When an XDSTranslation extension returns an invalid system_ca_certificates Secret (for example, one with the validation context cleared), this repair runs after tCtx.ValidateAll() has already joined the validation error. Although ensureSystemTrustStoreSecret restores the canonical secret, Translate still returns a non-nil error, and the xDS runner only publishes snapshots when that error is nil (internal/xds/runner/runner.go:340-343), so the repaired snapshot is skipped and Envoy keeps the previous config. Move the repair before validation or discard validation errors for the repaired secret.
Useful? React with 👍 / 👎.
Signed-off-by: Guy Daich <guy.daich@sap.com>
|
/retest |
1 similar comment
|
/retest |
| return false | ||
| } | ||
|
|
||
| // emitSystemTrustStoreSecret ensures a system CA trust store SDS secret with the given name |
There was a problem hiding this comment.
thoughts on simplifying the logic so we always insert the system ca, to avoid the extra cost of find, which might speed up translation and resource convergence
There was a problem hiding this comment.
For the new default path (dedupe) - yes, we can just see if the flag recording emission is true and skip entirely: no need to even insert. There's anyway a reconciliation at the end of the cycle in case somehow the secret is missing.
For the old path (no dedupe) - prefer to keep behavior as it was previously (check-then-insert), although it's highly unlikely that the find would actually find anything.
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
cc @zhaohuabing - somewhat related to concerns you raised in #9489 |
|
/retest |
* use shared system trust store sds secret Signed-off-by: Guy Daich <guy.daich@sap.com>
Background
To support dynamic reload of the system trust store, an sds secret must be created pointing to the filesystem location of the CA certificates. Currently, EG creates an SDS secret for each BTLSP/Backend resource with relevant TLS config. Each such SDS secret causes envoy to establish a inotify file watcher.
Summary
All
BackendTLSPolicyandBackendresources usingWellKnownCACertificates: Systemnow share a single SDS secret namedsystem_ca_certificatesinstead of secert per resource, reducing inotify watchers and xDS snapshot size. The newPerResourceSystemCASecretruntime flag (default false) restores the old per-resource behavior as an opt-out.Main Changes
gateway api translation
PerResourceSystemCASecret.mergeServerValidationTLSConfigsnow correctly clearsUseSystemTrustStorewhen a BTLSP overrides aBackend's system trust store with an explicit CA certificate.xds translation
emitSystemTrustStoreSecretlazily emits the secret (keyed onCACertificate.Name) and records emission viaGlobalResourceStatus.SystemTrustStoreon theResourceVersionTable.system_ca_certificatesare rejected at application time with the error attributed directly to the offending policy's status.GlobalResourceStatus.SystemTrustStoreis used to determine if a shared secret should exist, and it's ensured. Any tampering is recorded with an error log and reverted.Notes to reviewers
WellKnownCACertificates: Systemoption. Eagerly creating the SDS secret without users explicitly requiring it may lead to a XDS NACK, e.g. in custom proxy images where the trust store is not available in the default location.Which issue(s) this PR fixes:
Fixes #
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.