Lcore mcps by Akrog · Pull Request #19 · openstack-k8s-operators/lightspeed-operator

Akrog · 2026-07-14T16:30:42Z

This PR adds support for the rhoso-mcps image that provides openstack-cli and openshift-cli tools.

The MCP server is deployed as a sidecar container with the openshift-cli tool always enabled and the openstack-cli tool only enabled once the OpenStackControlPlane is ready.

By default the rhoso-mcps is not deployed and requires the rhoso_mcps feature flag to be enabled.

This PR depends on the /health endpoint added to the rhoso-mcps by openstack-k8s-operators/lightspeed-mcps#18

openshift-ci · 2026-07-14T16:30:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Akrog

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Akrog]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Akrog · 2026-07-21T10:46:24Z

/retest

Akrog · 2026-07-22T10:13:59Z

I have added another commit to fix the vulnerability reported by the CI job

lpiwowar

Overall LGTM!:) Thank you!

I wrote a comment for anything that crossed my mind not everything is blocking.

I personally would like to resolve these two things somehow (either by an agreement or by change in the code) before we merge:

MCP image handling in downstream and pullspecs_repleceatron.sh (setting to empty string)
The usage of RawClient() and question whether we should not add openstack into WATCH_NAMESPACES

lpiwowar · 2026-07-22T09:19:05Z

+	OpenStackLightspeedMCPServerWaitingAC = "Waiting for application credential secret"
+
+	// OpenStackLightspeedMCPServerDisabledMessage
+	OpenStackLightspeedMCPServerDisabledMessage = "RHOS MCP server is disabled (rhos_mcps feature flag not set)"


question (non-blocking): Shouldn't we use "RHOSO MCP" server or "OpenStack Lightspeed MCP server"? I feel like the "RHOS" naming is something we do not use much.

I'll change it, but for the record the service is called rhos-lightspeed

lightspeed-operator/internal/controller/constants.go

Line 129 in e770335

ServiceIDRHOSO = "rhos-lightspeed"

lpiwowar · 2026-07-22T09:23:48Z

 	OKPContainerImage = "registry.redhat.io/offline-knowledge-portal/rhokp-rhel9:latest"

+	// MCPServerContainerImage is the fall-back container image for the MCP server
+	MCPServerContainerImage = "quay.io/openstack-lightspeed/rhos-mcps:latest"


question (blocking): Do we know how we are going to deal with the image downstream? Related to the conversation we already had. Suggested solution was to use empty string downstream for the RELATED_IMAGE_MCP_* environment variable. The issue is that the the pullspecs_repleceatron.sh ends with an error when any RELATED_IMAGE_* env var is an empty string. In my opinion before we merge we should:

Update pullspecs_repleceatron.sh to make it work with empty string for RELATED_IMAGE_* env vars.

Check with somebody an empty string for RELATED_IMAGE_* will be accepted well downstream.

Perhaps we can discuss this with Miguel ? He seems pretty open to suggestions

Given today's developments regarding the release I think we can postpone this until later, as we may release with MCP support ;-)

lpiwowar · 2026-07-22T09:44:52Z

 // must be consistent within modules and service operators
 replace github.com/openshift/api => github.com/openshift/api v0.0.0-20250711200046-c86d80652a9e

+require k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 // indirect


nit (non-blocking): Why this can not be in the require block below together with all the other // indirect dependencies?

