Merge pull request #146 from EDITD/correct-image-registry-handling

Correct image registry handling

.gitignore

@@ -4,5 +4,6 @@
 dist/
 build/
 
-docs/_html
-docs/build
+/docs/_html
+/docs/build
+/.coverage

CHANGELOG.md

@@ -2,10 +2,15 @@
 
 ### Unreleased
 
+# v13.9.6
+- Fix default registry option to not override registry for images specified
+  directly, so they keep using the docker server default registry (dockerhub)
+
 # v13.9.5.1
 - No functional change, this is just validating the change of CI to Github Actions
 
 # v13.9.5
+DO NOT USE: This has a bug fixed in v13.9.6
 - Allow adding a default registry in command line instead of
   specifying the registry in kubetools.yml file.
 

kubetools/deploy/image.py

@@ -48,7 +48,7 @@ def has_app_commit_image(registry, app_name, context_name, commit_hash):
     return True
 
 
-def _get_container_contexts_from_config(app_config):
+def get_container_contexts_from_config(app_config):
     context_name_to_build = {}
     container_contexts = app_config.get('containerContexts', {})
     for deployment, data in app_config.get('deployments', {}).items():
@@ -95,15 +95,15 @@ def _ensure_docker_images(
         additional_tags = []
     project_name = kubetools_config['name']
 
-    context_name_to_build = _get_container_contexts_from_config(kubetools_config)
+    context_name_to_build = get_container_contexts_from_config(kubetools_config)
     context_name_to_registry = {
         context_name: build_context.get('registry', default_registry)
         for context_name, build_context in context_name_to_build.items()
     }
     build_context_keys = list(context_name_to_build.keys())
 
     # Check if the image already exists in the registry
-    if not build_context_keys or all(
+    if all(
         has_app_commit_image(
             context_name_to_registry[context_name],
             project_name,

kubetools/kubernetes/config/__init__.py

@@ -38,10 +38,6 @@ def _ensure_image(
     default_registry=None,
 ):
     if 'image' in container:
-        registry_image = container['image'].split('/')
-        if len(registry_image) == 1 and default_registry is not None:
-            # If registry is not specified but a default was provided
-            container['image'] = f'{default_registry}/{container["image"]}'
         return
 
     if 'containerContext' in container:
@@ -57,6 +53,11 @@ def _ensure_image(
 
     container['image'] = context_name_to_image[context_name]
 
+    registry_image = container['image'].split('/')
+    if len(registry_image) == 1 and default_registry is not None:
+        # If registry is not specified but a default was provided
+        container['image'] = f'{default_registry}/{container["image"]}'
+
 
 def _get_containers_data(
     containers,

tests/configs/docker_registry/Dockerfile

@@ -0,0 +1,2 @@
+FROM python:3.9-slim
+ENTRYPOINT ["python"]

tests/configs/docker_registry/k8s_deployments.yml

@@ -20,10 +20,40 @@ spec:
         - {name: KUBE, value: 'true'}
         image: generic-registry/generic-image
         imagePullPolicy: Always
-        name: with-generic-registry
+        name: image-with-embedded-registry
+#      - command: [uwsgi, --ini, /etc/uwsgi.conf]
+#        env:
+#        - {name: KUBE, value: 'true'}
+#        image: specific-registry/generic-image
+#        imagePullPolicy: Always
+#        name: image-with-specify-registry
       - command: [uwsgi, --ini, /etc/uwsgi.conf]
         env:
         - {name: KUBE, value: 'true'}
-        image: default-registry/generic-image
+        image: generic-image
         imagePullPolicy: Always
-        name: with-default-registry
+        name: image-without-registry
+      - command: [uwsgi, --ini, /etc/uwsgi.conf]
+        env:
+        - {name: KUBE, value: 'true'}
+        image: default-registry/generic-app:generic-app-build-dockerfile-commit-thisisacommithash
+        imagePullPolicy: Always
+        name: build-dockerfile
+      - command: [uwsgi, --ini, /etc/uwsgi.conf]
+        env:
+        - {name: KUBE, value: 'true'}
+        image: default-registry/generic-app:generic-containerContext-commit-thisisacommithash
+        imagePullPolicy: Always
+        name: build-containerContext
+      - command: [uwsgi, --ini, /etc/uwsgi.conf]
+        env:
+        - {name: KUBE, value: 'true'}
+        image: specific-registry/generic-app:generic-app-build-dockerfile-specify-registry-commit-thisisacommithash
+        imagePullPolicy: Always
+        name: build-dockerfile-specify-registry
+      - command: [uwsgi, --ini, /etc/uwsgi.conf]
+        env:
+        - {name: KUBE, value: 'true'}
+        image: specific-registry/generic-app:registry-containerContext-commit-thisisacommithash
+        imagePullPolicy: Always
+        name: build-containerContext-specify-registry

tests/configs/docker_registry/kubetools.yml

@@ -1,17 +1,46 @@
 name: generic-app
 
+containerContexts:
+  generic-containerContext:
+    build:
+      dockerfile: Dockerfile
+  registry-containerContext:
+    build:
+      dockerfile: Dockerfile
+      registry: specific-registry
 
 deployments:
   generic-app:
     containers:
-      with-generic-registry:
+      image-with-embedded-registry:
         image: generic-registry/generic-image
         command: [uwsgi, --ini, /etc/uwsgi.conf]
-        ports:
-          - 80
 
-      with-default-registry:
+# Not supported. Do we want to support this? I think it's covered by ^this^
+#      image-with-specify-registry:
+#        image: generic-image
+#        registry: specific-registry
+#        command: [uwsgi, --ini, /etc/uwsgi.conf]
+
+      image-without-registry:
         image: generic-image
         command: [uwsgi, --ini, /etc/uwsgi.conf]
-        ports:
-          - 9090
+
+      build-dockerfile:
+        build:
+          dockerfile: Dockerfile
+        command: [ uwsgi, --ini, /etc/uwsgi.conf ]
+
+      build-containerContext:
+        containerContext: generic-containerContext
+        command: [ uwsgi, --ini, /etc/uwsgi.conf ]
+
+      build-dockerfile-specify-registry:
+        build:
+          dockerfile: Dockerfile
+          registry: specific-registry
+        command: [ uwsgi, --ini, /etc/uwsgi.conf ]
+
+      build-containerContext-specify-registry:
+        containerContext: registry-containerContext
+        command: [ uwsgi, --ini, /etc/uwsgi.conf ]

tests/test_config_generation.py

@@ -4,29 +4,62 @@
 import yaml
 
 from kubetools.config import load_kubetools_config
+from kubetools.deploy.image import get_container_contexts_from_config, get_docker_tag_for_commit
 from kubetools.kubernetes.api import get_object_name
 from kubetools.kubernetes.config import generate_kubernetes_configs_for_project
 
 
-def _assert_yaml_objects(objects, yaml_filename):
-    with open(yaml_filename, 'r') as f:
-        desired_objects = list(yaml.safe_load_all(f))
+class TestKubernetesConfigGeneration(TestCase):
+    def test_basic_app_configs(self):
+        _test_configs('basic_app')
 
-    objects.sort(key=get_object_name)
-    desired_objects.sort(key=get_object_name)
+    def test_dependencies_configs(self):
+        _test_configs('dependencies')
 
-    assert objects == desired_objects
+    def test_dev_overrides_configs(self):
+        _test_configs('dev_overrides', dev=True)
+
+    def test_k8s_container_passthrough_configs(self):
+        _test_configs('k8s_container_passthrough')
+
+    def test_k8s_cronjobs_beta_api_version_configs(self):
+        _test_configs('k8s_cronjobs_beta_api_version')
+
+    def test_multiple_deployments_configs(self):
+        _test_configs('multiple_deployments')
+
+    def test_docker_registry_configs(self):
+        _test_configs('docker_registry', default_registry='default-registry')
 
 
 def _test_configs(folder_name, default_registry=None, **kwargs):
     app_dir = path.join('tests', 'configs', folder_name)
 
     kubetools_config = load_kubetools_config(app_dir, **kwargs)
 
+    # TODO: refactor deploy.image._ensure_docker_images to extract the logic to a function and
+    #  de-duplicate it from here
+    build_contexts = get_container_contexts_from_config(kubetools_config)
+    context_name_to_registry = {
+        context_name: build_context.get('registry', default_registry)
+        for context_name, build_context in build_contexts.items()
+    }
+    context_images = {
+        # Build the context name -> image dict
+        context_name: get_docker_tag_for_commit(
+            context_name_to_registry[context_name],
+            kubetools_config['name'],
+            context_name,
+            'thisisacommithash',
+        )
+        for context_name in build_contexts.keys()
+    }
+
     with mock.patch('kubetools.kubernetes.config.job.uuid4', lambda: 'UUID'):
         services, deployments, jobs, cronjobs = generate_kubernetes_configs_for_project(
             kubetools_config,
             default_registry=default_registry,
+            context_name_to_image=context_images,
         )
 
     k8s_files = listdir(app_dir)
@@ -43,24 +76,11 @@ def _test_configs(folder_name, default_registry=None, **kwargs):
         _assert_yaml_objects(cronjobs, path.join(app_dir, 'k8s_cronjobs_beta.yml'))
 
 
-class TestKubernetesConfigGeneration(TestCase):
-    def test_basic_app_configs(self):
-        _test_configs('basic_app')
-
-    def test_dependencies_configs(self):
-        _test_configs('dependencies')
-
-    def test_dev_overrides_configs(self):
-        _test_configs('dev_overrides', dev=True)
-
-    def test_k8s_container_passthrough_configs(self):
-        _test_configs('k8s_container_passthrough')
-
-    def test_k8s_cronjobs_beta_api_version_configs(self):
-        _test_configs('k8s_cronjobs_beta_api_version')
+def _assert_yaml_objects(objects, yaml_filename):
+    with open(yaml_filename, 'r') as f:
+        desired_objects = list(yaml.safe_load_all(f))
 
-    def test_multiple_deployments_configs(self):
-        _test_configs('multiple_deployments')
+    objects.sort(key=get_object_name)
+    desired_objects.sort(key=get_object_name)
 
-    def test_docker_registry_configs(self):
-        _test_configs('docker_registry', default_registry='default-registry')
+    assert objects == desired_objects

tests/test_make_job_config.py

@@ -40,9 +40,9 @@ def test_container_name_can_be_set_by_caller(self):
 
     def test_resources_being_passed(self):
         expected_resources = {
-                "requests": {
-                    "memory": "1Gi",
-                },
+            "requests": {
+                "memory": "1Gi",
+            },
         }
         job_config = make_job_config(load_job_spec())
         resource_config = job_config['spec']['template']['spec']['containers'][0]['resources']
@@ -55,6 +55,6 @@ def test_ttl_is_set_if_provided_in_config(self):
         job_config = make_job_config(job_spec)
         self.assertIn('ttlSecondsAfterFinished', job_config['spec'])
         self.assertEqual(
-                job_config['spec']['ttlSecondsAfterFinished'],
-                ttl_option['ttl_seconds_after_finished'],
+            job_config['spec']['ttlSecondsAfterFinished'],
+            ttl_option['ttl_seconds_after_finished'],
         )