HYPERFLEET-1033 - feat: consume OpenAPI schemas from hyperfleet-api-spec Go module#129
Closed
mliptak0 wants to merge 28 commits into
Closed
HYPERFLEET-1033 - feat: consume OpenAPI schemas from hyperfleet-api-spec Go module#129mliptak0 wants to merge 28 commits into
mliptak0 wants to merge 28 commits into
Conversation
|
[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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request moves OpenAPI schema sourcing from a tracked file to the external Go module github.com/openshift-hyperfleet/hyperfleet-api-spec. The tracked openapi/openapi.yaml file is removed and a new hack/extract-schema.go extracts the schema from the module during make generate. oapi-codegen still produces resolved artifacts embedded into the binary. Runtime schema validation now reads the embedded spec (via specschemas.FS and NewSchemaValidatorFromData). CLI/config flags and env bindings for a filesystem schema path are deprecated/removed, Docker no longer ships the schema file, and docs/tests were updated accordingly. Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CI
participant SpecMod as hyperfleet-api-spec (embedded FS)
participant Extract as hack/extract-schema.go
participant Oapi as oapi-codegen
participant Gen as pkg/api/openapi (generated)
Note over Dev,Gen: Build / generate flow
Dev->>Extract: run go run ./hack/extract-schema.go -output openapi/openapi.yaml
Extract->>SpecMod: specschemas.FS.ReadFile("core/openapi.yaml")
SpecMod-->>Extract: openapi bytes
Extract-->>Dev: writes openapi/openapi.yaml
Dev->>Oapi: run oapi-codegen against openapi/openapi.yaml
Oapi-->>Gen: produce resolved embedded YAML and Go types
sequenceDiagram
participant Client as HTTP Client
participant Server as hyperfleet-api (binary)
participant Middleware as Schema Validation Middleware
participant Validator as SchemaValidator
participant SpecEmbed as embedded spec (specschemas.FS / //go:embed)
participant Handler as API Handler / DB
Note over Client,Handler: Runtime request flow
Client->>Server: HTTP request
Server->>Middleware: middleware invoked
Middleware->>Validator: validate request body
Validator->>SpecEmbed: uses embedded schema loaded at startup
SpecEmbed-->>Validator: schema bytes / rules
Validator-->>Middleware: validation result
alt valid
Middleware->>Handler: forward to handler
Handler-->>Client: 2xx response
else invalid
Middleware-->>Client: 400 Bad Request (validation error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
160-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
run/docsnow relies on a generated file but doesn’t generate it
openapi/openapi.yamlis no longer tracked, somake run/docscan fail on clean clones unlessmake generatewas run first.Suggested fix
-.PHONY: run/docs -run/docs: check-container-tool ## Run swagger and host the api spec +.PHONY: run/docs +run/docs: check-container-tool generate ## Run swagger and host the api spec🤖 Prompt for 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. In `@Makefile` around lines 160 - 166, The run/docs Makefile target depends on the generated openapi/openapi.yaml but doesn't ensure it's created; update the run/docs target (currently declared as "run/docs: check-container-tool") to depend on the generation step (e.g., add "generate" or the specific generation target) so make will build openapi/openapi.yaml before starting the swagger container, and keep the existing use of $(CONTAINER_TOOL) and volume mounting intact.
🤖 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 `@AGENTS.md`:
- Around line 162-163: The change removing the tracked openapi/openapi.yaml
risks breaking downstream consumers (openshift-hyperfleet/hyperfleet-sentinel)
and docs; either (A) coordinate and update sentinel workflows and the
architecture docs in lockstep to consume the new generated-only flow (update
references in AGENTS.md and the sentinel repo to call make generate and/or the
new module hyfleet-api-spec), or (B) keep and publish a stable schema artifact
from this repo (retain openapi/openapi.yaml or add a CI job that publishes the
generated artifact to a known location) so external consumers continue to find
the contract; implement one coordinated path and add a validation step that
ensures changes conform to the HyperFleet architecture standards referenced in
the architecture repo.
In `@cmd/hyperfleet-api/server/routes.go`:
- Around line 122-131: The current logic treats failures from
specschemas.FS.ReadFile and validators.NewSchemaValidatorFromData as warnings
and continues with schema validation disabled; change this to fail-fast: when
specschemas.FS.ReadFile("core/openapi.yaml") returns an error or
validators.NewSchemaValidatorFromData(schemaData) returns an error, log the
error (use logger.WithError(ctx, err).Error or logger.WithError(...).Fatal) and
return the error (or abort startup) instead of falling back to disabled
validation; update the code paths around schemaData, schemaValidator,
specschemas.FS.ReadFile and validators.NewSchemaValidatorFromData to propagate
the error so server startup fails.
In `@docs/api-operator-guide.md`:
- Line 698: Replace the absolute sentence "Schema validation is always active."
with wording that it is "enabled by default from the embedded schema" and add a
brief fallback note that on schema read/parse errors the runtime logs a warning
and disables validation (fail-open). Specifically update the line containing the
phrase "Schema validation is always active." and append one clause describing
the fallback behavior (log warning + disable validation) so readers know
validation can be turned off on parse/read failure.
In `@go.mod`:
- Line 162: Remove the committed local replace directive that points to a
developer workstation path (the line replacing
github.com/openshift-hyperfleet/hyperfleet-api-spec with
/Users/mliptak/Repositories/...) from go.mod; instead, delete that replace entry
and, if you need a local override for development, add the same replacement to a
non-committed go.work or apply the replacement only in a local (uncommitted)
change so CI and other developers are not broken.
In `@openapi/CLAUDE.md`:
- Around line 21-24: The fenced code blocks are missing language tags and the
second block lacks a separating blank line; update the first block containing
the make commands ("make generate" / "make generate-all") to use a bash fence
(```bash), and insert a blank line before the second fenced example and mark it
as Go (```go) surrounding the replace directive for "replace
github.com/openshift-hyperfleet/hyperfleet-api-spec =>
/path/to/hyperfleet-api-spec" so both blocks comply with markdownlint.
In `@pkg/middleware/schema_validation_test.go`:
- Line 387: Replace the filesystem-based call to NewSchemaValidator(schemaPath)
with NewSchemaValidatorFromData using the existing raw schema bytes (testSchema)
in the test, remove the //nolint:staticcheck suppression and any temporary file
setup; ensure you still assign the returned values to validator and err (i.e.,
validator, err := validators.NewSchemaValidatorFromData(testSchema)) and keep
existing error handling in the test.
In `@pkg/validators/schema_validator.go`:
- Around line 46-66: The buildValidator function assumes doc.Components and
SchemaRef.Value are non-nil; add guards to return controlled errors instead of
panicking: in buildValidator check if doc.Components == nil and return an error
like "components section missing"; when fetching
clusterSpecSchema/nodePoolSpecSchema verify both the SchemaRef pointer and
SchemaRef.Value are non-nil before using them and propagate clear errors; also
update the validation logic that uses SchemaRef (e.g., where SchemaRef.Value is
accessed during validation) to nil-check SchemaRef.Value and return a
descriptive error if missing; reference functions/types: buildValidator,
SchemaValidator, ResourceSchema, and the ClusterSpec/NodePoolSpec
SchemaRef.Value fields.
---
Outside diff comments:
In `@Makefile`:
- Around line 160-166: The run/docs Makefile target depends on the generated
openapi/openapi.yaml but doesn't ensure it's created; update the run/docs target
(currently declared as "run/docs: check-container-tool") to depend on the
generation step (e.g., add "generate" or the specific generation target) so make
will build openapi/openapi.yaml before starting the swagger container, and keep
the existing use of $(CONTAINER_TOOL) and volume mounting intact.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 81a4d723-8749-46a3-a228-ffe487be170a
📒 Files selected for processing (18)
.gitignoreAGENTS.mdCLAUDE.mdDockerfileMakefilecmd/hyperfleet-api/server/routes.godocs/api-operator-guide.mddocs/deployment.mdgo.modhack/extract-schema.goopenapi/CLAUDE.mdopenapi/openapi.yamlpkg/config/flags.gopkg/config/server.gopkg/middleware/schema_validation_test.gopkg/validators/schema_validator.gotest/CLAUDE.mdtest/integration/integration_test.go
💤 Files with no reviewable changes (3)
- Dockerfile
- test/integration/integration_test.go
- openapi/openapi.yaml
Comment on lines
+122
to
+131
| schemaData, err := specschemas.FS.ReadFile("core/openapi.yaml") | ||
| if err != nil { | ||
| // Log warning but don't fail - schema validation is optional | ||
| logger.With(ctx, logger.FieldSchemaPath, schemaPath).WithError(err).Warn("Failed to load schema validator") | ||
| logger.WithError(ctx, err).Warn("Failed to read embedded OpenAPI schema") | ||
| logger.Warn(ctx, "Schema validation is disabled. Spec fields will not be validated.") | ||
| logger.Info(ctx, "To enable schema validation:") | ||
| logger.Info(ctx, " - Local: Run from repo root, or use --server-openapi-schema-path=openapi/openapi.yaml") | ||
| logger.Info(ctx, " - Config file: server.openapi_schema_path") | ||
| logger.Info(ctx, " - Environment: HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH") | ||
| } else { | ||
| // Apply schema validation middleware | ||
| logger.With(ctx, logger.FieldSchemaPath, schemaPath).Info("Schema validation enabled") | ||
| router.Use(middleware.SchemaValidationMiddleware(schemaValidator)) | ||
| schemaValidator, err := validators.NewSchemaValidatorFromData(schemaData) | ||
| if err != nil { | ||
| logger.WithError(ctx, err).Warn("Failed to parse embedded OpenAPI schema") | ||
| logger.Warn(ctx, "Schema validation is disabled. Spec fields will not be validated.") | ||
| } else { |
There was a problem hiding this comment.
Fail fast if embedded schema cannot be loaded/parsed
In the embedded-schema model, read/parse failure should be treated as startup failure, not a warning path. Continuing with validation disabled silently weakens request integrity guarantees.
As per coding guidelines, "cmd/**: Configuration validation at startup (fail-fast)".
🤖 Prompt for 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.
In `@cmd/hyperfleet-api/server/routes.go` around lines 122 - 131, The current
logic treats failures from specschemas.FS.ReadFile and
validators.NewSchemaValidatorFromData as warnings and continues with schema
validation disabled; change this to fail-fast: when
specschemas.FS.ReadFile("core/openapi.yaml") returns an error or
validators.NewSchemaValidatorFromData(schemaData) returns an error, log the
error (use logger.WithError(ctx, err).Error or logger.WithError(...).Fatal) and
return the error (or abort startup) instead of falling back to disabled
validation; update the code paths around schemaData, schemaValidator,
specschemas.FS.ReadFile and validators.NewSchemaValidatorFromData to propagate
the error so server startup fails.
| - **Environment variable:** `HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH=/etc/hyperfleet/schemas/openapi.yaml` | ||
| - **Config file:** `server.openapi_schema_path: /etc/hyperfleet/schemas/openapi.yaml` | ||
| - **Default:** `openapi/openapi.yaml` (provider-agnostic base schema) | ||
| The API validates resource `spec` fields (e.g., cluster, nodepool) against an OpenAPI schema. The schema is embedded in the binary from the `github.com/openshift-hyperfleet/hyperfleet-api-spec` Go module — no configuration is required. Schema validation is always active. |
There was a problem hiding this comment.
Avoid absolute “always active” wording for schema validation
Current runtime still has a fail-open path (read/parse error logs warning and disables validation). Please adjust this line to “enabled by default from embedded schema” and mention the fallback behavior.
🤖 Prompt for 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.
In `@docs/api-operator-guide.md` at line 698, Replace the absolute sentence
"Schema validation is always active." with wording that it is "enabled by
default from the embedded schema" and add a brief fallback note that on schema
read/parse errors the runtime logs a warning and disables validation
(fail-open). Specifically update the line containing the phrase "Schema
validation is always active." and append one clause describing the fallback
behavior (log warning + disable validation) so readers know validation can be
turned off on parse/read failure.
| } | ||
|
|
||
| validator, err := validators.NewSchemaValidator(schemaPath) | ||
| validator, err := validators.NewSchemaValidator(schemaPath) //nolint:staticcheck |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use NewSchemaValidatorFromData in tests and drop deprecated-path suppression
The helper already has raw schema bytes (testSchema), so this can avoid filesystem setup and the //nolint:staticcheck escape hatch.
🤖 Prompt for 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.
In `@pkg/middleware/schema_validation_test.go` at line 387, Replace the
filesystem-based call to NewSchemaValidator(schemaPath) with
NewSchemaValidatorFromData using the existing raw schema bytes (testSchema) in
the test, remove the //nolint:staticcheck suppression and any temporary file
setup; ensure you still assign the returned values to validator and err (i.e.,
validator, err := validators.NewSchemaValidatorFromData(testSchema)) and keep
existing error handling in the test.
Comment on lines
+46
to
+66
| func buildValidator(doc *openapi3.T) (*SchemaValidator, error) { | ||
| if err := doc.Validate(context.Background()); err != nil { | ||
| return nil, fmt.Errorf("invalid OpenAPI schema: %w", err) | ||
| } | ||
|
|
||
| // Extract ClusterSpec schema | ||
| clusterSpecSchema := doc.Components.Schemas["ClusterSpec"] | ||
| if clusterSpecSchema == nil { | ||
| return nil, fmt.Errorf("ClusterSpec schema not found in OpenAPI spec") | ||
| } | ||
|
|
||
| // Extract NodePoolSpec schema | ||
| nodePoolSpecSchema := doc.Components.Schemas["NodePoolSpec"] | ||
| if nodePoolSpecSchema == nil { | ||
| return nil, fmt.Errorf("NodePoolSpec schema not found in OpenAPI spec") | ||
| } | ||
|
|
||
| // Build schemas map | ||
| schemas := map[string]*ResourceSchema{ | ||
| "cluster": { | ||
| TypeName: "ClusterSpec", | ||
| Schema: clusterSpecSchema, | ||
| }, | ||
| "nodepool": { | ||
| TypeName: "NodePoolSpec", | ||
| Schema: nodePoolSpecSchema, | ||
| }, | ||
| } | ||
|
|
||
| return &SchemaValidator{ | ||
| doc: doc, | ||
| schemas: schemas, | ||
| doc: doc, | ||
| schemas: map[string]*ResourceSchema{ | ||
| "cluster": {TypeName: "ClusterSpec", Schema: clusterSpecSchema}, | ||
| "nodepool": {TypeName: "NodePoolSpec", Schema: nodePoolSpecSchema}, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the schema_validator.go file
find . -name "schema_validator.go" -type fRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Read the full schema_validator.go file to understand the implementation
cat -n pkg/validators/schema_validator.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 6222
🏁 Script executed:
#!/bin/bash
# Check the imports and understand what openapi3 types are being used
head -30 pkg/validators/schema_validator.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 908
🏁 Script executed:
#!/bin/bash
# Search for tests of schema_validator
find . -name "*schema_validator*test*" -o -name "*test*schema_validator*"Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Check if there are any tests and look for edge cases
find . -path "*test*" -name "*.go" | xargs grep -l "SchemaValidator\|buildValidator" 2>/dev/null | head -5Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Search for how SchemaRef is used elsewhere in the codebase
rg "\.Value\." --type go -A 2 -B 2 | head -50Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1759
🏁 Script executed (no clone):
# Check kin-openapi documentation and library behavior regarding nil Components
curl -s https://pkg.go.dev/github.com/getkin/kin-openapi/openapi3?tab=doc | grep -A 5 "type T struct" 2>/dev/null | head -20
Length of output: 13
🏁 Script executed:
#!/bin/bash
# Read the test file to see what edge cases are covered
cat -n pkg/validators/schema_validator_test.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 11190
🏁 Script executed:
#!/bin/bash
# Check if there are integration tests or examples of spec loading
find . -name "openapi*" -o -name "*spec*" | grep -E "\.(yaml|json|yml)$" | head -10Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Look at the go.mod to understand kin-openapi version
grep "kin-openapi" go.modRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Search for other usages of .Components in the codebase to see if nil checks are done elsewhere
rg "\.Components" --type go -B 2 -A 2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 769
🏁 Script executed:
#!/bin/bash
# Check if there are other OpenAPI loaders or validators that handle this
rg "Components\.Schemas\|LoadFromData\|LoadFromFile" --type go -B 1 -A 1Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 61
🏁 Script executed (no clone):
# Check kin-openapi library source code behavior for Components field
curl -s "https://raw.githubusercontent.com/getkin/kin-openapi/master/openapi3/openapi3.go" 2>/dev/null | head -150 | grep -A 30 "type T struct"
Length of output: 1681
Add nil guards for Components and schema Value to prevent panics at startup
The code assumes doc.Components is always present, but the kin-openapi library defines Components as an optional pointer field. A valid OpenAPI spec without a components: section will have doc.Components == nil, causing a panic at line 51 or 56 instead of returning a controlled error. Additionally, a SchemaRef pointer can exist while its Value field is nil, which would panic at line 104 during validation. The existing nil checks only verify the SchemaRef pointer itself, not the Value.
Suggested fix
func buildValidator(doc *openapi3.T) (*SchemaValidator, error) {
if err := doc.Validate(context.Background()); err != nil {
return nil, fmt.Errorf("invalid OpenAPI schema: %w", err)
}
+ if doc.Components == nil || doc.Components.Schemas == nil {
+ return nil, fmt.Errorf("OpenAPI components.schemas not found")
+ }
clusterSpecSchema := doc.Components.Schemas["ClusterSpec"]
- if clusterSpecSchema == nil {
+ if clusterSpecSchema == nil || clusterSpecSchema.Value == nil {
return nil, fmt.Errorf("ClusterSpec schema not found in OpenAPI spec")
}
nodePoolSpecSchema := doc.Components.Schemas["NodePoolSpec"]
- if nodePoolSpecSchema == nil {
+ if nodePoolSpecSchema == nil || nodePoolSpecSchema.Value == nil {
return nil, fmt.Errorf("NodePoolSpec schema not found in OpenAPI spec")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func buildValidator(doc *openapi3.T) (*SchemaValidator, error) { | |
| if err := doc.Validate(context.Background()); err != nil { | |
| return nil, fmt.Errorf("invalid OpenAPI schema: %w", err) | |
| } | |
| // Extract ClusterSpec schema | |
| clusterSpecSchema := doc.Components.Schemas["ClusterSpec"] | |
| if clusterSpecSchema == nil { | |
| return nil, fmt.Errorf("ClusterSpec schema not found in OpenAPI spec") | |
| } | |
| // Extract NodePoolSpec schema | |
| nodePoolSpecSchema := doc.Components.Schemas["NodePoolSpec"] | |
| if nodePoolSpecSchema == nil { | |
| return nil, fmt.Errorf("NodePoolSpec schema not found in OpenAPI spec") | |
| } | |
| // Build schemas map | |
| schemas := map[string]*ResourceSchema{ | |
| "cluster": { | |
| TypeName: "ClusterSpec", | |
| Schema: clusterSpecSchema, | |
| }, | |
| "nodepool": { | |
| TypeName: "NodePoolSpec", | |
| Schema: nodePoolSpecSchema, | |
| }, | |
| } | |
| return &SchemaValidator{ | |
| doc: doc, | |
| schemas: schemas, | |
| doc: doc, | |
| schemas: map[string]*ResourceSchema{ | |
| "cluster": {TypeName: "ClusterSpec", Schema: clusterSpecSchema}, | |
| "nodepool": {TypeName: "NodePoolSpec", Schema: nodePoolSpecSchema}, | |
| }, | |
| func buildValidator(doc *openapi3.T) (*SchemaValidator, error) { | |
| if err := doc.Validate(context.Background()); err != nil { | |
| return nil, fmt.Errorf("invalid OpenAPI schema: %w", err) | |
| } | |
| if doc.Components == nil || doc.Components.Schemas == nil { | |
| return nil, fmt.Errorf("OpenAPI components.schemas not found") | |
| } | |
| clusterSpecSchema := doc.Components.Schemas["ClusterSpec"] | |
| if clusterSpecSchema == nil || clusterSpecSchema.Value == nil { | |
| return nil, fmt.Errorf("ClusterSpec schema not found in OpenAPI spec") | |
| } | |
| nodePoolSpecSchema := doc.Components.Schemas["NodePoolSpec"] | |
| if nodePoolSpecSchema == nil || nodePoolSpecSchema.Value == nil { | |
| return nil, fmt.Errorf("NodePoolSpec schema not found in OpenAPI spec") | |
| } | |
| return &SchemaValidator{ | |
| doc: doc, | |
| schemas: map[string]*ResourceSchema{ | |
| "cluster": {TypeName: "ClusterSpec", Schema: clusterSpecSchema}, | |
| "nodepool": {TypeName: "NodePoolSpec", Schema: nodePoolSpecSchema}, | |
| }, |
🤖 Prompt for 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.
In `@pkg/validators/schema_validator.go` around lines 46 - 66, The buildValidator
function assumes doc.Components and SchemaRef.Value are non-nil; add guards to
return controlled errors instead of panicking: in buildValidator check if
doc.Components == nil and return an error like "components section missing";
when fetching clusterSpecSchema/nodePoolSpecSchema verify both the SchemaRef
pointer and SchemaRef.Value are non-nil before using them and propagate clear
errors; also update the validation logic that uses SchemaRef (e.g., where
SchemaRef.Value is accessed during validation) to nil-check SchemaRef.Value and
return a descriptive error if missing; reference functions/types:
buildValidator, SchemaValidator, ResourceSchema, and the
ClusterSpec/NodePoolSpec SchemaRef.Value fields.
mliptak0
force-pushed
the
HYPERFLEET-1033
branch
2 times, most recently
from
1b38da5 to
a7598e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
docs/api-operator-guide.md (1)
700-700:⚠️ Potential issue | 🟡 Minor | ⚖️ Poor tradeoffDocumentation inconsistent with fail-open implementation.
Line 700 states "Schema validation is always active" but
cmd/hyperfleet-api/server/routes.go(lines 115-121) logs warnings and disables validation when embedded schema read/parse fails. Update this line to reflect the fail-open behavior: "enabled by default from embedded schema" with a note about the fallback.🤖 Prompt for 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. In `@docs/api-operator-guide.md` at line 700, Update the docs to remove the assertion "Schema validation is always active" and instead state that schema validation is "enabled by default from the embedded schema" and add a brief note that on errors reading/parsing the embedded schema (see routes.go which logs warnings and disables validation on failure) the server will fail open and run without validation; change the sentence in docs/api-operator-guide.md accordingly and mirror the fallback behavior described in cmd/hyperfleet-api/server/routes.go.openapi/CLAUDE.md (2)
44-47:⚠️ Potential issue | 🟡 Minor | 💤 Low valueAdd blank line and language identifier to fenced code block.
Static analysis flags two issues: missing blank line before fence (MD031) and missing language tag (MD040).
📝 Proposed fix
For local development (before the spec module is published), the `replace` directive in `go.mod` points to a local checkout: -``` + +```go replace github.com/openshift-hyperfleet/hyperfleet-api-spec => /path/to/hyperfleet-api-spec</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@openapi/CLAUDE.mdaround lines 44 - 47, Add a blank line before the fenced
code block and include the language identifier "go" on the opening fence so the
block containing the replace directive (the line starting with "replace
github.com/openshift-hyperfleet/hyperfleet-api-spec =>
/path/to/hyperfleet-api-spec") becomes a properly fenced and language-tagged Go
block; update the CLAUDE.md snippet accordingly.</details> --- `21-24`: _⚠️ Potential issue_ | _🟡 Minor_ | _💤 Low value_ **Add language identifier to fenced code block.** The code block is missing a language tag. Static analysis (markdownlint MD040) flags this. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```bash make generate # Extracts schema from spec module, then runs oapi-codegen make generate-all # generate + generate-mocks</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@openapi/CLAUDE.mdaround lines 21 - 24, Update the fenced code block in the
CLAUDE.md file that contains the two make commands so it includes a language
identifier; change the opening triple-backticks to include "bash" (i.e. ```bash)
for the block containing "make generate # Extracts schema from spec
module, then runs oapi-codegen" and "make generate-all # generate +
generate-mocks" to satisfy markdownlint MD040.</details> </blockquote></details> <details> <summary>cmd/hyperfleet-api/server/routes.go (1)</summary><blockquote> `113-126`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Fail-fast on embedded schema errors per coding guidelines.** The current implementation logs warnings and disables validation when the embedded schema cannot be read or parsed. Since the schema is embedded in the binary (not external configuration), read/parse failures indicate binary corruption or module integration issues. These should abort startup rather than silently weakening request validation. As per coding guidelines, `cmd/**` requires "Configuration validation at startup (fail-fast)". <details> <summary>🛡️ Proposed fix to fail-fast on schema errors</summary> ```diff ctx := context.Background() schemaData, err := specschemas.FS.ReadFile("core/openapi.yaml") if err != nil { - logger.WithError(ctx, err).Warn("Failed to read embedded OpenAPI schema") - logger.Warn(ctx, "Schema validation is disabled. Spec fields will not be validated.") - } else { + logger.WithError(ctx, err).Fatal("Failed to read embedded OpenAPI schema") + } + schemaValidator, err := validators.NewSchemaValidatorFromData(schemaData) if err != nil { - logger.WithError(ctx, err).Warn("Failed to parse embedded OpenAPI schema") - logger.Warn(ctx, "Schema validation is disabled. Spec fields will not be validated.") - } else { - logger.Info(ctx, "Schema validation enabled from embedded spec module") - router.Use(middleware.SchemaValidationMiddleware(schemaValidator)) - } + logger.WithError(ctx, err).Fatal("Failed to parse embedded OpenAPI schema") } + + logger.Info(ctx, "Schema validation enabled from embedded spec module") + router.Use(middleware.SchemaValidationMiddleware(schemaValidator)) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/server/routes.go` around lines 113 - 126, The code currently swallows failures reading/parsing the embedded schema; instead, make startup fail-fast: when specschemas.FS.ReadFile("core/openapi.yaml") or validators.NewSchemaValidatorFromData(schemaData) returns an error, call the logger with the error and abort startup (e.g., logger.WithError(ctx, err).Fatal(...) or logger.WithError(ctx, err).Error(...) followed by os.Exit(1) / return the error to the caller) rather than logging warnings and continuing; keep the successful path that logs "Schema validation enabled..." and registers router.Use(middleware.SchemaValidationMiddleware(schemaValidator)). ``` </details> </blockquote></details> <details> <summary>pkg/validators/schema_validator.go (1)</summary><blockquote> `46-68`: _⚠️ Potential issue_ | _🔴 Critical_ | _⚡ Quick win_ **Add nil guards to prevent panics on valid OpenAPI specs.** The code assumes `doc.Components` and `SchemaRef.Value` are non-nil after `doc.Validate()` returns, but kin-openapi v0.133.0 defines both as optional pointers. Two reachable panic scenarios: 1. **Lines 51, 56**: A valid OpenAPI spec without a `components:` section has `doc.Components == nil`. Accessing `doc.Components.Schemas` directly will panic. 2. **Line 104**: A `SchemaRef` pointer can exist while its `Value` field is nil, causing `schemaRef.Value.VisitJSON()` to panic. <details> <summary>Proposed fix</summary> ```diff func buildValidator(doc *openapi3.T) (*SchemaValidator, error) { if err := doc.Validate(context.Background()); err != nil { return nil, fmt.Errorf("invalid OpenAPI schema: %w", err) } + + if doc.Components == nil || doc.Components.Schemas == nil { + return nil, fmt.Errorf("OpenAPI spec missing components.schemas section") + } clusterSpecSchema := doc.Components.Schemas["ClusterSpec"] - if clusterSpecSchema == nil { + if clusterSpecSchema == nil || clusterSpecSchema.Value == nil { return nil, fmt.Errorf("ClusterSpec schema not found in OpenAPI spec") } nodePoolSpecSchema := doc.Components.Schemas["NodePoolSpec"] - if nodePoolSpecSchema == nil { + if nodePoolSpecSchema == nil || nodePoolSpecSchema.Value == nil { return nil, fmt.Errorf("NodePoolSpec schema not found in OpenAPI spec") } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/validators/schema_validator.go` around lines 46 - 68, The buildValidator function assumes doc.Components and SchemaRef.Value are non-nil; add explicit nil guards: verify doc.Components != nil after validation and return a clear error if missing, then fetch ClusterSpec and NodePoolSpec as schema refs and ensure each schemaRef != nil and schemaRef.Value != nil (return an error like "ClusterSpec schema missing or empty"); update the SchemaValidator.schemas map to only use validated schemaRefs; also audit places that call SchemaRef.Value.VisitJSON (e.g., where schemaRef.Value is used later) and add nil checks or errors before calling VisitJSON to avoid panics when SchemaRef.Value is nil. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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 @.gitignore:
- Around line 54-55: The .gitignore change hides openapi/openapi.yaml which
downstream consumers (e.g., hyperfleet-sentinel's make generate) still fetch;
restore a compatibility path by ensuring the file remains tracked—either remove
the "/openapi/openapi.yaml" ignore entry or add a negation rule
("!/openapi/openapi.yaml") so the generated OpenAPI spec stays in the repo, and
update any packaging/publishing pipeline if you intend to change where consumers
fetch the spec; reference the .gitignore entry for openapi/openapi.yaml and the
downstream make generate usage in hyperfleet-sentinel to locate the impacted
pieces.In
@Makefile:
- Line 159: The run/docs Makefile target currently mounts openapi/openapi.yaml
which may not exist on a clean checkout; update the run/docs target to depend on
the generate target (or invoke make generate) so openapi/openapi.yaml is
produced before running the container, and keep the docker invocation that uses
$(CONTAINER_TOOL) and the SWAGGER_JSON env pointing to /openapi.yaml (or adjust
the -v to mount the openapi directory after generation). Ensure the target name
run/docs and the prerequisite generate are referenced so the file is created
prior to the docker run command.
Duplicate comments:
In@cmd/hyperfleet-api/server/routes.go:
- Around line 113-126: The code currently swallows failures reading/parsing the
embedded schema; instead, make startup fail-fast: when
specschemas.FS.ReadFile("core/openapi.yaml") or
validators.NewSchemaValidatorFromData(schemaData) returns an error, call the
logger with the error and abort startup (e.g., logger.WithError(ctx,
err).Fatal(...) or logger.WithError(ctx, err).Error(...) followed by os.Exit(1)
/ return the error to the caller) rather than logging warnings and continuing;
keep the successful path that logs "Schema validation enabled..." and registers
router.Use(middleware.SchemaValidationMiddleware(schemaValidator)).In
@docs/api-operator-guide.md:
- Line 700: Update the docs to remove the assertion "Schema validation is always
active" and instead state that schema validation is "enabled by default from the
embedded schema" and add a brief note that on errors reading/parsing the
embedded schema (see routes.go which logs warnings and disables validation on
failure) the server will fail open and run without validation; change the
sentence in docs/api-operator-guide.md accordingly and mirror the fallback
behavior described in cmd/hyperfleet-api/server/routes.go.In
@openapi/CLAUDE.md:
- Around line 44-47: Add a blank line before the fenced code block and include
the language identifier "go" on the opening fence so the block containing the
replace directive (the line starting with "replace
github.com/openshift-hyperfleet/hyperfleet-api-spec =>
/path/to/hyperfleet-api-spec") becomes a properly fenced and language-tagged Go
block; update the CLAUDE.md snippet accordingly.- Around line 21-24: Update the fenced code block in the CLAUDE.md file that
contains the two make commands so it includes a language identifier; change the
opening triple-backticks to include "bash" (i.e. ```bash) for the block
containing "make generate # Extracts schema from spec module, then runs
oapi-codegen" and "make generate-all # generate + generate-mocks" to satisfy
markdownlint MD040.In
@pkg/validators/schema_validator.go:
- Around line 46-68: The buildValidator function assumes doc.Components and
SchemaRef.Value are non-nil; add explicit nil guards: verify doc.Components !=
nil after validation and return a clear error if missing, then fetch ClusterSpec
and NodePoolSpec as schema refs and ensure each schemaRef != nil and
schemaRef.Value != nil (return an error like "ClusterSpec schema missing or
empty"); update the SchemaValidator.schemas map to only use validated
schemaRefs; also audit places that call SchemaRef.Value.VisitJSON (e.g., where
schemaRef.Value is used later) and add nil checks or errors before calling
VisitJSON to avoid panics when SchemaRef.Value is nil.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Central YAML (base), Organization UI (inherited) **Review profile**: ASSERTIVE **Plan**: Enterprise **Run ID**: `3dbdb946-742a-4fcf-a569-ad3a664e685f` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 37414670b64a9907d375b4a2065a0b3179449b15 and a7598e80c11ecd92b9bde45274cc22cc09952329. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (22)</summary> * `.gitignore` * `AGENTS.md` * `CHANGELOG.md` * `CLAUDE.md` * `Dockerfile` * `Makefile` * `cmd/hyperfleet-api/server/routes.go` * `docs/api-operator-guide.md` * `docs/deployment.md` * `go.mod` * `hack/extract-schema.go` * `openapi/CLAUDE.md` * `openapi/openapi.yaml` * `pkg/config/flags.go` * `pkg/config/loader.go` * `pkg/config/server.go` * `pkg/logger/fields.go` * `pkg/middleware/schema_validation_test.go` * `pkg/validators/schema_validator.go` * `test/CLAUDE.md` * `test/integration/clusters_test.go` * `test/integration/integration_test.go` </details> <details> <summary>💤 Files with no reviewable changes (7)</summary> * Dockerfile * pkg/logger/fields.go * test/integration/clusters_test.go * pkg/config/loader.go * openapi/openapi.yaml * pkg/config/flags.go * test/integration/integration_test.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+54
to
+55
| # Ignore extracted OpenAPI spec (generated by make generate from hyperfleet-api-spec module) | ||
| /openapi/openapi.yaml |
There was a problem hiding this comment.
Preserve a compatibility path for downstream OpenAPI consumers
Line 55 makes openapi/openapi.yaml non-tracked, but linked findings show openshift-hyperfleet/hyperfleet-sentinel currently fetches this file from hyperfleet-api during its make generate flow. Without a lockstep sentinel update (or a compatibility publication path), this change can break sentinel code generation.
As per coding guidelines, "**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity. Validate changes against HyperFleet architecture standards from the linked architecture repository."
🤖 Prompt for 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.
In @.gitignore around lines 54 - 55, The .gitignore change hides
openapi/openapi.yaml which downstream consumers (e.g., hyperfleet-sentinel's
make generate) still fetch; restore a compatibility path by ensuring the file
remains tracked—either remove the "/openapi/openapi.yaml" ignore entry or add a
negation rule ("!/openapi/openapi.yaml") so the generated OpenAPI spec stays in
the repo, and update any packaging/publishing pipeline if you intend to change
where consumers fetch the spec; reference the .gitignore entry for
openapi/openapi.yaml and the downstream make generate usage in
hyperfleet-sentinel to locate the impacted pieces.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…idental overwrites
…aw SQL Co-Authored-By: Claude <noreply@anthropic.com>
…aptersConditionMet
status from GET responses
validation to model layer
Remove the OCM client dependency and replace authentication with a self-contained JWT handler using JWKS endpoint validation. - Add pkg/auth/jwt_handler.go with RS256 JWT validation via JWKS - Add pkg/auth/jwt_handler_test.go with full handler test coverage - Remove pkg/client/ocm/ (authorization, authorization_mock, client) - Remove pkg/config/ocm.go and OCM-specific config flags - Remove pkg/logger/ocm_bridge.go (OCM SDK log adapter) - Remove test/mocks/ocm.go (no longer needed) - Update pkg/auth/authz_middleware.go to use new JWT handler - Update pkg/auth/context.go to remove OCM identity types - Update environments to drop OCM env var requirements - Update go.mod/go.sum: add MicahParks/jwkset, keyfunc; remove ocm-sdk-go - Update docs/authentication.md, docs/config.md to reflect new auth setup - Update configs/config.yaml.example with JWT/JWKS config fields
Last OCM reference in the codebase. Safe to rename now since no long-running deployments consume the old variable.
AuthorizationMiddleware was never enabled (mock passthrough only). Removed: interface, mock, Clients struct, ConfigDefaults struct, AuthzConfig, authz error codes, and all threading through routes, plugins, and environments. Updated Helm chart, docs, and Makefile.
…lt SSO URLs Remove vestigial OCM-era configuration that was never used by HyperFleet: - ACL config struct, flags, env bindings, Helm entries, and deprecation warning - Hardcoded Red Hat console CORS origins from api_server.go - Default sso.redhat.com issuer_url and cert_url — now required via validation - Unused jwks_url Makefile variable Add jwt.issuer_url and jwt.audience to Helm chart configmap and values. Update all docs and test configs to use placeholder IdP URLs. Fix Helm values path mismatch in authentication.md.
…pec Go module - Add hyperfleet-api-spec v1.0.11 as a Go module dependency - Extract schema at build time via hack/extract-schema.go; openapi/openapi.yaml is now generated by make generate and is no longer tracked in git - Load embedded schema bytes in routes.go using specschemas.FS instead of a configurable file path - Add NewSchemaValidatorFromData() to schema_validator.go; deprecate NewSchemaValidator() (file-path variant) - Remove server.openapi_schema_path config field, --server-openapi-schema-path CLI flag, and HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH env var - Remove provider-schema integration test (always skipped with embedded schema) - Restore JWT env vars in TestMain that were accidentally removed alongside the schema path setup - Remove unused FieldSchemaPath logger constant Co-Authored-By: Claude <noreply@anthropic.com>
…m module cache Remove hack/extract-schema.go and replace the generate target with a direct cp from the hyperfleet-api-spec module cache, located via go list -m. Add VARIANT ?= core Makefile variable to support core and gcp schema variants. Add download as a prerequisite so make generate works on a fresh clone. Add hack/tools.go to keep the module pinned in go.mod via a blank import with //go:build tools.
Replace references to hack/extract-schema.go with the current approach (cp from module cache via go list -m). Consolidate generation details into openapi/CLAUDE.md as single source of truth; other docs now reference it instead of duplicating content. Fix stale openapi-generator-cli reference in README.md.
mliptak0
force-pushed
the
HYPERFLEET-1033
branch
2 times, most recently
from
714e526 to
d792ea3
Compare
mliptak0
added a commit
to mliptak0/hyperfleet-api
that referenced
this pull request
May 14, 2026
…hift-hyperfleet#155 - Fix CHANGELOG.md PR links: replace openshift-hyperfleet#129 with openshift-hyperfleet#155 on all three bullets - Add init() to routes.go to validate schemaVariant at startup with a clear panic message before reaching the embedded FS read - Fix contradictory Behavior bullets in docs/api-operator-guide.md to reflect the always-on embedded schema model (no fallback mode) - Add shell/go language identifiers to bare fenced code blocks in openapi/CLAUDE.md - Update README.md openapi/ project structure comment to reflect generated artifacts, not source Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)