Skip to content

Add CEL validation to OIDCConfigRef and AuthzConfigRef union types#4299

Merged
ChrisJBurns merged 2 commits intomainfrom
cel-validation-union-types
Mar 21, 2026
Merged

Add CEL validation to OIDCConfigRef and AuthzConfigRef union types#4299
ChrisJBurns merged 2 commits intomainfrom
cel-validation-union-types

Conversation

@ChrisJBurns
Copy link
Collaborator

Summary

OIDCConfigRef and AuthzConfigRef use a type discriminator field but had no CEL validation rules — nothing prevented setting both configMap and inline simultaneously, or omitting the required field for a given type. Validation only happened at controller reconciliation time, producing confusing error conditions instead of immediate API rejection.

This adds CEL XValidation rules following the existing MCPExternalAuthConfigSpec pattern so misconfigurations are caught at admission time by the API server.

Closes #4247

Type of change

  • New feature (non-breaking change which adds functionality)

Changes

File Change
cmd/thv-operator/api/v1alpha1/mcpserver_types.go Add 3 CEL rules to OIDCConfigRef and 2 CEL rules to AuthzConfigRef
deploy/charts/operator-crds/files/crds/*.yaml Regenerated CRDs with new CEL validation rules
deploy/charts/operator-crds/templates/*.yaml Regenerated Helm-wrapped CRD templates
cmd/thv-operator/test-integration/mcp-server/mcpserver_cel_validation_integration_test.go 14 envtest integration tests covering all valid/invalid type-field combinations

Does this introduce a user-facing change?

Applying a manifest with a type/field mismatch (e.g. type: kubernetes but configMap set) or a missing required field (e.g. type: inline with no inline block) is now rejected immediately by the API server with a clear error message, instead of failing later during controller reconciliation.

Special notes for reviewers

  • OIDCConfigRef.kubernetes uses a one-directional CEL rule (type != 'kubernetes' ? !has(self.kubernetes) : true) because the kubernetes sub-field is optional even when type == "kubernetes" — it has sensible defaults.
  • configMap and inline fields use the strict bidirectional pattern (type == 'X' ? has(self.X) : !has(self.X)) matching MCPExternalAuthConfigSpec.
  • Existing controller-level validation is retained as defense-in-depth.

Test plan

  • task lint passes (operator code)
  • task operator-test-integration — all 7 suites pass, including 14 new CEL validation tests
  • Verified invalid combinations (type mismatch, missing fields) are rejected by envtest API server
  • Verified valid combinations (correct type/field pairing, kubernetes defaults) are accepted

Generated with Claude Code

Misconfigurations of OIDCConfigRef and AuthzConfigRef (e.g. setting
type: kubernetes but populating the configMap field) were only caught
at controller reconciliation time, producing confusing errors. This
adds CEL validation rules matching the existing MCPExternalAuthConfig
pattern so the API server rejects invalid combinations at admission.

Closes #4247

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added the size/M Medium PR: 300-599 lines changed label Mar 20, 2026
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Mar 20, 2026
@codecov
Copy link

codecov bot commented Mar 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.76%. Comparing base (4bb1016) to head (da4ff36).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4299      +/-   ##
==========================================
- Coverage   68.77%   68.76%   -0.01%     
==========================================
  Files         473      473              
  Lines       47917    47919       +2     
==========================================
- Hits        32955    32952       -3     
+ Misses      12309    12301       -8     
- Partials     2653     2666      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@jerm-dro jerm-dro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CEL rules are the right approach here, tests are thorough for individual rule violations.

Minor suggestion: consider adding a few multi-violation cases to the test matrix — e.g., type=configMap with only inline set (triggers both "missing configMap" and "extra inline"), or type=inline with inline + kubernetes set. These verify how multiple CEL violations surface to the user simultaneously. Not a blocker since every individual rule is already tested in both directions.

@ChrisJBurns ChrisJBurns merged commit 0108b11 into main Mar 21, 2026
84 of 87 checks passed
@ChrisJBurns ChrisJBurns deleted the cel-validation-union-types branch March 21, 2026 14:16
@ChrisJBurns
Copy link
Collaborator Author

ChrisJBurns commented Mar 21, 2026

@claude can you please raise a PR to cover the test that @jerm-dro describes?

@claude
Copy link
Contributor

claude bot commented Mar 21, 2026

Claude finished @ChrisJBurns's task in 2m 16s —— View jobclaude/pr-4299-20260321-1418Create PR ➔


Adding multi-violation CEL test cases

  • Gather context — read existing test file
  • Understand the multi-violation test cases @jerm-dro described
  • Implement 3 new multi-violation test cases:
    • OIDCConfigRef: type=configMap + only inline set → asserts both "configMap must be set when type is 'configMap'" AND "inline must be set when type is 'inline'" appear in the same rejection
    • OIDCConfigRef: type=inline + inline + kubernetes set → asserts "kubernetes must not be set when type is not 'kubernetes'"
    • AuthzConfigRef: type=configMap + only inline set → same dual-violation assertion as the OIDC case above
  • Commit and push changes

@ChrisJBurns
Copy link
Collaborator Author

@claude please create the above PR from the branch you used to commit the changes

@claude
Copy link
Contributor

claude bot commented Mar 21, 2026

Claude finished @ChrisJBurns's task in 26s —— View jobCreate PR ➔


Creating PR from branch claude/pr-4299-20260321-1418

  • Gather context
  • Generate PR link

The branch claude/pr-4299-20260321-1418 already contains the multi-violation CEL test cases. Here is the link to create the PR:

claude/pr-4299-20260321-1418

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CEL Validation to Existing Union Types

2 participants