add service minio-service to kfp api to patch upstream kfp bug (#387)

This adds to kfp-api a service called `minio-service` which points to the related object-storage's s3 service.  This has been added to address a bug in upstream kfp, as explained [here](canonical/minio-operator#151).  This service was originally added to the minio charm in [minio pr 151](canonical/minio-operator#151), but has been refactored so it is added here instead as described in [minio issue 153](canonical/minio-operator#153).

.github/workflows/integrate.yaml

@@ -99,8 +99,12 @@ jobs:
           juju-channel: 3.1/stable
           charmcraft-channel: latest/candidate
 
-      - run: |
-          sg snap_microk8s -c "tox -e ${{ matrix.charm }}-integration"
+      - name: Integration tests
+        run: |
+          # Requires the model to be called kubeflow due to
+          # https://github.com/canonical/kfp-operators/issues/389
+          juju add-model kubeflow
+          sg snap_microk8s -c "tox -e ${{ matrix.charm }}-integration -- --model kubeflow"
 
       - name: Collect charm debug artifacts
         uses: canonical/kubeflow-ci/actions/dump-charm-debug-artifacts@main
@@ -110,6 +114,7 @@ jobs:
     name: Test the bundle
     runs-on: ubuntu-20.04
     strategy:
+      fail-fast: false
       matrix:
         sdk:
           - v1

charms/kfp-api/src/charm.py

@@ -45,6 +45,7 @@
 K8S_RESOURCE_FILES = [
     "src/templates/auth_manifests.yaml.j2",
     "src/templates/ml-pipeline-service.yaml.j2",
+    "src/templates/minio-service.yaml.j2",
 ]
 MYSQL_WARNING = "Relation mysql is deprecated."
 UNBLOCK_MESSAGE = "Remove deprecated mysql relation to unblock."
@@ -77,14 +78,6 @@ def __init__(self, *args):
         self._database_name = "mlpipeline"
         self._container = self.unit.get_container(self._container_name)
 
-        # setup context to be used for updating K8S resources
-        self._context = {
-            "app_name": self._name,
-            "namespace": self._namespace,
-            "service": self._name,
-            "grpc_port": self._grcp_port,
-            "http_port": self._http_port,
-        }
         self._k8s_resource_handler = None
 
         grpc_port = ServicePort(int(self._grcp_port), name="grpc-port")
@@ -160,6 +153,26 @@ def container(self):
         """Return container."""
         return self._container
 
+    @property
+    def _context(self):
+        """Return the context used for generating kubernetes resources."""
+        interfaces = self._get_interfaces()
+        object_storage = self._get_object_storage(interfaces)
+
+        minio_url = f"{object_storage['service']}.{object_storage['namespace']}.svc.cluster.local"
+
+        context = {
+            "app_name": self._name,
+            "namespace": self._namespace,
+            "service": self._name,
+            "grpc_port": self._grcp_port,
+            "http_port": self._http_port,
+            # Must include .svc.cluster.local for DNS resolution
+            "minio_url": minio_url,
+            "minio_port": str(object_storage["port"]),
+        }
+        return context
+
     @property
     def k8s_resource_handler(self):
         """Update K8S with K8S resources."""
@@ -281,6 +294,16 @@ def _generate_environment(self) -> dict:
 
         return env_vars
 
+    def _check_model_name(self):
+        if self.model.name != "kubeflow":
+            # Remove when this bug is resolved:
+            # https://github.com/canonical/kfp-operators/issues/389
+            raise ErrorWithStatus(
+                "kfp-api must be deployed to model named `kubeflow` due to"
+                " https://github.com/canonical/kfp-operators/issues/389",
+                BlockedStatus,
+            )
+
     def _check_status(self):
         """Check status of workload and set status accordingly."""
         self._check_leader()
@@ -556,10 +579,15 @@ def _apply_k8s_resources(self, force_conflicts: bool = False) -> None:
                 raise GenericCharmRuntimeError("K8S resources creation failed") from error
         self.model.unit.status = MaintenanceStatus("K8S resources created")
 
-    def _on_install(self, _):
+    def _on_install(self, event):
         """Installation only tasks."""
-        # deploy K8S resources to speed up deployment
-        self._apply_k8s_resources()
+        try:
+            # deploy K8S resources early to speed up deployment
+            self._apply_k8s_resources()
+        except ErrorWithStatus as err:
+            self.model.unit.status = err.status
+            self.logger.error(f"Failed to handle {event} with error: {err}")
+            return
 
     def _on_upgrade(self, _):
         """Perform upgrade steps."""
@@ -663,6 +691,7 @@ def _on_relational_db_relation_remove(self, event):
     def _on_event(self, event, force_conflicts: bool = False) -> None:
         # Set up all relations/fetch required data
         try:
+            self._check_model_name()
             self._check_leader()
             self._apply_k8s_resources(force_conflicts=force_conflicts)
             update_layer(self._container_name, self._container, self._kfp_api_layer, self.logger)

charms/kfp-api/src/templates/minio-service.yaml.j2

@@ -0,0 +1,13 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: minio-service
+  namespace: '{{ namespace }}'
+spec:
+  type: ExternalName
+  externalName: {{ minio_url }}
+  ports:
+    - name: minio
+      protocol: TCP
+      port: 9000
+      targetPort: {{ minio_port }}

charms/kfp-api/tests/unit/test_operator.py

@@ -6,6 +6,7 @@
 
 import pytest
 import yaml
+from charmed_kubeflow_chisme.kubernetes import KubernetesResourceHandler
 from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus
 from ops.testing import Harness
 
@@ -16,18 +17,16 @@
 
 @pytest.fixture()
 def mocked_resource_handler(mocker):
-    """Yields a mocked resource handler."""
+    """Yields a mocked resource handler with a mocked Lightkube Client."""
     mocked_resource_handler = MagicMock()
     mocked_resource_handler_factory = mocker.patch("charm.KubernetesResourceHandler")
-    mocked_resource_handler_factory.return_value = mocked_resource_handler
-    yield mocked_resource_handler
 
+    def return_krh_with_mocked_lightkube(*args, **kwargs):
+        kwargs["lightkube_client"] = MagicMock()
+        return KubernetesResourceHandler(*args, **kwargs)
 
-@pytest.fixture()
-def mocked_lightkube_client(mocker, mocked_resource_handler):
-    """Prevents lightkube clients from being created, returning a mock instead."""
-    mocked_resource_handler.lightkube_client = MagicMock()
-    yield mocked_resource_handler.lightkube_client
+    mocked_resource_handler_factory.side_effect = return_krh_with_mocked_lightkube
+    yield mocked_resource_handler
 
 
 @pytest.fixture()
@@ -45,6 +44,9 @@ def harness() -> Harness:
     # setup container networking simulation
     harness.set_can_connect(KFP_API_CONTAINER_NAME, True)
 
+    # Set required model name
+    harness.set_model_name("kubeflow")
+
     return harness
 
 
@@ -58,6 +60,15 @@ def test_not_leader(self, k8s_resource_handler: MagicMock, harness: Harness):
         harness.container_pebble_ready(KFP_API_CONTAINER_NAME)
         assert harness.charm.model.unit.status == WaitingStatus("Waiting for leadership")
 
+    @patch("charm.KubernetesServicePatch", lambda x, y: None)
+    @patch("charm.KfpApiOperator.k8s_resource_handler")
+    def test_check_model_name_failure(self, k8s_resource_handler: MagicMock, harness: Harness):
+        """Tests that the charm blocks if model name is not 'kubeflow'."""
+        harness.set_model_name("not-kubeflow")
+        harness.begin_with_initial_hooks()
+        assert isinstance(harness.charm.model.unit.status, BlockedStatus)
+        assert harness.charm.model.unit.status.message.startswith("kfp-api must be deployed to")
+
     @pytest.mark.parametrize(
         "relation_data,expected_returned_data,expected_raises,expected_status",
         (
@@ -108,7 +119,6 @@ def test_mysql_relation(
         expected_returned_data,
         expected_raises,
         expected_status,
-        mocked_lightkube_client,
         harness: Harness,
     ):
         harness.set_leader(True)
@@ -133,7 +143,7 @@ def test_mysql_relation(
             harness.charm._get_db_data()
 
     @patch("charm.KubernetesServicePatch", lambda x, y: None)
-    def test_mysql_relation_too_many_relations(self, mocked_lightkube_client, harness: Harness):
+    def test_mysql_relation_too_many_relations(self, harness: Harness):
         harness.set_leader(True)
         harness.begin()
         harness.container_pebble_ready(KFP_API_CONTAINER_NAME)
@@ -151,7 +161,7 @@ def test_mysql_relation_too_many_relations(self, mocked_lightkube_client, harnes
         )
 
     @patch("charm.KubernetesServicePatch", lambda x, y: None)
-    def test_kfp_viz_relation_missing(self, mocked_lightkube_client, harness: Harness):
+    def test_kfp_viz_relation_missing(self, harness: Harness):
         harness.set_leader(True)
         harness.begin()
         harness.container_pebble_ready(KFP_API_CONTAINER_NAME)
@@ -310,8 +320,8 @@ def test_relations_that_provide_data(
         expected_returned_data,
         expected_raises,
         expected_status,
-        mocked_lightkube_client,
         harness: Harness,
+        mocked_resource_handler,
     ):
         harness.set_leader(True)
         harness.begin()
@@ -343,7 +353,7 @@ def test_install_with_all_inputs_and_pebble(
         """Test complete installation with all required relations and verify pebble layer."""
         harness.set_leader(True)
         kfpapi_relation_name = "kfp-api"
-        model_name = "test_model"
+        model_name = "kubeflow"
         service_port = "8888"
         harness.set_model_name(model_name)
         harness.update_config({"http-port": service_port})
@@ -477,7 +487,7 @@ def test_install_with_all_inputs_and_pebble(
         test_env = pebble_plan_info["services"][KFP_API_SERVICE_NAME]["environment"]
 
         assert test_env == expected_env
-        assert "test_model" == test_env["POD_NAMESPACE"]
+        assert model_name == test_env["POD_NAMESPACE"]
 
     @patch("charm.KubernetesServicePatch", lambda x, y: None)
     @patch("charm.KfpApiOperator._apply_k8s_resources")
@@ -513,7 +523,6 @@ def _get_relation_db_only_side_effect_func(self, relation):
     def test_relational_db_relation_no_data(
         self,
         mocked_resource_handler,
-        mocked_lightkube_client,
         mocked_kubernetes_service_patcher,
         harness: Harness,
     ):
@@ -536,7 +545,6 @@ def test_relational_db_relation_no_data(
     def test_relational_db_relation_missing_attributes(
         self,
         mocked_resource_handler,
-        mocked_lightkube_client,
         mocked_kubernetes_service_patcher,
         harness: Harness,
     ):
@@ -559,7 +567,6 @@ def test_relational_db_relation_missing_attributes(
     def test_relational_db_relation_bad_data(
         self,
         mocked_resource_handler,
-        mocked_lightkube_client,
         mocked_kubernetes_service_patcher,
         harness: Harness,
     ):
@@ -582,7 +589,6 @@ def test_relational_db_relation_bad_data(
     def test_relational_db_relation_with_data(
         self,
         mocked_resource_handler,
-        mocked_lightkube_client,
         mocked_kubernetes_service_patcher,
         harness: Harness,
     ):
@@ -615,11 +621,11 @@ def test_relational_db_relation_with_data(
     def test_relational_db_relation_broken(
         self,
         mocked_resource_handler,
-        mocked_lightkube_client,
         mocked_kubernetes_service_patcher,
         harness: Harness,
     ):
         """Test that a relation broken event is properly handled."""
+        # Arrange
         database = MagicMock()
         fetch_relation_data = MagicMock(side_effect=KeyError())
         database.fetch_relation_data = fetch_relation_data
@@ -631,7 +637,22 @@ def test_relational_db_relation_broken(
         rel_id = harness.add_relation(rel_name, "relational-db-provider")
 
         harness.begin()
+
+        # Mock the object storage data access to keep it from blocking the charm
+        # Cannot mock by adding a relation to the harness because harness.model.get_relation is
+        # mocked above to be specific to the db relation.  This test's mocks could use a refactor.
+        objectstorage_data = {
+            "access-key": "access-key",
+            "namespace": "namespace",
+            "port": 1234,
+            "secret-key": "secret-key",
+            "secure": True,
+            "service": "service",
+        }
+        harness.charm._get_object_storage = MagicMock(return_value=objectstorage_data)
         harness.set_leader(True)
+
+        # Act and Assert
         harness.container_pebble_ready(KFP_API_CONTAINER_NAME)
 
         assert harness.model.unit.status == WaitingStatus("Waiting for relational-db data")
@@ -647,3 +668,63 @@ def test_relational_db_relation_broken(
 
         harness.charm.on.remove.emit()
         assert harness.model.unit.status == MaintenanceStatus("K8S resources removed")
+
+    def test_minio_service_rendered_as_expected(
+        self,
+        mocker,
+        mocked_kubernetes_service_patcher,
+        harness: Harness,
+    ):
+        # Arrange
+
+        # object storage relation
+        objectstorage_data = {
+            "access-key": "access-key",
+            "namespace": "namespace",
+            "port": 1234,
+            "secret-key": "secret-key",
+            "secure": True,
+            "service": "service",
+        }
+        objectstorage_data_dict = {
+            "_supported_versions": "- v1",
+            "data": yaml.dump(objectstorage_data),
+        }
+        objectstorage_rel_id = harness.add_relation("object-storage", "storage-provider")
+        harness.add_relation_unit(objectstorage_rel_id, "storage-provider/0")
+        harness.update_relation_data(
+            objectstorage_rel_id, "storage-provider", objectstorage_data_dict
+        )
+
+        harness.set_leader(True)
+        model_name = "kubeflow"
+        harness.set_model_name(model_name)
+        harness.begin()
+
+        # Mock the KubernetesResourceHandler to always have a mocked Lightkube Client
+        mocked_resource_handler_factory = mocker.patch("charm.KubernetesResourceHandler")
+
+        def return_krh_with_mocked_lightkube(*args, **kwargs):
+            kwargs["lightkube_client"] = MagicMock()
+            return KubernetesResourceHandler(*args, **kwargs)
+
+        mocked_resource_handler_factory.side_effect = return_krh_with_mocked_lightkube
+
+        # Act
+        krh = harness.charm.k8s_resource_handler
+        manifests = krh.render_manifests()
+
+        # Assert that manifests include a Service(name='minio-service'), and that it has the
+        # expected configuration data from object-storage
+        minio_service = next(
+            (m for m in manifests if m.kind == "Service" and m.metadata.name == "minio-service"),
+            None,
+        )
+        assert minio_service.metadata.namespace == harness.charm.model.name
+        assert (
+            minio_service.spec.externalName
+            == f"{objectstorage_data['service']}.{objectstorage_data['namespace']}"
+            f".svc.cluster.local"
+        )
+        assert len(minio_service.spec.ports) == 1
+        assert minio_service.spec.ports[0].targetPort == objectstorage_data["port"]