From 92de04cd803afc3e344c2fd0dbcf4782e3712271 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 28 Mar 2024 10:41:29 +1300 Subject: [PATCH 1/7] Fix the config consistency checker type checking. --- scenario/consistency_checker.py | 27 ++++++++++++- tests/test_consistency_checker.py | 63 +++++++++++++++++++++++++++++++ tests/test_e2e/test_config.py | 2 +- 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index ea9ad489..aeff7c94 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -2,6 +2,7 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. import os +import re from collections import Counter from collections.abc import Sequence from numbers import Number @@ -326,10 +327,20 @@ def check_storages_consistency( return Results(errors, []) +def _is_secret_identifier(value): + """Return true iff the value is in the form `secret:{secret id}`.""" + if not value.startswith("secret:"): + return False + secret_id = value.split(":", 1)[1] + # cf. https://github.com/juju/juju/blob/13eb9df3df16a84fd471af8a3c95ddbd04389b71/core/secrets/secret.go#L48 + return re.match(r"^[0-9a-z]{20}$", secret_id) + + def check_config_consistency( *, state: "State", charm_spec: "_CharmSpec", + juju_version: Tuple[int, ...], **_kwargs, # noqa: U101 ) -> Results: """Check the consistency of the state.config with the charm_spec.config (config.yaml).""" @@ -348,11 +359,16 @@ def check_config_consistency( converters = { "string": str, "int": int, - "integer": int, # fixme: which one is it? - "number": float, + "float": float, "boolean": bool, # "attrs": NotImplemented, # fixme: wot? } + if juju_version >= (3, 4): + converters["secret"] = str + + validators = { + "secret": _is_secret_identifier, + } expected_type_name = meta_config[key].get("type", None) if not expected_type_name: @@ -371,6 +387,13 @@ def check_config_consistency( f"but is of type {type(value)}.", ) + elif expected_type_name in validators and not validators[expected_type_name]( + value, + ): + errors.append( + f"config invalid: option {key!r} value {value!r} is not valid.", + ) + return Results(errors, []) diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index ef92e6d9..1a79f32d 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -156,6 +156,69 @@ def test_bad_config_option_type(): ) +@pytest.mark.parametrize( + "config_type", + ( + ("string", "foo", 1), + ("int", 1, "1"), + ("float", 1.0, 1), + ("boolean", False, "foo"), + ), +) +def test_config_types(config_type): + type_name, valid_value, invalid_value = config_type + assert_consistent( + State(config={"foo": valid_value}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": type_name}}}), + ) + assert_inconsistent( + State(config={"foo": invalid_value}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": type_name}}}), + ) + + +@pytest.mark.parametrize("juju_version", ("3.4", "3.5", "4.0")) +def test_config_secret(juju_version): + assert_consistent( + State(config={"foo": "secret:co28kefmp25c77utl3n0"}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "secret"}}}), + juju_version=juju_version, + ) + assert_inconsistent( + State(config={"foo": 1}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "secret"}}}), + ) + assert_inconsistent( + State(config={"foo": "co28kefmp25c77utl3n0"}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "secret"}}}), + ) + assert_inconsistent( + State(config={"foo": "secret:secret"}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "secret"}}}), + ) + assert_inconsistent( + State(config={"foo": "secret:co28kefmp25c77utl3n!"}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "secret"}}}), + ) + + +@pytest.mark.parametrize("juju_version", ("2.9", "3.3")) +def test_config_secret_old_juju(juju_version): + assert_inconsistent( + State(config={"foo": "secret:co28kefmp25c77utl3n0"}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "secret"}}}), + juju_version=juju_version, + ) + + @pytest.mark.parametrize("bad_v", ("1.0", "0", "1.2", "2.35.42", "2.99.99", "2.99")) def test_secrets_jujuv_bad(bad_v): secret = Secret("secret:foo", {0: {"a": "b"}}) diff --git a/tests/test_e2e/test_config.py b/tests/test_e2e/test_config.py index 55c5b70d..27b25c29 100644 --- a/tests/test_e2e/test_config.py +++ b/tests/test_e2e/test_config.py @@ -32,7 +32,7 @@ def check_cfg(charm: CharmBase): "update_status", mycharm, meta={"name": "foo"}, - config={"options": {"foo": {"type": "string"}, "baz": {"type": "integer"}}}, + config={"options": {"foo": {"type": "string"}, "baz": {"type": "int"}}}, post_event=check_cfg, ) From bb93896498d4494d28e48aeee05c1bf263fb9623 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 28 Mar 2024 11:42:37 +1300 Subject: [PATCH 2/7] Update scenario/consistency_checker.py --- scenario/consistency_checker.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index aeff7c94..95ff775f 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -329,11 +329,8 @@ def check_storages_consistency( def _is_secret_identifier(value): """Return true iff the value is in the form `secret:{secret id}`.""" - if not value.startswith("secret:"): - return False - secret_id = value.split(":", 1)[1] # cf. https://github.com/juju/juju/blob/13eb9df3df16a84fd471af8a3c95ddbd04389b71/core/secrets/secret.go#L48 - return re.match(r"^[0-9a-z]{20}$", secret_id) + return re.match(r"secret:[0-9a-z]{20}$", secret_id) def check_config_consistency( From a7707264d537c2a93808f215f4d2a26d8ebbe44a Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 28 Mar 2024 12:49:48 +1300 Subject: [PATCH 3/7] Avoid unpleasant wrapping, as per review. --- scenario/consistency_checker.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index 95ff775f..d4a87bfb 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -384,9 +384,7 @@ def check_config_consistency( f"but is of type {type(value)}.", ) - elif expected_type_name in validators and not validators[expected_type_name]( - value, - ): + elif not validators.get(expected_type_name, lambda _: True)(value): errors.append( f"config invalid: option {key!r} value {value!r} is not valid.", ) From 4b7a783a54cbebcfcef5232c97d13818108bbbfa Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 28 Mar 2024 12:51:07 +1300 Subject: [PATCH 4/7] Update scenario/consistency_checker.py --- scenario/consistency_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index d4a87bfb..a075f598 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -327,7 +327,7 @@ def check_storages_consistency( return Results(errors, []) -def _is_secret_identifier(value): +def _is_secret_identifier(value: str): """Return true iff the value is in the form `secret:{secret id}`.""" # cf. https://github.com/juju/juju/blob/13eb9df3df16a84fd471af8a3c95ddbd04389b71/core/secrets/secret.go#L48 return re.match(r"secret:[0-9a-z]{20}$", secret_id) From 259a97428b3c8d5dedcb3d22a0ac3241e3ffdfd1 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 28 Mar 2024 12:54:13 +1300 Subject: [PATCH 5/7] Add type hints. --- scenario/consistency_checker.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index a075f598..8dfbb853 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -6,7 +6,7 @@ from collections import Counter from collections.abc import Sequence from numbers import Number -from typing import TYPE_CHECKING, Iterable, List, NamedTuple, Tuple +from typing import TYPE_CHECKING, Iterable, List, NamedTuple, Tuple, Union from scenario.runtime import InconsistentScenarioError from scenario.runtime import logger as scenario_logger @@ -327,10 +327,10 @@ def check_storages_consistency( return Results(errors, []) -def _is_secret_identifier(value: str): +def _is_secret_identifier(value: Union[str, int, float, bool]): """Return true iff the value is in the form `secret:{secret id}`.""" # cf. https://github.com/juju/juju/blob/13eb9df3df16a84fd471af8a3c95ddbd04389b71/core/secrets/secret.go#L48 - return re.match(r"secret:[0-9a-z]{20}$", secret_id) + return re.match(r"secret:[0-9a-z]{20}$", str(value)) def check_config_consistency( From af102aa7638fb7d407e1e959bf117324dd1897eb Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 2 Apr 2024 12:00:36 +1300 Subject: [PATCH 6/7] Minor tweaks as per code review. --- scenario/consistency_checker.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index 8dfbb853..9deb3404 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -327,10 +327,10 @@ def check_storages_consistency( return Results(errors, []) -def _is_secret_identifier(value: Union[str, int, float, bool]): +def _is_secret_identifier(value: Union[str, int, float, bool]) -> bool: """Return true iff the value is in the form `secret:{secret id}`.""" # cf. https://github.com/juju/juju/blob/13eb9df3df16a84fd471af8a3c95ddbd04389b71/core/secrets/secret.go#L48 - return re.match(r"secret:[0-9a-z]{20}$", str(value)) + return bool(re.match(r"secret:[0-9a-z]{20}$", str(value))) def check_config_consistency( @@ -371,6 +371,7 @@ def check_config_consistency( if not expected_type_name: errors.append(f"config.yaml invalid; option {key!r} has no 'type'.") continue + validator = validators.get(expected_type_name) expected_type = converters.get(expected_type_name) if not expected_type: @@ -384,7 +385,7 @@ def check_config_consistency( f"but is of type {type(value)}.", ) - elif not validators.get(expected_type_name, lambda _: True)(value): + elif validator and not validator(value): errors.append( f"config invalid: option {key!r} value {value!r} is not valid.", ) From f8c48deac19d3fbe4e96159f06033dd1e5d40c33 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 2 Apr 2024 12:17:13 +1300 Subject: [PATCH 7/7] Remove the mystery type:attrs disabled code. --- scenario/consistency_checker.py | 1 - 1 file changed, 1 deletion(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index 9deb3404..05efcba3 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -358,7 +358,6 @@ def check_config_consistency( "int": int, "float": float, "boolean": bool, - # "attrs": NotImplemented, # fixme: wot? } if juju_version >= (3, 4): converters["secret"] = str