envoyproxy / envoyproxy/gateway

BackendTrafficPolicy: a `compressor` entry missing its codec object is silently dropped (Accepted=True, no filter, no fallback)

Open
#9,870 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
3k
Forks
864
Avg merge
2d 2h
Merged PRs (30d)
140

Description

### Description

A `BackendTrafficPolicy` entry such as

```yaml
compressor:
- type: Gzip
minContentLength: 1024
```

is accepted by the API server **and** by the controller (`Accepted=True`, `observedGeneration` current), produces **no** `envoy.filters.http.compressor` in the listener chain, no per-route `typed_per_filter_config`, and **no** log line or status condition anywhere. The only way to discover it is to diff the xDS config dump.

This is the same symptom as #9314, which was closed after a maintainer supplied the workaround (add `gzip: {}`) — with the note *"I can agree its not the best UX and could probably be improved."* I'm opening this to track that improvement specifically, rather than to re-litigate the behaviour itself.

### Why it fails closed

[`buildCompression()`](https://github.com/envoyproxy/gateway/blob/main/internal/gatewayapi/backendtrafficpolicy.go#L2540) (line numbers from `main`):

```go
// Handle the Compressor field first (higher priority)
if len(compressor) > 0 {
result := make([]*ir.Compression, 0, len(compressor))
for i, c := range compressor {
// Only add compression if the corresponding compressor not null
if (c.Type == egv1a1.GzipCompressorType && c.Gzip != nil) ||
(c.Type == egv1a1.BrotliCompressorType && c.Brotli != nil) ||
(c.Type == egv1a1.ZstdCompressorType && c.Zstd != nil) {
result = append(result, &irCompression)
}
}
return result // <-- returns even when result is empty
}

// Fallback to the deprecated Compression field
```

Two things compound:

1. an entry whose codec object is nil is **skipped**, and
2. the `compressor` branch is taken whenever `len(compressor) > 0`, so an all-skipped list returns **empty** and never falls through to `compression`.

Net result is an empty IR compression list that is indistinguishable, from the user's side, from "no compression configured".

### Why the schema gives no hint

The task docs are correct — every example in `response-compression.md` includes `gzip: {}` / `brotli: {}` / `zstd: {}`. But the API says otherwise:

```go
// +required
Type CompressorType `json:"type"`

// +optional
Gzip *GzipCompressor `json:"gzip,omitempty"`
```

`GzipCompressor` is an empty struct, so `gzip: {}` reads as a no-op to anyone building config from `kubectl explain` or the CRD schema rather than from the docs. There is no CEL rule guarding it either, even though `BackendTrafficPolicySpec` already uses CEL for exactly this class of constraint:

```go
// +kubebuilder:validation:XValidation:rule="!has(self.compression) || !has(self.compressor)", message="either compression or compressor can be set, not both"
```

Notably, #6924 — which introduced the codec-object requirement so that `gzip: null` could express "disabled" — stated that *"Users should be required to provide both the `type` and the corresponding `brotli`/`gzip` attribute."* The translator half landed; the validation half did not.

### Suggested fix

I'd suggest **surfacing it** rather than rejecting it, because a CEL rule requiring the codec object may collide with the `gzip: null`-means-disabled semantics discussed in #6924 / #6775. In rough order of preference:

1. **Status condition or controller log** when a `compressor` entry is dropped for a missing codec object — turns a silent no-op into something greppable.
2. **Fall through to `compression`** when the `compressor` branch yields an empty result, so the deprecated field still works if someone sets both-ish configurations. (Behaviour change; less clearly correct.)
3. **CEL validation**, if `gzip: null` is *not* in fact load-bearing as a disable signal:
```
!has(self.compressor) || self.compressor.all(c,
(c.type == 'Gzip' && has(c.gzip)) ||
(c.type == 'Brotli' && has(c.brotli)) ||
(c.type == 'Zstd' && has(c.zstd)))
```

Happy to put up a PR for whichever direction maintainers prefer.

### Repro

```yaml
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
name: compress
spec:
targetRefs:
- group: gateway.networking.k8s.io
kind: Gateway
name: my-gateway
compressor:
- type: Gzip
minContentLength: 1024
```

Policy reports `Accepted=True`. `envoy.filters.http.compressor.gzip` is absent from the listener's `http_filters`. Adding `gzip: {}` makes it appear, with `min_content_length: 1024` and a `CompressorPerRoute` override on the routes.

⚠️ One gotcha worth recording for anyone verifying this: `grep compressor` over a full `config_dump` returns ~26 hits **even when it is not working** — Envoy lists every compiled-in extension under `BootstrapConfigDump`. Only the per-listener `filter_chains[].filters[].typed_config.http_filters` reflects the active chain.

### Environment

Envoy Gateway v1.8.3 (also present on `main` and v1.9.0 as of writing).

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at internal/gatewayapi/backendtrafficpolicy.go and inspect buildCompression(), then reproduce the BackendTrafficPolicy from the issue and verify the active listener filter chain rather than compiled-in extensions. Compare the possible validation, status/logging, and fallback directions with the existing gzip: null semantics; done requires an agreed behavior that no longer silently hides the invalid entry.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
api, backend-api-design
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.