Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not delete sig/att/sbom if subject image exists #116

Merged
merged 1 commit into from
May 27, 2024
+105 −11
Merged

Do not delete sig/att/sbom if subject image exists #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion config/registry_image_pruner/cronjob.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ spec:
command:
- /bin/bash
- '-c'
- echo 'The pruner is temporarily disabled, see https://github.com/konflux-ci/image-controller/pull/115'
- python /image-pruner/prune_images.py --namespace $(NAMESPACE)
volumeMounts:
- name: image-pruner-volume
mountPath: /image-pruner
Expand Down
40 changes: 35 additions & 5 deletions config/registry_image_pruner/image_pruner/prune_images.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,49 @@ def delete_image_tag(quay_token: str, namespace: str, name: str, tag: str) -> No
raise(ex)


def manifest_exists(quay_token: str, namespace: str, name: str, manifest: str) -> bool:
api_url = f"{QUAY_API_URL}/repository/{namespace}/{name}/manifest/{manifest}"
request = Request(api_url, headers={
"Authorization": f"Bearer {quay_token}",
})
resp: HTTPResponse
manifest_exists = True
try:
with urlopen(request) as resp:
if resp.status != 200 and resp.status != 204:
raise RuntimeError(resp.reason)

except HTTPError as ex:
if ex.status != 404:
raise(ex)
else:
manifest_exists = False

return manifest_exists


def remove_tags(tags: List[Dict[str, Any]], quay_token: str, namespace: str, name: str, dry_run: bool = False) -> None:
image_digests = [image["manifest_digest"] for image in tags]
tags_map = {tag_info["name"]: tag_info for tag_info in tags}
tag_regex = re.compile(r"^sha256-([0-9a-f]+)(\.sbom|\.att|\.src|\.sig)$")
manifests_checked = {}
for tag in tags:
# attestation or sbom image
if (match := tag_regex.match(tag["name"])) is not None:
if f"sha256:{match.group(1)}" not in image_digests:
chmeliik marked this conversation as resolved.
Show resolved Hide resolved
if dry_run:
LOGGER.info("Tag %s from %s/%s should be removed", tag["name"], namespace, name)
else:
LOGGER.info("Removing tag %s from %s/%s", tag["name"], namespace, name)
delete_image_tag(quay_token, namespace, name, tag["name"])
# verify that manifest really doesn't exist, because if tag was removed, it won't be in tag list, but may still be in the registry
manifest_existence = manifests_checked.get(f"sha256:{match.group(1)}")
if manifest_existence is None:
manifest_existence = manifest_exists(quay_token, namespace, name, f"sha256:{match.group(1)}")
manifests_checked[f"sha256:{match.group(1)}"] = manifest_existence

if not manifest_existence:
if dry_run:
LOGGER.info("Tag %s from %s/%s should be removed", tag["name"], namespace, name)
else:
LOGGER.info("Removing tag %s from %s/%s", tag["name"], namespace, name)
delete_image_tag(quay_token, namespace, name, tag["name"])
chmeliik marked this conversation as resolved.
Show resolved Hide resolved

elif tag["name"].endswith(".src"):
to_delete = False

Expand Down
74 changes: 69 additions & 5 deletions config/registry_image_pruner/image_pruner/test_prune_images.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ def test_no_image_with_expected_suffixes_is_found(self, delete_image_tag, urlope
@patch.dict(os.environ, {"QUAY_TOKEN": QUAY_TOKEN})
@patch("sys.argv", ["prune_images", "--namespace", "sample"])
@patch("prune_images.urlopen")
def test_remove_orphan_tags_with_expected_suffixes(self, urlopen):
@patch("prune_images.manifest_exists")
def test_remove_orphan_tags_with_expected_suffixes(self, manifest_exists, urlopen):
fetch_repos_rv = MagicMock()
response = MagicMock()
response.status = 200
Expand Down Expand Up @@ -187,6 +188,19 @@ def test_remove_orphan_tags_with_expected_suffixes(self, urlopen):
delete_tag_rv,
]

manifest_exists.side_effect = [
False,
False,
False,
False,
False,
False,
False,
False,
False,
False,
]

main()

