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

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

This PR adds support for the rhos-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 rhos-mcps is not deployed and requires the rhos_mcps feature flag to be enabled.

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

openshift-ci · 2026-07-21T08:41:51Z

@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-21T10:46:24Z

/retest

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 own rhos-mcps repository [1]. On installation the `openshift-cli` tool is enabled when OpenStack Lightspeed is configured, 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 not using TLS is not a real security risk. [1]: https://github.com/openstack-lightspeed/rhos-mcps Jira: OSPRH-27075 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Job govulncheck is failing with the following error. ``` Vulnerability openstack-k8s-operators#1: GO-2026-5970 Infinite loop on invalid input in golang.org/x/text More info: https://pkg.go.dev/vuln/GO-2026-5970 Module: golang.org/x/text Found in: golang.org/x/text@v0.37.0 Fixed in: golang.org/x/text@v0.39.0 ``` This commit updates the text dependency and transitive version bumps of other dependencies.

For now we don't want to automatically deploy the MCP tools, because: - Releasing the rhos-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 `rhos_mcps` as shown in the config ample. 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. Credential Rotation is handled by the code as well. Jira: OSPRH-27075 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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.

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

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.

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"

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.

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.

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 ?

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

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)

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

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


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

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.

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 from bd2487c to 96f2ab6 Compare July 21, 2026 23:28

Akrog and others added 3 commits July 22, 2026 10:59

Akrog force-pushed the lcore-mcps branch 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

Uh oh!

Conversation

Akrog commented Jul 14, 2026

Uh oh!

openshift-ci Bot commented Jul 14, 2026

Uh oh!

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

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

openshift-ci Bot commented Jul 21, 2026 •

edited

Loading