envoyproxy / envoyproxy/gateway

SecurityPolicy: authorization.rules cannot merge across hierarchy — missing patchMergeKey/patchStrategy annotations

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

Description

## Description

`SecurityPolicy.spec.authorization.rules` cannot be merged across hierarchy levels (Gateway → HTTPRoute) because the `Authorization.Rules` field is missing the Kubernetes strategic-merge annotations (`patchMergeKey` and `patchStrategy`). As a result, when a route-level `SecurityPolicy` uses `mergeType: StrategicMerge` and defines its own `rules`, the parent Gateway-level `rules` are silently **replaced** instead of element-merged by `name`.

This makes it impossible to centralize a baseline allowlist at the Gateway level (e.g. cluster pod CIDRs, GCP health-check ranges, office CIDRs) while letting each route declare additional rules — every route ends up having to duplicate the baseline.

This mirrors the issue fixed in PR #6951 for `Compression` in `BackendTrafficPolicy`, but for `Authorization.Rules` in `SecurityPolicy`.

## Use case

We use Envoy Gateway as the platform ingress for a multi-tenant GKE platform. Today every per-route `SecurityPolicy` duplicates the same baseline CIDRs (cluster `10.0.0.0/8`, GCP health-check probes `35.191.0.0/16` + `130.211.0.0/22`, plus a Vault-sourced list of office CIDRs). With v1.8.0 we expected `mergeType: StrategicMerge` to let us:

- attach **one** Gateway-level `SecurityPolicy` carrying the baseline allowlist
- attach per-route `SecurityPolicy` with `mergeType: StrategicMerge` carrying only route-specific additions

…and have the effective Envoy RBAC rules be the **concatenation** of both rule sets. Empirical testing on EG 1.8.0 shows this does not happen.

## Reproducer

EG `v1.8.0`, Envoy proxy `distroless-v1.38.0`, GKE 1.31, single Gateway, `mergeGateways: true`.

```yaml
# Parent SP at Gateway-level
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
name: parent-allow-internal
namespace: eg-merge-test
spec:
targetRefs:
- group: gateway.networking.k8s.io
kind: Gateway
name: eg-merge-test-gw
authorization:
defaultAction: Allow
rules:
- name: parent-rule
action: Allow
principal:
clientCIDRs: [10.0.0.0/8]
---
# Child SP at Route-level
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
name: child-route-allowlist
namespace: eg-merge-test
spec:
targetRefs:
- group: gateway.networking.k8s.io
kind: HTTPRoute
name: echo-route
mergeType: StrategicMerge
authorization:
defaultAction: Deny
rules:
- name: child-rule
action: Allow
principal:
clientCIDRs: [1.2.3.4/32]
```

### Results (curl from in-cluster pod, source IP `10.27.7.12`, target `eg-merge-test.lab.homeserve.io`)

| # | Child SP | Result | Behavior observed |
|---|---|---|---|
| 1 | No `rules`, `defaultAction: Deny`, `mergeType: StrategicMerge` | **HTTP 200** | Parent's `[Allow 10/8]` preserved via merge — confirms scalar/object-level merge works |
| 2 | `rules: [Allow 1.2.3.4/32]`, `mergeType: StrategicMerge` | **HTTP 403** | Parent rules **replaced** by child rules. `10/8` pod blocked |
| 3 | `rules: [Allow 1.2.3.4/32]`, `mergeType: JSONMerge` | **HTTP 403** | Same — RFC 7396 replaces arrays by spec |
| 4 | `mergeType: Replace` | **CRD rejection** | `spec.mergeType: Invalid value: "Replace": Replace is not a valid MergeType for SecurityPolicy` (CEL) |

Test 1 demonstrates the merge plumbing does work end-to-end — but the array replacement in test 2 prevents the actual use case from being expressible.

## Root cause

`api/v1alpha1/authorization_types.go` declares:

```go
type Authorization struct {
// +optional
Rules []AuthorizationRule `json:"rules,omitempty"`

// +optional
DefaultAction *AuthorizationAction `json:"defaultAction"`
}
```

The `Rules` slice has neither `patchStrategy:"merge"` nor `patchMergeKey:"name"` in its struct tag. `internal/utils/merge.go` calls `k8s.io/apimachinery/pkg/util/strategicpatch.StrategicMergePatch(originalJSON, patchJSON, empty)` where `empty T = SecurityPolicySpec`; without those tags `strategicpatch` falls back to whole-slice replace. The `AuthorizationRule.Name` field is already declared optional and the existing CEL validation `spec.authorization.rules[].name` makes `name` a perfectly suitable merge key.

This is the same gap that #6951 fixed for `Compression` in `BackendTrafficPolicy` and that #5915 fixed for the rate-limit rules.

## Proposed fix

Add the K8s SMP struct tags on every per-feature rule/list slice that is expected to merge across hierarchy levels — at minimum `Authorization.Rules`:

```go
type Authorization struct {
// +optional
// +patchMergeKey=name
// +patchStrategy=merge
// +listType=map
// +listMapKey=name
Rules []AuthorizationRule `json:"rules,omitempty" patchStrategy:"merge" patchMergeKey:"name"`

// +optional
DefaultAction *AuthorizationAction `json:"defaultAction"`
}
```

Make `AuthorizationRule.Name` required (it's currently `*string` with `+optional`) so the merge key is always present, **or** keep it optional and have the controller auto-generate a stable name when omitted (it already does for IR purposes).

The same audit should cover the other rule/list slices that users will reasonably want to merge:
- `CORS.AllowOrigins` / `AllowMethods` / `AllowHeaders` / `ExposeHeaders` (currently replace)
- `OIDC.Scopes`, `OIDC.Resources`
- `JWT.Providers` (already has a name field — easy candidate)
- `BasicAuth` ConfigMap refs

## Why not `Replace` (issue #8728)?

`Replace` is the opposite of what we need — it discards the parent entirely. Our use case requires **element-level merge of arrays by name**, not whole-object replacement. The fix above is orthogonal to #8728.

## Workarounds considered

1. **Duplicate baseline in every child SP** — keeps `mergeType` cosmetic, defeats the centralization purpose.
2. **Single Gateway-level SP with operation-based routing** — only works if per-route deltas can be expressed as `Operation` matches against the same shared CIDR pool; not the case for per-tenant CIDR allowlists.
3. **Two-layer scheme using different features per layer** (parent does authorization, child does JWT/CORS) — works for distinct features but does not solve allowlist composition.

## Environment

- Envoy Gateway: `v1.8.0`
- Envoy proxy: `distroless-v1.38.0`
- Kubernetes / GKE: 1.31
- Gateway API CRDs: v1.5.1 (Standard channel)
- `mergeGateways: true`

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.