feat: Add auto-provisioning of PostgreSQL database #281
Conversation
WalkthroughAdds optional in-cluster Postgres auto-provisioning via spec.postgres.generateDeployment across APIs, CRDs, webhooks, controller reconciliation (Secret, PVC, Deployment, Service), templates, samples, conversions/deepcopy, RBAC, and tests; introduces a manual conversion function and registers conversion handling for ModelRegistrySpec. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API_Server as K8s API
participant Controller
participant Cluster as K8s_Resources
User->>API_Server: Create ModelRegistry with spec.postgres.generateDeployment=true
API_Server->>Controller: Reconcile(ModelRegistry)
Controller->>Controller: Check spec.postgres.generateDeployment
alt generateDeployment == true
Controller->>Cluster: Ensure Secret (credentials)
Controller->>Cluster: Ensure PVC (postgres storage)
Controller->>Cluster: Ensure Deployment (postgres)
Controller->>Cluster: Ensure Service (postgres)
Controller->>API_Server: Update ModelRegistry in-memory spec with Host/Port/credentials
else
Controller->>Controller: Skip postgres provisioning
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Chanakya-TS
changed the title
![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 comments (1)
api/v1beta1/modelregistry_types.go (1)
251-251: Update outdated comment to reflect auto-provisioning option.The comment states that one of
postgresormysqlconfigurations MUST be provided, but this is no longer accurate with the new auto-provisioning feature.Update the comment to reflect the new options:
-// One of `postgres` or `mysql` database configurations MUST be provided! +// One of `postgres`, `mysql` database configurations, or `database.generate: true` MUST be provided!
🧹 Nitpick comments (4)
config/samples/postgres-auto/kustomization.yaml (1)
4-4: Fix YAML formatting issues.The file has formatting issues that should be corrected:
- Missing newline at end of file
- Trailing spaces
Apply this diff to fix the formatting:
-resources: -- modelregistry_v1beta1_modelregistry.yaml +resources: +- modelregistry_v1beta1_modelregistry.yaml +config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml (1)
11-11: Fix YAML formatting issues.The file has the same formatting issues as the kustomization file:
- Missing newline at end of file
- Trailing spaces
Apply this diff to fix the formatting:
- grpc: - port: 9090 + grpc: + port: 9090 +internal/controller/config/templates/postgres-deployment.yaml.tmpl (2)
20-20: Consider using a more recent PostgreSQL version.The
postgres:13image is quite old. Consider using a more recent version likepostgres:15orpostgres:16for better security and performance.- image: postgres:13 + image: postgres:15
18-41: Consider adding resource limits and health checks.The PostgreSQL container lacks:
- Resource requests/limits for proper resource management
- Readiness and liveness probes for health monitoring
containers: - name: postgres image: postgres:13 + resources: + requests: + memory: "256Mi" + cpu: "250m" + limits: + memory: "512Mi" + cpu: "500m" ports: - containerPort: 5432 + readinessProbe: + exec: + command: + - /bin/sh + - -c + - pg_isready -U $POSTGRES_USER -d $POSTGRES_DB + initialDelaySeconds: 15 + periodSeconds: 5 + livenessProbe: + exec: + command: + - /bin/sh + - -c + - pg_isready -U $POSTGRES_USER -d $POSTGRES_DB + initialDelaySeconds: 45 + periodSeconds: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
api/v1alpha1/zz_generated.manual.conversion.go(1 hunks)api/v1beta1/modelregistry_types.go(2 hunks)api/v1beta1/modelregistry_webhook.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(1 hunks)config/manager/kustomization.yaml(1 hunks)config/rbac/role.yaml(1 hunks)config/samples/postgres-auto/kustomization.yaml(1 hunks)config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml(1 hunks)internal/controller/config/templates/postgres-deployment.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-service.yaml.tmpl(1 hunks)internal/controller/modelregistry_controller.go(3 hunks)internal/controller/modelregistry_controller_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tarilabs
PR: opendatahub-io/model-registry-operator#269
File: .github/workflows/build-image-pr.yml:49-52
Timestamp: 2025-07-07T12:31:29.552Z
Learning: In the model-registry-operator project, the AppArmor profile removal for MySQL 8.x in KinD on GHA is a critical requirement. The CI job should fail if the profile cannot be removed, rather than continuing with a potentially broken setup, as this ensures proper MySQL deployment functionality.
internal/controller/modelregistry_controller_test.go (1)
Learnt from: tarilabs
PR: #269
File: .github/workflows/build-image-pr.yml:49-52
Timestamp: 2025-07-07T12:31:29.552Z
Learning: In the model-registry-operator project, the AppArmor profile removal for MySQL 8.x in KinD on GHA is a critical requirement. The CI job should fail if the profile cannot be removed, rather than continuing with a potentially broken setup, as this ensures proper MySQL deployment functionality.
🧬 Code Graph Analysis (3)
internal/controller/modelregistry_controller_test.go (1)
api/v1beta1/modelregistry_types.go (1)
DatabaseSpec(196-200)
api/v1alpha1/zz_generated.manual.conversion.go (1)
api/v1alpha1/zz_generated.conversion.go (2)
Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec(305-307)autoConvert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec(289-302)
internal/controller/modelregistry_controller.go (1)
api/v1beta1/modelregistry_types.go (3)
ModelRegistry(323-329)PostgresConfig(46-96)SecretKeyValue(37-44)
🪛 YAMLlint (1.37.1)
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
[error] 11-11: trailing spaces
(trailing-spaces)
config/samples/postgres-auto/kustomization.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
[error] 4-4: trailing spaces
(trailing-spaces)
🔇 Additional comments (13)
config/rbac/role.yaml (1)
27-27: LGTM! Necessary RBAC permission for PostgreSQL credential management.The addition of "secrets" resource permission is correctly placed and essential for the auto-provisioning feature to manage PostgreSQL credentials.
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml (1)
1-11: LGTM! Sample correctly demonstrates the auto-provisioning feature.The ModelRegistry configuration properly showcases the new
database.generate: truefunctionality with appropriate port settings.api/v1beta1/modelregistry_types.go (2)
196-200: LGTM! Well-designed DatabaseSpec struct following Kubernetes conventions.The new
DatabaseSpecwith theGeneratefield is properly implemented:
- Uses boolean pointer for optional field semantics
- Appropriate kubebuilder annotations
- Clear documentation
266-268: LGTM! Database field properly integrated into ModelRegistrySpec.The new
Databasefield is correctly added as an optional pointer field with appropriate placement and documentation.config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml (1)
891-898: LGTM! Schema properly defines the new database auto-provisioning feature.The CRD schema correctly adds the new optional
databaseproperty with agenerateboolean field that defaults tofalse. This maintains backward compatibility while enabling the new PostgreSQL auto-provisioning functionality.api/v1beta1/modelregistry_webhook.go (1)
194-200: LGTM! Validation logic correctly handles the new auto-provisioning option.The updated validation properly allows bypassing the postgres/mysql requirement when
database.generateis set to true, and the error messages clearly indicate this exception.internal/controller/config/templates/postgres-service.yaml.tmpl (1)
1-12: LGTM! Service template is properly configured for PostgreSQL.The template correctly defines a Kubernetes Service that:
- Uses appropriate template variables for dynamic naming
- Selects PostgreSQL pods with the correct label selector
- Exposes the standard PostgreSQL port 5432
api/v1alpha1/zz_generated.manual.conversion.go (1)
8-15: LGTM! Conversion function properly handles API versioning.The manual conversion function correctly:
- Delegates to the autogenerated conversion for common fields
- Explicitly ignores the
Databasefield since it doesn't exist in v1alpha1- Includes clear documentation explaining the field exclusion
This is the proper approach for maintaining backward compatibility during API evolution.
internal/controller/config/templates/postgres-deployment.yaml.tmpl (1)
40-41: Data persistence concern with emptyDir volume.Using
emptyDir: {}means PostgreSQL data will be lost when the pod restarts. This could be problematic for a database used in production scenarios.Consider adding configuration options for persistent storage or at least document this limitation clearly for users.
Is this intentional for development/testing purposes only, or should we consider adding persistence options?
internal/controller/modelregistry_controller_test.go (1)
303-337: LGTM! Well-structured test for auto-provisioning feature.The test case properly validates the auto-provisioning functionality by:
- Correctly setting up the
Database.Generateflag- Reusing the existing validation framework with
validateRegistryBase()- Specifically verifying creation of all three PostgreSQL resources (Secret, Deployment, Service)
- Using consistent naming conventions and appropriate timeouts
The implementation follows established testing patterns in the codebase.
internal/controller/modelregistry_controller.go (3)
335-335: RBAC update correctly grants Secret permissions.The addition of "secrets" to the core resource permissions is necessary for the controller to manage PostgreSQL credentials when auto-provisioning is enabled.
359-364: Proper conditional logic for database auto-provisioning.The implementation correctly checks for the
Database.Generateflag with appropriate nil safety and follows the established error handling pattern in the method.
451-532: Well-structured method for PostgreSQL auto-provisioning.The
createOrUpdatePostgresmethod follows established patterns in the codebase with:
- Consistent use of the template system for resource creation
- Proper error handling with early returns and logging
- Correct controller reference setting for resource ownership
- Appropriate in-memory spec updates for subsequent reconciliation steps
The method integrates well with the existing controller architecture.
| StringData: map[string]string{ | ||
| "username": "postgres", | ||
| "password": "password", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using randomly generated credentials for better security.
The hardcoded username "postgres" and password "password" pose security risks. For production deployments, consider generating random credentials to improve security posture.
- StringData: map[string]string{
- "username": "postgres",
- "password": "password",
- },
+ StringData: map[string]string{
+ "username": "postgres",
+ "password": generateRandomPassword(),
+ },Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/controller/modelregistry_controller.go around lines 468 to 471, the
username and password are hardcoded as "postgres" and "password", which is
insecure. Replace these hardcoded credentials with randomly generated username
and password values. Use a secure random generator to create these credentials
at runtime and assign them to the StringData map to enhance security.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml (1)
1-11: Fix YAML formatting issues.The sample correctly demonstrates the auto-provisioning feature, but there are formatting issues that should be addressed.
rest: port: 8080 grpc: - port: 9090 + port: 9090 +api/v1beta1/modelregistry_webhook_test.go (1)
86-104: Consider adding more comprehensive test coverage for auto-provisioning validation.While the current test cases cover the basic scenarios, consider adding test cases for other conflicting fields mentioned in the validation logic:
- Auto-provisioning with
HostAddressset- Auto-provisioning with
Databaseset- Auto-provisioning with
UsernamesetThis would provide more thorough coverage of the validation logic implemented in the webhook.
+ { + name: "invalid - postgres auto-provisioning with hostAddress", + mrSpec: &v1beta1.ModelRegistry{Spec: v1beta1.ModelRegistrySpec{ + Postgres: &v1beta1.PostgresConfig{ + Generate: &trueValue, + HostAddress: "192.168.1.1", + }, + }}, + wantErr: true, + }, + { + name: "invalid - postgres auto-provisioning with database", + mrSpec: &v1beta1.ModelRegistry{Spec: v1beta1.ModelRegistrySpec{ + Postgres: &v1beta1.PostgresConfig{ + Generate: &trueValue, + Database: "mydb", + }, + }}, + wantErr: true, + }, + { + name: "invalid - postgres auto-provisioning with username", + mrSpec: &v1beta1.ModelRegistry{Spec: v1beta1.ModelRegistrySpec{ + Postgres: &v1beta1.PostgresConfig{ + Generate: &trueValue, + Username: "myuser", + }, + }}, + wantErr: true, + },api/v1alpha1/modelregistry_webhook.go (1)
255-272: Consider consolidating validation errors for better user experience.Currently, the validation returns immediately on the first conflicting field found. Consider collecting all validation errors and returning them together to provide users with a complete picture of what needs to be fixed.
- if hasAutoProvisioning { - // When auto-provisioning is enabled, other postgres fields should not be set - if len(r.Spec.Postgres.Host) > 0 || len(r.Spec.Postgres.HostAddress) > 0 { - return nil, field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "host should not be set when auto-provisioning is enabled"), - } - } - if len(r.Spec.Postgres.Database) > 0 { - return nil, field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("postgres").Child("database"), r.Spec.Postgres.Database, "database should not be set when auto-provisioning is enabled"), - } - } - if len(r.Spec.Postgres.Username) > 0 { - return nil, field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("postgres").Child("username"), r.Spec.Postgres.Username, "username should not be set when auto-provisioning is enabled"), - } - } - } + if hasAutoProvisioning { + var errList field.ErrorList + // When auto-provisioning is enabled, other postgres fields should not be set + if len(r.Spec.Postgres.Host) > 0 || len(r.Spec.Postgres.HostAddress) > 0 { + errList = append(errList, field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "host should not be set when auto-provisioning is enabled")) + } + if len(r.Spec.Postgres.Database) > 0 { + errList = append(errList, field.Invalid(field.NewPath("spec").Child("postgres").Child("database"), r.Spec.Postgres.Database, "database should not be set when auto-provisioning is enabled")) + } + if len(r.Spec.Postgres.Username) > 0 { + errList = append(errList, field.Invalid(field.NewPath("spec").Child("postgres").Child("username"), r.Spec.Postgres.Username, "username should not be set when auto-provisioning is enabled")) + } + if len(errList) > 0 { + return nil, errList + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/v1alpha1/modelregistry_types.go(1 hunks)api/v1alpha1/modelregistry_webhook.go(2 hunks)api/v1alpha1/zz_generated.conversion.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/v1alpha1/zz_generated.manual.conversion.go(1 hunks)api/v1beta1/modelregistry_types.go(1 hunks)api/v1beta1/modelregistry_webhook.go(2 hunks)api/v1beta1/modelregistry_webhook_test.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(2 hunks)config/manager/kustomization.yaml(1 hunks)config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml(1 hunks)internal/controller/modelregistry_controller.go(3 hunks)internal/controller/modelregistry_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- config/manager/kustomization.yaml
- internal/controller/modelregistry_controller_test.go
- api/v1beta1/modelregistry_types.go
- api/v1alpha1/zz_generated.manual.conversion.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
- api/v1beta1/modelregistry_webhook.go
- internal/controller/modelregistry_controller.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tarilabs
PR: opendatahub-io/model-registry-operator#269
File: .github/workflows/build-image-pr.yml:49-52
Timestamp: 2025-07-07T12:31:29.552Z
Learning: In the model-registry-operator project, the AppArmor profile removal for MySQL 8.x in KinD on GHA is a critical requirement. The CI job should fail if the profile cannot be removed, rather than continuing with a potentially broken setup, as this ensures proper MySQL deployment functionality.
🧬 Code Graph Analysis (2)
api/v1alpha1/zz_generated.conversion.go (3)
api/v1alpha1/modelregistry_types.go (1)
ModelRegistrySpec(369-409)api/v1beta1/modelregistry_types.go (1)
ModelRegistrySpec(248-284)api/v1alpha1/zz_generated.manual.conversion.go (1)
Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec(8-15)
api/v1beta1/modelregistry_webhook_test.go (1)
api/v1beta1/modelregistry_types.go (3)
ModelRegistry(315-321)ModelRegistrySpec(248-284)PostgresConfig(46-98)
🪛 YAMLlint (1.37.1)
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
[error] 11-11: trailing spaces
(trailing-spaces)
🔇 Additional comments (12)
api/v1beta1/zz_generated.deepcopy.go (1)
310-314: LGTM! Correct deepcopy implementation for the new Generate field.The autogenerated deepcopy logic properly handles the new optional
Generateboolean pointer field by checking for nil, allocating a new pointer, and copying the value. This follows the established pattern used by other optional fields in the struct.api/v1alpha1/zz_generated.conversion.go (3)
145-149: LGTM! Correct switch to manual conversion registration.The change from
AddGeneratedConversionFunctoAddConversionFuncis correct and aligns with the manual conversion function provided inapi/v1alpha1/zz_generated.manual.conversion.gothat handles the version differences between v1beta1 and v1alpha1 (specifically ignoring the Database field that doesn't exist in v1alpha1).
416-416: LGTM! Proper conversion logic for the Generate field.The conversion logic correctly handles the new
Generatefield using unsafe pointer conversion, which is the standard pattern for autogenerated conversion code in Kubernetes.
439-439: LGTM! Bidirectional conversion support for Generate field.The reverse direction conversion logic is correctly implemented, ensuring the
Generatefield is properly converted in both directions between API versions.api/v1alpha1/zz_generated.deepcopy.go (1)
374-378: LGTM! Consistent deepcopy implementation across API versions.The autogenerated deepcopy logic for the
Generatefield correctly follows the same pattern as the v1beta1 version, ensuring consistency across API versions while properly handling the optional boolean pointer field.api/v1alpha1/modelregistry_types.go (2)
65-65: LGTM! Appropriate change to make Database field optional.Making the
Databasefield optional withomitemptyis correct for the auto-provisioning feature, where the database name may be automatically generated whenGenerateis true. This aligns with similar changes made in the v1beta1 API version.
72-74: LGTM! Well-documented Generate field addition.The new
Generatefield is properly implemented as an optional boolean pointer with appropriate kubebuilder annotations (default false) and clear documentation explaining its purpose for auto-provisioning PostgreSQL databases.api/v1beta1/modelregistry_webhook_test.go (3)
66-66: LGTM! Good practice using a helper variable.Using
trueValuevariable for the boolean pointer is a clean approach that avoids inline pointer creation and improves readability.
86-94: LGTM! Valid test case for auto-provisioning scenario.This test case correctly validates that when
Generateis set totruewithout any conflicting fields, the validation should pass.
95-104: LGTM! Good negative test case for conflicting configuration.This test case correctly validates that when auto-provisioning is enabled (
Generate: true), setting aHostshould result in a validation error, which aligns with the webhook validation logic.api/v1alpha1/modelregistry_webhook.go (2)
63-69: LGTM! Proper preservation of Postgres config for auto-provisioning.The logic correctly checks if auto-provisioning is enabled before removing the Postgres configuration. The nil check for
Generatefield and the boolean dereference are handled safely.
244-273: LGTM! Comprehensive validation logic for auto-provisioning.The validation logic properly:
- Checks for the presence of database configurations
- Validates that conflicting fields (Host, HostAddress, Database, Username) are not set when auto-provisioning is enabled
- Provides clear error messages for each validation failure
The implementation is consistent with the expected behavior described in the PR objectives.
| "password": "password", | ||
| }, | ||
| } | ||
| if err = ctrl.SetControllerReference(registry, &secret, r.Scheme); err != nil { |
There was a problem hiding this comment.
won't this make it so the DB is deleted if the MR instance is deleted? what if a user wants to persist the DB in the cluster?
There was a problem hiding this comment.
Thank you for pointing that out. I added a persist flag (which is false by default), which the user can use to make sure the DB isn't deleted when the MR instance is deleted.
Also do we want to use a persistent volume to store the DB data or does it not matter because this will only be used for "testing out" mr?
There was a problem hiding this comment.
Default to not delete any data. I think that's an issue with this approach to use Postgres for test MR instance. imho, we should default to something simple like SQLite DB instead for dev MR instances.
| spec: | ||
| containers: | ||
| - name: postgres | ||
| image: postgres:13 |
There was a problem hiding this comment.
this might need to be modified once we go into downstream, as this syntax will pull from docker and might fail because of rate limiting (I also think we required all images to be pinned by SHA, not sure about templates though). IMHO it would be nicer to avoid it in ODH as well, in my experience we get rate limited constantly.
There was a problem hiding this comment.
I updated it to pull from quay.io/sclorg/postgresql-13-c9s:latest, is this the correct one or should i use something else? Also which version of postgres is preferred?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/samples/postgres-auto-persistent/kustomization.yaml (1)
5-5: Fix YAML formatting issues.The static analysis tools identified formatting problems that should be addressed for consistency.
Apply this fix to resolve the trailing spaces and add missing newline:
-resources: -- modelregistry_v1beta1_modelregistry.yaml +resources: +- modelregistry_v1beta1_modelregistry.yamlconfig/samples/postgres-auto-persistent/modelregistry_v1beta1_modelregistry.yaml (1)
1-13: LGTM! Fix YAML formatting issues.The sample configuration correctly demonstrates the new PostgreSQL auto-provisioning feature with persistence enabled. The field values and structure are appropriate.
Apply this fix to resolve the trailing spaces and add missing newline:
grpc: - port: 9090 + port: 9090
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/v1alpha1/modelregistry_types.go(2 hunks)api/v1alpha1/zz_generated.conversion.go(5 hunks)api/v1alpha1/zz_generated.deepcopy.go(2 hunks)api/v1beta1/modelregistry_types.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(4 hunks)config/samples/postgres-auto-persistent/kustomization.yaml(1 hunks)config/samples/postgres-auto-persistent/modelregistry_v1beta1_modelregistry.yaml(1 hunks)internal/controller/modelregistry_controller.go(4 hunks)internal/controller/modelregistry_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/modelregistry_controller_test.go
- api/v1alpha1/zz_generated.conversion.go
- internal/controller/modelregistry_controller.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
🧰 Additional context used
🧬 Code Graph Analysis (4)
api/v1alpha1/zz_generated.deepcopy.go (2)
api/v1alpha1/modelregistry_types.go (2)
PostgresPersistenceConfig(103-111)PostgresStorageConfig(114-125)api/v1beta1/modelregistry_types.go (2)
PostgresPersistenceConfig(104-112)PostgresStorageConfig(115-126)
api/v1alpha1/modelregistry_types.go (1)
api/v1beta1/modelregistry_types.go (2)
PostgresPersistenceConfig(104-112)PostgresStorageConfig(115-126)
api/v1beta1/modelregistry_types.go (1)
api/v1alpha1/modelregistry_types.go (2)
PostgresPersistenceConfig(103-111)PostgresStorageConfig(114-125)
api/v1beta1/zz_generated.deepcopy.go (1)
api/v1beta1/modelregistry_types.go (2)
PostgresPersistenceConfig(104-112)PostgresStorageConfig(115-126)
🪛 YAMLlint (1.37.1)
config/samples/postgres-auto-persistent/kustomization.yaml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
[error] 5-5: trailing spaces
(trailing-spaces)
config/samples/postgres-auto-persistent/modelregistry_v1beta1_modelregistry.yaml
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
[error] 13-13: trailing spaces
(trailing-spaces)
🔇 Additional comments (13)
api/v1alpha1/zz_generated.deepcopy.go (3)
374-383: LGTM!The deepcopy logic for the new
GenerateandPersistencefields correctly handles pointer dereferencing and nested struct copying, following the established patterns in the codebase.
421-444: LGTM!The new deepcopy methods for
PostgresPersistenceConfigproperly handle the nested pointer fieldsPersistandStorage, maintaining consistency with the existing deepcopy implementations.
446-469: LGTM!The deepcopy implementation for
PostgresStorageConfigcorrectly handles theStorageClassNamepointer field andAccessModesslice, following established patterns for deep copying collections.api/v1beta1/zz_generated.deepcopy.go (3)
310-319: LGTM!The deepcopy logic for the new
GenerateandPersistencefields in the v1beta1 API correctly mirrors the v1alpha1 implementation, ensuring consistency across API versions.
357-380: LGTM!The new deepcopy methods for
PostgresPersistenceConfigin the v1beta1 API correctly handle the nested structures and maintain consistency with the v1alpha1 implementation.
382-405: LGTM!The deepcopy implementation for
PostgresStorageConfigin the v1beta1 API properly handles all field types and maintains consistency with the corresponding v1alpha1 methods.api/v1alpha1/modelregistry_types.go (3)
65-65: LGTM!Making the
Databasefield optional is appropriate for auto-provisioning scenarios where the database name can be auto-generated.
72-77: LGTM!The new
GenerateandPersistencefields are well-designed for the auto-provisioning feature. Using pointer types allows distinguishing between unset and explicitly set values, and the default values are appropriate.
102-125: LGTM!The new
PostgresPersistenceConfigandPostgresStorageConfigstructs provide comprehensive configuration options for auto-provisioned PostgreSQL instances. The kubebuilder annotations set reasonable defaults (1Gi storage, ReadWriteOnce access mode) and the field organization is logical.api/v1beta1/modelregistry_types.go (4)
60-60: LGTM! Username field correctly made optional for auto-provisioning.The removal of the
+kubebuilder:requiredannotation and addition ofomitemptymakes sense for auto-provisioning scenarios where the operator will generate default credentials.
66-66: LGTM! Database field correctly made optional for auto-provisioning.Making the database field optional with
omitemptyis consistent with the auto-provisioning feature where the operator can generate a default database name.
73-78: LGTM! Generate and Persistence fields are well-designed.The new fields are properly structured:
Generateuses a boolean pointer with appropriate default value of falsePersistenceis logically organized as an optional sub-configuration- Both fields have clear documentation and proper kubebuilder annotations
The design maintains backward compatibility while enabling the new auto-provisioning functionality.
103-126: LGTM! New struct definitions are well-designed and consistent.Both
PostgresPersistenceConfigandPostgresStorageConfigstructs are properly implemented:
- Proper defaults: Sensible default values for all fields (false for persistence, "1Gi" for size, "ReadWriteOnce" for access modes)
- Correct field types: Using appropriate Kubernetes types and pointer patterns
- Good documentation: Clear comments explaining the purpose and behavior of each field
- API consistency: The structs match the v1alpha1 version, ensuring proper version interoperability
The storage configuration follows Kubernetes PVC conventions and provides a clean abstraction for PostgreSQL persistence options.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/samples/postgres-auto-persistent/modelregistry_v1beta1_modelregistry.yaml (1)
12-12: Trim trailing space & add missing newline at EOFYAML-lint flags a trailing space after
9090and the absence of a final newline. Clean up to keep CI quiet and maintain diff hygiene.- port: 9090␠ + port: 9090 +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/v1alpha1/modelregistry_types.go(1 hunks)api/v1alpha1/zz_generated.conversion.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/v1beta1/modelregistry_types.go(1 hunks)api/v1beta1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(4 hunks)config/samples/postgres-auto-persistent/modelregistry_v1beta1_modelregistry.yaml(1 hunks)internal/controller/modelregistry_controller.go(4 hunks)internal/controller/modelregistry_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1beta1/zz_generated.deepcopy.go
- internal/controller/modelregistry_controller_test.go
- api/v1beta1/modelregistry_types.go
- api/v1alpha1/zz_generated.conversion.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
- internal/controller/modelregistry_controller.go
- api/v1alpha1/modelregistry_types.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
config/samples/postgres-auto-persistent/modelregistry_v1beta1_modelregistry.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
[error] 12-12: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
config/samples/postgres-auto-persistent/modelregistry_v1beta1_modelregistry.yaml (1)
6-8: Confirm persistence semantics & required storage settingsSetting
persist: trueimplies the DB volume should outlive theModelRegistryresource, but the sample does not show any PVC size/class parameters. Double-check that:
- The controller injects sensible defaults (e.g., 1 Gi,
standardStorageClass).- The CRD validation permits omitting explicit storage settings when
persist: true.If defaults are not guaranteed, document the behaviour or extend the sample to include an explicit
storage:block.
Chanakya-TS
changed the title
There was a problem hiding this comment.
imho, if this feature is truly meant to be for testing and lightweight deployments, we're better served using something like SQLite instead of auto-provisioning a prescriptive PostgreSQL instance.
I think this approach will end up raising other issues in the end.
| }, | ||
| StringData: map[string]string{ | ||
| "username": "postgres", | ||
| "password": "password", |
There was a problem hiding this comment.
Generate a random string password here. Can the user still change the password later in the secret? Or is it going to cause reconciliation issues?
| "password": "password", | ||
| }, | ||
| } | ||
| if err = ctrl.SetControllerReference(registry, &secret, r.Scheme); err != nil { |
There was a problem hiding this comment.
Default to not delete any data. I think that's an issue with this approach to use Postgres for test MR instance. imho, we should default to something simple like SQLite DB instead for dev MR instances.
| } | ||
| } | ||
|
|
||
| log.Info("Creating or updating postgres deployment") |
There was a problem hiding this comment.
This DB reconciliation will also cause issues if users want to customize the DB once it's created. Hence the above comment about using something truly lightweight like SQLite instead.
|
I think the main problem here after reading all the comments is that we have not defined a clear plan for this POC @rareddy @dhirajsb What are the requirements? How we expect an user to create a dev model registry using the UI? (e.g. a toggle to select an auto provisioned DB, can the user provide username and password for this db? select in which namespace?) What happens when deleting the model registry bound to this provisioned database? Once we know that we can direct @Chanakya-TS development without having him change things on the fly / revert / etc.. |
The goal here is to simplify the developer experience by removing the hurdle of setting up and configuring a database for the model registry in a development environment. The specific database choice isn't critical, the idea is to have a "click-and-go" setup that works right out of the box with minimal friction.
I expect in the dashboard UI we will have option to choose "Default DB (DEVELOPMENT ONLY)" instead of the typically provided DB credentials.
no expectations of data retention are required at this point; however, I expect data to survive if the MR pod is restarted for any reason. |
…ocker digest to 2a88121 (opendatahub-io#281) Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com> Co-authored-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
|
@dhirajsb, @Al-Pragliola, @rareddy, do we have a consensus on the database choice between Postgres and SQLite? |
IMO, we should hide the choice under configuration and provide both for now. What we choose by default can come later if that needs further discussion. The main point is when UI is supporting this, that is when this needs to be chosen to a perticular DB. |
In this PR, the |
|
@chambridge FYI, check this out for the Default Database PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/controller/config/templates/postgres-deployment.yaml.tmpl (1)
20-20: Address image version pinning and potential rate limiting issues.The current image uses the
latesttag, which can lead to unpredictable deployments and doesn't follow best practices for production environments. Additionally, as mentioned in past comments, this may cause rate limiting issues in downstream environments.Consider pinning to a specific version and SHA:
- image: quay.io/sclorg/postgresql-13-c9s:latest + image: quay.io/sclorg/postgresql-13-c9s:c9s@sha256:<specific-sha>Alternatively, use a specific version tag:
- image: quay.io/sclorg/postgresql-13-c9s:latest + image: quay.io/sclorg/postgresql-13-c9s:1-77
🧹 Nitpick comments (2)
internal/controller/config/templates/postgres-deployment.yaml.tmpl (2)
24-25: Consider making database name configurable.The database name is hardcoded as "model_registry". For flexibility and consistency with other configurable aspects, consider making this templatable.
- name: POSTGRESQL_DATABASE - value: "model_registry" + value: "{{.DatabaseName | default "model_registry"}}"
8-16: Add deployment strategy and pod disruption considerations.The deployment lacks a strategy definition and could benefit from better availability configuration for development environments.
Apply this diff to improve deployment reliability:
spec: replicas: 1 + strategy: + type: Recreate selector: matchLabels: app: {{.Name}}-postgres template: metadata: labels: app: {{.Name}}-postgres + annotations: + rollme: "{{.Name}}-{{.Generation}}"The
Recreatestrategy is appropriate for single-replica PostgreSQL deployments to avoid data corruption, and the annotation helps trigger pod recreation when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/v1alpha1/modelregistry_types.go(1 hunks)api/v1alpha1/modelregistry_webhook.go(2 hunks)api/v1alpha1/zz_generated.conversion.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/v1beta1/modelregistry_types.go(1 hunks)api/v1beta1/modelregistry_webhook.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(2 hunks)config/rbac/role.yaml(1 hunks)internal/controller/config/templates/postgres-deployment.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-pvc.yaml.tmpl(1 hunks)internal/controller/modelregistry_controller.go(3 hunks)internal/controller/modelregistry_controller_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/controller/config/templates/postgres-pvc.yaml.tmpl
🚧 Files skipped from review as they are similar to previous changes (11)
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1alpha1/zz_generated.conversion.go
- api/v1beta1/zz_generated.deepcopy.go
- config/rbac/role.yaml
- api/v1beta1/modelregistry_types.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
- api/v1alpha1/modelregistry_types.go
- api/v1beta1/modelregistry_webhook.go
- internal/controller/modelregistry_controller_test.go
- api/v1alpha1/modelregistry_webhook.go
- internal/controller/modelregistry_controller.go
🔇 Additional comments (1)
internal/controller/config/templates/postgres-deployment.yaml.tmpl (1)
36-42: Validate volume mount path and PVC reference.The mount path
/var/lib/pgsql/dataand PVC naming convention look correct for the PostgreSQL image being used. However, ensure the PVC template creates a volume with appropriate storage class and size.The volume configuration correctly references the expected PVC name pattern and uses the standard PostgreSQL data directory for this image.
internal/controller/config/templates/postgres-deployment.yaml.tmpl
Outdated
Show resolved
Hide resolved
Chanakya-TS
force-pushed
the
default-postgres
branch
from
a294d9d to
8e70b83
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/controller/modelregistry_controller.go (1)
479-483: Security concern: hardcoded credentials pose security risks.The hardcoded username "modelregistry" and password "password" are insecure for any deployment. Generate random credentials instead to improve security posture.
Consider implementing a function to generate secure random credentials:
- StringData: map[string]string{ - "username": "modelregistry", - "password": "password", - }, + StringData: map[string]string{ - "username": "modelregistry", + "password": generateRandomPassword(), + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
api/v1alpha1/modelregistry_types.go(1 hunks)api/v1alpha1/modelregistry_webhook.go(2 hunks)api/v1alpha1/zz_generated.conversion.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/v1alpha1/zz_generated.manual.conversion.go(1 hunks)api/v1beta1/modelregistry_types.go(1 hunks)api/v1beta1/modelregistry_webhook.go(2 hunks)api/v1beta1/modelregistry_webhook_test.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(2 hunks)config/rbac/role.yaml(1 hunks)config/samples/postgres-auto/kustomization.yaml(1 hunks)config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml(1 hunks)internal/controller/config/templates/postgres-deployment.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-pvc.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-service.yaml.tmpl(1 hunks)internal/controller/modelregistry_controller.go(3 hunks)internal/controller/modelregistry_controller_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- api/v1beta1/zz_generated.deepcopy.go
- internal/controller/config/templates/postgres-service.yaml.tmpl
- config/rbac/role.yaml
- internal/controller/modelregistry_controller_test.go
- api/v1beta1/modelregistry_webhook.go
- internal/controller/config/templates/postgres-pvc.yaml.tmpl
- internal/controller/config/templates/postgres-deployment.yaml.tmpl
- api/v1beta1/modelregistry_types.go
- api/v1alpha1/modelregistry_types.go
- api/v1alpha1/zz_generated.manual.conversion.go
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1beta1/modelregistry_webhook_test.go
- api/v1alpha1/zz_generated.conversion.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-07T12:31:29.552Z
Learnt from: tarilabs
PR: opendatahub-io/model-registry-operator#269
File: .github/workflows/build-image-pr.yml:49-52
Timestamp: 2025-07-07T12:31:29.552Z
Learning: In the model-registry-operator project, the AppArmor profile removal for MySQL 8.x in KinD on GHA is a critical requirement. The CI job should fail if the profile cannot be removed, rather than continuing with a potentially broken setup, as this ensures proper MySQL deployment functionality.
Applied to files:
api/v1alpha1/modelregistry_webhook.go
📚 Learning: 2025-08-01T19:12:33.339Z
Learnt from: pboyd
PR: opendatahub-io/model-registry-operator#276
File: internal/controller/modelregistry_controller.go:338-338
Timestamp: 2025-08-01T19:12:33.339Z
Learning: In the ModelRegistry controller (`internal/controller/modelregistry_controller.go`), the RBAC permissions for ConfigMaps only include `create;get;list;watch` verbs because the controller only needs to create ConfigMaps, not update them. The ConfigMaps are created once and then managed outside of the controller.
Applied to files:
internal/controller/modelregistry_controller.goconfig/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
📚 Learning: 2025-08-01T19:08:33.905Z
Learnt from: pboyd
PR: opendatahub-io/model-registry-operator#276
File: internal/controller/modelregistry_catalog_test.go:435-442
Timestamp: 2025-08-01T19:08:33.905Z
Learning: In the ModelRegistry catalog tests in `internal/controller/modelregistry_catalog_test.go`, the OpenShift-specific deletion tests correctly check for Route resource deletion using the Route variable and the `%s-catalog-https` naming pattern, which is separate from the general catalog Deployment deletion tests.
Applied to files:
internal/controller/modelregistry_controller.go
📚 Learning: 2025-08-01T19:10:43.535Z
Learnt from: pboyd
PR: opendatahub-io/model-registry-operator#276
File: internal/controller/modelregistry_catalog.go:62-96
Timestamp: 2025-08-01T19:10:43.535Z
Learning: In the ModelRegistry catalog controller, the ConfigMap created by `createOrUpdateCatalogConfig` is intentionally not deleted by `deleteCatalogResources`. The ConfigMap is created if it doesn't exist but is then managed outside of the controller, so it should persist even when the catalog feature is disabled.
Applied to files:
internal/controller/modelregistry_controller.go
🧬 Code Graph Analysis (1)
internal/controller/modelregistry_controller.go (1)
api/v1beta1/modelregistry_types.go (3)
ModelRegistry(314-320)PostgresConfig(46-97)SecretKeyValue(37-44)
🪛 YAMLlint (1.37.1)
config/samples/postgres-auto/kustomization.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
[error] 4-4: trailing spaces
(trailing-spaces)
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
[error] 11-11: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml (1)
6-7: LGTM! Proper demonstration of auto-provisioning feature.The sample correctly demonstrates how to enable PostgreSQL auto-provisioning using the
generate: trueflag.api/v1alpha1/modelregistry_webhook.go (2)
63-70: LGTM! Proper handling of auto-provisioning in defaulting logic.The logic correctly preserves the Postgres configuration when auto-provisioning is enabled, preventing unwanted removal of the config during defaulting.
245-267: LGTM! Comprehensive validation for auto-provisioning.The validation logic properly ensures that when auto-provisioning is enabled, manual host configuration is not allowed, which prevents configuration conflicts.
internal/controller/modelregistry_controller.go (3)
336-336: LGTM! RBAC permissions correctly updated for auto-provisioning.The addition of
secretsandpersistentvolumeclaimsto the RBAC permissions is necessary and correct for the auto-provisioning functionality.
362-368: LGTM! Proper conditional logic for auto-provisioning.The check for the
Generateflag is implemented correctly and follows the expected pattern for enabling auto-provisioning only when explicitly requested.
543-558: LGTM! Proper in-memory spec update with auto-provisioned details.The logic correctly updates the ModelRegistry spec with the auto-provisioned PostgreSQL connection details, including proper secret reference for the password.
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
Outdated
Show resolved
Hide resolved
chambridge
force-pushed
the
default-postgres
branch
from
8e70b83 to
f8b056b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml (1)
11-11: Remove trailing space and add EOF newline.Trailing whitespace after the port value and a missing newline at EOF violate YAML formatting standards and can trip linters.
Apply this diff:
- grpc: - port: 9090 + grpc: + port: 9090Also ensure the file ends with a newline.
config/samples/postgres-auto/kustomization.yaml (1)
4-4: Remove trailing space and add EOF newline.Minor formatting cleanup for YAML linters.
-- modelregistry_v1beta1_modelregistry.yaml +- modelregistry_v1beta1_modelregistry.yamlAlso ensure the file ends with a newline.
🧹 Nitpick comments (5)
api/v1alpha1/modelregistry_webhook.go (1)
245-266: Return field errors for both host and hostAddress when Generate=true.Currently, if only hostAddress is set, validation still returns an error tied to host (with an empty value), which is misleading. Accumulate errors for each set field.
Apply this diff:
- if hasAutoProvisioning { - // When auto-provisioning is enabled, host/hostAddress should not be set (they will be auto-generated) - if len(r.Spec.Postgres.Host) > 0 || len(r.Spec.Postgres.HostAddress) > 0 { - return nil, field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "host should not be set when auto-provisioning is enabled"), - } - } - // Note: database and username CAN be set when auto-provisioning - they specify the desired values - } + if hasAutoProvisioning { + // When auto-provisioning is enabled, host/hostAddress should not be set (they will be auto-generated) + var errs field.ErrorList + if len(r.Spec.Postgres.Host) > 0 { + errs = append(errs, field.Invalid( + field.NewPath("spec").Child("postgres").Child("host"), + r.Spec.Postgres.Host, + "host should not be set when auto-provisioning is enabled", + )) + } + if len(r.Spec.Postgres.HostAddress) > 0 { + errs = append(errs, field.Invalid( + field.NewPath("spec").Child("postgres").Child("hostAddress"), + r.Spec.Postgres.HostAddress, + "hostAddress should not be set when auto-provisioning is enabled", + )) + } + if len(errs) > 0 { + return nil, errs + } + // Note: database and username CAN be set when auto-provisioning - they specify the desired values + }If helpful, I can add unit tests that assert:
- Generate=true, host set → invalid host.
- Generate=true, hostAddress set → invalid hostAddress.
- Generate=true, both set → two errors accumulated.
api/v1alpha1/zz_generated.manual.conversion.go (1)
8-15: Update manual-conversion comment for database handlingThe existing comment in
api/v1alpha1/zz_generated.manual.conversion.gomistakenly refers to a top-level “Database” field on the spec, which no longer exists. Both v1beta1 and v1alpha1 use nestedPostgresConfig/MySQLConfigstructs that include their ownDatabasefields—and those are already handled by the auto-generated conversion.• File:
api/v1alpha1/zz_generated.manual.conversion.go
Function:Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpecSuggested patch:
func Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec( in *v1beta1.ModelRegistrySpec, out *ModelRegistrySpec, s conversion.Scope, ) error { // Manually convert all fields from in to out. if err := autoConvert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec(in, out, s); err != nil { return err } - // The Database field does not exist in v1alpha1, so it is ignored. + // No top-level Database field in v1alpha1 spec; nested Postgres/MySQL conversion covers database names. return nil }api/v1beta1/modelregistry_webhook.go (1)
198-218: Mirror v1alpha1: validate both host and hostAddress under auto-provisioning.Ensure errors point to the exact offending fields; otherwise, users get an error on “host” even when only “hostAddress” was set.
Apply this diff:
- if hasAutoProvisioning { - // When auto-provisioning is enabled, host/hostAddress should not be set (they will be auto-generated) - if len(r.Spec.Postgres.Host) > 0 || len(r.Spec.Postgres.HostAddress) > 0 { - return nil, field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "host should not be set when auto-provisioning is enabled"), - } - } - // Note: database and username CAN be set when auto-provisioning - they specify the desired values - } + if hasAutoProvisioning { + // When auto-provisioning is enabled, host/hostAddress should not be set (they will be auto-generated) + var errs field.ErrorList + if len(r.Spec.Postgres.Host) > 0 { + errs = append(errs, field.Invalid( + field.NewPath("spec").Child("postgres").Child("host"), + r.Spec.Postgres.Host, + "host should not be set when auto-provisioning is enabled", + )) + } + if len(r.Spec.Postgres.HostAddress) > 0 { + errs = append(errs, field.Invalid( + field.NewPath("spec").Child("postgres").Child("hostAddress"), + r.Spec.Postgres.HostAddress, + "hostAddress should not be set when auto-provisioning is enabled", + )) + } + if len(errs) > 0 { + return nil, errs + } + // Note: database and username CAN be set when auto-provisioning - they specify the desired values + }I can also mirror tests in api/v1beta1/modelregistry_webhook_test.go to cover these cases.
api/v1beta1/modelregistry_types.go (2)
65-66: Makingdatabaseoptional makes sense for auto-provisioning; add CRD-level guard to protect external-DB configs.Good call to relax this for
postgres.generate: true. However, relying only on webhook validation can be brittle; consider adding CEL XValidations so API-server enforces the minimal constraints even if the webhook is unavailable.Two complementary rules:
- If
generateis true,hostandhostAddressmust be empty.- If
generateis false or unset, require at least one ofhost/hostAddressand a non-emptydatabase(and optionallyusernameif that’s still required for external DBs in practice).Proposed type-level annotations (outside this line range, shown with context) you can paste above fields in
PostgresConfig:type PostgresConfig struct { + // +kubebuilder:validation:XValidation:rule="!(has(self.generate) && self.generate == true) || (!has(self.host) && !has(self.hostAddress))",message="When postgres.generate is true, host and hostAddress must be omitted" + // +kubebuilder:validation:XValidation:rule="(has(self.generate) && self.generate == true) || ((has(self.host) || has(self.hostAddress)) && has(self.database) && self.database != '')",message="When postgres.generate is false or unset, require database and one of host or hostAddress" // Name of host to connect to. Host string `json:"host,omitempty"`If
usernameis required for external DBs, add it to the second rule:&& has(self.username) && self.username != ''.
73-75: Confirm tri-state need forGenerate *bool; avoid adding a default unless nil vs false is semantically distinct.Using a pointer is appropriate if you need to distinguish “unset” from explicit false. Please confirm controller/webhooks treat nil as “no auto-provisioning” and do not auto-flip existing Postgres configs. If tri-state is not needed, simplify to
boolwith+kubebuilder:default=false.Also consider clarifying the field comment to document precedence/ignored fields when
generateis true (e.g., host/hostAddress/user/passwordSecret/database behavior).Here’s a minimal comment improvement (within this hunk):
-// Auto-provision a PostgreSQL database if true. +// Auto-provision a PostgreSQL database if true. +// When set, the operator will create a PostgreSQL Deployment/Service/Secret/PVC +// and ignore external connection fields such as host/hostAddress. Username/password +// are generated and stored in a Secret; database name may be set by the operator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
api/v1alpha1/modelregistry_types.go(1 hunks)api/v1alpha1/modelregistry_webhook.go(2 hunks)api/v1alpha1/zz_generated.conversion.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/v1alpha1/zz_generated.manual.conversion.go(1 hunks)api/v1beta1/modelregistry_types.go(1 hunks)api/v1beta1/modelregistry_webhook.go(2 hunks)api/v1beta1/modelregistry_webhook_test.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(2 hunks)config/rbac/role.yaml(1 hunks)config/samples/postgres-auto/kustomization.yaml(1 hunks)config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml(1 hunks)internal/controller/config/templates/postgres-deployment.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-pvc.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-service.yaml.tmpl(1 hunks)internal/controller/modelregistry_controller.go(3 hunks)internal/controller/modelregistry_controller_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- config/rbac/role.yaml
- internal/controller/config/templates/postgres-service.yaml.tmpl
- api/v1beta1/zz_generated.deepcopy.go
- internal/controller/config/templates/postgres-deployment.yaml.tmpl
- internal/controller/config/templates/postgres-pvc.yaml.tmpl
- api/v1alpha1/modelregistry_types.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
- internal/controller/modelregistry_controller_test.go
- api/v1beta1/modelregistry_webhook_test.go
- api/v1alpha1/zz_generated.deepcopy.go
- internal/controller/modelregistry_controller.go
🧰 Additional context used
🧬 Code graph analysis (1)
api/v1alpha1/zz_generated.manual.conversion.go (1)
api/v1beta1/modelregistry_types.go (1)
ModelRegistrySpec(247-283)
🪛 YAMLlint (1.37.1)
config/samples/postgres-auto/kustomization.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
[error] 4-4: trailing spaces
(trailing-spaces)
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
[error] 11-11: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
api/v1alpha1/modelregistry_webhook.go (1)
63-70: Defaulting preserves Postgres when auto-provisioning is enabled — good change.Retaining Postgres config when Generate=true avoids dropping user intent during defaulting. This aligns with the controller’s auto-provisioning flow.
api/v1beta1/modelregistry_webhook.go (1)
58-66: Defaulting preserves Postgres when auto-provisioning is enabled — good change.Same rationale as v1alpha1: keeps user intent and prevents accidental removal in KIND or other environments.
api/v1alpha1/zz_generated.conversion.go (3)
416-416: Generate field pointer-copy via unsafe: OK; keep field types consistent cross-versions.
out.Generate = (*bool)(unsafe.Pointer(in.Generate))is standard in these autogenerated conversions. Just ensure both API versions keepGenerateas*bool; otherwise, this becomes unsafe. Unit tests around round-trip conversion withnil,true, andfalsewould be valuable.
439-439: Symmetric conversion forGenerate: OK; add tests for nil handling.Mirrors the earlier direction. Please cover nil-preserving behavior in tests to avoid defaulting surprises when converting through the hub.
145-149: Confirm registration and wrapper sync for ModelRegistrySpec conversion
- Verify that the manual registration call exists in the generated conversions:
– the line should read something like:rg -n "AddConversionFunc.*ModelRegistrySpec" -C2 api/v1alpha1/zz_generated.conversion.goif err := s.AddConversionFunc( (*v1beta1.ModelRegistrySpec)(nil), (*ModelRegistrySpec)(nil), Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec, ); err != nil { return err }- Confirm the manual wrapper implementation in
api/v1alpha1/zz_generated.manual.conversion.go:– ensure it delegates torg -n "func Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec" -C5 api/v1alpha1/zz_generated.manual.conversion.goautoConvert_…and that any fields intentionally skipped are annotated with comments.- Keep the manual wrapper and its test coverage in sync with the auto-generated conversion.
Run this script to check both points in one go:
#!/usr/bin/env bash set -euo pipefail rg -n "AddConversionFunc.*ModelRegistrySpec" -C2 api/v1alpha1/zz_generated.conversion.go rg -n "func Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec" -C5 api/v1alpha1/zz_generated.manual.conversion.go
|
/ok-to-test |
config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
Outdated
Show resolved
Hide resolved
|
lgtm as far as I can tell. I'm not the most familiar with this code, but don't see any problems |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
api/v1alpha1/modelregistry_types.go (1)
72-74: Good, explicit flag name (generateDeployment).This resolves prior ambiguity with a generic “generate” name and clearly communicates the behavior. Keep in sync across v1alpha1/v1beta1 and samples.
To smoke-check repo-wide consistency and catch stale samples using
spec.postgres.generate:#!/bin/bash # Expect: only "generateDeployment" appears; "postgres.generate:" should NOT appear. rg -nP --color=never -C1 '(postgres\.(generate|generateDeployment))\s*:' config/ docs/ samples/ || trueapi/v1beta1/modelregistry_types.go (1)
73-75: Flag name is clear and aligns with prior feedback.
generateDeploymentis explicit and consistent with the operator’s behavior; thanks for addressing the earlier naming feedback.internal/controller/modelregistry_controller.go (1)
462-558: Fix insecure credentials, idempotency, and result aggregation in createOrUpdatePostgres; preserve user fields in spec.Issues:
- Hardcoded Secret with username "modelregistry" and password "password". This is a security risk and was raised before. Generate a strong random password at creation time. Do not overwrite on subsequent reconciles.
- Operation results from PVC/Deployment/Service are ignored, so the function often returns ResourceUnchanged even after creating/updating resources, affecting events and requeues.
- If the Secret already exists, ownerReferences are not ensured (no SetControllerReference), and content is not validated.
- You replace the entire PostgresConfig in memory, dropping user fields (e.g., sslMode, skipDBCreation, GenerateDeployment). Prefer patching only missing fields.
Apply this refactor in-place:
func (r *ModelRegistryReconciler) createOrUpdatePostgres(ctx context.Context, params *ModelRegistryParams, registry *v1beta1.ModelRegistry) (result OperationResult, err error) { log := klog.FromContext(ctx) log.Info("Creating or updating postgres database") - result = ResourceUnchanged - var secret corev1.Secret - secretName := params.Name + "-postgres-credentials" - err = r.Get(ctx, types.NamespacedName{Name: secretName, Namespace: params.Namespace}, &secret) - if err != nil { - if errors.IsNotFound(err) { - // Create the secret - log.Info("Creating postgres secret", "secret", secretName) - secret = corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: params.Namespace, - }, - StringData: map[string]string{ - "username": "modelregistry", - "password": "password", - }, - } - if err = ctrl.SetControllerReference(registry, &secret, r.Scheme); err != nil { - log.Error(err, "Failed to set controller reference on secret") - return result, err - } - if result, err = r.createOrUpdate(ctx, &corev1.Secret{}, &secret); err != nil { - log.Error(err, "Failed to create or update secret") - return result, err - } - } else { - log.Error(err, "Failed to get secret") - return result, err - } - } + result = ResourceUnchanged + secretName := params.Name + "-postgres-credentials" + // Secret: create if missing; do not overwrite on subsequent reconciles; ensure ownerRef if needed. + var existingSecret corev1.Secret + getErr := r.Get(ctx, types.NamespacedName{Name: secretName, Namespace: params.Namespace}, &existingSecret) + if client.IgnoreNotFound(getErr) != nil { + log.Error(getErr, "Failed to get secret") + return result, getErr + } + if errors.IsNotFound(getErr) { + log.Info("Creating postgres secret", "secret", secretName) + pwd, perr := generateRandomPassword(24) + if perr != nil { + log.Error(perr, "Failed generating random password") + return result, perr + } + sec := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: params.Namespace, + }, + Type: corev1.SecretTypeOpaque, + StringData: map[string]string{ + "username": "modelregistry", + "password": pwd, + }, + } + if err = ctrl.SetControllerReference(registry, &sec, r.Scheme); err != nil { + log.Error(err, "Failed to set controller reference on secret") + return result, err + } + if res, err := r.createOrUpdate(ctx, &corev1.Secret{}, &sec); err != nil { + log.Error(err, "Failed to create or update secret") + return result, err + } else if res != ResourceUnchanged { + result = res + } + } else if !metav1.IsControlledBy(&existingSecret, registry) { + // Ensure ownership on an existing secret we created previously. + log.Info("Patching postgres secret with owner reference", "secret", secretName) + updated := existingSecret.DeepCopy() + if err = ctrl.SetControllerReference(registry, updated, r.Scheme); err != nil { + log.Error(err, "Failed to set controller reference on existing secret") + return result, err + } + if res, err := r.createOrUpdate(ctx, &corev1.Secret{}, updated); err != nil { + log.Error(err, "Failed to update secret ownerRef") + return result, err + } else if res != ResourceUnchanged { + result = res + } + } log.Info("Creating or updating postgres PVC") var pvc corev1.PersistentVolumeClaim if err = r.Apply(params, "postgres-pvc.yaml.tmpl", &pvc); err != nil { log.Error(err, "Failed to apply postgres PVC template") return result, err } if err = ctrl.SetControllerReference(registry, &pvc, r.Scheme); err != nil { log.Error(err, "Failed to set controller reference on PVC") return result, err } - if _, err = r.createOrUpdate(ctx, &corev1.PersistentVolumeClaim{}, &pvc); err != nil { + if res, err := r.createOrUpdate(ctx, &corev1.PersistentVolumeClaim{}, &pvc); err != nil { log.Error(err, "Failed to create or update PVC") return result, err + } else if res != ResourceUnchanged { + result = res } log.Info("Creating or updating postgres deployment") var deployment appsv1.Deployment if err = r.Apply(params, "postgres-deployment.yaml.tmpl", &deployment); err != nil { log.Error(err, "Failed to apply postgres deployment template") return result, err } if err = ctrl.SetControllerReference(registry, &deployment, r.Scheme); err != nil { log.Error(err, "Failed to set controller reference on deployment") return result, err } - if _, err = r.createOrUpdate(ctx, &appsv1.Deployment{}, &deployment); err != nil { + if res, err := r.createOrUpdate(ctx, &appsv1.Deployment{}, &deployment); err != nil { log.Error(err, "Failed to create or update deployment") return result, err + } else if res != ResourceUnchanged { + result = res } log.Info("Creating or updating postgres service") var service corev1.Service if err = r.Apply(params, "postgres-service.yaml.tmpl", &service); err != nil { log.Error(err, "Failed to apply postgres service template") return result, err } if err = ctrl.SetControllerReference(registry, &service, r.Scheme); err != nil { log.Error(err, "Failed to set controller reference on service") return result, err } - if _, err = r.createOrUpdate(ctx, &corev1.Service{}, &service); err != nil { + if res, err := r.createOrUpdate(ctx, &corev1.Service{}, &service); err != nil { log.Error(err, "Failed to create or update service") return result, err + } else if res != ResourceUnchanged { + result = res } // Update the spec in memory log.Info("Updating spec in memory with postgres details") port := int32(5432) - registry.Spec.Postgres = &v1beta1.PostgresConfig{ - Host: params.Name + "-postgres", - Port: &port, - Username: "modelregistry", - Database: "model_registry", - PasswordSecret: &v1beta1.SecretKeyValue{ - Name: secretName, - Key: "password", - }, - } + pg := registry.Spec.Postgres + if pg == nil { + pg = &v1beta1.PostgresConfig{} + } + if pg.Host == "" && pg.HostAddress == "" { + pg.Host = params.Name + "-postgres" + } + if pg.Port == nil { + pg.Port = &port + } + if pg.Username == "" { + pg.Username = "modelregistry" + } + if pg.Database == "" { + pg.Database = "model_registry" + } + pg.PasswordSecret = &v1beta1.SecretKeyValue{Name: secretName, Key: "password"} + registry.Spec.Postgres = pg return result, nil }Add the helper (outside this hunk) and import:
// at top-level in this file: import ( // ... "crypto/rand" ) // helper function (in the same file) func generateRandomPassword(n int) (string, error) { const alphabet = "ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz23456789!#$%&*+-_=?:" b := make([]byte, n) if _, err := rand.Read(b); err != nil { return "", err } for i := range b { b[i] = alphabet[int(b[i])%len(alphabet)] } return string(b), nil }
- Security: eliminates hardcoded credential risk; still allows users to rotate the Secret without the reconciler overwriting it.
- Correctness: OperationResult now bubbles up, enabling proper events and requeues.
- Stability: preserves any user-provided PostgresConfig fields (sslMode, skipDBCreation, GenerateDeployment, etc.).
I can wire unit tests to cover the result propagation and “do not overwrite existing Secret” behavior if you want.
🧹 Nitpick comments (5)
api/v1alpha1/modelregistry_types.go (1)
72-74: Consider baking constraints into the CRD with XValidation (optional).Enforce at CRD-level that when
generateDeploymentis true, mutually exclusive fields are unset to prevent user confusion.Add the following kubebuilder XValidation on PostgresConfig (outside the shown hunk):
// +kubebuilder:validation:XValidation:rule="!(has(self.generateDeployment) && self.generateDeployment == true) || (!has(self.host) && !has(self.hostAddress) && !has(self.passwordSecret) && !has(self.port) && self.skipDBCreation == false)",message="When generateDeployment is true, do not set host/hostAddress/port/passwordSecret and skipDBCreation must be false"This complements the webhook and provides earlier feedback via CRD schema.
api/v1beta1/modelregistry_types.go (1)
73-75: Optional: Add CRD XValidation for mutually exclusive fields when auto-provisioning.Mirror the v1alpha1 suggestion to prevent users from setting fields that will be ignored or conflict when
generateDeploymentis true.Proposed tag to place on PostgresConfig:
// +kubebuilder:validation:XValidation:rule="!(has(self.generateDeployment) && self.generateDeployment == true) || (!has(self.host) && !has(self.hostAddress) && !has(self.passwordSecret) && !has(self.port) && self.skipDBCreation == false)",message="When generateDeployment is true, do not set host/hostAddress/port/passwordSecret and skipDBCreation must be false"api/v1beta1/modelregistry_webhook_test.go (2)
66-66: Minor nit: inline bool literal or helper to avoid shadowing.
trueValue := trueis fine, but to keep tests compact you can inline&[]bool{true}[0]or create a shared helperptr.To(true)if you already use such helpers. Not blocking.
86-104: Great coverage for auto-provisioning happy-path and host-conflict; add cases for hostAddress and other conflicting fields.To make validation airtight, also test:
- invalid: auto-provisioning with hostAddress set
- invalid: auto-provisioning with passwordSecret set (secret is generated)
- invalid: auto-provisioning with port set (operator fixes to 5432) — either forbid or enforce equality
- invalid: auto-provisioning with skipDBCreation=true (contradictory)
- valid: non-autoprovisioned Postgres requiring host/hostAddress, username, database (once webhook enforces)
Here’s an additive diff for two immediate cases:
@@ { name: "invalid - postgres auto-provisioning with host", mrSpec: &v1beta1.ModelRegistry{Spec: v1beta1.ModelRegistrySpec{ Postgres: &v1beta1.PostgresConfig{ GenerateDeployment: &trueValue, Host: "some-host", }, }}, wantErr: true, }, + { + name: "invalid - postgres auto-provisioning with hostAddress", + mrSpec: &v1beta1.ModelRegistry{Spec: v1beta1.ModelRegistrySpec{ + Postgres: &v1beta1.PostgresConfig{ + GenerateDeployment: &trueValue, + HostAddress: "10.0.0.10", + }, + }}, + wantErr: true, + }, + { + name: "invalid - postgres auto-provisioning with passwordSecret", + mrSpec: &v1beta1.ModelRegistry{Spec: v1beta1.ModelRegistrySpec{ + Postgres: &v1beta1.PostgresConfig{ + GenerateDeployment: &trueValue, + PasswordSecret: &v1beta1.SecretKeyValue{Name: "user-supplied", Key: "password"}, + }, + }}, + wantErr: true, + },internal/controller/modelregistry_controller.go (1)
336-340: RBAC: PVC verbs added correctly; deduplicate the secrets rule to avoid confusion.You now have two RBAC lines for core/secrets: one with full verbs (on Line 336) and one narrower (Line 339). controller-gen will merge, but keeping both is redundant and can mislead future maintainers.
// +kubebuilder:rbac:groups=core,resources=services;serviceaccounts;secrets;persistentvolumeclaims,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=core,resources=endpoints,verbs=get;list;watch -// +kubebuilder:rbac:groups=core,resources=configmaps,verbs=create;get;list;watch -// +kubebuilder:rbac:groups=core,resources=secrets,verbs=create;get;list;watch +// +kubebuilder:rbac:groups=core,resources=configmaps,verbs=create;get;list;watchNote: Keeping configmaps at create;get;list;watch aligns with the prior decision that we don’t update them post-create. Good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
api/v1alpha1/modelregistry_types.go(1 hunks)api/v1alpha1/modelregistry_webhook.go(2 hunks)api/v1alpha1/zz_generated.conversion.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/v1beta1/modelregistry_types.go(1 hunks)api/v1beta1/modelregistry_webhook.go(2 hunks)api/v1beta1/modelregistry_webhook_test.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(2 hunks)internal/controller/modelregistry_controller.go(3 hunks)internal/controller/modelregistry_controller_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- api/v1alpha1/zz_generated.deepcopy.go
- internal/controller/modelregistry_controller_test.go
- api/v1beta1/modelregistry_webhook.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T19:12:33.339Z
Learnt from: pboyd
PR: opendatahub-io/model-registry-operator#276
File: internal/controller/modelregistry_controller.go:338-338
Timestamp: 2025-08-01T19:12:33.339Z
Learning: In the ModelRegistry controller (`internal/controller/modelregistry_controller.go`), the RBAC permissions for ConfigMaps only include `create;get;list;watch` verbs because the controller only needs to create ConfigMaps, not update them. The ConfigMaps are created once and then managed outside of the controller.
Applied to files:
internal/controller/modelregistry_controller.go
🧬 Code graph analysis (3)
api/v1beta1/modelregistry_webhook_test.go (1)
api/v1beta1/modelregistry_types.go (3)
ModelRegistry(314-320)ModelRegistrySpec(247-283)PostgresConfig(46-97)
internal/controller/modelregistry_controller.go (1)
api/v1beta1/modelregistry_types.go (2)
PostgresConfig(46-97)SecretKeyValue(37-44)
api/v1alpha1/zz_generated.conversion.go (3)
api/v1alpha1/modelregistry_types.go (1)
ModelRegistrySpec(368-408)api/v1beta1/modelregistry_types.go (1)
ModelRegistrySpec(247-283)api/v1alpha1/zz_generated.manual.conversion.go (1)
Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec(8-15)
🔇 Additional comments (5)
api/v1alpha1/modelregistry_webhook.go (1)
63-70: Default(): correct preservation of Postgres spec during auto-provisioning.Good fix. Previously pruning Postgres when Host/HostAddress were empty broke the “just set generateDeployment” UX.
api/v1alpha1/zz_generated.conversion.go (3)
416-416: Carry overGenerateDeploymentduring v1alpha1 -> v1beta1 conversion.Looks correct; pointer cast matches neighbors and preserves user intent across versions.
439-439: Carry overGenerateDeploymentduring v1beta1 -> v1alpha1 conversion.Symmetric with the forward conversion; LGTM.
145-149: Manual conversion function verified and approved
- Confirmed that
api/v1alpha1/zz_generated.manual.conversion.godefines
Convert_v1beta1_ModelRegistrySpec_To_v1alpha1_ModelRegistrySpec, calling the autogenerated conversion and explicitly ignoring theDatabasefield absent in v1alpha1.- No additional manual adjustments are necessary, as version-specific differences are already handled.
api/v1beta1/zz_generated.deepcopy.go (1)
310-314: Deep-copy and conversion logic forGenerateDeploymentis consistent across versionsThe
GenerateDeploymentpointer is handled identically in both v1beta1 and v1alpha1 deepcopy implementations, and conversion functions correctly round-trip the field in both directions.• v1beta1 deepcopy (api/v1beta1/zz_generated.deepcopy.go:310–314) and v1alpha1 deepcopy (api/v1alpha1/zz_generated.deepcopy.go:374–377) both allocate and copy the
*boolvalue when non-nil.
• Conversions in v1alpha1 → v1beta1 (api/v1alpha1/zz_generated.conversion.go:414–418) and v1beta1 → v1alpha1 (api/v1alpha1/zz_generated.conversion.go:437–441) useunsafe.Pointerto preserve theGenerateDeploymentpointer and its value.No further action required; deep-copy and conversions for
GenerateDeploymentare correctly implemented.
| // The database name. | ||
| Database string `json:"database"` | ||
| Database string `json:"database,omitempty"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Making database optional needs compensating validation.
Removing the required constraint for PostgresConfig.Database is fine for auto-provisioning, but for user-supplied/external Postgres this can allow invalid specs to pass admission and fail later at runtime. Ensure the webhook enforces database (and typically username, and either host or hostAddress) when auto-provisioning is NOT enabled.
I’ve suggested concrete webhook validation in api/v1alpha1/modelregistry_webhook.go to cover this.
🤖 Prompt for AI Agents
In api/v1alpha1/modelregistry_types.go at line 65 the PostgresConfig.Database
field was made optional but there is no compensating webhook validation; update
the mutating/validating webhook (api/v1alpha1/modelregistry_webhook.go) to
enforce that when auto-provisioning is disabled the spec must include database
and username and must provide either host or hostAddress (i.e., add checks that
if .Spec.Postgres.AutoProvisioning == false then .Spec.Postgres.Database != \"\"
and .Spec.Postgres.Username != \"\" and ( .Spec.Postgres.Host != \"\" ||
.Spec.Postgres.HostAddress != \"\" ), returning a validation error if any are
missing).
| hasPostgres := r.Spec.Postgres != nil | ||
| hasMySQL := r.Spec.MySQL != nil | ||
| hasAutoProvisioning := hasPostgres && r.Spec.Postgres.GenerateDeployment != nil && *r.Spec.Postgres.GenerateDeployment | ||
|
|
||
| if !hasPostgres && !hasMySQL { | ||
| return nil, field.ErrorList{ | ||
| field.Required(field.NewPath("spec").Child("postgres"), "required one of `postgres` or `mysql` database"), | ||
| field.Required(field.NewPath("spec").Child("mysql"), "required one of `postgres` or `mysql` database"), | ||
| } | ||
| } | ||
|
|
||
| if hasAutoProvisioning { | ||
| // When auto-provisioning is enabled, host/hostAddress should not be set (they will be auto-generated) | ||
| if len(r.Spec.Postgres.Host) > 0 || len(r.Spec.Postgres.HostAddress) > 0 { | ||
| return nil, field.ErrorList{ | ||
| field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "host should not be set when auto-provisioning is enabled"), | ||
| } | ||
| } | ||
| // Note: database and username CAN be set when auto-provisioning - they specify the desired values | ||
| } | ||
|
|
||
| return nil, nil |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Strengthen auto-provisioning validation and add non-autoprovisioned requireds.
Current logic only blocks host, ignoring hostAddress, and allows conflicting fields like passwordSecret, port, and skipDBCreation. Also, when not auto-provisioning, there’s no check that host/hostAddress, database, and username are set after removing their required tags from the CRD.
Apply this diff to extend validation:
@@
- hasPostgres := r.Spec.Postgres != nil
- hasMySQL := r.Spec.MySQL != nil
- hasAutoProvisioning := hasPostgres && r.Spec.Postgres.GenerateDeployment != nil && *r.Spec.Postgres.GenerateDeployment
+ hasPostgres := r.Spec.Postgres != nil
+ hasMySQL := r.Spec.MySQL != nil
+ hasAutoProvisioning := hasPostgres && r.Spec.Postgres.GenerateDeployment != nil && *r.Spec.Postgres.GenerateDeployment
@@
- if !hasPostgres && !hasMySQL {
+ if !hasPostgres && !hasMySQL {
return nil, field.ErrorList{
field.Required(field.NewPath("spec").Child("postgres"), "required one of `postgres` or `mysql` database"),
field.Required(field.NewPath("spec").Child("mysql"), "required one of `postgres` or `mysql` database"),
}
}
- if hasAutoProvisioning {
- // When auto-provisioning is enabled, host/hostAddress should not be set (they will be auto-generated)
- if len(r.Spec.Postgres.Host) > 0 || len(r.Spec.Postgres.HostAddress) > 0 {
- return nil, field.ErrorList{
- field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "host should not be set when auto-provisioning is enabled"),
- }
- }
- // Note: database and username CAN be set when auto-provisioning - they specify the desired values
- }
+ if hasAutoProvisioning {
+ var errs field.ErrorList
+ // Disallow fields that will be auto-generated or ignored
+ if len(r.Spec.Postgres.Host) > 0 {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "must be empty when auto-provisioning is enabled"))
+ }
+ if len(r.Spec.Postgres.HostAddress) > 0 {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("hostAddress"), r.Spec.Postgres.HostAddress, "must be empty when auto-provisioning is enabled"))
+ }
+ if r.Spec.Postgres.Port != nil {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("port"), *r.Spec.Postgres.Port, "must be empty; operator uses 5432 for the managed Postgres Service"))
+ }
+ if r.Spec.Postgres.PasswordSecret != nil {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("passwordSecret"), r.Spec.Postgres.PasswordSecret, "must be empty; credentials are generated and stored in an operator-managed Secret"))
+ }
+ if r.Spec.Postgres.SkipDBCreation {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("skipDBCreation"), r.Spec.Postgres.SkipDBCreation, "cannot skip DB creation when auto-provisioning"))
+ }
+ if len(errs) > 0 {
+ return nil, errs
+ }
+ // database and username may be set to request desired values
+ } else if hasPostgres {
+ // Manual/external Postgres: enforce requireds that were removed from the CRD
+ var errs field.ErrorList
+ if len(r.Spec.Postgres.Host) == 0 && len(r.Spec.Postgres.HostAddress) == 0 {
+ errs = append(errs, field.Required(field.NewPath("spec").Child("postgres").Child("host"), "either host or hostAddress must be set when not auto-provisioning"))
+ }
+ if len(r.Spec.Postgres.Database) == 0 {
+ errs = append(errs, field.Required(field.NewPath("spec").Child("postgres").Child("database"), "database is required when not auto-provisioning"))
+ }
+ if len(r.Spec.Postgres.Username) == 0 {
+ errs = append(errs, field.Required(field.NewPath("spec").Child("postgres").Child("username"), "username is required when not auto-provisioning"))
+ }
+ if len(errs) > 0 {
+ return nil, errs
+ }
+ }To ensure v1beta1 webhook mirrors these checks, search for the v1beta1 implementation and confirm similar validations exist:
🏁 Script executed:
#!/bin/bash
# Locate v1beta1 ValidateDatabase and check it references generateDeployment and forbids host/hostAddress.
rg -nP -C3 'func\s+\(r\s+\*ModelRegistry\)\s+ValidateDatabase\(\)' api/v1beta1 | cat
rg -nP -C2 'GenerateDeployment|hostAddress|passwordSecret|skipDBCreation|port' api/v1beta1 | catLength of output: 13769
🏁 Script executed:
#!/bin/bash
# Extract the ValidateDatabase function body from the v1beta1 webhook implementation
awk '
/func \(r \*ModelRegistry\) ValidateDatabase\(\)/ {inside=1}
inside { print }
/return nil, nil/ && inside { exit }
' api/v1beta1/modelregistry_webhook.goLength of output: 1258
Ensure v1beta1 ValidateDatabase mirrors the enhanced Postgres validation
The ValidateDatabase method in api/v1beta1/modelregistry_webhook.go currently only checks that host (and implicitly hostAddress) are unset when auto-provisioning, and even then only reports an error on host. It does not:
- Forbid
hostAddressin the error list - Forbid
port,passwordSecret, orskipDBCreationwhenGenerateDeploymentis true - Enforce that
host/hostAddress,database, andusernameare provided when not auto-provisioning
Please extend ValidateDatabase() in v1beta1 to match the v1alpha1 refactor:
• In the hasAutoProvisioning block, disallow and error on any of:
– spec.postgres.host
– spec.postgres.hostAddress
– spec.postgres.port
– spec.postgres.passwordSecret
– spec.postgres.skipDBCreation
• In the !hasAutoProvisioning && hasPostgres block, require and error on any missing:
– spec.postgres.host or spec.postgres.hostAddress
– spec.postgres.database
– spec.postgres.username
— Diff sketch for api/v1beta1/modelregistry_webhook.go ValidateDatabase:
@@ func (r *ModelRegistry) ValidateDatabase() (admission.Warnings, field.ErrorList) {
- if hasAutoProvisioning {
- if len(r.Spec.Postgres.Host) > 0 || len(r.Spec.Postgres.HostAddress) > 0 {
- return nil, field.ErrorList{
- field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "host should not be set when auto-provisioning is enabled"),
- }
- }
- }
+ if hasAutoProvisioning {
+ var errs field.ErrorList
+ if len(r.Spec.Postgres.Host) > 0 {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("host"), r.Spec.Postgres.Host, "must be empty when auto-provisioning is enabled"))
+ }
+ if len(r.Spec.Postgres.HostAddress) > 0 {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("hostAddress"), r.Spec.Postgres.HostAddress, "must be empty when auto-provisioning is enabled"))
+ }
+ if r.Spec.Postgres.Port != nil {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("port"), *r.Spec.Postgres.Port, "must be empty; operator uses default port"))
+ }
+ if r.Spec.Postgres.PasswordSecret != nil {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("passwordSecret"), r.Spec.Postgres.PasswordSecret, "must be empty; credentials are operator-managed"))
+ }
+ if r.Spec.Postgres.SkipDBCreation {
+ errs = append(errs, field.Invalid(field.NewPath("spec").Child("postgres").Child("skipDBCreation"), r.Spec.Postgres.SkipDBCreation, "cannot skip DB creation when auto-provisioning"))
+ }
+ if len(errs) > 0 {
+ return nil, errs
+ }
+ }
+} else if hasPostgres {
+ // Manual Postgres must specify connection details
+ var errs field.ErrorList
+ if len(r.Spec.Postgres.Host) == 0 && len(r.Spec.Postgres.HostAddress) == 0 {
+ errs = append(errs, field.Required(field.NewPath("spec").Child("postgres").Child("host"), "either host or hostAddress must be set when not auto-provisioning"))
+ }
+ if len(r.Spec.Postgres.Database) == 0 {
+ errs = append(errs, field.Required(field.NewPath("spec").Child("postgres").Child("database"), "database is required when not auto-provisioning"))
+ }
+ if len(r.Spec.Postgres.Username) == 0 {
+ errs = append(errs, field.Required(field.NewPath("spec").Child("postgres").Child("username"), "username is required when not auto-provisioning"))
+ }
+ if len(errs) > 0 {
+ return nil, errs
+ }Please apply these changes to bring v1beta1 in line with the strengthened validation in v1alpha1.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/v1alpha1/modelregistry_webhook.go around lines 245 to 266, the
ValidateDatabase logic must be extended to mirror the v1beta1 refactor: when
hasAutoProvisioning is true, add validation errors forbidding any of
spec.postgres.host, spec.postgres.hostAddress, spec.postgres.port,
spec.postgres.passwordSecret, and spec.postgres.skipDBCreation (use
field.Invalid with appropriate paths and values); when not hasAutoProvisioning
but hasPostgres, add required errors for missing spec.postgres.host OR
spec.postgres.hostAddress, spec.postgres.database, and spec.postgres.username
(use field.Required with the correct child paths); ensure returned ErrorList
contains all relevant field errors and keep existing behavior for mysql checks
unchanged.
| // The database name. | ||
| Database string `json:"database"` | ||
| Database string `json:"database,omitempty"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Making database optional requires webhook-side validation (v1beta1 too).
Same concern as v1alpha1: without the old required markers, non-autoprovisioned Postgres should still require database (and typically username, and either host or hostAddress). Ensure v1beta1 webhook enforces this.
If already implemented in v1beta1 webhook, please add/extend tests to cover it (see test file suggestions).
🤖 Prompt for AI Agents
In api/v1beta1/modelregistry_types.go around line 66, making the `database`
field optional requires mirroring webhook-side validation for v1beta1: update
the v1beta1 admission/validation webhook to enforce that when a model registry
is NOT using autoprovisioned Postgres, `database` is required (and likewise
validate `username` is present and that at least one of `host` or `hostAddress`
is provided); add unit tests in the v1beta1 webhook test suite that cover
missing `database`, missing `username`, and missing `host`+`hostAddress` for
non-autoprovisioned Postgres cases, and extend any existing test helpers from
v1alpha1 equivalents to exercise these validation paths.
Signed-off-by: Chanakya Thirumala Setty <[email protected]>
Signed-off-by: Chanakya Thirumala Setty <[email protected]>
Signed-off-by: Chanakya Thirumala Setty <[email protected]>
Signed-off-by: Chanakya Thirumala Setty <[email protected]>
Signed-off-by: Chanakya Thirumala Setty <[email protected]>
Signed-off-by: Chanakya Thirumala Setty <[email protected]>
…ry.yaml fix: Fix extra whitespace and missing EOP newline Signed-off-by: Chris hambridge <[email protected]>
fix: Fix extra whitespace and missing EOP newline Signed-off-by: Chris hambridge <[email protected]>
Signed-off-by: Chris Hambridge <[email protected]>
Signed-off-by: Chris Hambridge <[email protected]>
chambridge
force-pushed
the
default-postgres
branch
from
3e4557d to
9b82b90
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
api/v1alpha1/modelregistry_types.go (1)
65-65: Making database optional requires compensating validation.If Postgres isn’t auto-provisioned, require database/username and host or hostAddress via webhook or CRD XValidations to avoid late runtime failures. Mirrors a prior comment.
Would you confirm that api/v1alpha1 and v1beta1 webhooks enforce:
- when generate is false: database != "", username != "", and (host != "" || hostAddress != "")?
internal/controller/modelregistry_controller.go (1)
344-349: Guard auto-provisioning when external Postgres is configured and propagate its result.Skip provisioning if host/hostAddress are set, and don’t drop the OperationResult.
- if registry.Spec.Postgres != nil && registry.Spec.Postgres.GenerateDeployment != nil && *registry.Spec.Postgres.GenerateDeployment { - result, err = r.createOrUpdatePostgres(ctx, params, registry) - if err != nil { - return result, err - } - } + if pg := registry.Spec.Postgres; pg != nil && pg.GenerateDeployment != nil && *pg.GenerateDeployment { + if pg.Host == "" && pg.HostAddress == "" { + if res, err := r.createOrUpdatePostgres(ctx, params, registry); err != nil { + return res, err + } else if res != ResourceUnchanged { + result = res + } + } else { + klog.FromContext(ctx).V(1).Info("Skipping auto-provisioned Postgres: external host configured") + } + }
🧹 Nitpick comments (3)
api/v1alpha1/modelregistry_types.go (1)
72-74: Rename JSON tag togenerate
Update both v1alpha1 and v1beta1 to usejson:"generate,omitempty"instead ofjson:"generateDeployment,omitempty", to reflect intent and match the PR description. After making this change, runmake manifests(orcontroller-gen) to regenerate the CRD schema underconfig/crdand refresh sample YAMLs.internal/controller/modelregistry_controller_test.go (1)
302-343: Expand assertions: verify ownerRefs and spec-driven wiring.Add checks that:
- Secret/Deployment/Service/PVC have ownerReferences to the ModelRegistry (cleanup guarantees).
- The ModelRegistry Deployment points to the generated Secret/Service (env vars or DSN), proving spec propagation.
@@ By("Checking if the Postgres Secret was successfully created in the reconciliation") Eventually(func() error { found := &corev1.Secret{} return k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres-credentials", Namespace: namespace.Name}, found) }, time.Minute, time.Second).Should(Succeed()) + // ownerRef present + secret := &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres-credentials", Namespace: namespace.Name}, secret)).To(Succeed()) + Expect(secret.OwnerReferences).ToNot(BeEmpty()) @@ Eventually(func() error { found := &appsv1.Deployment{} return k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres", Namespace: namespace.Name}, found) }, time.Minute, time.Second).Should(Succeed()) + dep := &appsv1.Deployment{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres", Namespace: namespace.Name}, dep)).To(Succeed()) + Expect(dep.OwnerReferences).ToNot(BeEmpty()) @@ Eventually(func() error { found := &corev1.Service{} return k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres", Namespace: namespace.Name}, found) }, time.Minute, time.Second).Should(Succeed()) + svc := &corev1.Service{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres", Namespace: namespace.Name}, svc)).To(Succeed()) + Expect(svc.OwnerReferences).ToNot(BeEmpty()) @@ Eventually(func() error { found := &corev1.PersistentVolumeClaim{} return k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres-storage", Namespace: namespace.Name}, found) }, time.Minute, time.Second).Should(Succeed()) + pvc := &corev1.PersistentVolumeClaim{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: registryName + "-postgres-storage", Namespace: namespace.Name}, pvc)).To(Succeed()) + Expect(pvc.OwnerReferences).ToNot(BeEmpty()) + + // MR Deployment wiring sanity check (uses generated service/secret) + mrDep := &appsv1.Deployment{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: registryName, Namespace: namespace.Name}, mrDep)).To(Succeed()) + // Example: verify env var points to <name>-postgres service + // (adjust to your template's actual env var names) + var hasHost bool + for _, c := range mrDep.Spec.Template.Spec.Containers { + for _, e := range c.Env { + if e.Name == "POSTGRES_HOST" && e.Value == registryName+"-postgres" { + hasHost = true + } + } + } + Expect(hasHost).To(BeTrue())internal/controller/modelregistry_controller.go (1)
323-327: RBAC duplication: secrets listed twice.Line 323 already includes secrets with full verbs; line 326 re-declares secrets. Drop the duplicate to keep the generated ClusterRole minimal.
-// +kubebuilder:rbac:groups=core,resources=secrets,verbs=create;get;list;watch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
api/v1alpha1/modelregistry_types.go(1 hunks)api/v1alpha1/modelregistry_webhook.go(2 hunks)api/v1alpha1/zz_generated.conversion.go(3 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/v1alpha1/zz_generated.manual.conversion.go(1 hunks)api/v1beta1/modelregistry_types.go(1 hunks)api/v1beta1/modelregistry_webhook.go(2 hunks)api/v1beta1/modelregistry_webhook_test.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(1 hunks)config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml(2 hunks)config/rbac/role.yaml(1 hunks)config/samples/postgres-auto/kustomization.yaml(1 hunks)config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml(1 hunks)internal/controller/config/templates/postgres-deployment.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-pvc.yaml.tmpl(1 hunks)internal/controller/config/templates/postgres-service.yaml.tmpl(1 hunks)internal/controller/modelregistry_controller.go(3 hunks)internal/controller/modelregistry_controller_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/controller/config/templates/postgres-pvc.yaml.tmpl
- config/samples/postgres-auto/kustomization.yaml
- config/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yaml
- config/rbac/role.yaml
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1beta1/zz_generated.deepcopy.go
- api/v1beta1/modelregistry_webhook_test.go
- api/v1beta1/modelregistry_webhook.go
- internal/controller/config/templates/postgres-deployment.yaml.tmpl
- api/v1beta1/modelregistry_types.go
- api/v1alpha1/zz_generated.manual.conversion.go
- config/crd/bases/modelregistry.opendatahub.io_modelregistries.yaml
- internal/controller/config/templates/postgres-service.yaml.tmpl
- api/v1alpha1/modelregistry_webhook.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T19:08:33.905Z
Learnt from: pboyd
PR: opendatahub-io/model-registry-operator#276
File: internal/controller/modelregistry_catalog_test.go:435-442
Timestamp: 2025-08-01T19:08:33.905Z
Learning: In the ModelRegistry catalog tests in `internal/controller/modelregistry_catalog_test.go`, the OpenShift-specific deletion tests correctly check for Route resource deletion using the Route variable and the `%s-catalog-https` naming pattern, which is separate from the general catalog Deployment deletion tests.
Applied to files:
internal/controller/modelregistry_controller_test.go
🧬 Code graph analysis (1)
internal/controller/modelregistry_controller_test.go (1)
internal/controller/config/defaults.go (1)
OAuthProxyImage(47-47)
🔇 Additional comments (4)
api/v1alpha1/zz_generated.conversion.go (2)
145-149: Manual conversion registration LGTM.Registering the v1beta1 -> v1alpha1 ModelRegistrySpec manual conversion here is correct given the wrapper lives in zz_generated.manual.conversion.go.
416-417: Propagating GenerateDeployment is correct.Copying the pointer bool across versions keeps parity for the new flag.
Also applies to: 439-439
internal/controller/modelregistry_controller_test.go (1)
435-435: Good cleanup.Unsetting OAuthProxyImage avoids cross-test contamination.
internal/controller/modelregistry_controller.go (1)
429-526: Generate strong random credentials and propagate all OperationResult values
- Replace the hard-coded
"password"with a crypto/rand–based generator.- Capture the
OperationResultreturned by eachr.createOrUpdatecall (Secret, PVC, Deployment, Service) and merge it into the overallresultso that any Created/Updated status triggers a requeue/status update.
| spec: | ||
| containers: | ||
| - name: postgres | ||
| image: quay.io/sclorg/postgresql-13-c9s:latest |
There was a problem hiding this comment.
Why such an old version? Can we use 15 at least?
There was a problem hiding this comment.
seems this is the [16]( sclorg / postgresql-16-c10s) version I could find; no 17 version yet. BTW the same image for RHEL image is also available.
feat: Add auto-provisioning of PostgreSQL database with persistent storage
Description
This pull request introduces a new feature that simplifies the deployment of the Model Registry Operator by automatically provisioning a PostgreSQL database with persistent storage when no database connection details are provided. This lowers the barrier to entry for new users and provides a more streamlined out-of-the-box experience.
The key changes in this PR include:
ModelRegistryCRD has been updated to include a newpostgres.generateflag. When this flag is set totrue, the operator will automatically provision a PostgreSQL database. This field is located under thepostgresconfiguration section, making it clear that auto-provisioning is specific to PostgreSQL databases.PersistentVolumeClaim(PVC) to ensure data persistence across pod restarts, providing a production-ready storage solution.Deployment,Service,Secret, andPersistentVolumeClaimfor the PostgreSQL instance whenpostgres.generateis enabled.postgres-deployment.yaml.tmpl,postgres-service.yaml.tmpl, andpostgres-pvc.yaml.tmpltemplates have been added to define the resources for the auto-provisioned database.quay.io/sclorg/postgresql-13-c9s:latest) with proper environment variables and data directory configuration for OpenShift/ODH compatibility.SecretandPersistentVolumeClaimresources.generatefield has been moved from a top-leveldatabaseconfiguration to be a field under thepostgresconfiguration, as PostgreSQL is currently the only database type that supports auto-provisioning.Configuration Example
How Has This Been Tested?
This feature has been thoroughly tested in a KIND cluster environment. The testing process involved the following comprehensive steps:
make docker-build IMG=localhost/model-registry-operator:test-v4kind load docker-image localhost/model-registry-operator:test-v4make install && make deployconfig/samples/postgres-auto/modelregistry_v1beta1_modelregistry.yamlsample withpostgres.generate: truemodelregistryusercurl http://localhost:8080/api/model_registry/v1alpha3/registered_models # Returns: {"items":[],"nextPageToken":"","pageSize":0,"size":0}/var/lib/pgsql/data) works correctly with Red Hat PostgreSQL imageTechnical Details
generatefield is now properly located underpostgresconfiguration rather than as a separate top-leveldatabasefieldPOSTGRESQL_*) and data directory (/var/lib/pgsql/data)modelregistryuser to avoid conflicts with built-in PostgreSQL usersBreaking Changes
None. This feature is additive and maintains full backward compatibility with existing configurations.
Merge criteria:
Summary by CodeRabbit
New Features
Improvements
Tests
Bug Fixes