Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .env.template
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Assisted Service MCP Server - .env.template
# Copy to `.env` and adjust values as needed. All variables are optional unless noted.

# --- Server ---
# Bind host for the HTTP/SSE server
MCP_HOST=0.0.0.0
# Port for the HTTP/SSE server (1024-65535)
MCP_PORT=8000
# Transport protocol for MCP server: sse | streamable-http
TRANSPORT=sse

# --- Assisted Service API ---
# Assisted Service API base URL
INVENTORY_URL=https://api.openshift.com/api/assisted-install/v2
# Enable verbose client logging (true/false)
CLIENT_DEBUG=false
# URL endpoint used to fetch the pull secret
PULL_SECRET_URL=https://api.openshift.com/api/accounts_mgmt/v1/access_token

# --- Authentication ---
# OCM offline token used to mint access tokens.
# You may omit this and instead provide the token per-request via the
# "OCM-Offline-Token" HTTP header from your MCP client configuration.
# OFFLINE_TOKEN=

# Red Hat SSO token endpoint used to exchange the offline token
SSO_URL=https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token

# --- Logging ---
# Logging level: DEBUG | INFO | WARNING | ERROR | CRITICAL
LOGGING_LEVEL=INFO
# Logger name (leave empty for default)
# LOGGER_NAME=assisted-service-mcp
# Write logs to file (true/false). Consider disabling in containers.
LOG_TO_FILE=true

# --- Features ---
# Enable troubleshooting tool calls: 0 (disabled) | 1 (enabled)
ENABLE_TROUBLESHOOTING_TOOLS=0
Comment on lines +38 to +39
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Align boolean representation with other variables.

ENABLE_TROUBLESHOOTING_TOOLS uses numeric 0/1 values, while CLIENT_DEBUG and LOG_TO_FILE use true/false strings. Inconsistent boolean formats may confuse users and risk type mismatches during settings validation by pydantic.

Apply this diff to align the representation:

  # --- Features ---
  # Enable troubleshooting tool calls: 0 (disabled) | 1 (enabled)
- ENABLE_TROUBLESHOOTING_TOOLS=0
+ ENABLE_TROUBLESHOOTING_TOOLS=false

Update the comment to match the new format:

- # Enable troubleshooting tool calls: 0 (disabled) | 1 (enabled)
+ # Enable troubleshooting tool calls (true/false)
📝 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.

Suggested change
# Enable troubleshooting tool calls: 0 (disabled) | 1 (enabled)
ENABLE_TROUBLESHOOTING_TOOLS=0
# Enable troubleshooting tool calls (true/false)
ENABLE_TROUBLESHOOTING_TOOLS=false
🤖 Prompt for AI Agents
In .env.template around lines 38 to 39, the ENABLE_TROUBLESHOOTING_TOOLS
variable uses numeric 0/1 while other booleans use "true"/"false"; update the
variable to use the same string boolean format (e.g.,
ENABLE_TROUBLESHOOTING_TOOLS=false) and change the comment to reflect true/false
usage (e.g., # Enable troubleshooting tool calls: false (disabled) | true
(enabled)) so formats are consistent for users and pydantic validation.

8 changes: 2 additions & 6 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,7 @@ COPY pyproject.toml .
COPY uv.lock .
RUN uv sync

COPY server.py .
COPY service_client ./service_client/
COPY static_net ./static_net/
COPY log_analyzer ./log_analyzer/
COPY metrics ./metrics/
COPY assisted_service_mcp ./assisted_service_mcp/

RUN chown -R 1001:0 ${APP_HOME}

Expand All @@ -26,4 +22,4 @@ ENV LOG_TO_FILE=false

EXPOSE 8000

CMD ["uv", "--cache-dir", "/tmp/uv-cache", "run", "server.py"]
CMD ["uv", "--cache-dir", "/tmp/uv-cache", "run", "python", "-m", "assisted_service_mcp.src.main"]
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ run:

.PHONY: run-local
run-local:
uv run server.py
uv run python -m assisted_service_mcp.src.main

.PHONY: run-mock-assisted
run-mock-assisted:
Expand All @@ -30,7 +30,7 @@ deploy-template:
scripts/deploy_from_template.sh

test-coverage:
uv run --group test pytest --cov=service_client --cov=server --cov-report=html --cov-report=term-missing
uv run --group test pytest --cov=assisted_service_mcp --cov-report=html --cov-report=term-missing

