Fix OpenAPIv2 definitions for dynamic resources #484
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes in this pull request involve updates to various files within the project. Notably, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
pkg/apis/apps/v1alpha1/register.go (1)
77-77: Enhance GVK registration with validation and error handlingThe current implementation could benefit from additional safeguards:
- Validate/deduplicate GVKs before appending
- Consider error handling for registration failures
- Implement bounds checking for the slice growth
Consider refactoring to include these improvements:
func RegisterDynamicTypes(scheme *runtime.Scheme, cfg *config.ResourceConfig) error { + // Pre-allocate slice to avoid multiple growths + if len(RegisteredGVKs) == 0 { + RegisteredGVKs = make([]schema.GroupVersionKind, 0, len(cfg.Resources)) + } for _, res := range cfg.Resources { kind := res.Application.Kind gvk := SchemeGroupVersion.WithKind(kind) + + // Check for duplicates + for _, existing := range RegisteredGVKs { + if existing == gvk { + log.Printf("Warning: Duplicate registration attempt for GVK: %v", gvk) + continue + } + } scheme.AddKnownTypeWithName(gvk, &Application{}) scheme.AddKnownTypeWithName(gvk.GroupVersion().WithKind(kind+"List"), &ApplicationList{}) log.Printf("Registered kind: %s\n", kind) RegisteredGVKs = append(RegisteredGVKs, gvk) } return nil }pkg/cmd/server/start.go (3)
161-161: Use English for code comments to maintain consistencyThe comment on line 161 is in Russian. For consistency and readability, please use English for all code comments.
Apply this diff to update the comment:
-// DeepCopySchema делает глубокую копию структуры spec.Schema +// DeepCopySchema makes a deep copy of the spec.Schema structure
200-260: Use structured logging instead offmt.PrintfCurrently,
fmt.Printfis used for logging messages in thePostProcessSpecfunction (lines 234 and 255). Consider using a standardized logging library likeklogor the logger provided bygenericapiserverto ensure consistent and configurable logging throughout the application.Apply this diff to replace
fmt.Printfwith structured logging:import ( "context" + "github.com/go-logr/logr" "encoding/json" // other imports... ) func (o *AppsServerOptions) Config() (*apiserver.Config, error) { // existing code... + logger := o.GenericConfig.Logger.WithName("PostProcessSpec") serverConfig.OpenAPIConfig.PostProcessSpec = func(swagger *spec.Swagger) (*spec.Swagger, error) { // existing code... - fmt.Printf("PostProcessSpec: Added OpenAPI definition for %s\n", resourceName) + logger.Info("Added OpenAPI definition", "resource", resourceName) // existing code... - fmt.Printf("PostProcessSpec: Added OpenAPI definition for %s\n", listResourceName) + logger.Info("Added OpenAPI definition", "resource", listResourceName) return swagger, nil } // existing code... }Ensure that you initialize the logger appropriately in your server configuration.
215-256: Refactor to eliminate code duplication inPostProcessSpecThe code blocks for processing resources and their list counterparts are very similar, leading to code duplication. Refactoring the repetitive code into helper functions can improve maintainability and readability.
For example, create a helper function to add definitions:
func addDefinition(defs map[string]spec.Schema, baseName string, gvk schema.GroupVersionKind, baseDef *spec.Schema, isList bool) error { newDef, err := DeepCopySchema(baseDef) if err != nil { kindType := "" if isList { kindType = "List" } return fmt.Errorf("failed to deepcopy schema for %s%s: %w", gvk.Kind, kindType, err) } if newDef.Extensions == nil { newDef.Extensions = map[string]interface{}{} } kindName := gvk.Kind if isList { kindName = fmt.Sprintf("%sList", gvk.Kind) } newDef.Extensions["x-kubernetes-group-version-kind"] = []map[string]interface{}{ { "group": gvk.Group, "version": gvk.Version, "kind": kindName, }, } defs[baseName] = *newDef fmt.Printf("PostProcessSpec: Added OpenAPI definition for %s\n", baseName) return nil }Then modify the loop to use this helper:
for _, gvk := range v1alpha1.RegisteredGVKs { resourceName := fmt.Sprintf("com.github.aenix.io.cozystack.pkg.apis.apps.v1alpha1.%s", gvk.Kind) - // Existing code for resource... - // Existing code for list resource... + if err := addDefinition(defs, resourceName, gvk, &appDef, false); err != nil { + return nil, err + } + + listResourceName := fmt.Sprintf("com.github.aenix.io.cozystack.pkg.apis.apps.v1alpha1.%sList", gvk.Kind) + if err := addDefinition(defs, listResourceName, gvk, &listDef, true); err != nil { + return nil, err + } }This refactoring reduces duplication and makes the code easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/system/cozystack-api/values.yaml(1 hunks)pkg/apis/apps/v1alpha1/register.go(2 hunks)pkg/apiserver/apiserver.go(0 hunks)pkg/cmd/server/start.go(5 hunks)
💤 Files with no reviewable changes (1)
- pkg/apiserver/apiserver.go
✅ Files skipped from review due to trivial changes (1)
- packages/system/cozystack-api/values.yaml
🔇 Additional comments (4)
pkg/apis/apps/v1alpha1/register.go (1)
Line range hint 31-77: Verify OpenAPI schema generation integration
Since this change is part of fixing OpenAPIv2 definitions for dynamic resources, we should verify how RegisteredGVKs is used in the OpenAPI schema generation process.
✅ Verification successful
OpenAPI schema generation integration is properly handled
The verification shows that RegisteredGVKs is correctly integrated with OpenAPI schema generation:
- The
RegisteredGVKsslice is used instart.goto dynamically add OpenAPI definitions for each registered GVK - The package has proper OpenAPI generation tags in
doc.go(+k8s:openapi-gen=true) - The generated OpenAPI code in
zz_generated.openapi.goincludes the necessary schema definitions for the Application types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OpenAPI schema generation code that might use RegisteredGVKs
echo "Searching for OpenAPI schema generation code..."
rg -l "openapi|swagger" --type go
# Search for direct usage of RegisteredGVKs
echo -e "\nSearching for RegisteredGVKs usage..."
rg "RegisteredGVKs" --type go
# Search for dynamic type registration patterns
echo -e "\nSearching for dynamic type registration patterns..."
ast-grep --pattern 'func $_($_: *runtime.Scheme, $_) error {
$$$
AddToScheme($_)
$$$
}'
Length of output: 807
Script:
#!/bin/bash
# Check how RegisteredGVKs is used in OpenAPI generation
echo "Checking OpenAPI usage in start.go..."
rg -A 10 "RegisteredGVKs" pkg/cmd/server/start.go
echo -e "\nChecking OpenAPI config in doc.go..."
cat pkg/apis/apps/v1alpha1/doc.go
echo -e "\nChecking generated OpenAPI code..."
head -n 50 pkg/generated/openapi/zz_generated.openapi.go
Length of output: 5720
pkg/cmd/server/start.go (3)
21-21: Import statement is appropriate
The addition of encoding/json is necessary for the DeepCopySchema function to perform JSON marshaling and unmarshaling.
41-41: Necessary import for OpenAPI schema manipulation
Importing k8s.io/kube-openapi/pkg/validation/spec is required for working with spec.Schema in OpenAPI definitions.
186-191: Dynamic type registration implemented correctly
The dynamic types are registered appropriately using v1alpha1.RegisterDynamicTypes. Error handling is correctly implemented.
| var ( | ||
| RegisteredGVKs []schema.GroupVersionKind | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for the exported variable and consider thread-safety
The RegisteredGVKs variable is exported but lacks documentation. Since it's used to track dynamically registered types, it should be documented for API consumers. Additionally, as a global mutable slice, consider thread-safety implications if accessed concurrently.
Add documentation and consider making it thread-safe:
+// RegisteredGVKs contains the GroupVersionKinds of all dynamically registered types.
+// This slice is populated during dynamic type registration and is used for OpenAPI
+// schema generation. Note: This variable is not thread-safe.
var (
RegisteredGVKs []schema.GroupVersionKind
)📝 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.
| var ( | |
| RegisteredGVKs []schema.GroupVersionKind | |
| ) | |
| // RegisteredGVKs contains the GroupVersionKinds of all dynamically registered types. | |
| // This slice is populated during dynamic type registration and is used for OpenAPI | |
| // schema generation. Note: This variable is not thread-safe. | |
| var ( | |
| RegisteredGVKs []schema.GroupVersionKind | |
| ) |
| func DeepCopySchema(schema *spec.Schema) (*spec.Schema, error) { | ||
| data, err := json.Marshal(schema) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal schema: %w", err) | ||
| } | ||
|
|
||
| var newSchema spec.Schema | ||
| err = json.Unmarshal(data, &newSchema) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal schema: %w", err) | ||
| } | ||
|
|
||
| return &newSchema, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using json.Marshal and json.Unmarshal for deep copying
Using json.Marshal and json.Unmarshal to deep copy spec.Schema may lead to inefficient code and could potentially lose unexported fields or metadata. Consider implementing a deep copy function or using an existing library that properly handles deep copying of complex structures.
For example, if spec.Schema supports DeepCopy methods or if you can use runtime.DeepCopy, it would be more appropriate:
func DeepCopySchema(schema *spec.Schema) (*spec.Schema, error) {
- data, err := json.Marshal(schema)
- if err != nil {
- return nil, fmt.Errorf("failed to marshal schema: %w", err)
- }
-
- var newSchema spec.Schema
- err = json.Unmarshal(data, &newSchema)
- if err != nil {
- return nil, fmt.Errorf("failed to unmarshal schema: %w", err)
- }
-
- return &newSchema, nil
+ // Assuming spec.Schema has a DeepCopy method
+ if schema == nil {
+ return nil, fmt.Errorf("input schema is nil")
+ }
+ newSchema := schema.DeepCopy()
+ return newSchema, nil
}If spec.Schema does not have a DeepCopy method, you might consider using a utility function or library that can perform a deep copy efficiently.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pkg/cmd/server/start.go (3)
162-162: Translate comment to English for consistencyThe comment should be in English to maintain consistency across the codebase.
-// DeepCopySchema делает глубокую копию структуры spec.Schema +// DeepCopySchema creates a deep copy of spec.Schema structure
235-235: Adjust log level for definition additionsV(6) is quite verbose for tracking definition additions. Consider using V(4) for better visibility of important initialization steps.
- klog.V(6).Infof("PostProcessSpec: Added OpenAPI definition for %s\n", resourceName) + klog.V(4).Infof("PostProcessSpec: Added OpenAPI definition for %s\n", resourceName) - klog.V(6).Infof("PostProcessSpec: Added OpenAPI definition for %s\n", listResourceName) + klog.V(4).Infof("PostProcessSpec: Added OpenAPI definition for %s\n", listResourceName)Also applies to: 256-256
223-233: Refactor repeated Extensions handlingThe code for handling Extensions is duplicated between resource and list definitions. Consider extracting this into a helper function.
+func setKubernetesGVKExtension(schema *spec.Schema, group, version, kind string) { + if schema.Extensions == nil { + schema.Extensions = map[string]interface{}{} + } + schema.Extensions["x-kubernetes-group-version-kind"] = []map[string]interface{}{ + { + "group": group, + "version": version, + "kind": kind, + }, + } +} // In the loop: -if newDef.Extensions == nil { - newDef.Extensions = map[string]interface{}{} -} -newDef.Extensions["x-kubernetes-group-version-kind"] = []map[string]interface{}{ - { - "group": gvk.Group, - "version": gvk.Version, - "kind": gvk.Kind, - }, -} +setKubernetesGVKExtension(newDef, gvk.Group, gvk.Version, gvk.Kind) // For list resource: -if newListDef.Extensions == nil { - newListDef.Extensions = map[string]interface{}{} -} -newListDef.Extensions["x-kubernetes-group-version-kind"] = []map[string]interface{}{ - { - "group": gvk.Group, - "version": gvk.Version, - "kind": fmt.Sprintf("%sList", gvk.Kind), - }, -} +setKubernetesGVKExtension(newListDef, gvk.Group, gvk.Version, fmt.Sprintf("%sList", gvk.Kind))Also applies to: 244-254
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/apis/apps/v1alpha1/register.go(2 hunks)pkg/cmd/server/start.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/apis/apps/v1alpha1/register.go
🔇 Additional comments (2)
pkg/cmd/server/start.go (2)
187-191: LGTM: Dynamic type registration with proper error handling
The implementation correctly registers dynamic types with appropriate error handling.
216-236: 🛠️ Refactor suggestion
Validate GVK fields before use
The GVK fields (Group, Version, Kind) should be validated before use to prevent potential issues with malformed OpenAPI definitions.
Consider adding validation:
for _, gvk := range v1alpha1.RegisteredGVKs {
+ if gvk.Group == "" || gvk.Version == "" || gvk.Kind == "" {
+ return nil, fmt.Errorf("invalid GVK: group=%q, version=%q, kind=%q", gvk.Group, gvk.Version, gvk.Kind)
+ }
resourceName := fmt.Sprintf("com.github.aenix.io.cozystack.pkg.apis.apps.v1alpha1.%s", gvk.Kind)
| defs[resourceName] = *newDef | ||
| klog.V(6).Infof("PostProcessSpec: Added OpenAPI definition for %s\n", resourceName) | ||
|
|
||
| // List resource |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
v1alpha1API version to simplify server initialization.Chores