Repository: app-sre/sretoolbox

Issue:
_parse_image_url() doesn't support image digests
A recent deploy of a Hive operator failed with:

`Image does not exist: quay.io/****/pagerduty-operator-registry@sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e`

The image exists:

```
docker pull quay.io/****/pagerduty-operator-registry@sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e
quay.io/****/pagerduty-operator-registry@sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e: Pulling from ****/pagerduty-operator-registry
Digest: sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e
Status: Image is up to date for quay.io/****/pagerduty-operator-registry@sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e
quay.io/****/pagerduty-operator-registry@sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e
```

```
13:58:43 [2021-04-16 17:58:43] [ERROR] [saasherder.py:_check_image:543] - [saas-pagerduty-operator/pagerduty-operator] https://github.com/openshift/pagerduty-operator/blob/master/hack/olm-registry/olm-artifacts-template.yaml: Image does not exist: quay.io/****/pagerduty-operator-registry@sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e
13:58:43 Traceback (most recent call last):
13:58:43   File "/usr/local/bin/qontract-reconcile", line 33, in <module>
13:58:43     sys.exit(load_entry_point('reconcile==0.2.2', 'console_scripts', 'qontract-reconcile')())
13:58:43   File "/usr/local/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 829, in __call__
13:58:43     return self.main(*args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 782, in main
13:58:43     rv = self.invoke(ctx)
13:58:43   File "/usr/local/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1259, in invoke
13:58:43     return _process_result(sub_ctx.command.invoke(sub_ctx))
13:58:43   File "/usr/local/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1066, in invoke
13:58:43     return ctx.invoke(self.callback, **ctx.params)
13:58:43   File "/usr/local/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 610, in invoke
13:58:43     return callback(*args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/utils/environ.py", line 15, in f_environ
13:58:43     f(*args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/utils/environ.py", line 15, in f_environ
13:58:43     f(*args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/utils/binary.py", line 18, in f_binary
13:58:43     f(*args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/utils/binary.py", line 60, in f_binary_version
13:58:43     f(*args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/decorators.py", line 21, in new_func
13:58:43     return f(get_current_context(), *args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/cli.py", line 758, in openshift_saas_deploy
13:58:43     saas_file_name, env_name, gitlab_project_id)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/cli.py", line 399, in run_integration
13:58:43     func_container.run(dry_run, *args, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/utils/defer.py", line 15, in func_wrapper
13:58:43     return func(*args, defer=defer, **kwargs)
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/openshift_saas_deploy.py", line 69, in run
13:58:43     if not saasherder.validate_promotions():
13:58:43   File "/usr/local/lib/python3.6/site-packages/reconcile-0.2.2-py3.6.egg/reconcile/utils/saasherder.py", line 928, in validate_promotions
13:58:43     commit_sha = item['commit_sha']
13:58:43 TypeError: 'NoneType' object is not subscriptable
```