test-verbose:
uv run --group test pytest -v
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ In VSCode for example:
"run",
"mcp",
"run",
"/path/to/assisted-service-mcp/server.py"
"/path/to/assisted-service-mcp/assisted_service_mcp/src/main.py"
],
"env": {
"OFFLINE_TOKEN": <your token>
Expand All @@ -43,7 +43,7 @@ For SSE (recommended):

Start the server in a terminal:

`OFFLINE_TOKEN=<your token> uv run server.py`
`OFFLINE_TOKEN=<your token> uv run assisted_service_mcp.src.main`
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Configure the server in the client:

Expand Down
6 changes: 6 additions & 0 deletions assisted_service_mcp/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
"""Assisted Service MCP Server.

MCP server for interacting with the OpenShift assisted installer API.
"""

__version__ = "0.1.0"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we using this for? Are we going to ever change it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is coming from the template but I don't really have plans for this
we'd better remove then?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if we don't have a use for it then let's just remove it.

1 change: 1 addition & 0 deletions assisted_service_mcp/src/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
"""Source code for Assisted Service MCP Server."""
23 changes: 23 additions & 0 deletions assisted_service_mcp/src/api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
"""FastAPI application setup for the Assisted Service MCP server.

This module initializes the FastAPI app and sets up the MCP server
with appropriate transport protocols.
"""

from assisted_service_mcp.src.mcp import AssistedServiceMCPServer
from assisted_service_mcp.src.settings import settings
from assisted_service_mcp.src.logger import log, configure_logging

# Ensure logging is configured before any module-level log usage
configure_logging()

# Initialize the MCP server
server = AssistedServiceMCPServer()

# Choose the appropriate transport protocol based on settings
if settings.TRANSPORT == "streamable-http":
app = server.mcp.streamable_http_app()
log.info("Using StreamableHTTP transport (stateless)")
else:
app = server.mcp.sse_app()
log.info("Using SSE transport (stateful)")
Comment thread
carbonin marked this conversation as resolved.
178 changes: 178 additions & 0 deletions assisted_service_mcp/src/logger.py
Comment thread
carbonin marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
"""
Logging utilities with sensitive information filtering.

This module provides logging configuration and formatting utilities that
automatically filter sensitive information like pull secrets, SSH keys,
and vSphere credentials from log messages.
"""

# -*- coding: utf-8 -*-
import logging
import re
import sys


class SensitiveFormatter(logging.Formatter):
"""Formatter that removes sensitive info."""

# Default log format used by this formatter
DEFAULT_FORMAT = "%(asctime)s - %(name)s - %(levelname)-8s - %(thread)d:%(process)d - %(message)s - (%(pathname)s:%(lineno)d)->%(funcName)s"

def __init__(self, fmt: str | None = None) -> None:
"""Initialize with default format if none provided."""
if fmt is None:
fmt = self.DEFAULT_FORMAT
super().__init__(fmt)

@staticmethod
def _filter(s: str) -> str:
# Dict filter
s = re.sub(r"('_pull_secret':\s+)'(.*?)'", r"\g<1>'*** PULL_SECRET ***'", s)
s = re.sub(r"('_ssh_public_key':\s+)'(.*?)'", r"\g<1>'*** SSH_KEY ***'", s)
s = re.sub(
r"('_vsphere_username':\s+)'(.*?)'", r"\g<1>'*** VSPHERE USER ***'", s
)
s = re.sub(
r"('_vsphere_password':\s+)'(.*?)'", r"\g<1>'*** VSPHERE PASSWORD ***'", s
)

# Object filter
def _redact_value(text: str, key: str, placeholder: str) -> str:
# Match quoted (single or double) or unquoted values, preserving spacing and quotes
pattern = re.compile(
rf"({re.escape(key)})(\s*=\s*)(?:'([^']*)'|\"([^\"]*)\"|([^\s,}}]+))"
)

def _repl(m: re.Match) -> str:
key_part = m.group(1)
eq_spaces = m.group(2)
if m.group(3) is not None: # single-quoted
return f"{key_part}{eq_spaces}'{placeholder}'"
if m.group(4) is not None: # double-quoted
return f'{key_part}{eq_spaces}"{placeholder}"'
# unquoted
return f"{key_part}{eq_spaces}{placeholder}"

return pattern.sub(_repl, text)

s = _redact_value(s, "pull_secret", "*** PULL_SECRET ***")
s = _redact_value(s, "ssh_public_key", "*** SSH_KEY ***")
s = _redact_value(s, "vsphere_username", "*** VSPHERE_USER ***")
s = _redact_value(s, "vsphere_password", "*** VSPHERE_PASSWORD ***")

return s

def format(self, record: logging.LogRecord) -> str:
"""
Format log record while filtering sensitive information.

Args:
record: The LogRecord instance to be formatted.

Returns:
str: The formatted log message with sensitive info redacted.
"""
original = logging.Formatter.format(self, record)
return self._filter(original)


def get_logging_level() -> int:
"""
Get the logging level from settings.

Returns:
int: The logging level (defaults to INFO if not set or invalid).
"""
# Import here to avoid circular dependency at module load time
from assisted_service_mcp.src.settings import settings

level = settings.LOGGING_LEVEL
return getattr(logging, str(level).upper(), logging.INFO) if level else logging.INFO
Comment on lines +79 to +90
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Make LOGGING_LEVEL handling robust (ints/strings) and silence C0415 on local import

  • int levels (e.g., 20) currently degrade to INFO.
  • Add pylint suppression for the intentional in-function import.
-    # Import here to avoid circular dependency at module load time
-    from assisted_service_mcp.src.settings import settings
-
-    level = settings.LOGGING_LEVEL
-    return getattr(logging, str(level).upper(), logging.INFO) if level else logging.INFO
+    # Import here to avoid circular dependency at module load time
+    from assisted_service_mcp.src.settings import settings  # pylint: disable=import-outside-toplevel
+
+    level = getattr(settings, "LOGGING_LEVEL", None)
+    if isinstance(level, int):
+        return level
+    if isinstance(level, str) and level:
+        return getattr(logging, level.upper(), logging.INFO)
+    return logging.INFO
📝 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.

Suggested change
def get_logging_level() -> int:
"""
Get the logging level from settings.
Returns:
int: The logging level (defaults to INFO if not set or invalid).
"""
# Import here to avoid circular dependency at module load time
from assisted_service_mcp.src.settings import settings
level = settings.LOGGING_LEVEL
return getattr(logging, str(level).upper(), logging.INFO) if level else logging.INFO
def get_logging_level() -> int:
"""
Get the logging level from settings.
Returns:
int: The logging level (defaults to INFO if not set or invalid).
"""
# Import here to avoid circular dependency at module load time
from assisted_service_mcp.src.settings import settings # pylint: disable=import-outside-toplevel
level = getattr(settings, "LOGGING_LEVEL", None)
if isinstance(level, int):
return level
if isinstance(level, str) and level:
return getattr(logging, level.upper(), logging.INFO)
return logging.INFO


Comment thread
coderabbitai[bot] marked this conversation as resolved.

logging.getLogger("requests").setLevel(logging.ERROR)
logging.getLogger("urllib3").setLevel(logging.ERROR)
logging.getLogger("asyncio").setLevel(logging.ERROR)


def add_log_file_handler(logger: logging.Logger, filename: str) -> logging.FileHandler:
"""
Add a file handler to the logger with sensitive information filtering.

Args:
logger: The logger instance to add the handler to.
filename: The path to the log file.

Returns:
logging.FileHandler: The created file handler.
"""
fh = logging.FileHandler(filename)
fh.setFormatter(SensitiveFormatter())
logger.addHandler(fh)
return fh


def add_stream_handler(logger: logging.Logger) -> None:
"""
Add a stream handler to the logger with sensitive information filtering.

Args:
logger: The logger instance to add the handler to.
"""
ch = logging.StreamHandler(sys.stderr)
ch.setFormatter(SensitiveFormatter())
logger.addHandler(ch)


# Export a module-level logger with a safe default name to avoid circular imports.
# Configuration should be done by calling configure_logging() after settings are ready.
log = logging.getLogger("assisted-service-mcp")


def configure_logging() -> logging.Logger:
"""
Configure logging after settings are available.

This sets logger names/levels, third-party logger levels, and attaches
file/stream handlers with the SensitiveFormatter. Importing settings here
avoids circular imports at module load time.

Returns:
logging.Logger: The configured application logger.
"""
# Import inside function to avoid circular dependency
from assisted_service_mcp.src.settings import settings

# Resolve logger name, falling back to a stable default
logger_name = settings.LOGGER_NAME or "assisted-service-mcp"
target_logger = logging.getLogger(logger_name)

# Configure third-party loggers
logging.getLogger("requests").setLevel(logging.ERROR)
urllib3_logger = logging.getLogger("urllib3")

# Reset handlers to prevent duplicates on reconfiguration
for handler in target_logger.handlers:
handler.close()
target_logger.handlers = []
for handler in urllib3_logger.handlers:
handler.close()
urllib3_logger.handlers = []

# Set levels
urllib3_logger.setLevel(logging.ERROR)
target_logger.setLevel(get_logging_level())

# Optional file logging
if settings.LOG_TO_FILE:
add_log_file_handler(target_logger, "assisted-service-mcp.log")
add_log_file_handler(urllib3_logger, "assisted-service-mcp.log")

# Always add stream handlers
add_stream_handler(target_logger)
add_stream_handler(urllib3_logger)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
# Ensure modules using `from ...logger import log` get the configured logger
global log # pylint: disable=global-statement
log = target_logger
return target_logger
Comment on lines +132 to +178
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify pylint suppressions were applied; handler management looks correct.

Past review comments indicated that pylint suppressions for C0415 (import-outside-toplevel) and W0603 (global-statement) should be added to lines 144 and 150. These were marked as addressed in commit 700a108, but the current code doesn't show visible suppression comments.

The handler management logic correctly clears existing handlers and sets up new ones without the NullHandler confusion mentioned in previous reviews.

Run the following script to verify no pylint warnings remain:


🏁 Script executed:

#!/bin/bash
# Check if pylint reports C0415 or W0603 issues in configure_logging
pylint assisted_service_mcp/src/logger.py --disable=all --enable=C0415,W0603 2>&1 | grep -A2 -B2 "configure_logging\|line 144\|line 150"

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check for pylint suppression comments for C0415 and W0603
grep -n -E "# pylint:.*disable=.*C0415" assisted_service_mcp/src/logger.py || echo "No C0415 suppression found"
grep -n -E "# pylint:.*disable=.*W0603" assisted_service_mcp/src/logger.py || echo "No W0603 suppression found"
# Run pylint for C0415 and W0603 without filtering output
pylint assisted_service_mcp/src/logger.py --disable=all --enable=C0415,W0603

Length of output: 529


Add missing pylint suppressions for import-outside-toplevel
Lines 87 and 144 import settings inside functions, triggering C0415. Append # pylint: disable=import-outside-toplevel to those import statements.

🤖 Prompt for AI Agents
In assisted_service_mcp/src/logger.py around lines 132 to 176, the in-function
import of settings triggers pylint C0415 (import-outside-toplevel); update the
import statements that are inside functions (the ones at the earlier function
around line ~87 and the one in configure_logging around line ~144) by appending
the inline pylint suppression comment "# pylint:
disable=import-outside-toplevel" to each import line so pylint no longer flags
them while preserving the in-function import to avoid circular imports.

46 changes: 46 additions & 0 deletions assisted_service_mcp/src/main.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
"""Main entry point for the Assisted Service MCP Server."""

import uvicorn
from assisted_service_mcp.src.api import app, server
from assisted_service_mcp.src.settings import settings
from assisted_service_mcp.src.metrics import metrics, initiate_metrics
from assisted_service_mcp.src.logger import log


def main() -> None:
"""Start the MCP server.

Initializes the server, sets up metrics, and starts the uvicorn server.
"""
try:
log.info("Starting Assisted Service MCP Server")
log.info(
"Configuration: TRANSPORT=%s, HOST=%s, PORT=%s",
settings.TRANSPORT,
settings.MCP_HOST,
settings.MCP_PORT,
)

# Initialize metrics with list of all tools
tool_names = server.list_tools_sync()
initiate_metrics(tool_names)
log.info("Initialized metrics for %s tools", len(tool_names))

# Add metrics endpoint
app.add_route("/metrics", metrics)
log.info("Metrics endpoint available at /metrics")

# Start the server using settings
uvicorn.run(app, host=settings.MCP_HOST, port=settings.MCP_PORT)

except KeyboardInterrupt:
log.info("Received keyboard interrupt, shutting down")
except Exception as e:
log.error("Server failed to start: %s", e, exc_info=True)
raise
finally:
log.info("Assisted Service MCP server shutting down")


if __name__ == "__main__":
main()
Loading