lpiwowar · 2026-07-22T09:53:52Z


 // getSecretResourceVersion retrieves the resource version of a Secret.
 func getSecretResourceVersion(ctx context.Context, h *common_helper.Helper, name string, namespace string) (string, error) {
+	rawClient, err := getRawClient(h)


suggestion (non-blocking): I think we should really consider adding openstack namespace into namespaces that are watched by default. That way it is obvious what namespaces the operator watches and we do need to bypass the WATCH_NAMESPACES with raw client. I think it would be a step in the right direction. If for whatever reason the OpenStack controlplane lives in a different namespace then the customer can control that with OperatorGroup.

I like this suggestion as I am a bit worry about the whole cross-namespacing aspects of things. I think that we could have it namespaced for now even if it's a tech debt for multi-namespaces (or non default namespaces) of the OpenStack depoyment until we have something else figure out. More like, "security first" approach for this first implementation.

This is not a tech debt only for multi-namespaces but also for any customer that decides to deploy in a different namespace.
There is no requirement in RHOSO saying that the namespace must be called openstack, that's why we cannot limit the scope. :-(
The only way I see to do this differently is to do a complete redesign of our architecture with a namespaced operator and changing how we do everything.
That seems out of scope for this PR.

lpiwowar · 2026-07-22T11:02:45Z

+      set -euo pipefail
+      READY=$(oc get openstacklightspeed openstack-lightspeed \
+        -n openstack-lightspeed \
+        -o jsonpath='{.status.openStackReady}' 2>/dev/null || true)
+      if [ "$READY" = "true" ]; then
+        echo "ERROR: openStackReady is still true"
+        exit 1
+      fi


suggestion (non-blocking): This can probably be:

apiVersion: lightspeed.openstack.org/v1beta1 kind: OpenStackLightspeed metadata: name: openstack-lightspeed namespace: openstack-lightspeed status: conditions: - type: Ready status: "True"

Unfortunately the script is needed because go's omitempty on bool omits false from serialization, making kuttl YAML assert fail with "key is missing from map". You can see the explanation on L5-L6

lpiwowar · 2026-07-22T11:05:33Z

+      MCP_DATA=$(oc get configmap mcp-config -n openstack-lightspeed \
+        -o jsonpath='{.data.config\.yaml}')
+      ENABLED=$(echo "$MCP_DATA" | grep -A1 "openstack:" | grep "enabled:" | tr -d ' ')
+      if [ "$ENABLED" != "enabled:false" ]; then
+        echo "ERROR: MCP config does not have openstack disabled"
+        echo "$MCP_DATA"
+        exit 1
+      fi


suggestion (non-blocking): If we really need to use the script (which seems to be the valid case here) I think we should move the repeating snippets into ../common/shared_funcs.go where they can be easily reused by the individual test steps:

source ../common/shared_funcs.go ssert_mcp_openstack_enabled true / false

I think we can find some other good candidates for this.

lpiwowar · 2026-07-22T11:14:34Z

+	case 0:
+		return nil, errors.New("no cloud entry found in clouds.yaml")
+	case 1:
+		for name = range clouds.Clouds {


issue (blocking): This does nothing.

It does, and the value is used on L320.
In this case we only have 1 element, so this code assigns the only key to name, regardless of what that is.
In other works this is how the code behaves based on the number of elements:

0 ==> Fail 1 ==> Use whatever name is there (can be different from `default`) >1 ==> Use `default` if it exists, fail if it doesn't

lpiwowar · 2026-07-22T11:16:12Z

+	}
+
+	// Delete AC CR
+	rawClient, err := getRawClient(helper)


suggestion (non-blocking): Already mentioned. I think we should ensure that our operator properly watches the openstack namespace and be transparent about it.

As mentioned, limiting it to a hardcoded namespace would create problems for any single deployment that used a different namespace.

lpiwowar · 2026-07-22T11:17:51Z

+
+// cleanupMCPResources removes MCP server resources when the rhos_mcps feature
+// flag is disabled.
+func (r *OpenStackLightspeedReconciler) cleanupMCPResources(


suggestion (non-blocking): This can be simplified e.g. like this:

func (r *OpenStackLightspeedReconciler) cleanupMCPResources( ctx context.Context, helper *common_helper.Helper, instance *apiv1beta1.OpenStackLightspeed, ) error { ns := instance.Namespace resources := []client.Object{ &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: MCPConfigYAMLConfigMapName, Namespace: ns}}, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: CloudsYAMLConfigMapName, Namespace: ns}}, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: SecureYAMLSecretName, Namespace: ns}}, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: CombinedCABundleSecretName, Namespace: ns}}, } for _, obj := range resources { if err := helper.GetClient().Delete(ctx, obj); err != nil && !k8s_errors.IsNotFound(err) { return fmt.Errorf("failed to delete %s %s: %w", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), err) } } if err := r.reconcileDeleteOpenStackResources(ctx, helper, instance); err != nil { return fmt.Errorf("failed to clean up OpenStack resources during RHOS MCP disable: %w", err) } helper.GetLogger().Info("RHOS MCP resources cleaned up") return nil }

umago

Thanks Gorka! Overall I think it looks solid, I'm a bit worried about the wide scope of some aspects of this change (commented inline). I wonder if we could go a bit more "namespaced-scoped" from the start rather than having to tight up things later.

umago · 2026-07-23T09:10:56Z

 	OKPContainerImage = "registry.redhat.io/offline-knowledge-portal/rhokp-rhel9:latest"

+	// MCPServerContainerImage is the fall-back container image for the MCP server
+	MCPServerContainerImage = "quay.io/openstack-lightspeed/rhos-mcps:latest"


Perhaps we can discuss this with Miguel ? He seems pretty open to suggestions

umago · 2026-07-23T09:21:59Z

