[WIP] DO NOT MERGE - Extension mechanism in BFF modular architecture #5572
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis pull request refactors the model registry BFF to decouple catalog settings from dependency injection while introducing a handler override mechanism. Key changes include: (1) simplifying Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/model-registry/upstream/bff/internal/api/model_registry_settings_handler.go (1)
20-50: Handlers should return immediately afterbadRequestResponseAll the settings handlers (
GetAllModelRegistriesSettingsHandler,GetModelRegistrySettingsHandler,CreateModelRegistrySettingsHandler,UpdateModelRegistrySettingsHandler,DeleteModelRegistrySettingsHandler) validate the namespace and callapp.badRequestResponsewhen it’s missing, but they don’t return afterward. The code then continues, builds sample registries, and callsWriteJSONagain.That leads to two problems when the namespace is absent:
- Using an empty namespace to build mock data (incorrect behavior).
- Attempting to write a second response after having already written an error response, which can trigger
http: superfluous response.WriteHeaderwarnings and confusing client behavior.Add
returnstatements immediately after eachapp.badRequestResponse(...)to stop further processing in the error case. For example:- if !ok || namespace == "" { - app.badRequestResponse(w, r, fmt.Errorf("missing namespace in the context")) - } + if !ok || namespace == "" { + app.badRequestResponse(w, r, fmt.Errorf("missing namespace in the context")) + return + }Apply the same pattern to the other handlers in this file.
Also applies to: 53-61, 79-87, 107-115, 129-137
🧹 Nitpick comments (13)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (1)
12-15: Address the TODO before merging.The TODO comment indicates uncertainty about whether
sources.yamlis the correct key. This should be verified and the TODO resolved before the PR is ready for merge.Would you like me to help search for documentation or usages that confirm the correct config map key?
packages/model-registry/upstream/bff/internal/integrations/kubernetes/shared_k8s_client.go (1)
162-199: Consider adding a context timeout for consistency.Other methods in this file (e.g.,
GetServiceDetails,GetServiceDetailsByName) create a derived context with a 30-second timeout. This method passessessionCtxdirectly to the Kubernetes API. Consider adding a timeout for consistency and to prevent indefinite blocking:func (kc *SharedClientLogic) GetAllCatalogSourceConfigs( sessionCtx context.Context, namespace string, ) (corev1.ConfigMap, corev1.ConfigMap, error) { if namespace == "" { return corev1.ConfigMap{}, corev1.ConfigMap{}, fmt.Errorf("namespace cannot be empty") } + ctx, cancel := context.WithTimeout(sessionCtx, 30*time.Second) + defer cancel() + sessionLogger := sessionCtx.Value(constants.TraceLoggerKey).(*slog.Logger) // Fetch default sources defaultCM, err := kc.Client.CoreV1(). ConfigMaps(namespace). - Get(sessionCtx, CatalogSourceDefaultConfigMapName, metav1.GetOptions{}) + Get(ctx, CatalogSourceDefaultConfigMapName, metav1.GetOptions{})packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_conversion.go (2)
89-100: Consider logging or surfacing the secret extraction error.When
getDatabaseSecretValuefails, the error is silently ignored andValueremains empty. This could mask configuration issues or permission problems. Consider at least logging the error:if extractSecrets && typed.Spec.DatabaseConfig.PasswordSecret.Name != "" { val, err := getDatabaseSecretValue( ctx, coreClient, typed.Metadata.Namespace, typed.Spec.DatabaseConfig.PasswordSecret.Name, typed.Spec.DatabaseConfig.PasswordSecret.Key, ) - if err == nil { - typed.Spec.DatabaseConfig.PasswordSecret.Value = val + if err != nil { + // Log but don't fail - secret extraction is optional + // Consider: log.Warn("failed to extract password secret", "error", err) + } else { + typed.Spec.DatabaseConfig.PasswordSecret.Value = val } }
107-119: Consider adding a context timeout for the secret fetch.The secret fetch uses the passed context directly. If no timeout is set upstream, this could block indefinitely. Consider adding a timeout:
func getDatabaseSecretValue(ctx context.Context, coreClient kubernetes.Interface, namespace, secretName, key string) (string, error) { + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + secret, err := coreClient.CoreV1().Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{})packages/model-registry/upstream/bff/internal/redhat/docs/extensions.md (1)
13-15: Clarify directory and entrypoint paths in the docThe doc refers to downstream artifacts under
clients/ui/bff/internal/redhatand a blank import inclients/ui/bff/cmd/main.go, while the code here lives underpackages/model-registry/upstream/bff/internal/redhat/...andpackages/model-registry/upstream/bff/cmd/main.go. It would help future readers to clarify whether theseclients/ui/...paths are conceptual, refer to a different repo layout, or should be updated to match the paths actually used here.Also applies to: 68-75
packages/model-registry/upstream/bff/internal/api/app.go (1)
177-199: Repositories wiring is correct, but there’s a dead error check nearby
repositories.NewRepositories(mrClient, modelCatalogClient)matches the new constructor signature and centralizesModelCatalogSettingsRepositorycreation inrepositories.go, which is good.- However, the
if err != nil { return nil, fmt.Errorf("failed to create ModelCatalogSettings client: %w", err) }block above is now unreachable:errhas already been checked after creating the catalog client, and no additional client is created in between.You can safely drop that extra error check to reduce confusion.
packages/model-registry/upstream/bff/internal/api/test_app.go (1)
12-25: NewTestApp is a useful minimal constructor for testsThe helper cleanly bypasses production bootstrap while still wiring config, logger (with a sensible nil default), Kubernetes client factory, and repositories. This should simplify unit tests and downstream extensions that need a lightweight
Appinstance.You might add a short doc note clarifying that
factoryandreposmay be nil and that it’s the caller’s responsibility to provide the dependencies required by their specific tests.packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
11-11: Field type changed from interface to concrete pointer.The field
ModelCatalogSettingsRepositoryis now a concrete pointer type*ModelCatalogSettingsRepositoryrather than an interface. This reduces flexibility for dependency injection and mocking in tests. If testability is a concern, consider keeping an interface type here.packages/model-registry/upstream/bff/internal/redhat/handlers/model_registry_settings.go (2)
324-335: Potential nil pointer dereference innamespaceFromContext.On line 326, if
r == nil, the function callsapp.BadRequest(w, r, ...)with the nil request, which may cause issues in the underlying response helper if it accessesr. However, it returns early, so the impact is limited to potentially confusing error logging.Consider returning early without calling
BadRequestifris nil, or logging the error directly:func namespaceFromContext(app *api.App, w http.ResponseWriter, r *http.Request) (string, bool) { if r == nil { - app.BadRequest(w, r, fmt.Errorf("missing request context")) + http.Error(w, "missing request context", http.StatusBadRequest) return "", false }
337-344: Missing nil check for KubernetesClientFactory.If
app.KubernetesClientFactory()returns nil (e.g., during misconfigured test setup), callingGetClientwill panic.func getKubernetesClient(app *api.App, w http.ResponseWriter, r *http.Request) (k8s.KubernetesClientInterface, bool) { + factory := app.KubernetesClientFactory() + if factory == nil { + app.ServerError(w, r, fmt.Errorf("kubernetes client factory not configured")) + return nil, false + } - client, err := app.KubernetesClientFactory().GetClient(r.Context()) + client, err := factory.GetClient(r.Context()) if err != nil { app.ServerError(w, r, fmt.Errorf("failed to get Kubernetes client: %w", err)) return nil, false } return client, true }packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go (1)
139-154: Minor: Remove trailing blank line.There's an extra blank line before the closing brace.
if strSlice, ok := value.([]string); ok { return strSlice } return []string{} - }packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_settings_repository.go (2)
510-514: Simplify key resolution—use the known key directly.The loop iterates over
created.Datato get the first key, but we already know the key since we just created the ConfigMap with it. This logic is unnecessary.- resolvedKey := key - for dataKey := range created.Data { - resolvedKey = dataKey - break - } - - return &models.Entry{Name: created.Name, Key: resolvedKey}, nil + return &models.Entry{Name: created.Name, Key: key}, nil
528-530: Silent no-op when secretRef is incomplete.Returning
nilwhenNameorKeyis empty silently succeeds. Consider logging a warning to aid debugging when password updates are unexpectedly skipped.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/model-registry/upstream/bff/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
packages/model-registry/upstream/bff/cmd/main.go(1 hunks)packages/model-registry/upstream/bff/go.mod(1 hunks)packages/model-registry/upstream/bff/internal/api/app.go(3 hunks)packages/model-registry/upstream/bff/internal/api/app_test.go(1 hunks)packages/model-registry/upstream/bff/internal/api/extensions.go(1 hunks)packages/model-registry/upstream/bff/internal/api/healthcheck__handler_test.go(1 hunks)packages/model-registry/upstream/bff/internal/api/model_catalog_settings_handler.go(5 hunks)packages/model-registry/upstream/bff/internal/api/model_registry_handler_test.go(1 hunks)packages/model-registry/upstream/bff/internal/api/model_registry_settings_groups_handler_test.go(1 hunks)packages/model-registry/upstream/bff/internal/api/model_registry_settings_handler.go(5 hunks)packages/model-registry/upstream/bff/internal/api/model_registry_settings_rbac_handler_test.go(1 hunks)packages/model-registry/upstream/bff/internal/api/namespaces_handler_test.go(2 hunks)packages/model-registry/upstream/bff/internal/api/public_helpers.go(1 hunks)packages/model-registry/upstream/bff/internal/api/suite_test.go(0 hunks)packages/model-registry/upstream/bff/internal/api/test_app.go(1 hunks)packages/model-registry/upstream/bff/internal/api/test_utils.go(1 hunks)packages/model-registry/upstream/bff/internal/api/user_handler_test.go(1 hunks)packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go(2 hunks)packages/model-registry/upstream/bff/internal/integrations/kubernetes/factory.go(3 hunks)packages/model-registry/upstream/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go(3 hunks)packages/model-registry/upstream/bff/internal/integrations/kubernetes/shared_k8s_client.go(1 hunks)packages/model-registry/upstream/bff/internal/integrations/kubernetes/token_k8s_client.go(5 hunks)packages/model-registry/upstream/bff/internal/mocks/model_catalog_settings_mock.go(0 hunks)packages/model-registry/upstream/bff/internal/mocks/static_data_mock.go(2 hunks)packages/model-registry/upstream/bff/internal/models/catalog_source_list.go(1 hunks)packages/model-registry/upstream/bff/internal/models/model_registry_kind.go(1 hunks)packages/model-registry/upstream/bff/internal/redhat/docs/extensions.md(1 hunks)packages/model-registry/upstream/bff/internal/redhat/handlers/model_registry_settings.go(1 hunks)packages/model-registry/upstream/bff/internal/redhat/handlers/model_registry_settings_test.go(1 hunks)packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_conversion.go(1 hunks)packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_settings_repository.go(1 hunks)packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go(4 hunks)packages/model-registry/upstream/bff/internal/repositories/repositories.go(1 hunks)
💤 Files with no reviewable changes (2)
- packages/model-registry/upstream/bff/internal/mocks/model_catalog_settings_mock.go
- packages/model-registry/upstream/bff/internal/api/suite_test.go
🧰 Additional context used
🧬 Code graph analysis (23)
packages/model-registry/upstream/bff/internal/api/user_handler_test.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/api/model_registry_settings_rbac_handler_test.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/api/test_utils.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/api/model_registry_handler_test.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (1)
frontend/src/types.ts (1)
ConfigMap(128-130)
packages/model-registry/upstream/bff/internal/api/app_test.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/api/extensions.go (1)
packages/model-registry/upstream/bff/internal/api/app.go (1)
App(87-96)
packages/model-registry/upstream/bff/internal/api/model_registry_settings_groups_handler_test.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/token_k8s_client.go (2)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (1)
KubernetesClientInterface(17-42)packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (1)
RESTConfig(50-52)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/factory.go (3)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (1)
KubernetesClientInterface(17-42)packages/model-registry/upstream/bff/internal/integrations/kubernetes/token_k8s_client.go (1)
NewTokenKubernetesClient(58-92)packages/model-registry/upstream/bff/internal/config/environment.go (1)
AuthMethodInternal(15-15)
packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_settings_repository.go (1)
packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (7)
RESTConfig(50-52)ModelRegistryKind(7-13)DatabaseConfig(66-76)PasswordSecret(78-82)Metadata(20-25)Entry(61-64)Status(84-86)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go (1)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (4)
CatalogSourceDefaultConfigMapName(14-14)CatalogSourceKey(13-13)CatalogSourceUserConfigMapName(15-15)ComponentLabelValue(9-9)
packages/model-registry/upstream/bff/internal/api/healthcheck__handler_test.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_conversion.go (1)
packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (8)
ModelRegistryKind(7-13)MySQL(57-57)Postgres(58-58)DatabaseType(54-54)DatabaseConfig(66-76)Entry(61-64)PasswordSecret(78-82)Metadata(20-25)
packages/model-registry/upstream/bff/internal/api/model_registry_settings_handler.go (1)
packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (2)
ModelRegistryKind(7-13)Metadata(20-25)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/shared_k8s_client.go (3)
frontend/src/types.ts (1)
ConfigMap(128-130)packages/model-registry/upstream/bff/internal/constants/keys.go (1)
TraceLoggerKey(23-23)packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (2)
CatalogSourceDefaultConfigMapName(14-14)CatalogSourceUserConfigMapName(15-15)
packages/model-registry/upstream/bff/internal/api/namespaces_handler_test.go (1)
packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/redhat/handlers/model_registry_settings.go (8)
packages/model-registry/upstream/bff/internal/api/extensions.go (2)
HandlerID(13-13)HandlerFactory(17-17)packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (4)
ModelRegistryKind(7-13)Status(84-86)Metadata(20-25)ModelRegistryAndCredentials(15-18)packages/model-registry/upstream/bff/internal/api/app.go (2)
App(87-96)ModelRegistryId(32-32)packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_settings_repository.go (1)
NewModelRegistrySettingsRepository(41-43)packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_conversion.go (1)
BuildModelRegistryPatch(183-189)packages/model-registry/upstream/bff/internal/api/model_registry_settings_handler.go (4)
ModelRegistrySettingsListEnvelope(15-15)ModelRegistrySettingsPayloadEnvelope(18-18)ModelRegistrySettingsEnvelope(16-16)ModelRegistryAndCredentialsSettingsEnvelope(17-17)packages/model-registry/upstream/bff/internal/constants/keys.go (1)
NamespaceHeaderParameterKey(10-10)packages/model-registry/upstream/bff/internal/integrations/kubernetes/factory.go (1)
KubernetesClientFactory(19-23)
packages/model-registry/upstream/bff/internal/api/app.go (2)
packages/model-registry/upstream/bff/internal/api/extensions.go (1)
HandlerID(13-13)packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
NewRepositories(16-28)
packages/model-registry/upstream/bff/internal/mocks/static_data_mock.go (3)
packages/model-registry/upstream/bff/internal/models/catalog_source_list.go (1)
CatalogSource(3-10)packages/model-registry/upstream/frontend/src/app/modelCatalogTypes.ts (1)
CatalogSource(19-24)packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (1)
Status(84-86)
packages/model-registry/upstream/bff/internal/api/public_helpers.go (3)
packages/model-registry/upstream/bff/internal/api/app.go (1)
App(87-96)packages/model-registry/upstream/bff/internal/integrations/kubernetes/factory.go (1)
KubernetesClientFactory(19-23)packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
Repositories(4-14)
packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go (2)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (2)
KubernetesClientInterface(17-42)CatalogSourceKey(13-13)packages/model-registry/upstream/bff/internal/models/model_catalog_source_configs.go (2)
CatalogSourceConfigList(19-21)CatalogSourceConfig(3-15)
packages/model-registry/upstream/bff/internal/redhat/handlers/model_registry_settings_test.go (6)
packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (1)
KubernetesClientInterface(17-42)packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (2)
ModelRegistryKind(7-13)Metadata(20-25)packages/model-registry/upstream/bff/internal/api/model_registry_settings_handler.go (3)
ModelRegistrySettingsListEnvelope(15-15)ModelRegistryAndCredentialsSettingsEnvelope(17-17)ModelRegistrySettingsEnvelope(16-16)packages/model-registry/upstream/bff/internal/integrations/kubernetes/factory.go (1)
KubernetesClientFactory(19-23)packages/model-registry/upstream/bff/internal/api/test_app.go (1)
NewTestApp(15-25)packages/gen-ai/bff/internal/integrations/types.go (1)
RequestIdentity(3-6)
🔇 Additional comments (45)
packages/model-registry/upstream/bff/go.mod (1)
14-17: Kubernetes package version consistency mismatch requires verification.The direct Kubernetes dependencies (k8s.io/api, k8s.io/apimachinery, k8s.io/client-go) have been updated to v0.34.2. Verify whether all related Kubernetes packages in the indirect dependencies, particularly k8s.io/apiextensions-apiserver, have also been updated to v0.34.2 for consistency. Cross-version mismatches across Kubernetes packages can introduce subtle compatibility issues.
packages/model-registry/upstream/bff/internal/models/model_registry_kind.go (1)
78-82: LGTM!The addition of the
Valuefield withomitemptyis appropriate for carrying extracted secret data. The comment clearly documents that this field is conditionally populated, which helps prevent misuse.packages/model-registry/upstream/bff/internal/integrations/kubernetes/token_k8s_client.go (3)
19-22: LGTM!Adding the
restConfigfield to store the REST configuration is appropriate for enabling access via the newRESTConfig()accessor.
57-91: LGTM on export and DI support.Exporting
NewTokenKubernetesClientenables the dependency injection pattern used in the factory. The implementation correctly clears all alternative auth mechanisms before setting the bearer token.
254-257: Verify that exposingRESTConfigdoesn't leak credentials.The
restConfigcontains the bearer token (set at line 68). Ensure callers ofRESTConfig()handle the returned config securely and don't inadvertently log or expose the token.packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go (1)
38-41: LGTM on interface extension.The new
GetAllCatalogSourceConfigsmethod signature is appropriate for fetching both default and user ConfigMaps. The TODO is acceptable for a WIP PR.packages/model-registry/upstream/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go (1)
180-221: LGTM on test fixture setup.The new helper functions provide comprehensive test data for catalog sources, correctly using the constants from the kubernetes package.
Also applies to: 223-292
packages/model-registry/upstream/bff/internal/integrations/kubernetes/factory.go (2)
86-99: LGTM on dependency injection pattern.Adding
NewTokenKubernetesClientFnenables proper unit testing by allowing injection of mock client factories. The initialization inNewTokenClientFactorycorrectly wires up the default implementation.
148-157: LGTM on factory selection.The
NewKubernetesClientFactoryfunction cleanly selects between static and token-based factories based on the configuration. Error handling for invalid auth methods is appropriate.packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_conversion.go (3)
16-47: LGTM on conversion logic.The conversion from unstructured to typed model is well-structured, with proper error handling for missing spec fields and unsupported database types.
123-179: LGTM on reverse conversion.The
convertModelToUnstructuredfunction properly handles the transformation back to Kubernetes CR format, correctly removing read-only fields likestatusandcreationTimestamp.
200-212: LGTM on type handling.The
getInthelper correctly handles the common JSON number representations (int,int64,float64) that can result from unmarshaling.packages/model-registry/upstream/bff/cmd/main.go (1)
14-16: Blank import for override registration looks goodUsing a blank import to register handler overrides via
init()is idiomatic and keeps the main wiring minimal. No issues from a startup or dependency perspective.packages/model-registry/upstream/bff/internal/api/test_utils.go (1)
42-47: Repositories constructor usage is consistent
repositories.NewRepositories(mockMRClient, mockModelCatalogClient)matches the updated two‑argument constructor and keeps test wiring aligned with production code.packages/model-registry/upstream/bff/internal/api/user_handler_test.go (1)
29-33: Updated repositories wiring matches new constructorThe switch to
repositories.NewRepositories(mockMRClient, mockModelCatalogClient)correctly follows the updated repositories API; user handler tests should behave the same.packages/model-registry/upstream/bff/internal/api/app_test.go (1)
26-31: Test App now uses the simplified repositories constructorUsing
repositories.NewRepositories(mockMRClient, mockModelCatalogClient)here is consistent with the new repositories API; static file tests should remain unaffected.packages/model-registry/upstream/bff/internal/api/model_registry_handler_test.go (1)
24-28: Repositories initialization aligned with new APIThe test’s App setup now uses the updated
NewRepositories(mockMRClient, mockModelCatalogClient)signature; this keeps the model registry handler tests in sync with the production repositories wiring.packages/model-registry/upstream/bff/internal/api/model_registry_settings_rbac_handler_test.go (1)
28-33: RBAC tests correctly updated to new repositories constructorUsing
repositories.NewRepositories(mockMRClient, mockModelCatalogClient)here is consistent with the shared repositories constructor and should not change RBAC test behavior.packages/model-registry/upstream/bff/internal/models/catalog_source_list.go (1)
3-10: New optional Status and Error fields are backward‑compatibleAdding
StatusandErroras*stringwithomitemptyextends the CatalogSource payload without breaking existing consumers that ignore unknown fields; the struct definition looks good.packages/model-registry/upstream/bff/internal/api/model_registry_settings_groups_handler_test.go (1)
22-28: Updated repositories constructor usage is consistentUsing
repositories.NewRepositories(mockMRClient, mockModelCatalogClient)matches the new two-arg constructor and keeps the test wiring aligned with production wiring. No issues here.packages/model-registry/upstream/bff/internal/api/app.go (2)
78-85: Handler IDs and override hook look consistentThe unexported
HandlerIDconstants for model registry settings CRUD are well-scoped and give clear, stable identifiers for the override registry. Keeping them package-private helps ensure downstream overrides stay decoupled from the upstream implementation details.
260-292: handlerWithOverride integration for settings endpoints is soundWrapping the settings CRUD routes with
app.handlerWithOverride(...)and passing in fully-decorated default handlers (includingAttachNamespace) keeps existing behavior while enabling per-endpoint overrides. This is a clean extension point for downstream implementations.packages/model-registry/upstream/bff/internal/api/healthcheck__handler_test.go (1)
20-24: Healthcheck test wiring matches new repositories constructorUsing
repositories.NewRepositories(mockMRClient, mockModelCatalogClient)keeps the healthcheck test aligned with the repositories API. Given the handler only needs the healthcheck repo, this remains a safe and minimal setup.packages/model-registry/upstream/bff/internal/api/namespaces_handler_test.go (1)
24-32: Namespaces tests correctly updated to the new repositories constructorBoth dev-mode contexts now create
Appinstances withrepositories.NewRepositories(mockMRClient, mockModelCatalogClient), which matches the updated API and keeps the tests consistent with runtime wiring.Also applies to: 133-142
packages/model-registry/upstream/bff/internal/mocks/static_data_mock.go (1)
803-858: Mock catalog sources and configs now better model real states
GetCatalogSourceMocksnow exercisesStatusandErrorcombinations (available,error,disabled), plus an entry withStatus == nilto represent “starting”/unknown. This should make UI and API tests more representative.adminModel3usingEnabled: &disabledBoolwithStatus: &disabledStatusis a nice explicit disabled case.CreateSampleCatalogSourcenow setsIncludedModels/ExcludedModelsonly for non-default catalogs, which matches the typical “default includes everything, non-default applies filters” behavior.These changes look coherent with the
models.CatalogSource/CatalogSourceConfigshapes.Also applies to: 1350-1376
packages/model-registry/upstream/bff/internal/api/extensions.go (1)
1-54: Clean extension mechanism with proper thread-safety.The handler override registry is well-designed:
- RWMutex provides appropriate read/write locking semantics
getHandlerOverrideuses read lock for concurrent readsRegisterHandlerOverrideuses exclusive lock for writes- Nil checks in
logHandlerOverrideprevent panicsThe pattern of allowing downstream to register overrides via
init()and having upstream fall back gracefully is a solid approach for the extension mechanism.packages/model-registry/upstream/bff/internal/repositories/repositories.go (1)
16-27: Simplified constructor removes external dependency injection.The change from 3-param to 2-param constructor is cleaner but now constructs
ModelCatalogSettingsRepositoryinternally. This is acceptable if there's no need to inject different implementations for testing or different environments.packages/model-registry/upstream/bff/internal/redhat/handlers/model_registry_settings_test.go (4)
311-320: Good test isolation pattern witht.Cleanup.The
withRepositoryandwithPatchBuilderhelpers properly restore original values usingt.Cleanup, ensuring test isolation. This is the idiomatic Go approach for test fixtures that modify package-level variables.
345-386: Well-structured mock repository.The mock repository pattern with optional function fields and sensible error defaults ("not implemented") is a clean approach. Each test only needs to set the function it cares about, keeping tests focused and readable.
287-294: Test helper functions are well-organized.The
newRedHatTestApp,noopLogger,failDefault, anddecodeResponsehelpers reduce boilerplate and improve test readability.
331-343:fakeKubeFactory.GetClientreturns nil client.The fake factory returns
nilfor both client and error. Since the handlers may call methods on the client, this could cause nil pointer dereferences if the test flow reaches that code. Verify whether the repository layer is mocked out in tests or if handlers exercise code paths that directly use the client.packages/model-registry/upstream/bff/internal/api/public_helpers.go (1)
35-53: Accessor methods provide clean extension API.These accessors appropriately expose internal App state for use by extensions without breaking encapsulation. The pattern aligns with the extension mechanism design.
packages/model-registry/upstream/bff/internal/redhat/handlers/model_registry_settings.go (5)
29-35: Handler overrides registered viainit()for automatic activation.The init-based registration pattern ensures overrides are installed when the package is imported. This is a common Go idiom for plugin-like behavior and works well with the anonymous import pattern mentioned in the summary.
54-63: Package-level variables enable test injection.Using
vardeclarations fornewModelRegistrySettingsRepositoryandbuildModelRegistryPatchallows tests to override these functions, as demonstrated in the test file'swithRepositoryandwithPatchBuilderhelpers.
65-96: List handler follows a clean pattern.The handler properly:
- Gates on
shouldUseRedHatOverrides- Extracts namespace and validates
- Obtains Kubernetes client with error handling
- Calls repository and handles errors
- Returns structured response
This pattern is consistently applied across all CRUD handlers.
187-257: Update handler supports both explicit patch and model-derived patch.The logic at lines 217-233 handles two input modes:
- Pre-built patch map in
envelope.Data.Patch- Building patch from
envelope.Data.ModelRegistryusingbuildModelRegistryPatchThis flexibility is well-designed, though the discarded return value at line 237 (
result, _, err) should be documented if intentional.
302-311: Clear payload structures for patch endpoint.The envelope and payload types are well-defined with appropriate JSON tags and optional fields using pointers. This supports partial updates cleanly.
packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go (2)
15-17: LGTM!The simplified constructor without the logger parameter aligns with the PR's goal to auto-construct repositories internally.
88-137: LGTM!The YAML parsing logic properly handles the internal struct mapping, optional fields, and nested properties extraction.
packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_settings_repository.go (6)
41-43: LGTM!Constructor properly injects logger for structured logging.
70-87: Consider narrowing the interface assertion.The
restConfigProviderinterface is defined locally but only used here. This is fine for now, but as noted in the TODO, consolidating this into the upstream factory would be cleaner.
336-350: Verify empty password deletion behavior.When
databasePasswordis an empty string, the secret is deleted. Ensure callers understand this convention—passingnilvs""has different semantics (no change vs delete).
451-459: OpenShift-specific annotations and labels are appropriate for downstream.The template annotations and managed labels align with the TODO comment indicating this is downstream-specific logic.
89-125: LGTM!The List implementation properly validates namespace, handles label selectors, and converts results to typed models.
366-426: LGTM!The Delete implementation correctly fetches the existing resource first to identify associated secrets, then cleans up both the ModelRegistry and its password secret with proper dry-run support.
packages/model-registry/upstream/bff/internal/api/model_catalog_settings_handler.go
Outdated
Show resolved
Hide resolved
| if _, err := k8sClient.CoreV1().ConfigMaps(namespace).Create(ctx, cm, metav1.CreateOptions{}); err != nil { | ||
| return fmt.Errorf("failed to create model-catalog-default-sources configmap: %w", err) | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message.
The error message incorrectly references "default-sources" but this function creates the user sources ConfigMap.
if _, err := k8sClient.CoreV1().ConfigMaps(namespace).Create(ctx, cm, metav1.CreateOptions{}); err != nil {
- return fmt.Errorf("failed to create model-catalog-default-sources configmap: %w", err)
+ return fmt.Errorf("failed to create model-catalog-sources configmap: %w", err)
}🤖 Prompt for AI Agents
In
packages/model-registry/upstream/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go
around lines 287-289, the returned error message wrongly mentions
"model-catalog-default-sources" while this block creates the user sources
ConfigMap; change the error string to accurately reference the user sources
ConfigMap (for example "failed to create model-catalog-user-sources configmap"
or include the actual namespace/name), preserving the %w error wrapping so the
original error is still returned.
| if payload.NewDatabaseCACertificate != nil && *payload.NewDatabaseCACertificate != "" { | ||
| if model.Spec.DatabaseConfig.SSLRootCertificateConfigMap == nil || model.Spec.DatabaseConfig.SSLRootCertificateConfigMap.Name == "" { | ||
| return models.ModelRegistryKind{}, errors.New("sslRootCertificateConfigMap.name is required when providing a new CA certificate") | ||
| } | ||
| entry, err := r.createCACertificateConfigMap(ctx, clients, namespace, model.Spec.DatabaseConfig.SSLRootCertificateConfigMap, *payload.NewDatabaseCACertificate) | ||
| if err != nil { | ||
| return models.ModelRegistryKind{}, err | ||
| } | ||
| model.Spec.DatabaseConfig.SSLRootCertificateConfigMap = entry | ||
| } |
There was a problem hiding this comment.
Dry-run contract violation: ConfigMap is created unconditionally.
The CA certificate ConfigMap is created before the createOnce closure and outside the dry-run flow. If dryRunOnly is true, this ConfigMap will still be persisted, violating dry-run semantics.
Move the ConfigMap creation inside createOnce or add a dry-run guard:
+ var caCertEntry *models.Entry
if payload.NewDatabaseCACertificate != nil && *payload.NewDatabaseCACertificate != "" {
if model.Spec.DatabaseConfig.SSLRootCertificateConfigMap == nil || model.Spec.DatabaseConfig.SSLRootCertificateConfigMap.Name == "" {
return models.ModelRegistryKind{}, errors.New("sslRootCertificateConfigMap.name is required when providing a new CA certificate")
}
- entry, err := r.createCACertificateConfigMap(ctx, clients, namespace, model.Spec.DatabaseConfig.SSLRootCertificateConfigMap, *payload.NewDatabaseCACertificate)
- if err != nil {
- return models.ModelRegistryKind{}, err
- }
- model.Spec.DatabaseConfig.SSLRootCertificateConfigMap = entry
+ caCertEntry = model.Spec.DatabaseConfig.SSLRootCertificateConfigMap
}
createOnce := func(dryRun bool) (models.ModelRegistryKind, error) {
modelCopy := model
+
+ if caCertEntry != nil && !dryRun {
+ entry, err := r.createCACertificateConfigMap(ctx, clients, namespace, caCertEntry, *payload.NewDatabaseCACertificate)
+ if err != nil {
+ return models.ModelRegistryKind{}, err
+ }
+ modelCopy.Spec.DatabaseConfig.SSLRootCertificateConfigMap = entry
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_settings_repository.go
around lines 187-196: the code unconditionally creates the CA certificate
ConfigMap during payload handling which violates dry-run semantics because it
runs before the createOnce closure; move the call to
r.createCACertificateConfigMap(...) inside the createOnce closure (or wrap it
with an explicit if !dryRunOnly guard) so the ConfigMap is only persisted when
not performing a dry run, and ensure the created entry is returned/assigned to
model.Spec.DatabaseConfig.SSLRootCertificateConfigMap within that
guarded/closure scope.
| if newCACertificate != nil && *newCACertificate != "" { | ||
| spec, _ := patch["spec"].(map[string]interface{}) | ||
| if spec == nil { | ||
| return models.ModelRegistryKind{}, nil, errors.New("spec patch is required when providing a new CA certificate") | ||
| } | ||
|
|
||
| var dbKey string | ||
| for _, candidate := range []string{"mysql", "postgres"} { | ||
| if _, ok := spec[candidate]; ok { | ||
| dbKey = candidate | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if dbKey == "" { | ||
| return models.ModelRegistryKind{}, nil, errors.New("database configuration must be included in the patch when updating the CA certificate") | ||
| } | ||
|
|
||
| dbSpec, _ := spec[dbKey].(map[string]interface{}) | ||
| if dbSpec == nil { | ||
| return models.ModelRegistryKind{}, nil, errors.New("invalid database spec in patch") | ||
| } | ||
|
|
||
| entryMap, _ := dbSpec["sslRootCertificateConfigMap"].(map[string]interface{}) | ||
| if entryMap == nil { | ||
| return models.ModelRegistryKind{}, nil, errors.New("sslRootCertificateConfigMap is required when uploading a new certificate") | ||
| } | ||
|
|
||
| entry := &models.Entry{ | ||
| Name: getString(entryMap, "name"), | ||
| Key: getString(entryMap, "key"), | ||
| } | ||
| if entry.Name == "" { | ||
| return models.ModelRegistryKind{}, nil, errors.New("sslRootCertificateConfigMap.name is required") | ||
| } | ||
|
|
||
| resolvedEntry, err := r.createCACertificateConfigMap(ctx, clients, namespace, entry, *newCACertificate) | ||
| if err != nil { | ||
| return models.ModelRegistryKind{}, nil, err | ||
| } | ||
|
|
||
| dbSpec["sslRootCertificateConfigMap"] = map[string]interface{}{ | ||
| "name": resolvedEntry.Name, | ||
| "key": resolvedEntry.Key, | ||
| } | ||
| } |
There was a problem hiding this comment.
Same dry-run violation for CA certificate in Patch.
The CA certificate ConfigMap creation at line 303 occurs before the patchOnce dry-run logic, causing the ConfigMap to persist even when dryRunOnly is true.
Defer the actual ConfigMap creation to inside the patchOnce closure when dryRun is false, similar to the fix suggested for Create.
🤖 Prompt for AI Agents
In
packages/model-registry/upstream/bff/internal/redhat/repositories/model_registry_settings_repository.go
around lines 267 to 312, the code creates the CA certificate ConfigMap
(createCACertificateConfigMap) unconditionally before the patchOnce/dry-run
logic, causing persistent resources during dry-run; move the actual call to
createCACertificateConfigMap and the replacement of
dbSpec["sslRootCertificateConfigMap"] with the resolved name/key into the
patchOnce closure and only execute it when dryRunOnly is false, leaving the
patch data referencing the original entry for dry-run; ensure errors from
createCACertificateConfigMap are returned from within the closure and that
dbSpec remains valid outside the closure so validation still occurs before
patchOnce.
packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go
Show resolved
Hide resolved
| if raw, ok := userCM.Data[k8s.CatalogSourceKey]; ok { | ||
| userManagedSources, err := parseCatalogYaml(raw, false) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse default catalogs: %w", err) | ||
| } | ||
| catalogSources.Catalogs = append(catalogSources.Catalogs, userManagedSources...) | ||
| } |
There was a problem hiding this comment.
Fix misleading error message.
The error message says "failed to parse default catalogs" but this block parses user-managed catalogs.
if raw, ok := userCM.Data[k8s.CatalogSourceKey]; ok {
userManagedSources, err := parseCatalogYaml(raw, false)
if err != nil {
- return nil, fmt.Errorf("failed to parse default catalogs: %w", err)
+ return nil, fmt.Errorf("failed to parse user catalogs: %w", err)
}
catalogSources.Catalogs = append(catalogSources.Catalogs, userManagedSources...)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if raw, ok := userCM.Data[k8s.CatalogSourceKey]; ok { | |
| userManagedSources, err := parseCatalogYaml(raw, false) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse default catalogs: %w", err) | |
| } | |
| catalogSources.Catalogs = append(catalogSources.Catalogs, userManagedSources...) | |
| } | |
| if raw, ok := userCM.Data[k8s.CatalogSourceKey]; ok { | |
| userManagedSources, err := parseCatalogYaml(raw, false) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse user catalogs: %w", err) | |
| } | |
| catalogSources.Catalogs = append(catalogSources.Catalogs, userManagedSources...) | |
| } |
🤖 Prompt for AI Agents
packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go
around lines 37-43: the error message incorrectly says "failed to parse default
catalogs" while this block parses user-managed catalogs; update the fmt.Errorf
call to a clear, accurate message such as "failed to parse user-managed
catalogs" (optionally include the key or raw value for more context) so the
logged error reflects the actual source being parsed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5572 +/- ##
==========================================
+ Coverage 63.74% 63.89% +0.14%
==========================================
Files 2307 2309 +2
Lines 68812 68838 +26
Branches 17066 17068 +2
==========================================
+ Hits 43865 43981 +116
+ Misses 24947 24857 -90 see 46 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
lucferbux
force-pushed
the
model-registry-extension
branch
from
fc02309 to
55a8df6
Compare
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
github-actions
bot
added
the
Stale
lucferbux
force-pushed
the
model-registry-extension
branch
from
55a8df6 to
faeb34a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
github-actions
bot
removed
the
Stale
2 participants
Description
@mturley @ppadti This is a current implementation of the override, we should first merge kubeflow/model-registry#1935 and then add the overrides. Take this as the final solution.
How Has This Been Tested?
3 terminals:
cd packages/model-registry/upstream make dev-start-federatedThen enable the dev feature flag for Model Registry KF.
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.