diff --git a/.github/workflows/integrate.yaml b/.github/workflows/integrate.yaml index 72983b00..dabe6422 100644 --- a/.github/workflows/integrate.yaml +++ b/.github/workflows/integrate.yaml @@ -51,7 +51,8 @@ jobs: - 1.26-strict/stable integration-types: - integration - - integration-tls + - integration-tls-provider + - integration-tls-secret steps: - name: Check out repo uses: actions/checkout@v3 diff --git a/charms/istio-pilot/actions.yaml b/charms/istio-pilot/actions.yaml new file mode 100644 index 00000000..1a1617dc --- /dev/null +++ b/charms/istio-pilot/actions.yaml @@ -0,0 +1,26 @@ +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. + +set-tls: + description: | + Manually pass SSL cert and key values to configure the Ingress Gateway with TLS. + Configuring TLS with this action is mutually exclusive to using TLS certificate providers. + params: + ssl-key: + type: string + pattern: "^.*[a-zA-Z0-9]+.*$" + minLength: 1 + description: | + The SSL key output as a string. Can be set with + $ juju run set-tls istio-pilot/ ssl-key="$(cat KEY_FILE)" + ssl-crt: + type: string + minLength: 1 + pattern: "^.*[a-zA-Z0-9]+.*$" + description: | + The SSL cert output as a string. Can be set with + $ juju run set-tls istio-pilot/ ssl-crt="$(cat CERT_FILE)" + required: [ssl-key, ssl-crt] +unset-tls: + description: Remove SSL cert and key values from the Ingress Gateway TLS configuration. + additionalProperties: false diff --git a/charms/istio-pilot/src/charm.py b/charms/istio-pilot/src/charm.py index ab0d1f2b..df7751e6 100755 --- a/charms/istio-pilot/src/charm.py +++ b/charms/istio-pilot/src/charm.py @@ -3,7 +3,7 @@ import base64 import logging import subprocess -from typing import List, Optional +from typing import Dict, List, Optional import tenacity import yaml @@ -26,7 +26,14 @@ from lightkube.resources.core_v1 import Secret, Service from ops.charm import CharmBase, RelationBrokenEvent from ops.main import main -from ops.model import ActiveStatus, Application, BlockedStatus, MaintenanceStatus, WaitingStatus +from ops.model import ( + ActiveStatus, + Application, + BlockedStatus, + MaintenanceStatus, + SecretNotFoundError, + WaitingStatus, +) from packaging.version import Version from serialized_data_interface import ( NoCompatibleVersions, @@ -66,6 +73,12 @@ VIRTUAL_SERVICE_TEMPLATE_FILES = ["src/manifests/virtual_service.yaml.j2"] ISTIOCTL_PATH = "./istioctl" ISTIOCTL_DEPOYMENT_PROFILE = "minimal" +INSTALL_FAILED_MSG = ( + "Failed to install Istio Control Plane. " + "{message} Make sure the cluster has no Istio installations already present and " + "that you have provided the right configuration values." +) +TLS_SECRET_LABEL = "istio-tls-secret" UPGRADE_FAILED_MSG = ( "Failed to upgrade Istio. {message} To recover Istio, see [the upgrade docs]" "(https://github.com/canonical/istio-operators/blob/main/charms/istio-pilot/README.md) for " @@ -107,6 +120,19 @@ def __init__(self, *args): # available, revoked, invalidated, or if the certs relation is broken self.framework.observe(self._cert_handler.on.cert_changed, self.reconcile) + # ---- Start of the block + # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.21. + # ---- For details please refer to canonical/istio-operators#380. + # ---- FIXME: Remove this block after releasing 1.21. + # Save SSL information and reconcile + self.framework.observe(self.on.set_tls_action, self.set_tls) + self.framework.observe(self.on.secret_changed, self.reconcile) + + # Remove SSL information and reconcile + self.framework.observe(self.on.unset_tls_action, self.unset_tls) + self.framework.observe(self.on.secret_remove, self.reconcile) + # ---- End of the block + # Event handling for managing the Istio control plane self.framework.observe(self.on.install, self.install) self.framework.observe(self.on.remove, self.remove) @@ -212,6 +238,39 @@ def install(self, _): self.unit.status = ActiveStatus() + # ---- Start of the block + # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.21. + # ---- For details please refer to canonical/istio-operators#380. + # ---- FIXME: Remove this block after releasing 1.21. + def unset_tls(self, event) -> None: + """Remove the secret that saves TLS information and reconcile in case of changes.""" + try: + secret = self.model.get_secret(label=TLS_SECRET_LABEL) + secret.remove_all_revisions() + except SecretNotFoundError: + self.log.info("No secret was removed.") + self.reconcile(event) + + def set_tls(self, event) -> None: + """Save TLS information in a juju secret and reconcile in case of changes.""" + + # Because the action itself has some validation, we are guaranteed that + # BOTH of these values are passed as a string with minimum length 1 + ssl_key = event.params.get("ssl-key", None) + ssl_crt = event.params.get("ssl-crt", None) + + content = {"ssl-key": ssl_key, "ssl-crt": ssl_crt} + + # Save the secret with the content that was passed through the action + try: + secret = self.model.get_secret(label=TLS_SECRET_LABEL) + secret.set_content(content) + except SecretNotFoundError: + self.app.add_secret({"ssl-key": ssl_key, "ssl-crt": ssl_crt}, label=TLS_SECRET_LABEL) + self.reconcile(event) + + # ---- End of the block + def reconcile(self, event): """Reconcile the state of the charm. @@ -576,12 +635,9 @@ def _reconcile_gateway(self): # Secure the gateway, if certificates relation is enabled and # both the CA cert and key are provided if self._use_https(): - context["ssl_crt"] = base64.b64encode(self._cert_handler.cert.encode("ascii")).decode( - "utf-8" - ) - context["ssl_key"] = base64.b64encode(self._cert_handler.key.encode("ascii")).decode( - "utf-8" - ) + self._log_and_set_status(MaintenanceStatus("Setting TLS Ingress")) + context["ssl_crt"] = self._ssl_info["ssl-crt"] + context["ssl_key"] = self._ssl_info["ssl-key"] context["secure"] = True krh = KubernetesResourceHandler( @@ -768,7 +824,107 @@ def _istiod_svc(self): else: return exporter_ip - def _use_https(self): + @property + def _ssl_info(self) -> Dict[str, str]: + """Return a dictionary with SSL cert and key values. + + The dictionary is built based on available information, if + the istio-tls-secret is found, it is prioritised and returned; + otherwise, the information shared by a TLS certificate provider. + """ + + # FIXME: remove the try/catch block and just return the dictionary that contains + # data from the CertHandler after 1.21 + try: + ssl_secret = self.model.get_secret(label=TLS_SECRET_LABEL) + return { + "ssl-crt": base64.b64encode( + ssl_secret.get_content(refresh=True)["ssl-crt"].encode("ascii") + ).decode("utf-8"), + "ssl-key": base64.b64encode( + ssl_secret.get_content(refresh=True)["ssl-key"].encode("ascii") + ).decode("utf-8"), + } + except SecretNotFoundError: + return { + "ssl-crt": base64.b64encode(self._cert_handler.cert.encode("ascii")).decode( + "utf-8" + ), + "ssl-key": base64.b64encode(self._cert_handler.key.encode("ascii")).decode( + "utf-8" + ), + } + + # ---- Start of the block + # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.21. + # ---- For details please refer to canonical/istio-operators#380. + # ---- FIXME: Remove this block after releasing 1.21. + def _use_https(self) -> bool: + """Return True if only one of the TLS configurations are enabled, False if none are. + + Raises: + ErrorWithStatus: if both configurations are enabled. + """ + if self._use_https_with_tls_provider() and self._use_https_with_tls_secret(): + self.log.error( + "Only one TLS configuration is supported at a time." + "Either remove the TLS certificate provider relation," + "or the TLS manual configuration with the remove-tls-secret action." + ) + raise ErrorWithStatus( + "Only one TLS configuration is supported at a time. See logs for details.", + BlockedStatus, + ) + return self._use_https_with_tls_provider() or self._use_https_with_tls_secret() + + def _use_https_with_tls_secret(self) -> bool: + """Return True if both SSL key and crt are set with save-tls-secret, False otherwise. + + Raises: + ErrorWithStatus: if one of the values is missing. + """ + + # Ensure the secret that holds the values exist otherwise fail as this + # is an error in our code + try: + secret = self.model.get_secret(label=TLS_SECRET_LABEL) + except SecretNotFoundError: + return False + + ssl_key = secret.get_content(refresh=True)["ssl-key"] + ssl_crt = secret.get_content(refresh=True)["ssl-crt"] + + # This block of code is more a validation than a behaviour of the charm + # Ideally this will never be executed, but we need a mechanism to know that + # these values were correctly saved in the secret + if _xor(ssl_key, ssl_crt): + missing = "ssl-key" + if not secret.get_content(refresh=True)["ssl-crt"]: + missing = "ssl-crt" + self.log.error(f"Missing {missing}, this is most likely an error with the charm.") + raise GenericCharmRuntimeError(f"Missing {missing}, cannot configure TLS.") + elif not ssl_key and not ssl_crt: + self.log.error( + "Missing both SSL key and cert values, this is most likely an error with the charm." + ) + raise GenericCharmRuntimeError("Missing SSL values, cannot configure TLS.") + + # If both the SSL key and cert are provided, we configure TLS + if ssl_key and ssl_crt: + return True + + # ---- End of the block + + # FIXME: Replace the below line with the one commented out after releasing 1.21 + # def _use_https(self) -> bool: + def _use_https_with_tls_provider(self) -> bool: + """Return True if SSL key and cert are provided by a TLS cert provider, False otherwise. + + Raises: + ErrorWithStatus: if one of the values is missing. + """ + + # Immediately return False if the CertHandler is not enabled if not self._cert_handler.enabled: return False diff --git a/charms/istio-pilot/tests/unit/test_charm.py b/charms/istio-pilot/tests/unit/test_charm.py index ab796bc2..ca709ae8 100644 --- a/charms/istio-pilot/tests/unit/test_charm.py +++ b/charms/istio-pilot/tests/unit/test_charm.py @@ -15,16 +15,24 @@ from lightkube.models.meta_v1 import ObjectMeta from lightkube.resources.core_v1 import Secret from ops.charm import ( + ActionEvent, RelationBrokenEvent, RelationChangedEvent, RelationCreatedEvent, RelationJoinedEvent, ) -from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus +from ops.model import ( + ActiveStatus, + BlockedStatus, + MaintenanceStatus, + SecretNotFoundError, + WaitingStatus, +) from ops.testing import Harness from charm import ( GATEWAY_PORTS, + TLS_SECRET_LABEL, Operator, _get_gateway_address_from_svc, _remove_envoyfilter, @@ -1195,7 +1203,7 @@ def test_send_gateway_info( (True, "", "y", None, pytest.raises(ErrorWithStatus)), ], ) - def test_use_https( + def test_use_https_with_tls_provider( self, cert_handler_enabled, ssl_cert, @@ -1205,10 +1213,7 @@ def test_use_https( harness, mocked_cert_subject, ): - """Tests that the gateway_port selection works as expected. - - Implicitly tests _use_https() as well. - """ + """Test the method returns the correct boolean when the CertHandler is defined.""" harness.begin() harness.charm._cert_handler = MagicMock() harness.charm._cert_handler.enabled = cert_handler_enabled @@ -1216,7 +1221,7 @@ def test_use_https( harness.charm._cert_handler.key = ssl_key with expected_context: - assert harness.charm._use_https() == expected_return + assert harness.charm._use_https_with_tls_provider() == expected_return @pytest.mark.parametrize( "left, right, expected", @@ -1397,6 +1402,157 @@ def test_wait_for_update_rollout_timeout(self, mocked_istioctl): assert mocked_istioctl.version.call_count == 5 + # ---- Start of block + # ---- Test cases added for testing the TLS secret feature, remove after 1.21 + @pytest.mark.parametrize( + "https_with_tls_provider, https_with_tls_secret, expected_return, expected_context", + [ + (True, False, True, does_not_raise()), + (False, True, True, does_not_raise()), + (False, False, False, does_not_raise()), + (True, True, None, pytest.raises(ErrorWithStatus)), + ], + ) + def test_use_https( + self, + https_with_tls_provider, + https_with_tls_secret, + expected_return, + expected_context, + harness, + mocked_cert_subject, + ): + """Test the method returns a correct bool when comparing two TLS options. + + Parameters: + https_with_tls_provider(bool): the return of the method that checks + the TLS certificates provider is related and configured to this charm + https_with_tls_secret(bool): the return of the method that checks the TLS + configuration is done from secret data + expected_return(bool, None): the expected return of the methods above + expected_context: the exception(if any) that is expected when calling + the above methods. + + Example: + If https_with_tls_provider returns False and https_with_tls_secret returns True, + the expected return of _use_https() is True. + If both https_with_tls_provider and https_with_tls_secret return True, _use_https() + must raise ErrorWithStatus. + """ + harness.begin() + + harness.charm._use_https_with_tls_secret = MagicMock() + harness.charm._use_https_with_tls_secret.return_value = https_with_tls_secret + harness.charm._use_https_with_tls_provider = MagicMock() + harness.charm._use_https_with_tls_provider.return_value = https_with_tls_provider + + with expected_context: + assert harness.charm._use_https() == expected_return + + @pytest.mark.skip("Skipping due to ValueError: Secret owner cannot use refresh=True") + @pytest.mark.parametrize( + "ssl_cert, ssl_key, expected_return, expected_context", + [ + ("", "", None, pytest.raises(GenericCharmRuntimeError)), + ("x", "y", True, does_not_raise()), + ("x", "", None, pytest.raises(GenericCharmRuntimeError)), + ("", "y", None, pytest.raises(GenericCharmRuntimeError)), + ], + ) + def test_use_https_with_tls_secret_found( + self, + ssl_cert, + ssl_key, + expected_return, + expected_context, + harness, + mocked_cert_subject, + ): + """Test the method returns a correct bool when the secret is added. + + Parameters: + ssl_cert(str): SSL cert value (comes from a secret) + ssl_key(str): SSL key value (comes from a secret) + expected_return(bool): the expected return of the use_https_with_tls_secret method + expected_context: if the method should raise or not + Example: + If ssl_cert and ssl_key both have values, the use_https_with_tls_secret() method + must return True; if both are empty the return must be False; and if just one + value is set, the method should raise ErrorWithStatus. + """ + harness.begin() + harness.charm.app.add_secret( + content={"ssl-key": ssl_key, "ssl-crt": ssl_cert}, label=TLS_SECRET_LABEL + ) + + with expected_context: + assert harness.charm._use_https_with_tls_secret() == expected_return + + def test_use_https_with_tls_secret_not_found(self, harness, mocked_cert_subject): + """Test the method returns False when the secret is not added.""" + harness.begin() + assert harness.charm._use_https_with_tls_secret() is False + + def test_set_tls_set_secret_content( + self, + harness, + mocked_cert_subject, + all_operator_reconcile_handlers_mocked, + ): + """Test the method sets secret content when secret exists.""" + harness.begin() + harness.add_relation("peers", harness.charm.app.name) + harness.set_leader(True) + + mocked_action_event = MagicMock(spec=ActionEvent) + mocked_action_event.params = {"ssl-key": "new-key", "ssl-crt": "new-crt"} + + # Add secret first so the charm code detects it and just updates the value + harness.charm.app.add_secret( + content={"ssl-key": "current-key", "ssl-crt": "current-crt"}, label=TLS_SECRET_LABEL + ) + harness.charm.set_tls(mocked_action_event) + assert ( + harness.model.get_secret(label=TLS_SECRET_LABEL).get_content() + == mocked_action_event.params + ) + + def test_set_tls_add_secret( + self, harness, mocked_cert_subject, all_operator_reconcile_handlers_mocked + ): + """Test the method adds a secret when it does not exist.""" + harness.begin() + harness.add_relation("peers", harness.charm.app.name) + harness.set_leader(True) + + mocked_action_event = MagicMock(spec=ActionEvent) + mocked_action_event.params = {"ssl-key": "new-key", "ssl-crt": "new-crt"} + + harness.charm.set_tls(mocked_action_event) + assert ( + harness.model.get_secret(label=TLS_SECRET_LABEL).get_content() + == mocked_action_event.params + ) + + def test_unset_tls(self, harness, all_operator_reconcile_handlers_mocked, mocked_cert_subject): + """Test the secret gets removed.""" + harness.begin() + harness.add_relation("peers", harness.charm.app.name) + harness.set_leader(True) + + mocked_action_event = MagicMock(spec=ActionEvent) + + # Add secret first so the charm code detects it and is able to delete it + harness.charm.app.add_secret( + content={"ssl-key": "key", "ssl-crt": "crt"}, label=TLS_SECRET_LABEL + ) + harness.charm.unset_tls(mocked_action_event) + + with pytest.raises(SecretNotFoundError): + harness.model.get_secret(label=TLS_SECRET_LABEL) + + # ---- End of block + # Fixtures @pytest.fixture diff --git a/tests/test_bundle_tls.py b/tests/test_bundle_tls_provider.py similarity index 98% rename from tests/test_bundle_tls.py rename to tests/test_bundle_tls_provider.py index 1ffbb437..1bb0254b 100644 --- a/tests/test_bundle_tls.py +++ b/tests/test_bundle_tls_provider.py @@ -18,7 +18,7 @@ @pytest.fixture(scope="session") def lightkube_client() -> lightkube.Client: - client = lightkube.Client(field_manager="kserve") + client = lightkube.Client() return client diff --git a/tests/test_bundle_tls_secret.py b/tests/test_bundle_tls_secret.py new file mode 100644 index 00000000..a401f87e --- /dev/null +++ b/tests/test_bundle_tls_secret.py @@ -0,0 +1,97 @@ +import lightkube +import pytest +import tenacity +from lightkube.generic_resource import create_namespaced_resource +from lightkube.resources.core_v1 import Secret +from pytest_operator.plugin import OpsTest + +ISTIO_PILOT = "istio-pilot" +ISTIO_GATEWAY_APP_NAME = "istio-ingressgateway" +DEFAULT_GATEWAY_NAME = "test-gateway" +GATEWAY_RESOURCE = create_namespaced_resource( + group="networking.istio.io", + version="v1alpha3", + kind="Gateway", + plural="gateways", +) + + +@pytest.fixture(scope="session") +def lightkube_client() -> lightkube.Client: + client = lightkube.Client() + return client + + +@pytest.mark.abort_on_fail +async def test_build_and_deploy_istio_charms(ops_test: OpsTest): + """Build and deploy istio-operators with TLS configuration.""" + charms_path = "./charms/istio" + istio_charms = await ops_test.build_charms(f"{charms_path}-gateway", f"{charms_path}-pilot") + + await ops_test.model.deploy( + istio_charms["istio-pilot"], + application_name=ISTIO_PILOT, + config={"default-gateway": DEFAULT_GATEWAY_NAME}, + trust=True, + ) + + await ops_test.model.deploy( + istio_charms["istio-gateway"], + application_name=ISTIO_GATEWAY_APP_NAME, + config={"kind": "ingress"}, + trust=True, + ) + + await ops_test.model.add_relation( + f"{ISTIO_PILOT}:istio-pilot", f"{ISTIO_GATEWAY_APP_NAME}:istio-pilot" + ) + + await ops_test.model.wait_for_idle( + status="active", + raise_on_blocked=False, + timeout=90 * 10, + ) + + await run_save_tls_secret_action(ops_test) + + +@tenacity.retry( + stop=tenacity.stop_after_delay(50), + wait=tenacity.wait_exponential(multiplier=1, min=1, max=3), + reraise=True, +) +@pytest.mark.abort_on_fail +def test_tls_configuration(lightkube_client, ops_test: OpsTest): + """Check the Gateway and Secret are configured with TLS.""" + secret = lightkube_client.get( + Secret, f"{DEFAULT_GATEWAY_NAME}-gateway-secret", namespace=ops_test.model_name + ) + gateway = lightkube_client.get( + GATEWAY_RESOURCE, DEFAULT_GATEWAY_NAME, namespace=ops_test.model_name + ) + + # Assert the Secret is not None and has correct values + assert secret is not None + assert secret.data["tls.crt"] is not None + assert secret.data["tls.key"] is not None + assert secret.type == "kubernetes.io/tls" + + # Assert the Gateway is correctly configured + servers_dict = gateway.spec["servers"][0] + servers_dict_port = servers_dict["port"] + servers_dict_tls = servers_dict["tls"] + + assert servers_dict_port["name"] == "https" + assert servers_dict_port["protocol"] == "HTTPS" + + assert servers_dict_tls["mode"] == "SIMPLE" + assert servers_dict_tls["credentialName"] == secret.metadata.name + + +async def run_save_tls_secret_action(ops_test: OpsTest): + """Run the save-tls-secret action.""" + istio_pilot_unit = ops_test.model.applications[ISTIO_PILOT].units[0] + istio_pilot_unit_action = await istio_pilot_unit.run_action( + action_name="set-tls", **{"ssl-key": "key", "ssl-crt": "crt"} + ) + await ops_test.model.get_action_output(action_uuid=istio_pilot_unit_action.entity_id, wait=120) diff --git a/tox.ini b/tox.ini index e5b43982..0b31eecd 100644 --- a/tox.ini +++ b/tox.ini @@ -3,7 +3,7 @@ max-line-length = 100 [tox] skipsdist = True -envlist = {pilot,gateway}-{unit,lint},integration, integration-tls +envlist = {pilot,gateway}-{unit,lint},integration, integration-tls-provider, integration-tls-secret [vars] all_path = {[vars]src_path} {[vars]tst_path} @@ -69,10 +69,17 @@ commands = deps = -r requirements-integration.txt -[testenv:integration-tls] +[testenv:integration-tls-provider] allowlist_externals = rm commands = - pytest --show-capture=no --log-cli-level=INFO -vvs --tb=native {posargs} tests/test_bundle_tls.py + pytest --show-capture=no --log-cli-level=INFO -vvs --tb=native {posargs} tests/test_bundle_tls_provider.py +deps = + -r requirements-integration.txt + +[testenv:integration-tls-secret] +allowlist_externals = rm +commands = + pytest --show-capture=no --log-cli-level=INFO -vvs --tb=native {posargs} tests/test_bundle_tls_secret.py deps = -r requirements-integration.txt