api: support custom compression settings in BackendTrafficPolicy#9422
api: support custom compression settings in BackendTrafficPolicy#9422miltalex wants to merge 1 commit into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
miltalex
force-pushed
the
feat/compression
branch
from
0111452 to
550c968
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: 01114521ab
ℹ️ 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".
| case egv1a1.GzipCompressorType: | ||
| extensionName = "envoy.compression.gzip.compressor" | ||
| extensionMsg = &gzipv3.Gzip{} | ||
| extensionMsg = buildGzipProto(compression.Gzip) |
There was a problem hiding this comment.
When two routes on the same listener configure the same compressor type with different settings, the later route will silently use the first route's settings: patchHCM deduplicates filters only by compressorFilterName(irComp.Type), while patchRoute only enables that filter name and does not override the compressor library. Embedding route-specific compression.Gzip settings in the listener-level envoy.filters.http.compressor.gzip filter means a second Gzip BackendTrafficPolicy on the listener cannot get its own level/strategy/min-content-length.
Useful? React with 👍 / 👎.
| // +optional | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +kubebuilder:validation:Maximum=9 | ||
| CompressionLevel *uint32 `json:"compressionLevel,omitempty"` |
There was a problem hiding this comment.
Because ProxyPrometheusProvider.Compression reuses this Compression type, these newly added fields are now admitted and documented under EnvoyProxy Prometheus compression as well, but the bootstrap path still only reads proxyMetrics.Prometheus.Compression.Type and drops the per-library settings. A user setting spec.telemetry.metrics.prometheus.compression.gzip.compressionLevel would get a valid resource with no effect, so either the metric compression path needs to consume these fields or the shared API type should be split/validated to avoid advertising unsupported knobs there.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9422 +/- ##
==========================================
+ Coverage 75.28% 75.30% +0.02%
==========================================
Files 252 252
Lines 41536 41598 +62
==========================================
+ Hits 31269 31327 +58
- Misses 8143 8145 +2
- Partials 2124 2126 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
miltalex
force-pushed
the
feat/compression
branch
from
550c968 to
428c779
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 428c7795bc
ℹ️ 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".
| // +optional | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +kubebuilder:validation:Maximum=9 | ||
| CompressionLevel *uint32 `json:"compressionLevel,omitempty"` |
There was a problem hiding this comment.
When this shared Compression type is used by ProxyPrometheusProvider (api/v1alpha1/envoyproxy_metric_types.go:142), the generated EnvoyProxy CRD now accepts fields such as prometheus.compression.gzip.compressionLevel, but the bootstrap path still only reads Compression.Type (internal/xds/bootstrap/bootstrap.go:230-232). In that EnvoyProxy metrics context, users can set the new gzip/brotli/zstd knobs and get a valid resource while Envoy is rendered with only the compressor library choice, so the accepted settings are silently ignored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I haven't act on this cause in the past I noticed that some fields where added as no-op for prometheus metrics. Waiting on a maintainer's review for further action
| @@ -0,0 +1 @@ | |||
| Added support for tuning Gzip, Brotli and Zstd compressor settings in the `compressor` field of `BackendTrafficPolicy`. | |||
There was a problem hiding this comment.
This note describes only a new feature, but the translator changes existing xDS names for configured compressor filters/per-route keys from stable names like envoy.filters.http.compressor.gzip to hashed names when minContentLength or custom structs are present (for example, the updated internal/xds/translator/testdata/out/xds-ir/compressor.listeners.yaml and .routes.yaml). Existing EnvoyPatchPolicies or extension integrations that match the previous filter names will break on upgrade, so this needs a breaking-change release-note fragment rather than only new_features.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I will added once Reviewer's agree that current approach is the best solution, or something better can be implemented
Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
miltalex
force-pushed
the
feat/compression
branch
from
428c779 to
e232d4b
Compare
1 participant
What this PR does / why we need it:
The compressor field in BackendTrafficPolicy only allows selecting the compressor type (Gzip, Brotli, Zstd); the per-type config structs (GzipCompressor, BrotliCompressor, ZstdCompressor) are empty. This PR adds the tuning knobs exposed by the Envoy compressor library extensions, so users can trade off compression ratio against CPU and memory usage:
All fields are optional; unset fields are omitted from the generated xDS config so Envoy's documented defaults apply, keeping existing configurations unchanged. Field ranges are enforced with OpenAPI validation mirroring Envoy's proto validation (e.g. gzip windowBits 9-15, chunkSize 4096-65536), so invalid values are rejected at admission instead of at xDS validation. The deprecated compression field carries the new settings through the same code path.
Which issue(s) this PR fixes:
Fixes #8314
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/.