Merge pull request #1135 from betatim/fix-unescaped-ref

[MRG] Fix escaping of non SHA1 refs

Author: betatim

binderhub/builder.py

@@ -54,6 +54,61 @@
 LAUNCHES_INPROGRESS = Gauge('binderhub_inprogress_launches', 'Launches currently in progress')
 
 
+def _generate_build_name(build_slug, ref, prefix='', limit=63, ref_length=6):
+    """Generate a unique build name with a limited character length.
+
+    Guaranteed (to acceptable level) to be unique for a given user, repo,
+    and ref.
+
+    We really, *really* care that we always end up with the same
+    'build_name' for a particular repo + ref, but the default max
+    character limit for build names is 63. To meet this constraint, we
+    include a prefixed hash of the user / repo in all build names and do
+    some length limiting :)
+
+    Note that ``build`` names only need to be unique over a shorter period
+    of time, while ``image`` names need to be unique for longer. Hence,
+    different strategies are used.
+
+    We also ensure that the returned value is DNS safe, by only using
+    ascii lowercase + digits. everything else is escaped
+    """
+    # escape parts that came from providers (build slug, ref)
+    # build names are case-insensitive `.lower()` is called at the end
+    build_slug = _safe_build_slug(build_slug, limit=limit - len(prefix) - ref_length - 1)
+    ref = _safe_build_slug(ref, limit=ref_length, hash_length=2)
+
+    return '{prefix}{safe_slug}-{ref}'.format(
+        prefix=prefix,
+        safe_slug=build_slug,
+        ref=ref[:ref_length],
+    ).lower()
+
+
+def _safe_build_slug(build_slug, limit, hash_length=6):
+    """Create a unique-ish name from a slug.
+
+    This function catches a bug where a build slug may not produce a valid
+    image name (e.g. arepo name ending with _, which results in image name
+    ending with '-' which is invalid). This ensures that the image name is
+    always safe, regardless of build slugs returned by providers
+    (rather than requiring all providers to return image-safe build slugs
+    below a certain length).
+
+    Since this changes the image name generation scheme, all existing cached
+    images will be invalidated.
+    """
+    build_slug_hash = hashlib.sha256(build_slug.encode('utf-8')).hexdigest()
+    safe_chars = set(string.ascii_letters + string.digits)
+    def escape(s):
+        return escapism.escape(s, safe=safe_chars, escape_char='-')
+    build_slug = escape(build_slug)
+    return '{name}-{hash}'.format(
+        name=build_slug[:limit - hash_length - 1],
+        hash=build_slug_hash[:hash_length],
+    ).lower()
+
+
 class BuildHandler(BaseHandler):
     """A handler for working with GitHub."""
 
@@ -125,63 +180,6 @@ def initialize(self):
 
         self.event_log = self.settings['event_log']
 
