Add generic resource type with CRD-driven API #50
Conversation
Implements a generic resource API that allows defining new resource types via YAML configuration files, without requiring code changes. Features: - Generic resource handler with CRUD operations for any resource type - CRD-style resource definitions in config/crds/ - Kubernetes CRDs in Helm chart for API discoverability - RBAC ClusterRoles for viewer and admin access - Database migration for generic resources table - Plugin architecture for resource type registration - ARM64 cross-compilation support in Dockerfile Resource types defined: - Cluster (root-level, cluster-scoped in K8s) - NodePool (owned by Cluster, namespaced in K8s) - IDP (owned by Cluster, namespaced in K8s) The CRDs are for schema discoverability only - PostgreSQL remains the source of truth (not a K8s operator pattern). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove all cluster-specific and nodepool-specific handlers, services, DAOs, presenters, and API types - Remove old database migrations for clusters/node_pools tables - Add migration to drop legacy cluster/nodepool tables - Update CRD registry to load resource definitions from Kubernetes API instead of filesystem (config/crds directory removed) - Add hyperfleet.io annotations to Helm CRDs for API configuration: - hyperfleet.io/scope: Root or Owned - hyperfleet.io/owner-kind: Parent resource kind - hyperfleet.io/owner-path-param: URL path parameter - hyperfleet.io/required-adapters: Comma-separated adapter list - hyperfleet.io/enabled: Enable/disable flag - Update RBAC to allow listing CRDs cluster-wide - Add generic test factory for Resource instances - Update Dockerfile to remove config/crds copy The generic resources table now handles all resource types dynamically based on CRD definitions installed in the Kubernetes cluster. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace static OpenAPI specification with dynamic generation from CRD definitions loaded from Kubernetes. This ensures the API documentation always matches the actual CRD schemas. Changes: - Add pkg/openapi package with generator, paths, schemas, and common modules - Extract openAPIV3Schema from CRDs when loading into registry - Update OpenAPI handler to use dynamic generator - Update schema validator to work with generated specs - Remove static openapi/openapi.yaml and oapi-codegen.yaml - Update Makefile to remove generate target (types are now static) - Update Dockerfile to remove static OpenAPI file copy The generated Go types in pkg/api/openapi/openapi.gen.go are preserved as they are still used for request/response handling throughout the codebase. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add ability to load CRDs from local YAML files via CRD_PATH environment variable, allowing local development without requiring a Kubernetes cluster. Changes: - Add LoadFromDirectory() to pkg/crd/registry.go for loading CRDs from files - Update plugins/resources/plugin.go to check CRD_PATH env var first - Set CRD_PATH in Makefile run/run-no-auth targets - Remove erroneous global list endpoints for owned resources - Update docs/development.md for new CRD-driven workflow - Rename Default() to DefaultRegistry() to avoid gomega conflict Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove references to obsolete make generate and make generate-all - Update README.md installation steps and common commands - Update AGENTS.md development workflow and troubleshooting - Remove global list endpoint from NodePool documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Revert to standard amd64 build instead of ARM64 cross-compilation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
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. |
|
[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 |
WalkthroughThis pull request implements a major architectural shift from static TypeSpec/OpenAPI code generation to a CRD-driven, runtime-generated API model. CRD YAML manifests in Sequence Diagram(s)sequenceDiagram
participant App as Application
participant CRDReg as CRD Registry
participant K8s as Kubernetes API/<br/>Local Filesystem
participant OpenAPI as OpenAPI Generator
participant Router as Route Registrar
participant ResService as Resource Service
participant ResHandler as Resource Handler
participant DB as Database
App->>CRDReg: Load CRDs from K8s/local
CRDReg->>K8s: Fetch CRD manifests
K8s-->>CRDReg: Return CRD definitions
CRDReg-->>App: Registry populated
App->>OpenAPI: GenerateSpec(registry)
OpenAPI->>CRDReg: Retrieve resource definitions
CRDReg-->>OpenAPI: Return definitions (root & owned)
OpenAPI-->>App: Return OpenAPI 3.0 spec
App->>Router: Register routes for each CRD
Router->>ResService: Create ResourceService
ResService-->>Router: Service ready
Router->>ResHandler: Create ResourceHandler per CRD
ResHandler-->>Router: Handler ready
Router-->>App: Routes registered
Note over App: Server ready to accept requests
App->>ResHandler: POST /api/hyperfleet/v1/clusters
ResHandler->>ResService: Create(resource, requiredAdapters)
ResService->>DB: Persist resource
DB-->>ResService: Resource stored
ResService->>App: Return created resource
ResHandler-->>App: HTTP 201 Created
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@ciaranRoche: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Makefile (1)
72-72:⚠️ Potential issue | 🟡 MinorGo version mismatch between Makefile and go.mod.
The Makefile checks for
GO_VERSION:=go1.24.butgo.modspecifiesgo 1.25.0. This will cause the version warning to always trigger during builds.🔧 Suggested fix
# Encourage consistent tool versions -GO_VERSION:=go1.24. +GO_VERSION:=go1.25.AGENTS.md (1)
94-124:⚠️ Potential issue | 🟠 MajorUpdate AGENTS.md project structure to reflect the new generic resource-driven architecture.
The documentation at lines 94-124 references removed cluster/nodepool-specific files (
cluster.go,nodepool.go,cluster_handler.go,nodepool_handler.go,cluster_service.go,nodepool_service.go) that no longer exist. Replace with the current generic resource files:
pkg/dao/resource.go(resource CRUD operations)pkg/handlers/resource.go(resource endpoint handlers)pkg/services/resource.go(resource business operations)pkg/api/resource_definition.go,resource_id.go,resource_types.go(API models)
🤖 Fix all issues with AI agents
In `@charts/crds/cluster-crd.yaml`:
- Around line 60-65: The Condition status enum in the CRD `conditions` schema
only includes "True" and "False" but must also include "Unknown" per Kubernetes
metav1 ConditionStatus; update the enum for the condition status field (the YAML
block with "type: string", "description: Status of the condition", "enum: -
\"True\" - \"False\"") to add "- \"Unknown\"" and apply this same change
consistently to the Cluster CRD, IDP CRD and Nodepool CRD condition schemas.
In `@charts/crds/idp-crd.yaml`:
- Around line 75-81: The Condition "status" enum currently only lists "True" and
"False" which is invalid for Kubernetes conditions; update the enum under the
condition's status field (the "status:" block shown in the CRD) to include
"Unknown" as a third value (so the enum contains "True", "False", and "Unknown")
to allow valid ConditionStatus values and pass CRD validation.
In `@charts/crds/nodepool-crd.yaml`:
- Around line 65-70: Update the CRD's condition status enum to include "Unknown"
so it matches AdapterConditionStatus defined in pkg/openapi/common.go; locate
the status field under conditions in nodepool-crd.yaml and add "Unknown" to the
enum values ("True", "False", "Unknown") to prevent validation mismatches when
adapters report Unknown.
In `@charts/templates/rbac/clusterrole-admin.yaml`:
- Around line 1-35: Add a matching ClusterRoleBinding for the admin ClusterRole:
create a new template that, when .Values.rbac.create is true, defines a
ClusterRoleBinding named {{ include "hyperfleet-api.fullname" . }}-admin (or
similar) with roleRef pointing to the ClusterRole named {{ include
"hyperfleet-api.fullname" . }}-admin (apiGroup rbac.authorization.k8s.io, kind
ClusterRole) and a subject that binds the chart's ServiceAccount (use the chart
helper or value used elsewhere, e.g. {{ include
"hyperfleet-api.serviceAccountName" . }} or .Values.serviceAccount.name) in the
same namespace; follow the existing viewer ClusterRoleBinding structure for
field names and templating patterns.
In `@go.mod`:
- Line 3: Update the Makefile to match go.mod by changing the GO_VERSION
variable (currently referenced as GO_VERSION:=go1.24.) to go1.25., and remove
any BoringCrypto/GOEXPERIMENT=boringcrypto cgo-based build flags; instead add
the new Go 1.25 FIPS options: set the build/time environment to enable GOFIPS140
and ensure runtime uses GODEBUG=fips140=on where appropriate (e.g., in
test/run/build targets that previously used GOEXPERIMENT or BoringCrypto), and
document the change in the Makefile comments so CI and local build targets use
go1.25 and the native FIPS 140-3 strategy.
- Line 61: The project pulls a vulnerable transitive dependency
github.com/dgrijalva/jwt-go via github.com/auth0/go-jwt-middleware v0.x; update
the go.mod to require github.com/auth0/go-jwt-middleware v2.x or later (which
removes the dgrijalva jwt-go transitively), run go get
github.com/auth0/go-jwt-middleware@v2 and go mod tidy to update the lock, and
then adjust any usage sites of the middleware (references to
github.com/auth0/go-jwt-middleware types/constructors such as New or Middleware)
to the v2 API surface if needed so imports and function calls compile. Ensure
the vulnerable github.com/dgrijalva/jwt-go entry is no longer present in
go.sum/go.mod after the upgrade.
In `@pkg/crd/registry.go`:
- Around line 298-304: The current loop builds enumValues by converting
schema.Enum entries using string(e.Raw), which preserves JSON quotes for string
enums; instead unmarshal each e.Raw into a variable (e.g., var v interface{})
using json.Unmarshal and assign that unmarshaled value into enumValues[i] so
strings become plain Go strings and numbers/booleans become their native types;
for robustness handle json.Unmarshal error (fallback to raw string or skip) and
then set result["enum"] = enumValues as before.
In `@pkg/dao/resource.go`:
- Around line 161-181: ResourceService.ListByKind must guard against page size
== 0 and log+return early like generic.go: before calling dao.ListByKind, create
a logger with logger.NewOCMLogger(ctx), check the incoming ListArguments.Size
(or pageSize) == 0 and use the logger to emit a structured info/debug message,
then return an empty api.ResourceList and total 0 without calling
sqlResourceDao.ListByKind; ensure the same behavior and message pattern as the
generic handler (early return when Size==0) so callers passing pageSize=0 don't
receive the full result set.
In `@pkg/db/migrations/202602060001_add_resources.go`:
- Around line 37-40: Rename the polymorphic ownership column from owner_kind to
owner_type in the migration (the diff adding owner_id, owner_kind, owner_href)
and update any references to owner_kind across the codebase/migrations to
owner_type so the schema follows the owner_type + owner_id pattern; ensure the
migration uses owner_type VARCHAR(63) NULL (replacing owner_kind) and verify
related queries, structs, and tests reference owner_type instead of owner_kind.
In `@pkg/db/migrations/migration_structs.go`:
- Around line 29-37: The addResources() migration currently only creates the
resources table and indexes but does not backfill existing data, so
dropClusterNodePoolTables() then deletes legacy records; modify addResources()
(or add a new migration immediately after it but before
dropClusterNodePoolTables()) to perform a data migration that copies rows from
clusters and node_pools into the new resources table (mapping fields and
preserving IDs/relations and any required status/condition data and indexes),
ensure the backfill runs inside the same transactional migration or signals
completion before dropClusterNodePoolTables() executes, and verify by
referencing the MigrationList ordering so dropClusterNodePoolTables() is
deferred until after successful backfill.
In `@pkg/handlers/resource.go`:
- Around line 280-287: The ownerHref is being built with h.plural (child
resource plural) instead of the owner's plural, producing incorrect owner URLs;
update the construction in the h.isOwned block to use the owner's plural (e.g.,
h.ownerPlural) rather than h.plural — if h.ownerPlural doesn't exist, add a
field or derive the correct owner path from h.ownerKind (via a small helper that
maps kind → plural) and set ownerHref = fmt.Sprintf("/api/hyperfleet/v1/%s/%s",
ownerPlural, ownerID); keep resource.OwnerID, resource.OwnerKind and
resource.OwnerHref assignment otherwise unchanged.
- Around line 186-193: The Patch handler currently updates found.Spec (marshal
of patch.Spec) but does not increment the resource's generation; ensure
optimistic concurrency by incrementing found.Generation whenever the spec
changes (e.g., inside the Patch handler right after setting found.Spec) before
calling Replace(), or alternatively increment generation inside the service
Replace() path; update the code path that handles spec updates (look for
patch.Spec, found.Spec and the call to Replace()) so that found.Generation++ (or
equivalent) is applied for every spec mutation and persisted by Replace().
In `@pkg/openapi/generator.go`:
- Around line 69-77: Registry.All() may return an internal slice that must not
be mutated; before the sort.Slice call that sorts defs, make a copy of the slice
(e.g., create a new slice with make and copy the elements) and sort the copy so
you do not mutate registry-owned state; update subsequent use to refer to the
copied slice (defsCopy) instead of defs to avoid races with concurrent readers.
In `@pkg/openapi/paths.go`:
- Around line 306-341: buildCreateStatusOperation currently omits the OpenAPI
security requirement; update the returned *openapi3.Operation in
buildCreateStatusOperation to include the same Security value used by other
endpoints (see buildListOperation/buildCreateOperation) by setting the
Operation.Security field to the shared security requirement (use the same helper
or literal used elsewhere so adapters must authenticate when posting status).
- Around line 233-248: The PATCH operation in the OpenAPI path generator
currently references the CreateRequest schema; update the RequestBody for the
PATCH operation (in the code that builds the operation in pkg/openapi/paths.go
where you use def.Kind) to reference "#/components/schemas/" + def.Kind +
"PatchRequest" instead of CreateRequest, ensure RequestBody.Required is false
(or removed) to allow partial payloads, and make sure a corresponding
{Kind}PatchRequest schema is generated in the components/schemas (matching the
handler's ResourcePatchRequest with optional/omitempty Spec and Labels used by
pkg/handlers/resource.go:162).
- Around line 192-195: The GET-by-ID OpenAPI operation currently includes the
global "search" parameter but the handler uses only the path ID; update
buildGetOperation so the params slice contains only the path parameter by
removing the {Ref: "#/components/parameters/search"} entry (i.e., leave only
buildPathParameter(def.Singular+"_id", def.Kind+" ID") in the params slice).
Ensure buildGetOperation (and any related parameter construction code) no longer
references the search parameter for the single-resource GET.
In `@pkg/openapi/schemas.go`:
- Around line 270-358: The code in applySchemaProperties and
convertCRDSchemaToOpenAPI is silently dropping constraints because it
type-asserts to narrow Go types (e.g., []string, int64) after unmarshalling;
update these functions to accept generic unmarshalled types: convert required
when it's []interface{} to []string and assign to schema.Required; accept
minLength/maxLength as float64 or int (from interface{}), convert to uint64 and
set schema.MinLength/schema.MaxLength (use pointers where needed); handle
additionalProperties when it's a bool or an object (map[string]interface{}) and
set schema.AdditionalProperties accordingly; likewise ensure items/properties
handling remains by converting map[string]interface{} to SchemaRefs. Apply
changes to the functions convertCRDSchemaToOpenAPI and applySchemaProperties and
to fields schema.Required, schema.MinLength, schema.MaxLength,
schema.AdditionalProperties, and schema.Properties/Items conversions so
constraints aren't lost.
In `@pkg/services/resource.go`:
- Around line 111-117: The Replace method currently calls s.resourceDao.Replace
without bumping the resource.Generation; update sqlResourceService.Replace to
load the current resource (via s.resourceDao.Get or equivalent), compare the
incoming resource.Spec with the stored resource.Spec, and if they differ
increment resource.Generation (e.g., resource.Generation++ ) before calling
s.resourceDao.Replace; ensure you handle the case where the existing resource is
not found or Get returns an error and propagate that error through the same
error handling path (use handleUpdateError for Replace failures).
In `@plugins/resources/plugin.go`:
- Around line 159-165: The fallback in getOwnerPlural returns kind+"s" which
preserves uppercase (e.g., "Cluster" -> "Clusters") causing route mismatches;
update the fallback to use lowercase by importing "strings" and returning
strings.ToLower(kind) + "s" in the getOwnerPlural function so API paths become
correct (e.g., "clusters").
- Around line 102-107: Remove the redundant value-type presenter registrations
for api.Resource: delete the calls that register presenters using api.Resource{}
(the value type) — specifically the presenters.RegisterPath(api.Resource{},
"resources") and presenters.RegisterKind(api.Resource{}, "Resource") lines — and
keep only the pointer registrations (presenters.RegisterPath(&api.Resource{},
"resources") and presenters.RegisterKind(&api.Resource{}, "Resource")) so
runtime lookups using &api.Resource{} will match.
🧹 Nitpick comments (13)
scripts/test-api.sh (1)
5-31: Consider failing fast on HTTP errors in the test script.
set -ealone won’t fail on non‑2xxcurlresponses. Addingpipefailand usingcurl -f/-Smakes the script more reliable when the API returns errors.🔧 Suggested change
-set -e +set -euo pipefailAGENTS.md (1)
132-138: Add language identifier to fenced code block.The code block showing the CRD-driven flow lacks a language specifier, which triggers the markdown linter warning.
📝 Suggested fix
-``` +```text charts/crds/*.yaml (CRD definitions) ↓ loaded at startup pkg/crd/registry.go (CRD registry) ↓ generates Dynamic routes + OpenAPI spec at runtime</details> </blockquote></details> <details> <summary>pkg/handlers/resource.go (1)</summary><blockquote> `204-204`: **TODO: Hardcoded user identity should be extracted from auth context.** The `UpdatedBy` field is hardcoded to `"system@hyperfleet.local"`. Similar hardcoding exists at lines 276-277 for `CreatedBy` and `UpdatedBy` in the create path. This is acceptable for initial implementation, but ensure this is tracked for follow-up before production use. Would you like me to open an issue to track implementing proper user identity extraction from the authentication context? </blockquote></details> <details> <summary>test/factories/resources.go (1)</summary><blockquote> `100-104`: **Redundant `OwnerID` assignment.** Line 102 sets `nodePool.OwnerID = &cluster.ID`, but this is already set in `NewNodePoolResource` at line 66 where `OwnerID: &clusterID` is passed. The comment says "for test compatibility" but the value is identical. If specific tests require a different pattern, consider documenting why; otherwise, this assignment is redundant. <details> <summary>♻️ Suggested cleanup</summary> ```diff // Create the nodepool owned by the cluster nodePool, err := f.NewNodePoolResource(id, cluster.ID) if err != nil { return nil, fmt.Errorf("failed to create nodepool: %w", err) } - // Set OwnerID for test compatibility (some tests check this) - nodePool.OwnerID = &cluster.ID - return nodePool, nilcmd/hyperfleet-api/server/routes.go (1)
119-152: Consider caching the generated OpenAPI spec to avoid duplicate generation.The OpenAPI spec is generated twice at startup: once here for schema validation (line 139) and once in
NewOpenAPIHandler()(pkg/handlers/openapi.go). While this only affects startup time, consider caching the generated spec (e.g., on the CRD registry or a shared singleton) to avoid the redundant computation.💡 Sketch of potential consolidation
// In pkg/openapi/generator.go or pkg/crd/registry.go var ( cachedSpec *openapi3.T cachedSpecOnce sync.Once ) func GetOrGenerateSpec(registry *crd.Registry) *openapi3.T { cachedSpecOnce.Do(func() { cachedSpec = GenerateSpec(registry) }) return cachedSpec }Then both
routes.goandopenapi.gocan callopenapi.GetOrGenerateSpec(crd.DefaultRegistry()).pkg/crd/registry_test.go (1)
64-75: Make the empty-directory test deterministic.Relying on
../../binis environment-dependent;t.TempDir()makes this test stable.♻️ Suggested change
func TestLoadFromDirectory_EmptyDirectory(t *testing.T) { RegisterTestingT(t) registry := NewRegistry() - // Use a directory that exists but has no YAML files - err := registry.LoadFromDirectory("../../bin") + emptyDir := t.TempDir() + err := registry.LoadFromDirectory(emptyDir) - // Should succeed but load nothing (bin may not exist, so we just check no panic) - if err == nil { - Expect(registry.Count()).To(Equal(0)) - } + Expect(err).To(BeNil()) + Expect(registry.Count()).To(Equal(0)) }pkg/dao/resource.go (1)
143-150: Consider verifying kind in Delete to prevent cross-resource-type deletions.
Deleteaccepts only an ID and doesn't verify the resource kind. If UUIDs are reused or collide across resource types (unlikely but possible), this could delete the wrong resource. TheDeleteByKindAndIDmethod exists but callers must remember to use it.If intentional for internal use, consider documenting this or making
Deleteprivate.pkg/openapi/generator_test.go (1)
52-79: Consider adding negative test cases.The happy-path tests are solid. Consider adding tests for edge cases:
- Registering an owned resource without its owner
- Duplicate registrations
- Disabled resources (
Enabled: false)pkg/openapi/paths.go (1)
77-81: Silent failure when owner definition is missing could hide configuration errors.When an owned resource's owner definition isn't found in the registry, the function silently returns without generating paths. Consider logging a warning to help diagnose misconfigured CRDs.
Proposed fix
func addOwnedResourcePaths(doc *openapi3.T, def *api.ResourceDefinition, registry *crd.Registry) { ownerDef := getOwnerDefinitionFromRegistry(def, registry) if ownerDef == nil { + // Log warning to help diagnose missing owner CRD + // logger.Warn("Owner definition not found for owned resource", "kind", def.Kind, "owner", def.Owner.Kind) return // Cannot generate paths without owner definition }pkg/api/resource_definition.go (1)
45-52: Potential naming collision withvalidators.ResourceSchema.There's another
ResourceSchematype inpkg/validators/schema_validator.go(lines 12-15) with different fields (TypeName string,Schema *openapi3.SchemaRef). While they're in different packages and won't cause compilation errors, having two types with the same name serving different purposes could cause confusion.Consider renaming one of them (e.g.,
CRDResourceSchemahere orValidatorResourceSchemathere) to improve clarity.pkg/crd/registry.go (1)
70-112: Consider reducing lock scope during Kubernetes API calls.The write lock is held for the entire duration including the Kubernetes API call (line 86), which involves network I/O. While this is acceptable for startup-only loading, if this needs to support runtime reloading in the future, consider:
- Fetching CRDs without the lock
- Processing results
- Acquiring the lock only for the final registration
This is fine for the current use case since loading happens at startup.
pkg/services/resource.go (2)
186-189: Silent error handling may hide issues.When
json.Unmarshalfails (line 187), the adapter status is silently skipped withcontinue. This could mask data corruption or schema mismatches. Consider logging a warning:🔧 Proposed fix
// Unmarshal Conditions from JSONB var conditions []api.AdapterCondition if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err != nil { - continue // Skip if can't unmarshal + logger.WithError(ctx, err).Warn("Failed to unmarshal adapter conditions, skipping") + continue }
264-275: Fragile error handling with string matching.Lines 267-271 mix
errors.Iswith string-based error checking (strings.Contains). This is brittle because:
- Error messages can change
CodeNotFoundGenericin error strings doesn't guarantee the actual error typeConsider refactoring to use typed error checking consistently:
🔧 Proposed fix
existingStatus, findErr := s.adapterStatusDao.FindByResourceAndAdapter( ctx, kind, resourceID, adapterStatus.Adapter, ) - if findErr != nil && !stderrors.Is(findErr, gorm.ErrRecordNotFound) { - if !strings.Contains(findErr.Error(), errors.CodeNotFoundGeneric) { - return nil, errors.GeneralError("Failed to get adapter status: %s", findErr) - } + if findErr != nil && !stderrors.Is(findErr, gorm.ErrRecordNotFound) { + // If it's not a "not found" scenario, it's a real error + return nil, errors.GeneralError("Failed to get adapter status: %s", findErr) }If
FindByResourceAndAdaptercan return a custom "not found" error type, consider having the DAO returngorm.ErrRecordNotFoundor a typed error that can be checked witherrors.Is.
| type: string | ||
| description: Status of the condition | ||
| enum: | ||
| - "True" | ||
| - "False" | ||
| reason: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the specific file
fd "cluster-crd.yaml" --type fRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 107
🏁 Script executed:
# Check the repository structure
git ls-files charts/crds/ | head -20Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 162
🏁 Script executed:
# Read the file around the specified lines
head -80 charts/crds/cluster-crd.yaml | tail -30Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1444
🏁 Script executed:
# Let's also check the broader context to understand the structure
cat -n charts/crds/cluster-crd.yaml | sed -n '50,75p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1403
🏁 Script executed:
# Check other CRD files for condition status patterns
grep -A 5 "enum:" charts/crds/*.yaml | grep -A 5 "status"Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 61
🏁 Script executed:
# Check the other CRD files for condition status patterns
cat -n charts/crds/idp-crd.yaml | grep -A 5 "enum:"Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 593
🏁 Script executed:
# Check nodepool CRD as well
cat -n charts/crds/nodepool-crd.yaml | grep -A 5 "enum:"Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 385
🌐 Web query:
Kubernetes condition status enum "Unknown" ConditionStatus
💡 Result:
In Kubernetes, ConditionStatus is a string “enum” defined in k8s.io/apimachinery/pkg/apis/meta/v1 with exactly three allowed values:
type ConditionStatus string
const (
ConditionTrue ConditionStatus = "True"
ConditionFalse ConditionStatus = "False"
ConditionUnknown ConditionStatus = "Unknown"
)ConditionUnknown means Kubernetes (the condition producer/controller) can’t determine whether the condition is true or false at that time. [1]
Source: metav1.ConditionStatus docs in apimachinery. [1]
[1] (pkg.go.dev)
Citations:
Add Unknown to condition status enums.
Kubernetes ConditionStatus is defined in k8s.io/apimachinery/pkg/apis/meta/v1 with three allowed values: "True", "False", and "Unknown". Controllers use "Unknown" when unable to determine the condition status. The current enum is incomplete and violates Kubernetes standards.
Note: This issue also appears in charts/crds/idp-crd.yaml and charts/crds/nodepool-crd.yaml and should be fixed consistently across all CRDs.
✅ Suggested fix
enum:
- "True"
- "False"
+ - "Unknown"📝 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.
| type: string | |
| description: Status of the condition | |
| enum: | |
| - "True" | |
| - "False" | |
| reason: | |
| type: string | |
| description: Status of the condition | |
| enum: | |
| - "True" | |
| - "False" | |
| - "Unknown" | |
| reason: |
🤖 Prompt for AI Agents
In `@charts/crds/cluster-crd.yaml` around lines 60 - 65, The Condition status enum
in the CRD `conditions` schema only includes "True" and "False" but must also
include "Unknown" per Kubernetes metav1 ConditionStatus; update the enum for the
condition status field (the YAML block with "type: string", "description: Status
of the condition", "enum: - \"True\" - \"False\"") to add "- \"Unknown\"" and
apply this same change consistently to the Cluster CRD, IDP CRD and Nodepool CRD
condition schemas.
| status: | ||
| type: string | ||
| description: Status of the condition | ||
| enum: | ||
| - "True" | ||
| - "False" | ||
| reason: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes ConditionStatus valid values True False Unknown
💡 Result:
Kubernetes ConditionStatus is a string enum with exactly these valid values:
"True""False""Unknown"[1]
Go constants (metav1): metav1.ConditionTrue, metav1.ConditionFalse, metav1.ConditionUnknown. [1]
Source: Kubernetes apimachinery metav1.ConditionStatus. [1]
[1] https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#ConditionStatus
🏁 Script executed:
find . -name "idp-crd.yaml" -type fRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 105
🏁 Script executed:
sed -n '70,85p' ./charts/crds/idp-crd.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 748
Add Unknown to the condition status enum.
Kubernetes ConditionStatus allows exactly three values: True, False, and Unknown. Omitting Unknown will cause valid Kubernetes condition updates to fail CRD validation.
Suggested fix
enum:
- "True"
- "False"
+ - "Unknown"📝 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.
| status: | |
| type: string | |
| description: Status of the condition | |
| enum: | |
| - "True" | |
| - "False" | |
| reason: | |
| status: | |
| type: string | |
| description: Status of the condition | |
| enum: | |
| - "True" | |
| - "False" | |
| - "Unknown" | |
| reason: |
🤖 Prompt for AI Agents
In `@charts/crds/idp-crd.yaml` around lines 75 - 81, The Condition "status" enum
currently only lists "True" and "False" which is invalid for Kubernetes
conditions; update the enum under the condition's status field (the "status:"
block shown in the CRD) to include "Unknown" as a third value (so the enum
contains "True", "False", and "Unknown") to allow valid ConditionStatus values
and pass CRD validation.
| status: | ||
| type: string | ||
| description: Status of the condition | ||
| enum: | ||
| - "True" | ||
| - "False" |
There was a problem hiding this comment.
Consider adding "Unknown" to the status enum for consistency.
The status enum for conditions only includes "True" and "False", but pkg/openapi/common.go defines AdapterConditionStatus with "True", "False", and "Unknown". This mismatch could cause validation failures if adapters report "Unknown" status.
Proposed fix
status:
type: string
description: Status of the condition
enum:
- "True"
- "False"
+ - "Unknown"📝 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.
| status: | |
| type: string | |
| description: Status of the condition | |
| enum: | |
| - "True" | |
| - "False" | |
| status: | |
| type: string | |
| description: Status of the condition | |
| enum: | |
| - "True" | |
| - "False" | |
| - "Unknown" |
🤖 Prompt for AI Agents
In `@charts/crds/nodepool-crd.yaml` around lines 65 - 70, Update the CRD's
condition status enum to include "Unknown" so it matches AdapterConditionStatus
defined in pkg/openapi/common.go; locate the status field under conditions in
nodepool-crd.yaml and add "Unknown" to the enum values ("True", "False",
"Unknown") to prevent validation mismatches when adapters report Unknown.
| {{- if .Values.rbac.create -}} | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: {{ include "hyperfleet-api.fullname" . }}-admin | ||
| labels: | ||
| {{- include "hyperfleet-api.labels" . | nindent 4 }} | ||
| rules: | ||
| # Full CRUD access to HyperFleet resources | ||
| - apiGroups: | ||
| - hyperfleet.io | ||
| resources: | ||
| - clusters | ||
| - nodepools | ||
| - idps | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| # Status subresource update permissions | ||
| - apiGroups: | ||
| - hyperfleet.io | ||
| resources: | ||
| - clusters/status | ||
| - nodepools/status | ||
| - idps/status | ||
| verbs: | ||
| - get | ||
| - patch | ||
| - update | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, explore the RBAC templates directory structure
echo "=== RBAC Templates Directory ==="
find charts/templates/rbac -type f -name "*.yaml" | sort
echo -e "\n=== ClusterRoleBinding Content ==="
if [ -f "charts/templates/rbac/clusterrolebinding.yaml" ]; then
cat -n charts/templates/rbac/clusterrolebinding.yaml
else
echo "clusterrolebinding.yaml not found"
fi
echo -e "\n=== Searching for 'admin' in RBAC yaml files ==="
grep -r "admin" charts/templates/rbac/ --include="*.yaml" -iRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1051
🏁 Script executed:
#!/bin/bash
# Check if clusterrole.yaml is the viewer role to confirm the pattern
echo "=== Content of clusterrole.yaml ==="
head -20 charts/templates/rbac/clusterrole.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 664
Create a ClusterRoleBinding for the admin role to match the viewer role pattern.
The admin ClusterRole is defined but has no corresponding ClusterRoleBinding. The viewer role (crd-viewer) follows a clear pattern: role definition in clusterrole.yaml paired with binding in clusterrolebinding.yaml. The admin role should follow the same pattern with a binding that grants the admin role to the ServiceAccount.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In `@charts/templates/rbac/clusterrole-admin.yaml` around lines 1 - 35, Add a
matching ClusterRoleBinding for the admin ClusterRole: create a new template
that, when .Values.rbac.create is true, defines a ClusterRoleBinding named {{
include "hyperfleet-api.fullname" . }}-admin (or similar) with roleRef pointing
to the ClusterRole named {{ include "hyperfleet-api.fullname" . }}-admin
(apiGroup rbac.authorization.k8s.io, kind ClusterRole) and a subject that binds
the chart's ServiceAccount (use the chart helper or value used elsewhere, e.g.
{{ include "hyperfleet-api.serviceAccountName" . }} or
.Values.serviceAccount.name) in the same namespace; follow the existing viewer
ClusterRoleBinding structure for field names and templating patterns.
| go 1.24.0 | ||
|
|
||
| toolchain go1.24.9 | ||
| go 1.25.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Makefile" -o -name "makefile" | head -20Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 89
🌐 Web query:
Go 1.25 boringcrypto GOEXPERIMENT FIPS support compatibility
💡 Result:
In Go 1.25, GOEXPERIMENT=boringcrypto (Go+BoringCrypto) and FIPS support are essentially two different worlds:
1) Upstream Go (golang.org) FIPS: use the native Go Cryptographic Module (recommended)
Starting in Go 1.24 (and continuing in 1.25), upstream Go ships a pure-Go FIPS 140-3 cryptographic module (crypto/internal/fips140/...). You enable FIPS mode at runtime with GODEBUG=fips140=on (or the stricter only) and you can “pin” the validated module snapshot at build time with GOFIPS140=... (for example v1.0.0). [1][2][3][4]
Practical compatibility point: if your goal is FIPS in Go 1.25, you generally should not be using GOEXPERIMENT=boringcrypto; use GOFIPS140 + GODEBUG=fips140=... instead. [1][3]
2) GOEXPERIMENT=boringcrypto: legacy/unsupported integration (expect compatibility sharp edges)
Go+BoringCrypto is a cgo-based integration with BoringSSL’s FIPS module, historically used internally (notably at Google), and the Go team has explicitly treated it as not supported for external use and is moving away from it (partly due to cgo/FFI pitfalls). [5][6]
Compatibility implications:
- It’s not the same as the new upstream FIPS 140-3 mode (
fips140=on/only) and doesn’t automatically benefit from it. [1][5] - You can run into algorithm/protocol restrictions specific to that BoringCrypto module (for example, longstanding gaps/constraints discussed in Go issue trackers). [6]
3) Microsoft “Go 1.25” toolchain note (if that’s what you meant by Go 1.25)
Microsoft’s Go 1.25 distribution enables GOEXPERIMENT=systemcrypto by default (OpenSSL on Linux, CNG on Windows), which is separate from boringcrypto. You can opt out with GOEXPERIMENT=nosystemcrypto. [7][8]
If you tell me which toolchain you’re actually using (upstream Go vs microsoft/go vs a distro toolchain like RHEL/SUSE), plus your target OS/container constraints (e.g., CGO off, distroless, cross-compile), I can give the exact recommended build/run settings for Go 1.25.
Sources
[1] https://go.dev/blog/fips140
[2] https://go.dev/doc/godebug
[3] https://tip.golang.org/doc/security/fips140
[4] https://tip.golang.org/doc/go1.24
[5] https://go.dev/blog/tob-crypto-audit
[6] golang/go#65321
[7] https://devblogs.microsoft.com/go/microsoft-go-defaults-to-system-crypto/
[8] https://devblogs.microsoft.com/go/go-1-25-0-1-microsoft-build-now-available/
🏁 Script executed:
sed -n '65,80p' ./MakefileRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 629
Update Go version in Makefile and migrate to native FIPS 140-3 strategy for Go 1.25.
The Makefile still references GO_VERSION:=go1.24. (line 72) while go.mod specifies Go 1.25.0. Additionally, Go 1.25 introduces a new native FIPS 140-3 module and deprecates the legacy GOEXPERIMENT=boringcrypto approach previously used. Update the Makefile version to go1.25. and migrate the FIPS build configuration from BoringCrypto (cgo-based) to the new upstream FIPS strategy using GOFIPS140 and GODEBUG=fips140=on at runtime.
🤖 Prompt for AI Agents
In `@go.mod` at line 3, Update the Makefile to match go.mod by changing the
GO_VERSION variable (currently referenced as GO_VERSION:=go1.24.) to go1.25.,
and remove any BoringCrypto/GOEXPERIMENT=boringcrypto cgo-based build flags;
instead add the new Go 1.25 FIPS options: set the build/time environment to
enable GOFIPS140 and ensure runtime uses GODEBUG=fips140=on where appropriate
(e.g., in test/run/build targets that previously used GOEXPERIMENT or
BoringCrypto), and document the change in the Makefile comments so CI and local
build targets use go1.25 and the native FIPS 140-3 strategy.
| // buildCreateStatusOperation creates a POST operation for creating/updating adapter status. | ||
| func buildCreateStatusOperation(def *api.ResourceDefinition, ownerDef *api.ResourceDefinition) *openapi3.Operation { | ||
| operationID := fmt.Sprintf("post%sStatuses", def.Kind) | ||
| summary := "Create or update adapter status" | ||
|
|
||
| params := []*openapi3.ParameterRef{ | ||
| buildPathParameter(def.Singular+"_id", def.Kind+" ID"), | ||
| } | ||
|
|
||
| // Add owner path parameter for owned resources | ||
| if ownerDef != nil { | ||
| params = append([]*openapi3.ParameterRef{ | ||
| buildPathParameter(def.GetOwnerPathParam(), ownerDef.Kind+" ID"), | ||
| }, params...) | ||
| } | ||
|
|
||
| return &openapi3.Operation{ | ||
| OperationID: operationID, | ||
| Summary: summary, | ||
| Description: fmt.Sprintf("Adapter creates or updates its status report for this %s.\nIf adapter already has a status, it will be updated (upsert by adapter name).\n\nResponse includes the full adapter status with all conditions.\nAdapter should call this endpoint every time it evaluates the %s.", def.Singular, def.Singular), | ||
| Parameters: params, | ||
| RequestBody: &openapi3.RequestBodyRef{ | ||
| Value: &openapi3.RequestBody{ | ||
| Required: true, | ||
| Content: openapi3.Content{ | ||
| "application/json": &openapi3.MediaType{ | ||
| Schema: &openapi3.SchemaRef{ | ||
| Ref: "#/components/schemas/AdapterStatusCreateRequest", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Responses: buildStatusCreateResponses(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Security requirement on status create operation.
buildCreateStatusOperation (Line 322-340) doesn't set Security unlike other operations (e.g., buildListOperation at Line 147, buildCreateOperation at Line 183). Adapters posting status updates should likely still require authentication.
Proposed fix
return &openapi3.Operation{
OperationID: operationID,
Summary: summary,
Description: fmt.Sprintf("Adapter creates or updates its status report for this %s.\nIf adapter already has a status, it will be updated (upsert by adapter name).\n\nResponse includes the full adapter status with all conditions.\nAdapter should call this endpoint every time it evaluates the %s.", def.Singular, def.Singular),
Parameters: params,
RequestBody: &openapi3.RequestBodyRef{
Value: &openapi3.RequestBody{
Required: true,
Content: openapi3.Content{
"application/json": &openapi3.MediaType{
Schema: &openapi3.SchemaRef{
Ref: "#/components/schemas/AdapterStatusCreateRequest",
},
},
},
},
},
Responses: buildStatusCreateResponses(),
+ Security: &openapi3.SecurityRequirements{{"BearerAuth": {}}},
}🤖 Prompt for AI Agents
In `@pkg/openapi/paths.go` around lines 306 - 341, buildCreateStatusOperation
currently omits the OpenAPI security requirement; update the returned
*openapi3.Operation in buildCreateStatusOperation to include the same Security
value used by other endpoints (see buildListOperation/buildCreateOperation) by
setting the Operation.Security field to the shared security requirement (use the
same helper or literal used elsewhere so adapters must authenticate when posting
status).
| // applySchemaProperties applies properties from a CRD schema map to an OpenAPI schema. | ||
| func applySchemaProperties(schema *openapi3.Schema, crdSchema map[string]interface{}) { | ||
| if props, ok := crdSchema["properties"].(map[string]interface{}); ok { | ||
| schema.Properties = make(openapi3.Schemas) | ||
| for name, propSchema := range props { | ||
| if propMap, ok := propSchema.(map[string]interface{}); ok { | ||
| schema.Properties[name] = convertCRDSchemaToOpenAPI(propMap) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if required, ok := crdSchema["required"].([]string); ok { | ||
| schema.Required = required | ||
| } | ||
|
|
||
| if desc, ok := crdSchema["description"].(string); ok { | ||
| schema.Description = desc | ||
| } | ||
|
|
||
| // Handle additionalProperties | ||
| if addProps, ok := crdSchema["additionalProperties"].(bool); ok && addProps { | ||
| schema.AdditionalProperties = openapi3.AdditionalProperties{Has: boolPtr(true)} | ||
| } | ||
| } | ||
|
|
||
| // convertCRDSchemaToOpenAPI converts a CRD schema map to an OpenAPI SchemaRef. | ||
| func convertCRDSchemaToOpenAPI(crdSchema map[string]interface{}) *openapi3.SchemaRef { | ||
| schema := &openapi3.Schema{} | ||
|
|
||
| if t, ok := crdSchema["type"].(string); ok { | ||
| schema.Type = &openapi3.Types{t} | ||
| } | ||
|
|
||
| if desc, ok := crdSchema["description"].(string); ok { | ||
| schema.Description = desc | ||
| } | ||
|
|
||
| if format, ok := crdSchema["format"].(string); ok { | ||
| schema.Format = format | ||
| } | ||
|
|
||
| if enum, ok := crdSchema["enum"].([]interface{}); ok { | ||
| schema.Enum = enum | ||
| } | ||
|
|
||
| if min, ok := crdSchema["minimum"].(float64); ok { | ||
| schema.Min = &min | ||
| } | ||
|
|
||
| if max, ok := crdSchema["maximum"].(float64); ok { | ||
| schema.Max = &max | ||
| } | ||
|
|
||
| if minLen, ok := crdSchema["minLength"].(int64); ok { | ||
| schema.MinLength = uint64(minLen) | ||
| } | ||
|
|
||
| if maxLen, ok := crdSchema["maxLength"].(int64); ok { | ||
| uval := uint64(maxLen) | ||
| schema.MaxLength = &uval | ||
| } | ||
|
|
||
| if pattern, ok := crdSchema["pattern"].(string); ok { | ||
| schema.Pattern = pattern | ||
| } | ||
|
|
||
| if props, ok := crdSchema["properties"].(map[string]interface{}); ok { | ||
| schema.Properties = make(openapi3.Schemas) | ||
| for name, propSchema := range props { | ||
| if propMap, ok := propSchema.(map[string]interface{}); ok { | ||
| schema.Properties[name] = convertCRDSchemaToOpenAPI(propMap) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if items, ok := crdSchema["items"].(map[string]interface{}); ok { | ||
| schema.Items = convertCRDSchemaToOpenAPI(items) | ||
| } | ||
|
|
||
| if required, ok := crdSchema["required"].([]string); ok { | ||
| schema.Required = required | ||
| } | ||
|
|
||
| if addProps, ok := crdSchema["additionalProperties"].(bool); ok && addProps { | ||
| schema.AdditionalProperties = openapi3.AdditionalProperties{Has: boolPtr(true)} | ||
| } | ||
|
|
||
| return &openapi3.SchemaRef{Value: schema} | ||
| } |
There was a problem hiding this comment.
CRD schema coercion drops required/length constraints.
When CRD YAML is unmarshaled into map[string]interface{}, required is usually []interface{} and numeric values are often int/float64. The current assertions ([]string, int64) silently skip these constraints, weakening validation.
🛠️ Suggested fix
func applySchemaProperties(schema *openapi3.Schema, crdSchema map[string]interface{}) {
if props, ok := crdSchema["properties"].(map[string]interface{}); ok {
schema.Properties = make(openapi3.Schemas)
for name, propSchema := range props {
if propMap, ok := propSchema.(map[string]interface{}); ok {
schema.Properties[name] = convertCRDSchemaToOpenAPI(propMap)
}
}
}
- if required, ok := crdSchema["required"].([]string); ok {
- schema.Required = required
- }
+ if reqRaw, ok := crdSchema["required"]; ok {
+ if required := toStringSlice(reqRaw); len(required) > 0 {
+ schema.Required = required
+ }
+ }
if desc, ok := crdSchema["description"].(string); ok {
schema.Description = desc
}
@@
func convertCRDSchemaToOpenAPI(crdSchema map[string]interface{}) *openapi3.SchemaRef {
schema := &openapi3.Schema{}
@@
- if minLen, ok := crdSchema["minLength"].(int64); ok {
- schema.MinLength = uint64(minLen)
- }
+ if minLen, ok := toInt64(crdSchema["minLength"]); ok {
+ schema.MinLength = uint64(minLen)
+ }
- if maxLen, ok := crdSchema["maxLength"].(int64); ok {
- uval := uint64(maxLen)
- schema.MaxLength = &uval
- }
+ if maxLen, ok := toInt64(crdSchema["maxLength"]); ok {
+ uval := uint64(maxLen)
+ schema.MaxLength = &uval
+ }
@@
- if required, ok := crdSchema["required"].([]string); ok {
- schema.Required = required
- }
+ if reqRaw, ok := crdSchema["required"]; ok {
+ if required := toStringSlice(reqRaw); len(required) > 0 {
+ schema.Required = required
+ }
+ }
@@
return &openapi3.SchemaRef{Value: schema}
}
+
+func toStringSlice(v interface{}) []string {
+ switch t := v.(type) {
+ case []string:
+ return t
+ case []interface{}:
+ out := make([]string, 0, len(t))
+ for _, item := range t {
+ if s, ok := item.(string); ok {
+ out = append(out, s)
+ }
+ }
+ return out
+ default:
+ return nil
+ }
+}
+
+func toInt64(v interface{}) (int64, bool) {
+ switch t := v.(type) {
+ case int:
+ return int64(t), true
+ case int64:
+ return t, true
+ case float64:
+ return int64(t), true
+ case float32:
+ return int64(t), true
+ default:
+ return 0, false
+ }
+}📝 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.
| // applySchemaProperties applies properties from a CRD schema map to an OpenAPI schema. | |
| func applySchemaProperties(schema *openapi3.Schema, crdSchema map[string]interface{}) { | |
| if props, ok := crdSchema["properties"].(map[string]interface{}); ok { | |
| schema.Properties = make(openapi3.Schemas) | |
| for name, propSchema := range props { | |
| if propMap, ok := propSchema.(map[string]interface{}); ok { | |
| schema.Properties[name] = convertCRDSchemaToOpenAPI(propMap) | |
| } | |
| } | |
| } | |
| if required, ok := crdSchema["required"].([]string); ok { | |
| schema.Required = required | |
| } | |
| if desc, ok := crdSchema["description"].(string); ok { | |
| schema.Description = desc | |
| } | |
| // Handle additionalProperties | |
| if addProps, ok := crdSchema["additionalProperties"].(bool); ok && addProps { | |
| schema.AdditionalProperties = openapi3.AdditionalProperties{Has: boolPtr(true)} | |
| } | |
| } | |
| // convertCRDSchemaToOpenAPI converts a CRD schema map to an OpenAPI SchemaRef. | |
| func convertCRDSchemaToOpenAPI(crdSchema map[string]interface{}) *openapi3.SchemaRef { | |
| schema := &openapi3.Schema{} | |
| if t, ok := crdSchema["type"].(string); ok { | |
| schema.Type = &openapi3.Types{t} | |
| } | |
| if desc, ok := crdSchema["description"].(string); ok { | |
| schema.Description = desc | |
| } | |
| if format, ok := crdSchema["format"].(string); ok { | |
| schema.Format = format | |
| } | |
| if enum, ok := crdSchema["enum"].([]interface{}); ok { | |
| schema.Enum = enum | |
| } | |
| if min, ok := crdSchema["minimum"].(float64); ok { | |
| schema.Min = &min | |
| } | |
| if max, ok := crdSchema["maximum"].(float64); ok { | |
| schema.Max = &max | |
| } | |
| if minLen, ok := crdSchema["minLength"].(int64); ok { | |
| schema.MinLength = uint64(minLen) | |
| } | |
| if maxLen, ok := crdSchema["maxLength"].(int64); ok { | |
| uval := uint64(maxLen) | |
| schema.MaxLength = &uval | |
| } | |
| if pattern, ok := crdSchema["pattern"].(string); ok { | |
| schema.Pattern = pattern | |
| } | |
| if props, ok := crdSchema["properties"].(map[string]interface{}); ok { | |
| schema.Properties = make(openapi3.Schemas) | |
| for name, propSchema := range props { | |
| if propMap, ok := propSchema.(map[string]interface{}); ok { | |
| schema.Properties[name] = convertCRDSchemaToOpenAPI(propMap) | |
| } | |
| } | |
| } | |
| if items, ok := crdSchema["items"].(map[string]interface{}); ok { | |
| schema.Items = convertCRDSchemaToOpenAPI(items) | |
| } | |
| if required, ok := crdSchema["required"].([]string); ok { | |
| schema.Required = required | |
| } | |
| if addProps, ok := crdSchema["additionalProperties"].(bool); ok && addProps { | |
| schema.AdditionalProperties = openapi3.AdditionalProperties{Has: boolPtr(true)} | |
| } | |
| return &openapi3.SchemaRef{Value: schema} | |
| } | |
| // applySchemaProperties applies properties from a CRD schema map to an OpenAPI schema. | |
| func applySchemaProperties(schema *openapi3.Schema, crdSchema map[string]interface{}) { | |
| if props, ok := crdSchema["properties"].(map[string]interface{}); ok { | |
| schema.Properties = make(openapi3.Schemas) | |
| for name, propSchema := range props { | |
| if propMap, ok := propSchema.(map[string]interface{}); ok { | |
| schema.Properties[name] = convertCRDSchemaToOpenAPI(propMap) | |
| } | |
| } | |
| } | |
| if reqRaw, ok := crdSchema["required"]; ok { | |
| if required := toStringSlice(reqRaw); len(required) > 0 { | |
| schema.Required = required | |
| } | |
| } | |
| if desc, ok := crdSchema["description"].(string); ok { | |
| schema.Description = desc | |
| } | |
| // Handle additionalProperties | |
| if addProps, ok := crdSchema["additionalProperties"].(bool); ok && addProps { | |
| schema.AdditionalProperties = openapi3.AdditionalProperties{Has: boolPtr(true)} | |
| } | |
| } | |
| // convertCRDSchemaToOpenAPI converts a CRD schema map to an OpenAPI SchemaRef. | |
| func convertCRDSchemaToOpenAPI(crdSchema map[string]interface{}) *openapi3.SchemaRef { | |
| schema := &openapi3.Schema{} | |
| if t, ok := crdSchema["type"].(string); ok { | |
| schema.Type = &openapi3.Types{t} | |
| } | |
| if desc, ok := crdSchema["description"].(string); ok { | |
| schema.Description = desc | |
| } | |
| if format, ok := crdSchema["format"].(string); ok { | |
| schema.Format = format | |
| } | |
| if enum, ok := crdSchema["enum"].([]interface{}); ok { | |
| schema.Enum = enum | |
| } | |
| if min, ok := crdSchema["minimum"].(float64); ok { | |
| schema.Min = &min | |
| } | |
| if max, ok := crdSchema["maximum"].(float64); ok { | |
| schema.Max = &max | |
| } | |
| if minLen, ok := toInt64(crdSchema["minLength"]); ok { | |
| schema.MinLength = uint64(minLen) | |
| } | |
| if maxLen, ok := toInt64(crdSchema["maxLength"]); ok { | |
| uval := uint64(maxLen) | |
| schema.MaxLength = &uval | |
| } | |
| if pattern, ok := crdSchema["pattern"].(string); ok { | |
| schema.Pattern = pattern | |
| } | |
| if props, ok := crdSchema["properties"].(map[string]interface{}); ok { | |
| schema.Properties = make(openapi3.Schemas) | |
| for name, propSchema := range props { | |
| if propMap, ok := propSchema.(map[string]interface{}); ok { | |
| schema.Properties[name] = convertCRDSchemaToOpenAPI(propMap) | |
| } | |
| } | |
| } | |
| if items, ok := crdSchema["items"].(map[string]interface{}); ok { | |
| schema.Items = convertCRDSchemaToOpenAPI(items) | |
| } | |
| if reqRaw, ok := crdSchema["required"]; ok { | |
| if required := toStringSlice(reqRaw); len(required) > 0 { | |
| schema.Required = required | |
| } | |
| } | |
| if addProps, ok := crdSchema["additionalProperties"].(bool); ok && addProps { | |
| schema.AdditionalProperties = openapi3.AdditionalProperties{Has: boolPtr(true)} | |
| } | |
| return &openapi3.SchemaRef{Value: schema} | |
| } | |
| func toStringSlice(v interface{}) []string { | |
| switch t := v.(type) { | |
| case []string: | |
| return t | |
| case []interface{}: | |
| out := make([]string, 0, len(t)) | |
| for _, item := range t { | |
| if s, ok := item.(string); ok { | |
| out = append(out, s) | |
| } | |
| } | |
| return out | |
| default: | |
| return nil | |
| } | |
| } | |
| func toInt64(v interface{}) (int64, bool) { | |
| switch t := v.(type) { | |
| case int: | |
| return int64(t), true | |
| case int64: | |
| return t, true | |
| case float64: | |
| return int64(t), true | |
| case float32: | |
| return int64(t), true | |
| default: | |
| return 0, false | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/openapi/schemas.go` around lines 270 - 358, The code in
applySchemaProperties and convertCRDSchemaToOpenAPI is silently dropping
constraints because it type-asserts to narrow Go types (e.g., []string, int64)
after unmarshalling; update these functions to accept generic unmarshalled
types: convert required when it's []interface{} to []string and assign to
schema.Required; accept minLength/maxLength as float64 or int (from
interface{}), convert to uint64 and set schema.MinLength/schema.MaxLength (use
pointers where needed); handle additionalProperties when it's a bool or an
object (map[string]interface{}) and set schema.AdditionalProperties accordingly;
likewise ensure items/properties handling remains by converting
map[string]interface{} to SchemaRefs. Apply changes to the functions
convertCRDSchemaToOpenAPI and applySchemaProperties and to fields
schema.Required, schema.MinLength, schema.MaxLength,
schema.AdditionalProperties, and schema.Properties/Items conversions so
constraints aren't lost.
| func (s *sqlResourceService) Replace(ctx context.Context, resource *api.Resource) (*api.Resource, *errors.ServiceError) { | ||
| resource, err := s.resourceDao.Replace(ctx, resource) | ||
| if err != nil { | ||
| return nil, handleUpdateError(resource.Kind, err) | ||
| } | ||
| return resource, nil | ||
| } |
There was a problem hiding this comment.
Missing generation increment on spec update.
The Replace method updates the resource without incrementing the Generation field. Per coding guidelines, the generation should be incremented on each spec update to support optimistic concurrency control with adapters reporting observed_generation.
🔧 Proposed fix
func (s *sqlResourceService) Replace(ctx context.Context, resource *api.Resource) (*api.Resource, *errors.ServiceError) {
+ resource.Generation++
resource, err := s.resourceDao.Replace(ctx, resource)
if err != nil {
return nil, handleUpdateError(resource.Kind, err)
}
return resource, nil
}As per coding guidelines: "Increment the generation field on each spec update and have adapters report observed_generation in status for optimistic concurrency control."
📝 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.
| func (s *sqlResourceService) Replace(ctx context.Context, resource *api.Resource) (*api.Resource, *errors.ServiceError) { | |
| resource, err := s.resourceDao.Replace(ctx, resource) | |
| if err != nil { | |
| return nil, handleUpdateError(resource.Kind, err) | |
| } | |
| return resource, nil | |
| } | |
| func (s *sqlResourceService) Replace(ctx context.Context, resource *api.Resource) (*api.Resource, *errors.ServiceError) { | |
| resource.Generation++ | |
| resource, err := s.resourceDao.Replace(ctx, resource) | |
| if err != nil { | |
| return nil, handleUpdateError(resource.Kind, err) | |
| } | |
| return resource, nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/services/resource.go` around lines 111 - 117, The Replace method
currently calls s.resourceDao.Replace without bumping the resource.Generation;
update sqlResourceService.Replace to load the current resource (via
s.resourceDao.Get or equivalent), compare the incoming resource.Spec with the
stored resource.Spec, and if they differ increment resource.Generation (e.g.,
resource.Generation++ ) before calling s.resourceDao.Replace; ensure you handle
the case where the existing resource is not found or Get returns an error and
propagate that error through the same error handling path (use handleUpdateError
for Replace failures).
| // Presenter registration for Resource type | ||
| presenters.RegisterPath(api.Resource{}, "resources") | ||
| presenters.RegisterPath(&api.Resource{}, "resources") | ||
| presenters.RegisterKind(api.Resource{}, "Resource") | ||
| presenters.RegisterKind(&api.Resource{}, "Resource") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=go 'func Register(Path|Kind)' -A 15Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1808
🏁 Script executed:
rg -nP --type=go 'LoadDiscoveredPaths|LoadDiscoveredKinds' -B 2 -A 2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1287
🏁 Script executed:
rg -nP --type=go 'api\.Resource' plugins/resources/ -B 2 -A 2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 926
🏁 Script executed:
rg -nP --type=go 'path\(|ObjectKind\(' -A 2 -B 2 | head -50Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1139
🏁 Script executed:
rg -nP --type=go 'api\.Resource' -A 3 -B 1 | grep -A 3 -B 1 'ObjectPath\|ObjectKind\|path\|Kind'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 16110
🏁 Script executed:
rg -nP --type=go 'api\.Resource\{[^&]' | head -20Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 958
Remove redundant value-type presenter registrations.
Lines 103 and 105 register presenters for api.Resource{} (value type), but the codebase always instantiates api.Resource as a pointer (e.g., &api.Resource{} in handlers). Since fmt.Sprintf("%T", obj) produces different type names for values vs. pointers ("api.Resource" vs. "*api.Resource"), and registry lookups use the runtime value's type, only the pointer registrations (lines 104, 106) will ever be matched. Remove lines 103 and 105.
🤖 Prompt for AI Agents
In `@plugins/resources/plugin.go` around lines 102 - 107, Remove the redundant
value-type presenter registrations for api.Resource: delete the calls that
register presenters using api.Resource{} (the value type) — specifically the
presenters.RegisterPath(api.Resource{}, "resources") and
presenters.RegisterKind(api.Resource{}, "Resource") lines — and keep only the
pointer registrations (presenters.RegisterPath(&api.Resource{}, "resources") and
presenters.RegisterKind(&api.Resource{}, "Resource")) so runtime lookups using
&api.Resource{} will match.
| func getOwnerPlural(kind string) string { | ||
| if def, found := crd.GetByKind(kind); found { | ||
| return def.Plural | ||
| } | ||
| // Fallback: lowercase + "s" | ||
| return kind + "s" | ||
| } |
There was a problem hiding this comment.
Fallback in getOwnerPlural produces incorrect casing.
The fallback kind + "s" (Line 164) would produce "Clusters" for kind "Cluster", but API paths use lowercase (clusters). This could cause route mismatches for owner resources not in the registry.
Proposed fix
func getOwnerPlural(kind string) string {
if def, found := crd.GetByKind(kind); found {
return def.Plural
}
- // Fallback: lowercase + "s"
- return kind + "s"
+ // Fallback: lowercase + "s"
+ return strings.ToLower(kind) + "s"
}Add import:
import "strings"📝 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.
| func getOwnerPlural(kind string) string { | |
| if def, found := crd.GetByKind(kind); found { | |
| return def.Plural | |
| } | |
| // Fallback: lowercase + "s" | |
| return kind + "s" | |
| } | |
| func getOwnerPlural(kind string) string { | |
| if def, found := crd.GetByKind(kind); found { | |
| return def.Plural | |
| } | |
| // Fallback: lowercase + "s" | |
| return strings.ToLower(kind) + "s" | |
| } |
🤖 Prompt for AI Agents
In `@plugins/resources/plugin.go` around lines 159 - 165, The fallback in
getOwnerPlural returns kind+"s" which preserves uppercase (e.g., "Cluster" ->
"Clusters") causing route mismatches; update the fallback to use lowercase by
importing "strings" and returning strings.ToLower(kind) + "s" in the
getOwnerPlural function so API paths become correct (e.g., "clusters").
2 participants
Summary
Changes
CRD-Driven API
charts/crds/CRD_PATHenv var) and Kubernetes APILocal Development
LoadFromDirectory()topkg/crd/registry.gofor loading CRDs from local filesmake runandmake run-no-authto setCRD_PATHautomaticallyBug Fixes
image-devtarget to use linux/amd64 platformDocumentation
README.md,AGENTS.md, anddocs/development.mdfor new workflowmake generatecommandsTest plan
make testpasses (319 tests)make run-no-authstarts server with CRDs loaded from local files/api/hyperfleet/v1/openapi.html🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Architecture Changes