def _assert_deletion_request(request: Request, tag: str) -> None:
Expand All @@ -207,7 +221,8 @@ def _assert_deletion_request(request: Request, tag: str) -> None:
@patch.dict(os.environ, {"QUAY_TOKEN": QUAY_TOKEN})
@patch("sys.argv", ["prune_images", "--namespace", "sample", "--dry-run"])
@patch("prune_images.urlopen")
def test_remove_tag_dry_run(self, urlopen):
@patch("prune_images.manifest_exists")
def test_remove_tag_dry_run(self, manifest_exists, urlopen):
fetch_repos_rv = MagicMock()
response = MagicMock()
response.status = 200
Expand Down Expand Up @@ -236,6 +251,9 @@ def test_remove_tag_dry_run(self, urlopen):
# return the repo info including tags
get_repo_rv,
]
manifest_exists.side_effect = [
False,
]

with self.assertLogs(LOGGER) as logs:
main()
Expand Down Expand Up @@ -297,7 +315,8 @@ def test_handle_image_repos_pagination(self, urlopen):
class TestRemoveTags(unittest.TestCase):

@patch("prune_images.delete_image_tag")
def test_remove_tags(self, delete_image_tag):
@patch("prune_images.manifest_exists")
def test_remove_tags(self, manifest_exists, delete_image_tag):
tags = [
{
"name": "sha256-502c8c35e31459e8774f88e115d50d2ad33ba0e9dfd80429bc70ed4c1fd9e0cd.att",
Expand All @@ -309,6 +328,11 @@ def test_remove_tags(self, delete_image_tag):
},
]

manifest_exists.side_effect = [
False,
False,
]

with self.assertLogs(LOGGER) as logs:
remove_tags(tags, QUAY_TOKEN, "some", "repository")
logs_output = "\n".join(logs.output)
Expand All @@ -320,7 +344,8 @@ def test_remove_tags(self, delete_image_tag):
delete_image_tag.assert_has_calls(calls)

@patch("prune_images.delete_image_tag")
def test_remove_tags_dry_run(self, delete_image_tag):
@patch("prune_images.manifest_exists")
def test_remove_tags_dry_run(self, manifest_exists, delete_image_tag):
tags = [
{
"name": "sha256-502c8c35e31459e8774f88e115d50d2ad33ba0e9dfd80429bc70ed4c1fd9e0cd.att",
Expand All @@ -332,6 +357,11 @@ def test_remove_tags_dry_run(self, delete_image_tag):
}
]

manifest_exists.side_effect = [
False,
False,
]

with self.assertLogs(LOGGER) as logs:
remove_tags(tags, QUAY_TOKEN, "some", "repository", dry_run=True)
logs_output = "\n".join(logs.output)
Expand Down Expand Up @@ -364,7 +394,34 @@ def test_remove_tags_nothing_to_remove(self, delete_image_tag):
delete_image_tag.assert_not_called()

@patch("prune_images.delete_image_tag")
def test_remove_tags_multiple_tags(self, delete_image_tag):
@patch("prune_images.manifest_exists")
def test_remove_tags_nothing_to_remove_digest_exists(self, manifest_exists, delete_image_tag):
tags = [
{
"name": "sha256-502c8c35e31459e8774f88e115d50d2ad33ba0e9dfd80429bc70ed4c1fd9e0cd.att",
"manifest_digest": "sha256:125c1d18ee1c3b9bde0c7810fcb0d4ffbc67e9b0c5b88bb8df9ca039bc1c9457",
},
{
"name": "sha256-502c8c35e31459e8774f88e115d50d2ad33ba0e9dfd80429bc70ed4c1fd9e0cd.sbom",
"manifest_digest": "sha256:351326f899759a9a7ae3ca3c1cbdadcc8012f43231c145534820a68bdf36d55b",
},
]

manifest_exists.side_effect = [
True,
True,
]

with self.assertRaisesRegex(AssertionError, expected_regex="no logs of level INFO"):
with self.assertLogs(LOGGER) as logs:
remove_tags(tags, QUAY_TOKEN, "some", "repository", dry_run=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not make sense to set dry_run=True here. When it is True, delete_image_tag will be never called under whatever conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well it was in that other test above :)


delete_image_tag.assert_not_called()


@patch("prune_images.delete_image_tag")
@patch("prune_images.manifest_exists")
def test_remove_tags_multiple_tags(self, manifest_exists, delete_image_tag):
tags = [
{
"name": "sha256-502c8c35e31459e8774f88e115d50d2ad33ba0e9dfd80429bc70ed4c1fd9e0cd.att",
Expand All @@ -384,6 +441,13 @@ def test_remove_tags_multiple_tags(self, delete_image_tag):
},
]

manifest_exists.side_effect = [
False,
False,
False,
False,
]

with self.assertLogs(LOGGER) as logs:
remove_tags(tags, QUAY_TOKEN, "some", "repository")
logs_output = "\n".join(logs.output)
Expand Down
Loading