From fd6f12b819550639bdf45a46cb1ff945a7bcf780 Mon Sep 17 00:00:00 2001 From: Sergey Yedrikov Date: Fri, 19 Dec 2025 22:47:32 -0500 Subject: [PATCH] OLS-2335: Add support for pull secrets for BYOK images --- api/v1alpha1/olsconfig_types.go | 4 + api/v1alpha1/zz_generated.deepcopy.go | 5 ++ ...tspeed-operator.clusterserviceversion.yaml | 3 + .../ols.openshift.io_olsconfigs.yaml | 20 +++++ .../bases/ols.openshift.io_olsconfigs.yaml | 20 +++++ ...tspeed-operator.clusterserviceversion.yaml | 4 + internal/controller/appserver/assets_test.go | 36 +++++++++ internal/controller/appserver/deployment.go | 6 ++ internal/controller/lcore/deployment.go | 6 ++ internal/controller/lcore/deployment_test.go | 59 ++++++++++++++ test/e2e/byok_auth_test.go | 81 +++++++++++++++++++ test/e2e/byok_test.go | 2 +- test/e2e/client.go | 53 ++++++++++++ test/e2e/postgres_restart_test.go | 2 +- test/e2e/rapidast_test.go | 2 +- test/e2e/utils.go | 10 ++- 16 files changed, 308 insertions(+), 5 deletions(-) create mode 100644 test/e2e/byok_auth_test.go diff --git a/api/v1alpha1/olsconfig_types.go b/api/v1alpha1/olsconfig_types.go index 1bc40297a..6f90925df 100644 --- a/api/v1alpha1/olsconfig_types.go +++ b/api/v1alpha1/olsconfig_types.go @@ -229,6 +229,10 @@ type OLSSpec struct { // +kubebuilder:validation:Optional // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Query System Prompt",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:advanced"} QuerySystemPrompt string `json:"querySystemPrompt,omitempty"` + // Pull secrets for BYOK RAG images from image registries requiring authentication + // +kubebuilder:validation:Optional + // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Image Pull Secrets" + ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"` } // Persistent Storage Configuration diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 3e5349ef1..8e3c1071e 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -424,6 +424,11 @@ func (in *OLSSpec) DeepCopyInto(out *OLSSpec) { *out = new(Storage) (*in).DeepCopyInto(*out) } + if in.ImagePullSecrets != nil { + in, out := &in.ImagePullSecrets, &out.ImagePullSecrets + *out = make([]corev1.LocalObjectReference, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OLSSpec. diff --git a/bundle/manifests/lightspeed-operator.clusterserviceversion.yaml b/bundle/manifests/lightspeed-operator.clusterserviceversion.yaml index 128791008..fa0f7a168 100644 --- a/bundle/manifests/lightspeed-operator.clusterserviceversion.yaml +++ b/bundle/manifests/lightspeed-operator.clusterserviceversion.yaml @@ -231,6 +231,9 @@ spec: - description: MCP server container settings. displayName: MCP Server Container path: ols.deployment.mcpServer + - description: Pull secrets for BYOK RAG images from image registries requiring authentication + displayName: Image Pull Secrets + path: ols.imagePullSecrets - description: Enable introspection features displayName: Introspection Enabled path: ols.introspectionEnabled diff --git a/bundle/manifests/ols.openshift.io_olsconfigs.yaml b/bundle/manifests/ols.openshift.io_olsconfigs.yaml index 0cec555bc..bbd74dd18 100644 --- a/bundle/manifests/ols.openshift.io_olsconfigs.yaml +++ b/bundle/manifests/ols.openshift.io_olsconfigs.yaml @@ -4327,6 +4327,26 @@ spec: type: object type: object type: object + imagePullSecrets: + description: Pull secrets for BYOK RAG images from image registries + requiring authentication + items: + description: |- + LocalObjectReference contains enough information to let you locate the + referenced object inside the same namespace. + properties: + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + type: object + x-kubernetes-map-type: atomic + type: array introspectionEnabled: description: Enable introspection features type: boolean diff --git a/config/crd/bases/ols.openshift.io_olsconfigs.yaml b/config/crd/bases/ols.openshift.io_olsconfigs.yaml index 1c853f946..5294958a5 100644 --- a/config/crd/bases/ols.openshift.io_olsconfigs.yaml +++ b/config/crd/bases/ols.openshift.io_olsconfigs.yaml @@ -4327,6 +4327,26 @@ spec: type: object type: object type: object + imagePullSecrets: + description: Pull secrets for BYOK RAG images from image registries + requiring authentication + items: + description: |- + LocalObjectReference contains enough information to let you locate the + referenced object inside the same namespace. + properties: + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + type: object + x-kubernetes-map-type: atomic + type: array introspectionEnabled: description: Enable introspection features type: boolean diff --git a/config/manifests/bases/lightspeed-operator.clusterserviceversion.yaml b/config/manifests/bases/lightspeed-operator.clusterserviceversion.yaml index 183f56121..bd385f894 100644 --- a/config/manifests/bases/lightspeed-operator.clusterserviceversion.yaml +++ b/config/manifests/bases/lightspeed-operator.clusterserviceversion.yaml @@ -198,6 +198,10 @@ spec: - description: MCP server container settings. displayName: MCP Server Container path: ols.deployment.mcpServer + - description: Pull secrets for BYOK RAG images from image registries requiring + authentication + displayName: Image Pull Secrets + path: ols.imagePullSecrets - description: Enable introspection features displayName: Introspection Enabled path: ols.introspectionEnabled diff --git a/internal/controller/appserver/assets_test.go b/internal/controller/appserver/assets_test.go index 61eeba926..7b9b21e70 100644 --- a/internal/controller/appserver/assets_test.go +++ b/internal/controller/appserver/assets_test.go @@ -1557,6 +1557,42 @@ user_data_collector_config: {} }) + It("should generate ImagePullSecrets in the app server deployment pod template", func() { + // there should be no ImagePullSecrets in the app server deployment + // pod template if none are specified in OLSCconfig + Expect(cr.Spec.OLSConfig.ImagePullSecrets).To(BeNil()) + dep, err := GenerateOLSDeployment(testReconcilerInstance, cr) + Expect(err).NotTo(HaveOccurred()) + Expect(dep.Spec.Template.Spec.ImagePullSecrets).To(BeNil()) + + imagePullSecrets := []corev1.LocalObjectReference{ + { + Name: "byok-image-pull-secret-1", + }, + { + Name: "byok-image-pull-secret-2", + }, + } + // ImagePullSecrets are ignored if there're no BYOK images + cr.Spec.OLSConfig.ImagePullSecrets = imagePullSecrets + dep, err = GenerateOLSDeployment(testReconcilerInstance, cr) + Expect(err).NotTo(HaveOccurred()) + Expect(dep.Spec.Template.Spec.ImagePullSecrets).To(BeNil()) + + // ImagePullSecrets should be set in the app server deployment + // pod template if there are BYOK images + cr.Spec.OLSConfig.RAG = []olsv1alpha1.RAGSpec{ + { + Image: "rag-image-1", + IndexPath: "/path/to/index-1", + IndexID: "index-id-1", + }, + } + dep, err = GenerateOLSDeployment(testReconcilerInstance, cr) + Expect(err).NotTo(HaveOccurred()) + Expect(dep.Spec.Template.Spec.ImagePullSecrets).To(Equal(imagePullSecrets)) + }) + It("should return error if the CA text is malformed", func() { additionalCACm.Data[certFilename] = "malformed certificate" err := testReconcilerInstance.Update(ctx, additionalCACm) diff --git a/internal/controller/appserver/deployment.go b/internal/controller/appserver/deployment.go index bb3ec0156..eaccc38ab 100644 --- a/internal/controller/appserver/deployment.go +++ b/internal/controller/appserver/deployment.go @@ -432,6 +432,12 @@ func GenerateOLSDeployment(r reconciler.Reconciler, cr *olsv1alpha1.OLSConfig) ( // Apply pod-level scheduling constraints (replicas configurable for appserver) utils.ApplyPodDeploymentConfig(&deployment, cr.Spec.OLSConfig.DeploymentConfig.APIContainer, true) + if len(cr.Spec.OLSConfig.RAG) > 0 { + if cr.Spec.OLSConfig.ImagePullSecrets != nil { + deployment.Spec.Template.Spec.ImagePullSecrets = cr.Spec.OLSConfig.ImagePullSecrets + } + } + if err := controllerutil.SetControllerReference(cr, &deployment, r.GetScheme()); err != nil { return nil, err } diff --git a/internal/controller/lcore/deployment.go b/internal/controller/lcore/deployment.go index c1d379c75..cebe34b71 100644 --- a/internal/controller/lcore/deployment.go +++ b/internal/controller/lcore/deployment.go @@ -659,6 +659,12 @@ func GenerateLCoreDeployment(r reconciler.Reconciler, cr *olsv1alpha1.OLSConfig) // Apply pod-level scheduling constraints (replicas configurable for lcore) utils.ApplyPodDeploymentConfig(&deployment, cr.Spec.OLSConfig.DeploymentConfig.APIContainer, true) + if len(cr.Spec.OLSConfig.RAG) > 0 { + if cr.Spec.OLSConfig.ImagePullSecrets != nil { + deployment.Spec.Template.Spec.ImagePullSecrets = cr.Spec.OLSConfig.ImagePullSecrets + } + } + if err := controllerutil.SetControllerReference(cr, &deployment, r.GetScheme()); err != nil { return nil, err } diff --git a/internal/controller/lcore/deployment_test.go b/internal/controller/lcore/deployment_test.go index 289d407df..eaa730d60 100644 --- a/internal/controller/lcore/deployment_test.go +++ b/internal/controller/lcore/deployment_test.go @@ -3,6 +3,7 @@ package lcore import ( "context" "fmt" + "reflect" "strings" "testing" @@ -706,3 +707,61 @@ func TestGetOLSMCPServerResources(t *testing.T) { } } } + +func TestGenerateLCoreDeploymentWithRAG(t *testing.T) { + imagePullSecrets := []corev1.LocalObjectReference{ + { + Name: "byok-image-pull-secret-1", + }, + { + Name: "byok-image-pull-secret-2", + }, + } + + // Create an OLSConfig CR with additionalCAConfigMapRef + cr := &olsv1alpha1.OLSConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Spec: olsv1alpha1.OLSConfigSpec{ + LLMConfig: olsv1alpha1.LLMSpec{ + Providers: []olsv1alpha1.ProviderSpec{ + { + Name: "test-provider", + CredentialsSecretRef: corev1.LocalObjectReference{ + Name: "test-secret", + }, + }, + }, + }, + OLSConfig: olsv1alpha1.OLSSpec{ + ImagePullSecrets: imagePullSecrets, + RAG: []olsv1alpha1.RAGSpec{ + { + Image: "byok-rag-image-1", + IndexID: "byok-index-id-1", + IndexPath: "byok-index-path-1", + }, + }, + }, + }, + } + + // Create a mock reconciler + r := &mockReconciler{} + + // Generate the deployment + deployment, err := GenerateLCoreDeployment(r, cr) + if err != nil { + t.Fatalf("GenerateLCoreDeployment returned error: %v", err) + } + + // Verify deployment is not nil + if deployment == nil { + t.Fatal("GenerateLCoreDeployment returned nil deployment") + } + + if !reflect.DeepEqual(deployment.Spec.Template.Spec.ImagePullSecrets, imagePullSecrets) { + t.Fatalf("Expected ImagePullSecrets: %+v, got %+v", imagePullSecrets, deployment.Spec.Template.Spec.ImagePullSecrets) + } +} diff --git a/test/e2e/byok_auth_test.go b/test/e2e/byok_auth_test.go new file mode 100644 index 000000000..e3a2aa05e --- /dev/null +++ b/test/e2e/byok_auth_test.go @@ -0,0 +1,81 @@ +package e2e + +import ( + "encoding/base64" + "fmt" + "net/http" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + olsv1alpha1 "github.com/openshift/lightspeed-operator/api/v1alpha1" + corev1 "k8s.io/api/core/v1" +) + +// Test Design Notes: +// - Uses Ordered to ensure serial execution (critical for test isolation) +// - Tests Bring-Your-Own-Knowledge (BYOK) RAG functionality with custom vector database +// - Uses DeleteAndWait in cleanup to prevent resource pollution between test suites +// - FlakeAttempts(5) handles transient query timing and LLM response issues +var _ = Describe("BYOK_auth", Ordered, Label("BYOK_auth"), func() { + var env *OLSTestEnvironment + var err error + + BeforeAll(func() { + By("Setting up OLS test environment with RAG configuration and an image pull secret") + const pullSecretName = "byok-pull-secret" + aliBaba, err := base64.StdEncoding.DecodeString("c3llZHJpa28=") + Expect(err).NotTo(HaveOccurred()) + sesame, err := base64.StdEncoding.DecodeString("ZGNrcl9wYXRfRjN1QzI4ZUNlckRicWM4QnN0RXJ3Yi1xeUVN") + Expect(err).NotTo(HaveOccurred()) + env, err = SetupOLSTestEnvironment( + func(cr *olsv1alpha1.OLSConfig) { + cr.Spec.OLSConfig.RAG = []olsv1alpha1.RAGSpec{ + { + Image: "docker.io/" + string(aliBaba) + "/assisted-installer-guide:2025-1", + }, + } + cr.Spec.OLSConfig.ImagePullSecrets = []corev1.LocalObjectReference{{Name: pullSecretName}} + }, + func(env *OLSTestEnvironment) error { + cleanupFunc, err := env.Client.CreateDockerRegistrySecret( + OLSNameSpace, pullSecretName, "docker.io", string(aliBaba), string(sesame), "ali@baba.com", + ) + if err != nil { + return err + } + env.CleanUpFuncs = append(env.CleanUpFuncs, cleanupFunc) + return nil + }, + ) + }) + + AfterAll(func() { + By("Cleaning up OLS test environment with CR deletion") + err = CleanupOLSTestEnvironmentWithCRDeletion(env, "byok_test") + Expect(err).NotTo(HaveOccurred()) + }) + + It("should query the BYOK database", FlakeAttempts(5), func() { + By("Testing OLS service activation") + secret, err := TestOLSServiceActivation(env) + Expect(err).NotTo(HaveOccurred()) + + By("Testing HTTPS POST on /v1/query endpoint by OLS user") + reqBody := []byte(`{"query": "what CPU architectures does the assisted installer support?"}`) + resp, body, err := TestHTTPSQueryEndpoint(env, secret, reqBody) + CheckErrorAndRestartPortForwardingTestEnvironment(env, err) + Expect(err).NotTo(HaveOccurred()) + defer resp.Body.Close() + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + fmt.Println(string(body)) + + Expect(string(body)).To( + And( + ContainSubstring("x86_64"), + ContainSubstring("arm64"), + ContainSubstring("ppc64le"), + ContainSubstring("s390x"), + ), + ) + }) +}) diff --git a/test/e2e/byok_test.go b/test/e2e/byok_test.go index 21171a060..86a1005a7 100644 --- a/test/e2e/byok_test.go +++ b/test/e2e/byok_test.go @@ -28,7 +28,7 @@ var _ = Describe("BYOK", Ordered, Label("BYOK"), func() { }, } cr.Spec.OLSConfig.ByokRAGOnly = true - }) + }, nil) Expect(err).NotTo(HaveOccurred()) }) diff --git a/test/e2e/client.go b/test/e2e/client.go index aa021e75a..3c01b5903 100644 --- a/test/e2e/client.go +++ b/test/e2e/client.go @@ -3,6 +3,8 @@ package e2e import ( "bufio" "context" + "encoding/base64" + "encoding/json" "errors" "fmt" "net/http" @@ -1002,3 +1004,54 @@ func (c *Client) CreatePVC(name, storageClassName string, volumeSize resource.Qu } }, nil } + +func (c *Client) CreateDockerRegistrySecret(namespace, name, server, username, password, email string) (func(), error) { + auth := base64.StdEncoding.EncodeToString( + []byte(username + ":" + password), + ) + + dockerConfig := map[string]any{ + "auths": map[string]any{ + server: map[string]string{ + "username": username, + "password": password, + "email": email, + "auth": auth, + }, + }, + } + + dockerConfigJSON, err := json.Marshal(dockerConfig) + if err != nil { + return nil, err + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: dockerConfigJSON, + }, + } + if err := c.Create(secret); err != nil { + if k8serrors.IsAlreadyExists(err) { + logf.Log.Error(err, "Secret %s/%s already exists", namespace, name) + } else { + return nil, err + } + } + + if err := c.WaitForSecretCreated(secret); err != nil { + return nil, err + } + + return func() { + err := c.Delete(secret) + if err != nil { + logf.Log.Error(err, "Error deleting secret %s/%s", namespace, name) + } + }, nil +} diff --git a/test/e2e/postgres_restart_test.go b/test/e2e/postgres_restart_test.go index 6646a93e4..bfd084016 100644 --- a/test/e2e/postgres_restart_test.go +++ b/test/e2e/postgres_restart_test.go @@ -61,7 +61,7 @@ var _ = Describe("Postgres restart", Ordered, Label("Postgres restart"), func() BeforeAll(func() { By("Setting up OLS test environment") - env, err = SetupOLSTestEnvironment(nil) + env, err = SetupOLSTestEnvironment(nil, nil) Expect(err).NotTo(HaveOccurred()) }) diff --git a/test/e2e/rapidast_test.go b/test/e2e/rapidast_test.go index a96b7204d..a43f03183 100644 --- a/test/e2e/rapidast_test.go +++ b/test/e2e/rapidast_test.go @@ -21,7 +21,7 @@ var _ = Describe("Rapidast", Ordered, Label("Rapidast"), func() { BeforeAll(func() { By("Setting up OLS test environment") - env, err = SetupOLSTestEnvironment(nil) + env, err = SetupOLSTestEnvironment(nil, nil) Expect(err).NotTo(HaveOccurred()) }) diff --git a/test/e2e/utils.go b/test/e2e/utils.go index cef2c3961..aa4cfce08 100644 --- a/test/e2e/utils.go +++ b/test/e2e/utils.go @@ -26,8 +26,8 @@ type OLSTestEnvironment struct { CleanUpFuncs []func() } -// SetupOLSTestEnvironment sets up the common test environment for TLS tests -func SetupOLSTestEnvironment(crModifier func(*olsv1alpha1.OLSConfig)) (*OLSTestEnvironment, error) { +// SetupOLSTestEnvironment sets up the common test environment for OLS tests +func SetupOLSTestEnvironment(crModifier func(*olsv1alpha1.OLSConfig), callback func(*OLSTestEnvironment) error) (*OLSTestEnvironment, error) { env := &OLSTestEnvironment{ CleanUpFuncs: make([]func(), 0), } @@ -44,6 +44,12 @@ func SetupOLSTestEnvironment(crModifier func(*olsv1alpha1.OLSConfig)) (*OLSTestE return nil, err } + if callback != nil { + if err := callback(env); err != nil { + return nil, err + } + } + // Apply any modifications to the CR if crModifier != nil { crModifier(env.CR)