SREP haven't hit this issue yet because on cluster operators are deployed via selector sync set inside the OC template. Saasherder doesn't validate the CatalogSource images inside the selector sync set [here](https://github.com/app-sre/qontract-reconcile/blob/master/reconcile/utils/saasherder.py#L524). This is different for Hive operators as we deploy the CatalogSource directly in the OC template.

It looks like that method is the issue.

Parsing a digest returns the following structure:

```
{'scheme': 'docker://', 'registry': 'docker.io', 'port_colon': None, 'port': None, 'registry_slash': None, 'repository': 'library', 'repo_slash': None, 'image': 'sha256', 'tag_colon': ':', 'tag': 'bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e'}
```

cc/ @2uasimojo @BumbleFeng @apahim 

Discussion:

Patch:
diff --git a/sretoolbox/container/image.py b/sretoolbox/container/image.py
index f750ecf..8e9e083 100644
--- a/sretoolbox/container/image.py
+++ b/sretoolbox/container/image.py
@@ -22,6 +22,16 @@ class ImageComparisonError(Exception):
     """
 
 
+class NoTagForImageByDigest(Exception):
+    """
+    Raised when the Image was constructed with a by-digest URL and an
+    operation is attempted that requires a tag.
+    """
+    def __init__(self, image):
+        super().__init__(
+            f"Can't determine a unique tag for Image: {str(image)}")
+
+
 class Image:
     """
     Represents a container image.
@@ -47,6 +57,11 @@ class Image:
         else:
             self.tag = tag_override
 
+        self._cache_digest = None
+        # If the URL was by-digest, we can cache this right away.
+        if image_data['digest']:
+            self._cache_digest = f"{image_data['digest']}"
+
         self.username = username
         self.password = password
         if all([username is not None,
@@ -68,12 +83,11 @@ class Image:
 
         self._cache_tags = None
         self._cache_manifest = None
-        self._cache_digest = None
 
     @property
     def digest(self):
         """
-        Property to cache the digest returned but get_manifest()
+        Property to cache the digest returned by get_manifest()
         """
         if self._cache_digest is None:
             manifest = self._get_manifest()
@@ -90,7 +104,12 @@ class Image:
         url = f'{self.registry_api}/v2'
         if self.repository is not None:
             url += f'/{self.repository}'
-        url += f'/{self.image}/manifests/{self.tag}'
+        # NOTE(efried): This should never go to the network. If the image was
+        # initialized by digest, the `digest` property uses that value.
+        reference = self.tag or self.digest
+        # NOTE(efried): At least for quay, this returns schemaVersion 1 by tag
+        # and 2 by digest.
+        url += f'/{self.image}/manifests/{reference}'
         return self._request_get(url)
 
     def _get_tags(self):
@@ -183,7 +202,7 @@ class Image:
             - docker://docker.io/user/fedora
             - docker://registry.example.com:5000/repo01/centos:latest
 
-        Regardless the components provided in the URL, we have to make
+        Regardless of the components provided in the URL, we have to make
         sure that we can properly split each of them and, for those
         not provided, assume safe defaults.
 
@@ -209,6 +228,10 @@ class Image:
         default_registry = 'docker.io'
         default_tag = 'latest'
 
+        # The image is either specified by digest (...@sha256:xxxx...) or
+        # by tag (...:tag-name). We decide based on the presence of the
+        # '@' or the ':'. If we find neither, by-tag is assumed,
+        # defaulting to 'latest'.
         parsed_image_url = re.search(
             r'(?P<scheme>\w+://)?'  # Scheme (optional) e.g. docker://
             r'(?P<registry>[\w\-]+[.][\w\-.]+)?'  # Registry domain (optional)
@@ -218,8 +241,15 @@ class Image:
             r'(?P<repository>[\w\-]+)?'  # Repository (optional)
             r'(?(repository)(?P<repo_slash>/))'  # Slash, if repo is present
             r'(?P<image>[\w\-.]+)'  # Image path (mandatory)
-            r'(?P<tag_colon>:)?'  # Tag colon (optional)
-            r'(?(tag_colon)(?P<tag>[\w\-.]+))'  # Tag (if tag colon is present)
+            # '@' delimiter iff it's a by-digest URI (optional)
+            r'(?P<digest_at>@)?'
+            # Digest ('sha256:' + 64 lowercase hex chars) iff '@' is present
+            r'(?(digest_at)(?P<digest>sha256:[0-9a-f]{64}))'
+            # Tag colon if it's a by-digest URI (optional)
+            # Not allowed if we found a digest
+            r'(?(digest)|(?P<tag_colon>:))?'
+            # Tag (if tag colon is present)
+            r'(?(tag_colon)(?P<tag>[\w\-.]+))'
             '$', image_url)
 
         if parsed_image_url is None:
@@ -243,7 +273,9 @@ class Image:
             else:
                 image_url_struct['repository'] = None
 
-        if image_url_struct.get('tag') is None:
+        # By-digest URIs don't use tags; but otherwise default to `latest` if
+        # absent
+        if all(image_url_struct.get(x) is None for x in ('tag', 'digest')):
             image_url_struct['tag'] = default_tag
 
         return image_url_struct
@@ -340,7 +372,14 @@ class Image:
     def url_tag(self):
         """
         Returns the image url in the tag format.
+
+        If we were constructed with a by-digest URL, this will raise
+        NoTagForImageByDigest since there may be more than one tag for a given
+        image.
         """
+        if self.tag is None:
+            raise NoTagForImageByDigest(self)
+
         url_tag = f'{self.registry}'
         if self.repository is not None:
             url_tag += f'/{self.repository}'
@@ -398,4 +437,8 @@ class Image:
         return f"{self.__class__.__name__}(url='{self}')"
 
     def __str__(self):
-        return f'{self.scheme}{self.url_tag}'
+        if self.tag is None:
+            url = self.url_digest
+        else:
+            url = self.url_tag
+        return f'{self.scheme}{url}'


Test patch:
diff --git a/tests/test_container.py b/tests/test_container.py
index 8472383..14ce7c7 100644
--- a/tests/test_container.py
+++ b/tests/test_container.py
@@ -2,6 +2,8 @@ import pytest
 
 from sretoolbox.container import Image
 
+A_SHA = (
+  'sha256:bc1ed82a75f2ca160225b8281c50b7074e7678c2a1f61b1fb298e545b455925e')
 PARSER_DATA = [
     ('memcached',
      {'scheme': 'docker://',
@@ -45,7 +47,16 @@ PARSER_DATA = [
       'registry': 'docker.io',
       'repository': 'tnozicka',
       'image': 'openshift-acme',
-      'tag': 'v0.8.0-pre-alpha'})
+      'tag': 'v0.8.0-pre-alpha'}),
+    # By digest
+    (f'quay.io/app-sre/pagerduty-operator-registry@{A_SHA}',
+     {'scheme': 'docker://',
+      'registry': 'quay.io',
+      'repository': 'app-sre',
+      'image': 'pagerduty-operator-registry',
+      # Importantly, tag is unset for by-digest URIs
+      'tag': None,
+      'digest': A_SHA}),
 ]
 
 STR_DATA = [
@@ -58,7 +69,13 @@ STR_DATA = [
     ('docker.io:8080/app-sre/fedora:30',
      'docker://docker.io:8080/app-sre/fedora:30'),
     ('quay.io/app-sre/qontract-reconcile:build',
-     'docker://quay.io/app-sre/qontract-reconcile:build')
+     'docker://quay.io/app-sre/qontract-reconcile:build'),
+    # By digest stringifies with the digest
+    (f'quay.io/app-sre/pagerduty-operator-registry@{A_SHA}',
+     f'docker://quay.io/app-sre/pagerduty-operator-registry@{A_SHA}'),
+    # By digest still defaults stuff
+    (f'pagerduty-operator-registry@{A_SHA}',
+     f'docker://docker.io/library/pagerduty-operator-registry@{A_SHA}'),
 ]
 
 
@@ -77,7 +94,11 @@ TAG_OVERRIDE_DATA = [
      'docker://docker.io:443/app-sre/fedora:31'),
     ('quay.io/app-sre/qontract-reconcile:build',
      'latest',
-     'docker://quay.io/app-sre/qontract-reconcile:latest')
+     'docker://quay.io/app-sre/qontract-reconcile:latest'),
+    # By digest allows tag override
+    (f'quay.io/app-sre/pagerduty-operator-registry@{A_SHA}',
+     'foo',
+     'docker://quay.io/app-sre/pagerduty-operator-registry:foo'),
 ]
 
 
@@ -90,7 +111,11 @@ class TestContainer:
         assert image.registry == expected_struct['registry']
         assert image.repository == expected_struct['repository']
         assert image.image == expected_struct['image']
-        assert image.tag == expected_struct['tag']
+        assert image.tag == expected_struct.get('tag')
+        expected_digest = expected_struct.get('digest')
+        # Condition this to avoid the network.
+        if expected_digest:
+            assert image.digest == expected_digest
 
     @pytest.mark.parametrize('image, expected_image_url', STR_DATA)
     def test_str(self, image, expected_image_url):
@@ -102,3 +127,9 @@ class TestContainer:
     def test_tag_override(self, image, tag, expected_image_url):
         image = Image(image, tag)
         assert str(image) == expected_image_url
+
+    def test_no_tag(self):
+        image = Image(f"quay.io/foo/bar@{A_SHA}")
+        with pytest.raises(Exception) as e:
+            _ = image.url_tag
+        assert e.typename == 'NoTagForImageByDigest'