-    def _generate_build_name(self, build_slug, ref, prefix='', limit=63, ref_length=6):
-        """
-        Generate a unique build name with a limited character length..
-
-        Guaranteed (to acceptable level) to be unique for a given user, repo,
-        and ref.
-
-        We really, *really* care that we always end up with the same
-        'build_name' for a particular repo + ref, but the default max
-        character limit for build names is 63. To meet this constraint, we
-        include a prefixed hash of the user / repo in all build names and do
-        some length limiting :)
-
-        Note that ``build`` names only need to be unique over a shorter period
-        of time, while ``image`` names need to be unique for longer. Hence,
-        different strategies are used.
-
-        We also ensure that the returned value is DNS safe, by only using
-        ascii lowercase + digits. everything else is escaped
-        """
-
-        # escape parts that came from providers (build slug, ref)
-        # only build_slug *really* needs this (refs should be sha1 hashes)
-        # build names are case-insensitive because ascii_letters are allowed,
-        # and `.lower()` is called at the end
-        safe_chars = set(string.ascii_letters + string.digits)
-        def escape(s):
-            return escapism.escape(s, safe=safe_chars, escape_char='-')
-
-        build_slug = self._safe_build_slug(build_slug, limit=limit - len(prefix) - ref_length - 1)
-        ref = escape(ref)
-
-        return '{prefix}{safe_slug}-{ref}'.format(
-            prefix=prefix,
-            safe_slug=build_slug,
-            ref=ref[:ref_length],
-        ).lower()
-
-    def _safe_build_slug(self, build_slug, limit, hash_length=6):
-        """
-        This function catches a bug where build slug may not produce a valid image name
-        (e.g. repo name ending with _, which results in image name ending with '-' which is invalid).
-        This ensures that the image name is always safe, regardless of build slugs returned by providers
-        (rather than requiring all providers to return image-safe build slugs below a certain length).
-        Since this changes the image name generation scheme, all existing cached images will be invalidated.
-        """
-        build_slug_hash = hashlib.sha256(build_slug.encode('utf-8')).hexdigest()
-        safe_chars = set(string.ascii_letters + string.digits)
-        def escape(s):
-            return escapism.escape(s, safe=safe_chars, escape_char='-')
-        build_slug = escape(build_slug)
-        return '{name}-{hash}'.format(
-            name=build_slug[:limit - hash_length - 1],
-            hash=build_slug_hash[:hash_length],
-        ).lower()
-
-
     async def fail(self, message):
         await self.emit({
             'phase': 'failed',
@@ -280,9 +278,9 @@ async def get(self, provider_prefix, _unescaped_spec):
         image_prefix = self.settings['image_prefix']
 
         # Enforces max 255 characters before image
-        safe_build_slug = self._safe_build_slug(provider.get_build_slug(), limit=255 - len(image_prefix))
+        safe_build_slug = _safe_build_slug(provider.get_build_slug(), limit=255 - len(image_prefix))
 
-        build_name = self._generate_build_name(provider.get_build_slug(), ref, prefix='build-')
+        build_name = _generate_build_name(provider.get_build_slug(), ref, prefix='build-')
 
         image_name = self.image_name = '{prefix}{build_slug}:{ref}'.format(
             prefix=image_prefix,

binderhub/tests/test_builder.py

@@ -0,0 +1,21 @@
+import pytest
+
+from binderhub.builder import _generate_build_name
+
+
+@pytest.mark.parametrize('ref,build_slug', [
+    # a long ref, no special characters at critical positions
+    ('3035124.v3.0', 'dataverse-dvn-2ftjclkp'),
+    # with ref_length=6 this has a full stop that gets escaped to a -
+    # as the last character, this used to cause an error
+    ('20460.v1.0', 'dataverse-s6-2fde95rt'),
+    # short ref, should just work and need no special processing
+    ('123456', 'dataverse-s6-2fde95rt')
+])
+def test_build_name(build_slug, ref):
+    # Build names have to be usable as pod names, which means they have to
+    # be usable as hostnames as well.
+    build_name = _generate_build_name(build_slug, ref)
+
+    last_char = build_name[-1]
+    assert last_char not in ("-", "_", ".")

binderhub/tests/test_repoproviders.py

@@ -134,12 +134,18 @@ async def test_hydroshare_doi():
     resolved_spec = await provider.get_resolved_spec()
     assert resolved_spec == repo_url
 
+
 @pytest.mark.parametrize('spec,resolved_spec,resolved_ref,resolved_ref_url,build_slug', [
     ['10.7910/DVN/TJCLKP',
      '10.7910/DVN/TJCLKP',
      '3035124.v3.0',
      'https://doi.org/10.7910/DVN/TJCLKP',
      'dataverse-dvn-2ftjclkp'],
+    ['10.25346/S6/DE95RT',
+     '10.25346/S6/DE95RT',
+     '20460.v1.0',
+     'https://doi.org/10.25346/S6/DE95RT',
+     'dataverse-s6-2fde95rt']
 ])
 async def test_dataverse(spec, resolved_spec, resolved_ref, resolved_ref_url, build_slug):
     provider = DataverseProvider(spec=spec)