+		return err
+	}
+
+	userID, err := osClient.CreateUser(log, openstack_lib.User{


Not sure if this is an issue or not, but we are calling CreateUser on every reconciliation. Does keystone handles it properly ?

It doesn't reach KeyStone because lib-common implementation is idempotent (get-or-create pattern) so it only tries to create when it doesn't exist.

umago · 2026-07-23T09:30:57Z

+  verbs:
+  - create
+  - get
+  - update


This comes from the old coderabbit review. I don't know if there's a way around it, maybe we can assume openstack is always depoyed in the "openstack" namespace and scope it only to that namespace as a compromise until we have a better way of doing it ?

I worry that this rule allows the operator to read and write on all Secrets cluster wide without any namespace restriction. it does sounds a bit too broad. I wonder if something like this can/will be flagged in the security assessments we've been doing recently.

umago · 2026-07-23T09:40:45Z

+		oldSecret, err := kclient.CoreV1().Secrets(oscpNamespace).Get(ctx, prevSecret, metav1.GetOptions{})
+		if err == nil && controllerutil.RemoveFinalizer(oldSecret, LightspeedACFinalizerName) {
+			if _, err := kclient.CoreV1().Secrets(oscpNamespace).Update(ctx, oldSecret, metav1.UpdateOptions{}); err != nil {
+				helper.GetLogger().Info("Failed to remove finalizer from old AC secret", "secret", prevSecret, "error", err)


nit: should this be an error log instead ? (as L540 above)

umago · 2026-07-23T09:41:15Z

+			ctx, instance.Status.ApplicationCredentialSecret, metav1.GetOptions{})
+		if err == nil && controllerutil.RemoveFinalizer(acSecret, LightspeedACFinalizerName) {
+			if _, err := helper.GetKClient().CoreV1().Secrets(oscpNS).Update(ctx, acSecret, metav1.UpdateOptions{}); err != nil {
+				log.Info("Failed to remove finalizer from AC secret", "error", err)


nit: this should be n error instad of info

Apparently using Info for best-effort cleanup is a common k8s controller pattern. However, logging failures as Info means operators won't notice them, so I'm ok to change it.

umago · 2026-07-23T09:45:42Z

+				apiv1beta1.OpenStackLightspeedMCPServerDeployed,
+			)
+		} else {
+			instance.Status.Conditions.MarkTrue(


Sounds like this should be False instead as it is still waiting for OpenStack to be ready (OpenStackLightspeedMCPServerWaitingOpenStack L337 below)

When OpenStack is not available or ready we still want to complete the deployment of Lightspeed and say we are ready, and since we enable MCP (for the oc-client tool) the condition must be True.

umago · 2026-07-23T09:48:03Z

+			},
+			ImagePullPolicy: corev1.PullIfNotPresent,
+		}
+		containers = append(containers, mcpContainer)


Nice to add: Some liveness/readiness check for the MCP container ?

That requires new code to the lightspeed-mcps repository to add the "/health" endpoint.

I have submitted the PR 1 and added the changes here to use it.

umago · 2026-07-23T09:53:11Z


 // getSecretResourceVersion retrieves the resource version of a Secret.
 func getSecretResourceVersion(ctx context.Context, h *common_helper.Helper, name string, namespace string) (string, error) {
+	rawClient, err := getRawClient(h)


I like this suggestion as I am a bit worry about the whole cross-namespacing aspects of things. I think that we could have it namespaced for now even if it's a tech debt for multi-namespaces (or non default namespaces) of the OpenStack depoyment until we have something else figure out. More like, "security first" approach for this first implementation.

With this commit we start deploying the MCP server as a sidecar container of the lightspeed-service container. The MCP server deployed is the one from our new lightspeed-mcps repository [1]. On installation the `openshift-cli` tool is enabled when OpenStack Lightspeed is configured (therefore deployed), and the `openstack-cli` tool is enabled when the `OpenStackControlPlane` is ready. From a security perspective since we are deploying the MCP tools in the pod's network namespace, it's a sidecar container and the port is not exposed, not using TLS is not a real security risk. [1]: https://github.com/openstack-lightspeed/lightspeed-mcps Jira: OSPRH-27075 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

For now we don't want to automatically deploy the MCP tools, because: - Releasing the lightspeed-mcps image may be done at a later time. - This is an experimental feature right now, so we want it disabled by default. The name of the feature flag is `rhoso_mcps` as shown in the config sample. Jira: OSPRH-27075 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Initial implementation of the MCP deployment uses the credentials from the `openstackclient` pod, which means that we are not the owners of that secret, just the copy we make in the `openstack-lightspeed` namespace, so those credentials could be removed/deleted and that would break our `openstack-cli` tool. In this patch we change the credentials and we leverage the `KeystoneApplicationCredential` CR to get our own credentials. Since OpenStack can be deployed on any namespace we are forced to have permissions to access any secret in the deployment. Alternatively we could change how the operator is deployed altogether and do a per-namespace deployment so only those secrets can be accessed. That is currently out of scope of this effort. Credential Rotation is handled by the code as well. Jira: OSPRH-27075 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Modify kuttl tests to minimize the usage of bash scripts and leverage as much as possible standard kuttl test mechanisms.

There is code in the bash scripts that is repeated in multiple kuttl test files. In this commit we consolidate that code into shared functions that can be used by the different tests.

Currently there is no way to modify the MCP server configuration, which means we cannot even configure the logs to be in debug mode. In this commit we add a section in the `dev` field (named `rhosMCPConfig`) to allow configuring anything within the lightspeed-mcp service except for enabling the tools, which is still done by the operator itself. Configuring the logging has not been added to the standard location in the CRD because the feature is still experimental, so we can't set its configuration in the CRD as if it were a normal feature. This is to avoid having to make non-compatible changes to the CRD down the road. Added a small comment to the openstacklightspeed_types.go file establishing this rule. It is the users responsibility to ensure that configuration makes sense and works. The operator will not be responsible if the service cannot start. For example, if we increase the number of workers the container will need more memory than it has currently assigned and the container will be restarted.

Add a liveness probe on the rhos-mcp container. This commit depends on the lightspeed-mcps PR openstack-k8s-operators#18 [1] that adds the `/health` endpoint. [1]: openstack-k8s-operators/lightspeed-mcps#18

Error is: ``` Vulnerability openstack-k8s-operators#1: GO-2026-6061 Vulnerabilities in the xDS RBAC authorization engine and the HTTP/2 transport server implementation in google.golang.org/grpc More info: https://pkg.go.dev/vuln/GO-2026-6061 Module: google.golang.org/grpc Found in: google.golang.org/grpc@v1.78.0 Fixed in: google.golang.org/grpc@v1.82.1 Example traces found: openstack-k8s-operators#1: test/utils/utils.go:113:21: utils.LoadImageToKindClusterWithName calls exec.Command, which eventually calls transport.NewHTTP2Client openstack-k8s-operators#2: test/utils/utils.go:113:21: utils.LoadImageToKindClusterWithName calls exec.Command, which eventually calls transport.http2Client.Close openstack-k8s-operators#3: test/utils/utils.go:113:21: utils.LoadImageToKindClusterWithName calls exec.Command, which eventually calls transport.http2Client.GracefulClose ```

openshift-ci · 2026-07-28T13:49:38Z

@Akrog: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/openstack-lightspeed-kuttl-4-18	`bd2487c`	link	true	`/test openstack-lightspeed-kuttl-4-18`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Akrog · 2026-07-28T14:38:00Z

/retest

openshift-ci Bot requested review from lpiwowar and umago July 14, 2026 16:30

openshift-ci Bot added the approved label Jul 14, 2026

Akrog force-pushed the lcore-mcps branch from 6ea5e2b to bd2487c Compare July 14, 2026 20:33

This was referenced Jul 15, 2026

OSC plugins openstack-k8s-operators/lightspeed-mcps#3

Merged

WIP: Support changing all the images #21

Open

Akrog force-pushed the lcore-mcps branch 2 times, most recently from 96f2ab6 to ab6e100 Compare July 22, 2026 10:13

lpiwowar requested changes Jul 22, 2026

View reviewed changes

lpiwowar mentioned this pull request Jul 22, 2026

Remove unnecessary RBAC verbs #29

Open

umago requested changes Jul 23, 2026

View reviewed changes

openshift-ci Bot added the needs-rebase label Jul 28, 2026

Akrog force-pushed the lcore-mcps branch from ab6e100 to 98826f1 Compare July 28, 2026 11:19

Akrog and others added 8 commits July 28, 2026 14:35

MCP: Improve kuttl tests

9ef0bd0

Modify kuttl tests to minimize the usage of bash scripts and leverage as much as possible standard kuttl test mechanisms.

MCP: Consolidate common kuttl bash scripts

d3133f3

There is code in the bash scripts that is repeated in multiple kuttl test files. In this commit we consolidate that code into shared functions that can be used by the different tests.

MCP: Liveness probe

6796305

Add a liveness probe on the rhos-mcp container. This commit depends on the lightspeed-mcps PR openstack-k8s-operators#18 [1] that adds the `/health` endpoint. [1]: openstack-k8s-operators/lightspeed-mcps#18

Akrog force-pushed the lcore-mcps branch from 98826f1 to 7d601e8 Compare July 28, 2026 12:36

openshift-ci Bot removed the needs-rebase label Jul 28, 2026

Uh oh!

Conversation

Akrog commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Jul 14, 2026

Uh oh!

Akrog commented Jul 21, 2026

Uh oh!

Akrog commented Jul 22, 2026

Uh oh!

lpiwowar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umago left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akrog commented Jul 28, 2026

Akrog commented Jul 14, 2026 •

edited

Loading

openshift-ci Bot commented Jul 28, 2026 •

edited

Loading