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
77 changes: 51 additions & 26 deletions artcommon/artcommonlib/ocp_version_ancestry.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,34 +349,21 @@ def _version_in_range(version: str, v_min: str, v_max: Optional[str]) -> bool:
return v_info.major == min_info.major and v_info.minor == min_info.minor


async def get_release_controller_versions_async(
async def _fetch_release_controller_tags(
url: str,
major: int,
minor: int,
go_arch: str,
release_controller_url: str = '',
timeout: float = 30.0,
timeout: float,
) -> list[str]:
"""
Query the release controller's stable stream to get all promoted versions for a major.minor.

The release controller tracks all versions that have been promoted to the 4-stable stream,
regardless of whether their cincinnati-graph-data PR has merged. This supplements Cincinnati
data to avoid missing recently-promoted z-streams.
Fetch tags from a single release controller stream URL and filter to major.minor.

:param major: Major version (e.g., 4 or 5)
:param minor: Minor version (e.g., 18)
:param go_arch: Go architecture name (e.g., 'amd64', 's390x', 'arm64', 'ppc64le', 'multi')
:param release_controller_url: Base URL for the release controller. If empty, defaults to
'https://{go_arch}.ocp.releases.ci.openshift.org'.
:param url: Full URL to the release controller tags endpoint
:param major: Major version to filter for
:param minor: Minor version to filter for
:param timeout: HTTP request timeout in seconds
:return: List of version strings matching major.minor, sorted descending by semver
:return: List of matching version strings (unsorted)
"""
if not release_controller_url:
release_controller_url = f'https://{go_arch}.ocp.releases.ci.openshift.org'

base_url = release_controller_url.rstrip('/')
url = f'{base_url}/api/v1/releasestream/{major}-stable/tags'

try:
async with httpx.AsyncClient() as client:
response = await client.get(url, timeout=timeout)
Expand All @@ -386,23 +373,61 @@ async def get_release_controller_versions_async(
return []

data = response.json()
tags = data.get('tags', [])
tags = data.get('tags') or []

# Filter tags to only those matching the requested major.minor.
# Tag names are version strings like "4.18.3" or "4.18.0-rc.0".
version_pattern = re.compile(rf'^{major}\.{minor}\.')
versions = []
for tag in tags:
name = tag.get('name', '')
if version_pattern.match(name):
# Validate it's a proper semver string before including
try:
semver.VersionInfo.parse(name)
versions.append(name)
Comment on lines +376 to 385

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="artcommon/artcommonlib/ocp_version_ancestry.py"

echo "== Line count =="
wc -l "$FILE"

echo
echo "== Outline =="
ast-grep outline "$FILE" --view expanded || true

echo
echo "== Relevant section =="
sed -n '320,450p' "$FILE" | cat -n

Repository: openshift-eng/art-tools

Length of output: 7535


Make release-controller parsing non-fatal. Invalid JSON, non-object payloads, non-list tags, or malformed tag entries can raise outside the httpx.HTTPError handler and stop the second stream from being queried. Validate the decoded payload per stream and return [] on parse/shape errors instead.

🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 377-377: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(rf'^{major}.{minor}.')
Note: [CWE-1333] Inefficient Regular Expression Complexity.

(redos-non-literal-regex-python)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@artcommon/artcommonlib/ocp_version_ancestry.py` around lines 376 - 385,
Update the release-controller response parsing around the tags/version
extraction loop to validate each stream’s decoded payload before accessing it:
handle invalid JSON, non-object payloads, non-list tags, and malformed tag
entries by returning [] for that stream. Keep HTTP errors handled as currently,
and ensure parsing/shape failures are contained so the second stream is still
queried.

except ValueError:
continue

return sort_semver(versions)
return versions


async def get_release_controller_versions_async(
major: int,
minor: int,
go_arch: str,
release_controller_url: str = '',
timeout: float = 30.0,
) -> list[str]:
"""
Query the release controller's stable and dev-preview streams to get all promoted versions
for a major.minor.

The release controller tracks all versions that have been promoted, regardless of whether
their cincinnati-graph-data PR has merged. This supplements Cincinnati data to avoid missing
recently-promoted z-streams.

Both {major}-stable and {major}-dev-preview streams are queried because EC releases
(e.g., 5.0.0-ec.5) are promoted to dev-preview, not stable.

:param major: Major version (e.g., 4 or 5)
:param minor: Minor version (e.g., 18)
:param go_arch: Go architecture name (e.g., 'amd64', 's390x', 'arm64', 'ppc64le', 'multi')
:param release_controller_url: Base URL for the release controller. If empty, defaults to
'https://{go_arch}.ocp.releases.ci.openshift.org'.
:param timeout: HTTP request timeout in seconds
:return: List of version strings matching major.minor, sorted descending by semver
"""
if not release_controller_url:
release_controller_url = f'https://{go_arch}.ocp.releases.ci.openshift.org'

base_url = release_controller_url.rstrip('/')

streams = [f'{major}-stable', f'{major}-dev-preview']
all_versions: set[str] = set()
for stream in streams:
url = f'{base_url}/api/v1/releasestream/{stream}/tags'
versions = await _fetch_release_controller_tags(url, major, minor, timeout)
all_versions.update(versions)

return sort_semver(list(all_versions))


async def calc_upgrade_sources_async(
Expand Down
148 changes: 113 additions & 35 deletions artcommon/tests/test_ocp_version_ancestry.py
Original file line number Diff line number Diff line change
Expand Up @@ -462,23 +462,34 @@ def test_rejects_ocp_3(self):
class TestGetReleaseControllerVersionsAsync(unittest.IsolatedAsyncioTestCase):
"""Test the get_release_controller_versions_async function"""

def _make_response(self, data):
"""Helper to create a mock HTTP response."""
mock_response = MagicMock()
mock_response.json.return_value = data
mock_response.raise_for_status = MagicMock()
return mock_response

def _make_stream_responses(self, stable_tags, dev_preview_tags):
"""Helper to create side_effect for stable + dev-preview calls."""
stable_resp = self._make_response({"name": "4-stable", "tags": stable_tags})
dev_preview_resp = self._make_response({"name": "4-dev-preview", "tags": dev_preview_tags})
return [stable_resp, dev_preview_resp]

async def test_filters_by_major_minor(self):
"""Only versions matching requested major.minor are returned"""
mock_response = MagicMock()
mock_response.json.return_value = {
"name": "4-stable",
"tags": [
responses = self._make_stream_responses(
stable_tags=[
{"name": "4.18.3", "phase": "Accepted"},
{"name": "4.18.2", "phase": "Accepted"},
{"name": "4.18.1", "phase": "Accepted"},
{"name": "4.17.5", "phase": "Accepted"},
{"name": "4.17.4", "phase": "Accepted"},
],
}
mock_response.raise_for_status = MagicMock()
dev_preview_tags=[],
)

with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.get = AsyncMock(return_value=mock_response)
mock_client.return_value.__aenter__.return_value.get = AsyncMock(side_effect=responses)

result = await get_release_controller_versions_async(4, 18, "amd64")

Expand All @@ -497,79 +508,146 @@ async def test_returns_empty_on_http_error(self):
self.assertEqual(result, [])

async def test_default_url_uses_go_arch(self):
"""Default URL should be constructed from go_arch"""
mock_response = MagicMock()
mock_response.json.return_value = {"name": "4-stable", "tags": []}
mock_response.raise_for_status = MagicMock()
"""Default URL should be constructed from go_arch, querying both streams"""
responses = self._make_stream_responses(stable_tags=[], dev_preview_tags=[])

with patch("httpx.AsyncClient") as mock_client:
mock_get = AsyncMock(return_value=mock_response)
mock_get = AsyncMock(side_effect=responses)
mock_client.return_value.__aenter__.return_value.get = mock_get

await get_release_controller_versions_async(4, 18, "arm64")

called_url = mock_get.call_args[0][0]
calls = mock_get.call_args_list
self.assertEqual(len(calls), 2)
self.assertEqual(
calls[0][0][0], "https://arm64.ocp.releases.ci.openshift.org/api/v1/releasestream/4-stable/tags"
)
self.assertEqual(
called_url, "https://arm64.ocp.releases.ci.openshift.org/api/v1/releasestream/4-stable/tags"
calls[1][0][0], "https://arm64.ocp.releases.ci.openshift.org/api/v1/releasestream/4-dev-preview/tags"
)

async def test_custom_url(self):
"""Custom release_controller_url should be used"""
mock_response = MagicMock()
mock_response.json.return_value = {"name": "4-stable", "tags": []}
mock_response.raise_for_status = MagicMock()
"""Custom release_controller_url should be used for both streams"""
responses = self._make_stream_responses(stable_tags=[], dev_preview_tags=[])

with patch("httpx.AsyncClient") as mock_client:
mock_get = AsyncMock(return_value=mock_response)
mock_get = AsyncMock(side_effect=responses)
mock_client.return_value.__aenter__.return_value.get = mock_get

await get_release_controller_versions_async(
4, 18, "amd64", release_controller_url="https://custom.example.com"
)

called_url = mock_get.call_args[0][0]
self.assertEqual(called_url, "https://custom.example.com/api/v1/releasestream/4-stable/tags")
calls = mock_get.call_args_list
self.assertEqual(calls[0][0][0], "https://custom.example.com/api/v1/releasestream/4-stable/tags")
self.assertEqual(calls[1][0][0], "https://custom.example.com/api/v1/releasestream/4-dev-preview/tags")

async def test_skips_invalid_semver_tags(self):
"""Tags with invalid semver names should be silently skipped"""
mock_response = MagicMock()
mock_response.json.return_value = {
"name": "4-stable",
"tags": [
responses = self._make_stream_responses(
stable_tags=[
{"name": "4.18.1", "phase": "Accepted"},
{"name": "4.18.latest", "phase": "Accepted"}, # not valid semver (no patch number)
{"name": "4.18.0", "phase": "Accepted"},
],
}
mock_response.raise_for_status = MagicMock()
dev_preview_tags=[],
)

with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.get = AsyncMock(return_value=mock_response)
mock_client.return_value.__aenter__.return_value.get = AsyncMock(side_effect=responses)

result = await get_release_controller_versions_async(4, 18, "amd64")

self.assertEqual(result, ['4.18.1', '4.18.0'])

async def test_sorted_descending(self):
"""Results should be sorted in descending semver order"""
mock_response = MagicMock()
mock_response.json.return_value = {
"name": "4-stable",
"tags": [
responses = self._make_stream_responses(
stable_tags=[
{"name": "4.18.0", "phase": "Accepted"},
{"name": "4.18.2", "phase": "Accepted"},
{"name": "4.18.1", "phase": "Accepted"},
],
}
mock_response.raise_for_status = MagicMock()
dev_preview_tags=[],
)

with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.get = AsyncMock(return_value=mock_response)
mock_client.return_value.__aenter__.return_value.get = AsyncMock(side_effect=responses)

result = await get_release_controller_versions_async(4, 18, "amd64")

self.assertEqual(result, ['4.18.2', '4.18.1', '4.18.0'])

async def test_null_tags_returns_empty(self):
"""When the release controller returns {"tags": null}, should return [] instead of crashing"""
responses = [
self._make_response({"name": "5-stable", "tags": None}),
self._make_response({"name": "5-dev-preview", "tags": None}),
]

with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.get = AsyncMock(side_effect=responses)

result = await get_release_controller_versions_async(5, 0, "amd64")

self.assertEqual(result, [])

async def test_dev_preview_versions_included(self):
"""EC releases from dev-preview stream should be included in results"""
responses = [
# stable stream has no 5.0 versions
self._make_response({"name": "5-stable", "tags": []}),
# dev-preview has EC releases
self._make_response(
{
"name": "5-dev-preview",
"tags": [
{"name": "5.0.0-ec.5", "phase": "Accepted"},
{"name": "5.0.0-ec.4", "phase": "Accepted"},
{"name": "5.0.0-ec.3", "phase": "Accepted"},
{"name": "4.22.0-ec.2", "phase": "Accepted"}, # different minor, should be filtered
],
}
),
]

with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.get = AsyncMock(side_effect=responses)

result = await get_release_controller_versions_async(5, 0, "amd64")

self.assertEqual(result, ['5.0.0-ec.5', '5.0.0-ec.4', '5.0.0-ec.3'])

async def test_union_of_stable_and_dev_preview(self):
"""Versions from both stable and dev-preview should be unioned and deduplicated"""
responses = [
self._make_response(
{
"name": "5-stable",
"tags": [
{"name": "5.0.1", "phase": "Accepted"},
{"name": "5.0.0", "phase": "Accepted"},
],
}
),
self._make_response(
{
"name": "5-dev-preview",
"tags": [
{"name": "5.0.0-ec.3", "phase": "Accepted"},
{"name": "5.0.0", "phase": "Accepted"}, # duplicate with stable
],
}
),
]

with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.get = AsyncMock(side_effect=responses)

result = await get_release_controller_versions_async(5, 0, "amd64")

self.assertEqual(result, ['5.0.1', '5.0.0', '5.0.0-ec.3'])


class TestCalcUpgradeSourcesAsync(unittest.IsolatedAsyncioTestCase):
"""Test the calc_upgrade_sources_async function"""
Expand Down