Skip to content

Commit 95b149d

Browse files
fix: address review feedback from ExternalModel CRD #586 (#612)
This PR addresses the review comments from @nirrozenbaum on #586 that were not resolved before merge. ##Changes 1. Remove namespace field from credentialRef Comment: "isn't it always the same namespace? is there a use case for other namespace? I would argue that it's the same as MaaSModelRef expecting to reference ExternalModel from the same namespace.. IMO we should be consistent." Remove optional namespace field from ExternalModel.spec.credentialRef Credential Secret must now reside in the same namespace as the ExternalModel (consistent with MaaSModelRef → ExternalModel reference pattern) 2. Clarify rate-limiting model identity Comment: "which field represents the model that MaaS is rate limiting on? is it MaaSModelRef metadata.name? model.ref.Name?" Update documentation to clarify that rate limiting is keyed on MaaSModelRef.metadata.name Add explanation of the relationship between MaaSModelRef and ExternalModel 3. Document single credential limitation Comment: "this works as long as there is a single api key per external model. I'm assuming this won't be the case for long time. just as a fyi" Add documentation note about current single-credential-per-ExternalModel limitation (Optional) Create tracking issue for multi-credential support ## Merge criteria: <!--- This PR will be merged by any repository approver when it meets all the points in the checklist --> <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> - [ ] The commits are squashed in a cohesive manner and have meaningful messages. - [ ] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). - [ ] The developer has manually tested the changes and verified that the changes work <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * Credential references no longer support namespace override; credentials must reside in the same namespace as the `ExternalModel`. * **Validation** * Name fields now require a minimum length of 1 character. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 66781e6 commit 95b149d

File tree

5 files changed

+5
-16
lines changed

5 files changed

+5
-16
lines changed

deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yaml

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,6 @@ spec:
6565
maxLength: 253
6666
minLength: 1
6767
type: string
68-
namespace:
69-
description: Namespace is the namespace of the Secret. Defaults
70-
to the ExternalModel namespace if omitted.
71-
maxLength: 253
72-
type: string
7368
required:
7469
- name
7570
type: object

deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maasmodelrefs.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ spec:
7979
For LLMInferenceService, this is the InferenceService name.
8080
For ExternalModel, this is the ExternalModel CR name.
8181
maxLength: 253
82+
minLength: 1
8283
type: string
8384
required:
8485
- kind

docs/content/reference/crds/external-model.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,7 @@ Defines an external AI/ML model hosted outside the cluster (e.g., OpenAI, Anthro
1414

1515
| Field | Type | Required | Description |
1616
|-------|------|----------|-------------|
17-
| name | string | Yes | Name of the Secret containing the credentials. Max length: 253 characters. |
18-
| namespace | string | No | Namespace of the Secret. Defaults to the ExternalModel's namespace if not specified. |
17+
| name | string | Yes | Name of the Secret containing the credentials. Must be in the same namespace as the ExternalModel. Max length: 253 characters. |
1918

2019
## ExternalModelStatus
2120

maas-controller/api/maas/v1alpha1/maasmodelref_types.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,12 @@ type MaaSModelSpec struct {
3232
}
3333

3434
// CredentialReference references a Kubernetes Secret with provider API credentials.
35+
// The Secret must be in the same namespace as the ExternalModel.
3536
type CredentialReference struct {
3637
// Name is the name of the Secret
3738
// +kubebuilder:validation:MinLength=1
3839
// +kubebuilder:validation:MaxLength=253
3940
Name string `json:"name"`
40-
// Namespace is the namespace of the Secret. Defaults to the ExternalModel namespace if omitted.
41-
// +kubebuilder:validation:MaxLength=253
42-
// +optional
43-
Namespace string `json:"namespace,omitempty"`
4441
}
4542

4643
// ModelReference references a model endpoint in the same namespace.
@@ -55,6 +52,7 @@ type ModelReference struct {
5552
// Name is the name of the model resource.
5653
// For LLMInferenceService, this is the InferenceService name.
5754
// For ExternalModel, this is the ExternalModel CR name.
55+
// +kubebuilder:validation:MinLength=1
5856
// +kubebuilder:validation:MaxLength=253
5957
Name string `json:"name"`
6058
}

maas-controller/pkg/controller/maas/providers_external_test.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -288,16 +288,12 @@ func TestExternalModel_CleanupOnDelete(t *testing.T) {
288288
func TestExternalModel_CredentialRef(t *testing.T) {
289289
externalModel := newExternalModelCR("gpt-4o", "default", "openai", "api.openai.com")
290290
externalModel.Spec.CredentialRef = maasv1alpha1.CredentialReference{
291-
Name: "openai-api-key",
292-
Namespace: "opendatahub",
291+
Name: "openai-api-key",
293292
}
294293

295294
if externalModel.Spec.CredentialRef.Name != "openai-api-key" {
296295
t.Errorf("CredentialRef.Name = %q, want %q", externalModel.Spec.CredentialRef.Name, "openai-api-key")
297296
}
298-
if externalModel.Spec.CredentialRef.Namespace != "opendatahub" {
299-
t.Errorf("CredentialRef.Namespace = %q, want %q", externalModel.Spec.CredentialRef.Namespace, "opendatahub")
300-
}
301297
}
302298

303299
func TestExternalModelRouteResolver(t *testing.T) {

0 commit comments

Comments
 (0)