From b0cd1e857855ebeb6c15a6fcd3f6bca01a2be052 Mon Sep 17 00:00:00 2001 From: "Thomas J. Fan" Date: Thu, 18 Apr 2024 17:49:26 -0700 Subject: [PATCH] Do not check for secret groups during runtime (#2355) * Do not check for secret groups during runtime Signed-off-by: Thomas J. Fan * TST Adds test for no groups Signed-off-by: Thomas J. Fan --------- Signed-off-by: Thomas J. Fan --- flytekit/configuration/plugin.py | 2 +- flytekit/core/context_manager.py | 10 ---------- tests/flytekit/unit/core/test_context_manager.py | 6 ------ tests/flytekit/unit/models/core/test_security.py | 13 +++++++++++++ 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/flytekit/configuration/plugin.py b/flytekit/configuration/plugin.py index 5c487ac696..cc8750deaa 100644 --- a/flytekit/configuration/plugin.py +++ b/flytekit/configuration/plugin.py @@ -76,7 +76,7 @@ def configure_pyflyte_cli(main: Group) -> Group: @staticmethod def secret_requires_group() -> bool: - """Return True if secrets require group entry.""" + """Return True if secrets require group entry during registration time.""" return True @staticmethod diff --git a/flytekit/core/context_manager.py b/flytekit/core/context_manager.py index f70f10bc94..1214dbb498 100644 --- a/flytekit/core/context_manager.py +++ b/flytekit/core/context_manager.py @@ -360,7 +360,6 @@ def get( Retrieves a secret using the resolution order -> Env followed by file. If not found raises a ValueError param encode_mode, defines the mode to open files, it can either be "r" to read file, or "rb" to read binary file """ - self.check_group_key(group) env_var = self.get_secrets_env_var(group, key, group_version) fpath = self.get_secrets_file(group, key, group_version) v = os.environ.get(env_var) @@ -380,7 +379,6 @@ def get_secrets_env_var( """ Returns a string that matches the ENV Variable to look for the secrets """ - self.check_group_key(group) l = [k.upper() for k in filter(None, (group, group_version, key))] return f"{self._env_prefix}{'_'.join(l)}" @@ -390,18 +388,10 @@ def get_secrets_file( """ Returns a path that matches the file to look for the secrets """ - self.check_group_key(group) l = [k.lower() for k in filter(None, (group, group_version, key))] l[-1] = f"{self._file_prefix}{l[-1]}" return os.path.join(self._base_dir, *l) - @staticmethod - def check_group_key(group: Optional[str]): - from flytekit.configuration.plugin import get_plugin - - if get_plugin().secret_requires_group() and (group is None or group == ""): - raise ValueError("secrets group is a mandatory field.") - @dataclass(frozen=True) class CompilationState(object): diff --git a/tests/flytekit/unit/core/test_context_manager.py b/tests/flytekit/unit/core/test_context_manager.py index 7338f820dc..70a2d552d8 100644 --- a/tests/flytekit/unit/core/test_context_manager.py +++ b/tests/flytekit/unit/core/test_context_manager.py @@ -151,8 +151,6 @@ def test_secrets_manager_default(): def test_secrets_manager_get_envvar(): sec = SecretsManager() - with pytest.raises(ValueError): - sec.get_secrets_env_var("", "x") cfg = SecretsConfig.auto() assert sec.get_secrets_env_var("group", "test") == f"{cfg.env_prefix}GROUP_TEST" assert sec.get_secrets_env_var("group", "test", "v1") == f"{cfg.env_prefix}GROUP_V1_TEST" @@ -168,8 +166,6 @@ def test_secret_manager_no_group(monkeypatch): sec = SecretsManager() cfg = SecretsConfig.auto() - sec.check_group_key(None) - sec.check_group_key("") assert sec.get_secrets_env_var(key="ABC") == f"{cfg.env_prefix}ABC" @@ -180,8 +176,6 @@ def test_secret_manager_no_group(monkeypatch): def test_secrets_manager_get_file(): sec = SecretsManager() - with pytest.raises(ValueError): - sec.get_secrets_file("", "x") cfg = SecretsConfig.auto() assert sec.get_secrets_file("group", "test") == os.path.join( cfg.default_dir, diff --git a/tests/flytekit/unit/models/core/test_security.py b/tests/flytekit/unit/models/core/test_security.py index a7ed006174..968afadc1b 100644 --- a/tests/flytekit/unit/models/core/test_security.py +++ b/tests/flytekit/unit/models/core/test_security.py @@ -1,5 +1,7 @@ from unittest.mock import Mock +import pytest + import flytekit.configuration.plugin from flytekit.models.security import Secret @@ -16,6 +18,17 @@ def test_secret(): assert obj2.group_version == "v1" +def test_secret_error(monkeypatch): + # Mock configuration to require groups for this test + plugin_mock = Mock() + plugin_mock.secret_requires_group.return_value = True + mock_global_plugin = {"plugin": plugin_mock} + monkeypatch.setattr(flytekit.configuration.plugin, "_GLOBAL_CONFIG", mock_global_plugin) + + with pytest.raises(ValueError, match="Group is a required parameter"): + Secret(key="my_key") + + def test_secret_no_group(monkeypatch): plugin_mock = Mock() plugin_mock.secret_requires_group.return_value = False