HYPERFLEET-1153 - feat: schema validation middleware driven by entity registry#265
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Sequence Diagram(s)sequenceDiagram
participant registerAPIMiddleware
participant registry
participant NewSchemaValidator
participant doc.Components.Schemas
registerAPIMiddleware->>registry: Validate()
registerAPIMiddleware->>NewSchemaValidator: construct
NewSchemaValidator->>registry: ValidateSpecSchemas(doc.Components.Schemas lookup)
registry->>doc.Components.Schemas: resolve SpecSchemaName
doc.Components.Schemas-->>registry: present or missing
registry-->>NewSchemaValidator: return or panic
NewSchemaValidator->>doc.Components.Schemas: buildSchemasMap
doc.Components.Schemas-->>NewSchemaValidator: present or missing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 4 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 343 lines (>200) | +1 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api/server | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/validators/schema_validator.go`:
- Around line 57-67: The hardcoded `ClusterSpec` and `NodePoolSpec` branches in
`buildSchemasMap` still return errors instead of matching the new panic contract
used by the registry-backed `WithSpecSchema` path. Update those
`buildSchemasMap` cases to fail fast with the same `panic(fmt.Sprintf(...))`
style as the existing `schemaRef == nil` check, or refactor them to go through
the registry lookup so `NewSchemaValidator` behaves consistently for all missing
spec schemas.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e9c58bd5-e006-4839-b308-cc219877bfdd
📒 Files selected for processing (5)
cmd/hyperfleet-api/server/routes.gopkg/registry/registry.gopkg/registry/registry_test.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
kuudori
force-pushed
the
HYPERFLEET-1153-feat-schema-validation-registry
branch
from
26c2bf1 to
8bf43be
Compare
| } { | ||
| schemaRef := doc.Components.Schemas[hc.schema] | ||
| if schemaRef == nil { | ||
| panic(fmt.Sprintf( |
There was a problem hiding this comment.
Maybe it's better to return an error here 🤔
| registry.ValidateSpecSchemas(func(name string) bool { | ||
| return doc.Components.Schemas[name] != nil | ||
| }) | ||
|
|
||
| schemas := buildSchemasMap(doc) |
There was a problem hiding this comment.
ChannelSpec, VersionSpec, and WifConfigSpec don't exist in the upstream hyperfleet-api-spec yet, but the Channel, Version, and WifConfig plugins register those as their SpecSchemaName. The switch from warn+skip to panic means make run with the default openapi.yaml will crash at startup.
A few options:
- Remove SpecSchemaName from those plugin registrations until the schemas are added to hyperfleet-api-spec
- Keep warn+skip for now and switch to panic once all schemas exist upstream
- Add the schemas to hyperfleet-api-spec first (separate PR)
| @@ -256,60 +217,9 @@ components: | |||
| err := os.WriteFile(schemaPath, []byte(schemaWithoutNodePool), 0600) | |||
| Expect(err).To(BeNil()) | |||
|
|
|||
| _, err = NewSchemaValidator(schemaPath) | |||
| Expect(err).ToNot(BeNil()) | |||
| Expect(err.Error()).To(ContainSubstring("NodePoolSpec schema not found")) | |||
| } | |||
|
|
|||
| func TestValidate_SkipsWhenOptionalEntitySchemaNotLoaded(t *testing.T) { | |||
| RegisterTestingT(t) | |||
|
|
|||
| var logBuf bytes.Buffer | |||
| logger.ReconfigureGlobalLogger(&logger.LogConfig{ | |||
| Level: slog.LevelWarn, | |||
| Format: logger.FormatText, | |||
| Output: &logBuf, | |||
| Component: "validators-test", | |||
| }) | |||
|
|
|||
| registerRequiredSpecValidationEntities() | |||
| registry.Register(registry.EntityDescriptor{ | |||
| Kind: "WifConfig", | |||
| Plural: "wifconfigs", | |||
| SpecSchemaName: "WifConfigSpec", | |||
| }) | |||
|
|
|||
| schemaWithoutWifConfig := ` | |||
| openapi: 3.0.0 | |||
| info: | |||
| title: Cluster NodePool Only | |||
| version: 1.0.0 | |||
| paths: {} | |||
| components: | |||
| schemas: | |||
| ClusterSpec: | |||
| type: object | |||
| properties: | |||
| region: | |||
| type: string | |||
| NodePoolSpec: | |||
| type: object | |||
| properties: | |||
| replicas: | |||
| type: integer | |||
| ` | |||
|
|
|||
| tmpDir := t.TempDir() | |||
| schemaPath := filepath.Join(tmpDir, "cluster-nodepool-only.yaml") | |||
| err := os.WriteFile(schemaPath, []byte(schemaWithoutWifConfig), 0600) | |||
| Expect(err).To(BeNil()) | |||
|
|
|||
| validator, err := NewSchemaValidator(schemaPath) | |||
| Expect(err).To(BeNil()) | |||
|
|
|||
| // Invalid spec would fail validation if WifConfigSpec were loaded. | |||
| err = validator.Validate("wifconfigs", map[string]interface{}{}) | |||
| Expect(err).To(BeNil()) | |||
| Expect(func() { | |||
| _, _ = NewSchemaValidator(schemaPath) | |||
| }).To(PanicWith(ContainSubstring("NodePoolSpec"))) | |||
There was a problem hiding this comment.
Minor naming nit: this test now asserts a panic, but the name still ends with _Fails. The sibling test was renamed to _Panics for the same behavior change. Worth renaming this one too for consistency.
kuudori
force-pushed
the
HYPERFLEET-1153-feat-schema-validation-registry
branch
from
8bf43be to
6ee6ca1
Compare
2 participants
Summary
ValidateSpecSchemas(schemaExists func(string) bool)topkg/registry— panics at startup if any registered entity'sSpecSchemaNamedoesn't resolve in the OpenAPI specregistry.Validate()at startup inregisterAPIMiddleware(never called before)ValidateSpecSchemasfromNewSchemaValidatorafter loading the OpenAPI docbuildSchemasMapwarn+skip to return error for missing schemas (defense-in-depth)ValidateSpecSchemas(panic/pass/skip)Hardcoded Cluster/NodePool entries intentionally preserved until HYPERFLEET-1159 migrates them to the registry.
Ticket
HYPERFLEET-1153
AC Coverage
WithSpecSchema()+ fatal check ensures schemas must resolveWithSpecSchema()(existing test proves it)shouldValidateRequestbehavior, unchangedValidateSpecSchemasin registry, called fromNewSchemaValidatorTestValidateSpecSchemas_*testsGate Results
make verify-all: 1273 tests pass, all static checks cleanUnresolved
E2E
Gated in CI, not run locally.
Base
base: 4fe21ec(main at time of branch)