Commit f0d58dd
committed
fix(e2e): comprehensive fix for PII detection E2E test failures (100% accuracy)
This commit addresses all root causes of the 0% PII detection accuracy in E2E tests
by applying 5 critical fixes across reconciler, policy checker, E2E framework, and CRDs.
The E2E PII test failures were caused by a chain of 5 distinct issues:
1. **CRD Decision Loading Bug** - Reconciler didn't copy decisions to top-level RouterConfig
2. **Race Condition** - Tests ran before CRD reconciliation completed
3. **Invalid CRD Model References** - Using LoRA adapter name as model name
4. **Missing Default Decision Fallback (IsPIIEnabled)** - PII detection disabled when no decision matched
5. **Missing Default Decision Fallback (CheckPolicy)** - PII policy enforcement failed even after detection
**File**: `src/semantic-router/pkg/k8s/reconciler.go:266`
Added:
```go
// CRITICAL: Also update top-level Decisions field for PII policy lookups
// The Decisions field is used by GetDecisionByName() which is called by PII policy checker
newConfig.Decisions = intelligentRouting.Decisions
```
**Impact**: This is the critical root cause fix. Without this, `GetDecisionByName()`
always returned nil because it looks up from `c.Decisions`, not `c.IntelligentRouting.Decisions`.
**File**: `e2e/profiles/dynamic-config/profile.go:239-251`
Added `waitForCRDReconciliation()` with 10-second delay after CRD deployment.
**Impact**: Prevents race condition where tests execute before the reconciler's
5-second polling cycle completes, ensuring CRD configuration is fully loaded.
**Files**:
- `deploy/helm/semantic-router/crds/vllm.ai_intelligentpools.yaml`
- `deploy/kubernetes/crds/vllm.ai_intelligentpools.yaml`
- `src/semantic-router/pkg/apis/vllm.ai/v1alpha1/types.go`
Added `PIIModelConfig` type with Kubernetes validation for:
- `modelPath` (required, 1-500 chars)
- `modelType` (optional, max 50 chars, e.g., "auto" for auto-detection)
- `threshold` (optional, 0.0-1.0)
- `useCPU` (optional boolean)
**Impact**: Enables proper CRD validation and configuration for LoRA PII auto-detection.
**Files**:
- `e2e/profiles/dynamic-config/crds/intelligentpool.yaml`
- `e2e/profiles/dynamic-config/crds/intelligentroute.yaml`
Added PII model configuration to IntelligentPool:
```yaml
piiModel:
modelPath: "models/lora_pii_detector_bert-base-uncased_model"
modelType: "auto"
threshold: 0.7
useCPU: true
```
Added default catch-all decision to IntelligentRoute:
```yaml
- name: "default_decision"
priority: 1
signals:
operator: "OR"
conditions:
- type: "keyword"
name: "catch_all"
modelRefs:
- model: "base-model"
loraName: "general-expert"
plugins:
- type: "pii"
configuration:
enabled: true
pii_types_allowed: []
```
**Impact**: Ensures PII detection is always enabled for unmatched requests with proper model configuration.
**File**: `src/semantic-router/pkg/utils/pii/policy.go`
Applied fallback to `"default_decision"` in both functions:
**IsPIIEnabled** (lines 17-21):
```go
if decisionName == "" {
decisionName = "default_decision"
logging.Infof("No decision specified, trying default decision: %s", decisionName)
}
```
**CheckPolicy** (lines 53-57):
```go
if decisionName == "" {
decisionName = "default_decision"
logging.Infof("No decision specified for CheckPolicy, trying default decision: %s", decisionName)
}
```
**Impact**: Enables PII detection and policy enforcement even when no specific route matches,
by falling back to the catch-all `default_decision` configured in the CRD.
**File**: `deploy/helm/semantic-router/values.yaml`
Added to model downloads:
```yaml
- name: lora_pii_detector_bert-base-uncased_model
repo: LLM-Semantic-Router/lora_pii_detector_bert-base-uncased_model
```
**Impact**: Ensures LoRA PII detection model is available for auto-detection feature.
**Before Fix**: 0% PII Detection Accuracy (0/100 tests passed)
**After Fix**: 100% PII Detection Accuracy (100/100 tests passed)
Verified locally using Kind cluster with `dynamic-config` profile:
- All 100 PII test cases correctly blocked
- No false negatives
- Proper PII entity detection (PERSON, CREDIT_CARD, EMAIL, IP_ADDRESS, etc.)
- Decision-based routing working correctly with CRD configuration
Each fix addresses a different layer of the PII detection pipeline:
1. **Reconciler Fix** - Enabled CRD decisions to be loaded into memory
2. **Race Condition Fix** - Ensured decisions were loaded before tests ran
3. **CRD Schema Updates** - Added proper validation and configuration support
4. **CRD Configuration** - Provided actual default decision and PII model config
5. **Policy Fallbacks** - Enabled PII detection/enforcement when no route matched
Without any single fix, the test would still fail with 0% accuracy.
Core Fixes:
- src/semantic-router/pkg/k8s/reconciler.go
- src/semantic-router/pkg/utils/pii/policy.go
- e2e/profiles/dynamic-config/profile.go
CRD Schemas:
- deploy/helm/semantic-router/crds/vllm.ai_intelligentpools.yaml
- deploy/kubernetes/crds/vllm.ai_intelligentpools.yaml
- src/semantic-router/pkg/apis/vllm.ai/v1alpha1/types.go
- src/semantic-router/pkg/apis/vllm.ai/v1alpha1/zz_generated.deepcopy.go
E2E Test Configuration:
- e2e/profiles/dynamic-config/crds/intelligentpool.yaml
- e2e/profiles/dynamic-config/crds/intelligentroute.yaml
- e2e/profiles/dynamic-config/values.yaml
Helm Chart:
- deploy/helm/semantic-router/values.yaml
Minor YAML Formatting (no functional change):
- deploy/helm/semantic-router/crds/vllm.ai_intelligentroutes.yaml
- deploy/kubernetes/crds/vllm.ai_intelligentroutes.yaml
Fixes #647
Signed-off-by: Yossi Ovadia <yovadia@redhat.com>1 parent 5d805a3 commit f0d58dd
File tree
14 files changed
+158
-34
lines changed- deploy
- helm/semantic-router
- crds
- kubernetes
- ai-gateway/semantic-router-values
- crds
- e2e/profiles/dynamic-config
- crds
- src/semantic-router/pkg
- apis/vllm.ai/v1alpha1
- k8s
- utils/pii
14 files changed
+158
-34
lines changedLines changed: 26 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
101 | 101 | | |
102 | 102 | | |
103 | 103 | | |
104 | | - | |
105 | 104 | | |
| 105 | + | |
106 | 106 | | |
107 | 107 | | |
108 | | - | |
109 | 108 | | |
| 109 | + | |
110 | 110 | | |
111 | 111 | | |
112 | 112 | | |
| |||
120 | 120 | | |
121 | 121 | | |
122 | 122 | | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
123 | 147 | | |
124 | 148 | | |
125 | 149 | | |
| |||
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
257 | 257 | | |
258 | 258 | | |
259 | 259 | | |
260 | | - | |
261 | | - | |
262 | 260 | | |
| 261 | + | |
| 262 | + | |
263 | 263 | | |
264 | 264 | | |
265 | 265 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
167 | 167 | | |
168 | 168 | | |
169 | 169 | | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
170 | 173 | | |
171 | 174 | | |
172 | 175 | | |
| |||
Lines changed: 5 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
467 | 467 | | |
468 | 468 | | |
469 | 469 | | |
470 | | - | |
471 | | - | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
472 | 474 | | |
473 | 475 | | |
474 | | - | |
| 476 | + | |
475 | 477 | | |
476 | 478 | | |
477 | 479 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
101 | 101 | | |
102 | 102 | | |
103 | 103 | | |
104 | | - | |
105 | 104 | | |
| 105 | + | |
106 | 106 | | |
107 | 107 | | |
108 | | - | |
109 | 108 | | |
| 109 | + | |
110 | 110 | | |
111 | 111 | | |
112 | 112 | | |
| |||
120 | 120 | | |
121 | 121 | | |
122 | 122 | | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
123 | 147 | | |
124 | 148 | | |
125 | 149 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
257 | 257 | | |
258 | 258 | | |
259 | 259 | | |
260 | | - | |
261 | | - | |
262 | 260 | | |
| 261 | + | |
| 262 | + | |
263 | 263 | | |
264 | 264 | | |
265 | 265 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
8 | 13 | | |
9 | 14 | | |
10 | 15 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
8 | 14 | | |
9 | 15 | | |
10 | 16 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
242 | 242 | | |
243 | 243 | | |
244 | 244 | | |
245 | | - | |
246 | | - | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
247 | 250 | | |
248 | 251 | | |
249 | 252 | | |
250 | 253 | | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
251 | 268 | | |
252 | 269 | | |
253 | 270 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
51 | | - | |
52 | | - | |
| 51 | + | |
| 52 | + | |
53 | 53 | | |
54 | 54 | | |
55 | | - | |
| 55 | + | |
56 | 56 | | |
57 | 57 | | |
58 | 58 | | |
| |||
0 commit comments