From 1a5d8b4a41230e7d2fef9902bde28c8a6b0ed8f6 Mon Sep 17 00:00:00 2001 From: Ruslan Gainanov Date: Mon, 10 Nov 2025 17:40:44 +0300 Subject: [PATCH 1/5] Fix KeyError in s3fs _prune_deleted_files function Fixes issue #68335 where fileserver.update fails with KeyError: 'Key' when using S3 buckets in multiple environments per bucket mode. The issue occurs in _prune_deleted_files function where the code assumes a different data structure than what is actually provided. The fix properly handles the nested bucket structure by iterating through buckets and objects within each bucket to extract the 'Key' field correctly. Signed-off-by: GRomR1 --- salt/fileserver/s3fs.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/salt/fileserver/s3fs.py b/salt/fileserver/s3fs.py index d3c3d9cd78f0..57f396efd15d 100644 --- a/salt/fileserver/s3fs.py +++ b/salt/fileserver/s3fs.py @@ -591,7 +591,9 @@ def _prune_deleted_files(metadata): roots.add(root) for meta in env_data: - cached_files.add(meta["Key"]) + for bucket in meta.keys(): + for obj in meta[bucket]: + cached_files.add(obj["Key"]) if log.isEnabledFor(logging.DEBUG): import pprint From a2752a4ec662de16bc20c79786b4643464ed319d Mon Sep 17 00:00:00 2001 From: Ruslan Gainanov Date: Mon, 10 Nov 2025 17:48:17 +0300 Subject: [PATCH 2/5] Add changelog entry for issue #68335 fix Signed-off-by: GRomR1 --- changelog/68335.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/68335.fixed.md diff --git a/changelog/68335.fixed.md b/changelog/68335.fixed.md new file mode 100644 index 000000000000..affec6db745f --- /dev/null +++ b/changelog/68335.fixed.md @@ -0,0 +1 @@ +Fixes KeyError in s3fs _prune_deleted_files function when using S3 buckets in multiple environments per bucket mode. The issue occurred because the code assumed a different data structure than what was actually provided. The fix properly handles the nested bucket structure by iterating through buckets and objects within each bucket to extract the 'Key' field correctly. \ No newline at end of file From 07e35fc347935f44467b3c35fc05e3a6039a2eaa Mon Sep 17 00:00:00 2001 From: Ruslan Gainanov Date: Mon, 17 Nov 2025 12:14:05 +0300 Subject: [PATCH 3/5] Improve code readability in s3fs _prune_deleted_files function Use .values() instead of .keys() for better Pythonic code style. This addresses the review comment from bdrx312. Signed-off-by: GRomR1 --- salt/fileserver/s3fs.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/fileserver/s3fs.py b/salt/fileserver/s3fs.py index 57f396efd15d..5c7b909b0967 100644 --- a/salt/fileserver/s3fs.py +++ b/salt/fileserver/s3fs.py @@ -591,8 +591,8 @@ def _prune_deleted_files(metadata): roots.add(root) for meta in env_data: - for bucket in meta.keys(): - for obj in meta[bucket]: + for objects in meta.values(): + for obj in objects: cached_files.add(obj["Key"]) if log.isEnabledFor(logging.DEBUG): From 37e6c03c6230f16a81a918b241aeb30d1fdcdbd7 Mon Sep 17 00:00:00 2001 From: Ruslan Gainanov Date: Mon, 17 Nov 2025 12:38:16 +0300 Subject: [PATCH 4/5] Add tests for s3fs _prune_deleted_files function Add comprehensive tests for both single environment per bucket and multiple environments per bucket modes to ensure the KeyError fix works correctly in both scenarios. Tests cover: - File deletion from cache when files are removed from S3 - Proper handling of nested metadata structure in multiple env mode - Cache cleanup for both bucket configuration modes Signed-off-by: GRomR1 --- tests/pytests/unit/fileserver/test_s3fs.py | 119 +++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/tests/pytests/unit/fileserver/test_s3fs.py b/tests/pytests/unit/fileserver/test_s3fs.py index 165eef4711f7..3331a81ced78 100644 --- a/tests/pytests/unit/fileserver/test_s3fs.py +++ b/tests/pytests/unit/fileserver/test_s3fs.py @@ -179,3 +179,122 @@ def test_ignore_pickle_load_exceptions(): # TODO: parameterized test with patched pickle.load that raises the # various allowable exception from _read_buckets_cache_file pass + + +@pytest.mark.skip_on_fips_enabled_platform +def test_prune_deleted_files_multiple_envs_per_bucket(tmp_path, bucket, s3): + """Test that _prune_deleted_files works correctly with multiple environments per bucket.""" + + # Create test files in S3 + keys = { + "base/test1.sls": {"content": "test1 content"}, + "base/test2.sls": {"content": "test2 content"}, + "dev/test3.sls": {"content": "test3 content"}, + } + make_keys(bucket, s3, keys) + + # Configure for multiple environments per bucket mode + opts = { + "cachedir": tmp_path, + "s3.buckets": [bucket], # List mode = multiple environments per bucket + "s3.location": "us-east-1", + "s3.s3_cache_expire": -1, + } + utils = {"s3.query": salt.utils.s3.query} + + # Update the module configuration + s3fs.__opts__ = opts + s3fs.__utils__ = utils + + # Initial update to populate cache + s3fs.update() + + # Verify files are cached + for key in keys: + env, filename = key.split("/", 1) + cache_file = s3fs._get_cached_file_name(bucket, env, filename) + assert os.path.exists(cache_file) + + # Delete one file from S3 + s3.delete_object(Bucket=bucket, Key="base/test1.sls") + del keys["base/test1.sls"] + + # Update metadata to reflect the deletion + # This simulates what would happen after S3 metadata refresh + metadata = { + "base": [ + {bucket: [{"Key": "base/test2.sls"}]} + ], + "dev": [ + {bucket: [{"Key": "dev/test3.sls"}]} + ] + } + + # Call _prune_deleted_files directly + s3fs._prune_deleted_files(metadata) + + # Verify that deleted file was removed from cache + deleted_cache_file = s3fs._get_cached_file_name(bucket, "base", "test1.sls") + assert not os.path.exists(deleted_cache_file) + + # Verify that remaining files still exist + remaining_cache_file = s3fs._get_cached_file_name(bucket, "base", "test2.sls") + assert os.path.exists(remaining_cache_file) + + dev_cache_file = s3fs._get_cached_file_name(bucket, "dev", "test3.sls") + assert os.path.exists(dev_cache_file) + + +@pytest.mark.skip_on_fips_enabled_platform +def test_prune_deleted_files_single_env_per_bucket(tmp_path, bucket, s3): + """Test that _prune_deleted_files works correctly with single environment per bucket.""" + + # Create test files in S3 + keys = { + "test1.sls": {"content": "test1 content"}, + "test2.sls": {"content": "test2 content"}, + } + make_keys(bucket, s3, keys) + + # Configure for single environment per bucket mode + opts = { + "cachedir": tmp_path, + "s3.buckets": {"base": [bucket]}, # Dict mode = single environment per bucket + "s3.location": "us-east-1", + "s3.s3_cache_expire": -1, + } + utils = {"s3.query": salt.utils.s3.query} + + # Update the module configuration + s3fs.__opts__ = opts + s3fs.__utils__ = utils + + # Initial update to populate cache + s3fs.update() + + # Verify files are cached + for key in keys: + cache_file = s3fs._get_cached_file_name(bucket, "base", key) + assert os.path.exists(cache_file) + + # Delete one file from S3 + s3.delete_object(Bucket=bucket, Key="test1.sls") + del keys["test1.sls"] + + # Update metadata to reflect the deletion + metadata = { + "base": [ + {bucket: [{"Key": "test2.sls"}]} + ] + } + + # Call _prune_deleted_files directly + s3fs._prune_deleted_files(metadata) + + # Verify that deleted file was removed from cache + deleted_cache_file = s3fs._get_cached_file_name(bucket, "base", "test1.sls") + assert not os.path.exists(deleted_cache_file) + + # Verify that remaining file still exists + remaining_cache_file = s3fs._get_cached_file_name(bucket, "base", "test2.sls") + assert os.path.exists(remaining_cache_file) From 256fd724685971b658368b192197ad3cec88424d Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 8 Jun 2026 22:13:16 -0700 Subject: [PATCH 5/5] Fix lint and assertions in s3fs _prune_deleted_files tests Use the existing configure_loader_modules fixture instead of direct assignment to s3fs.__opts__ / s3fs.__utils__ (which tripped pylint's unmocked-patch-dunder rule). Override s3.buckets via patch.dict for the multi-env-per-bucket scenario. Correct the cached-file path assertions: in multi-env-per-bucket mode the env prefix is part of the S3 key, so _get_cached_file_name() returns ////, not ///. The multi-env test now asserts the property the PR actually fixes - _prune_deleted_files returning without raising KeyError on the metadata shape produced by _init() in multi-env mode. Co-authored-by: GRomR1 --- changelog/68335.fixed.md | 2 +- tests/pytests/unit/fileserver/test_s3fs.py | 143 +++++++++------------ 2 files changed, 60 insertions(+), 85 deletions(-) diff --git a/changelog/68335.fixed.md b/changelog/68335.fixed.md index affec6db745f..1a7b895d6d3f 100644 --- a/changelog/68335.fixed.md +++ b/changelog/68335.fixed.md @@ -1 +1 @@ -Fixes KeyError in s3fs _prune_deleted_files function when using S3 buckets in multiple environments per bucket mode. The issue occurred because the code assumed a different data structure than what was actually provided. The fix properly handles the nested bucket structure by iterating through buckets and objects within each bucket to extract the 'Key' field correctly. \ No newline at end of file +Fixes KeyError in s3fs _prune_deleted_files function when using S3 buckets in multiple environments per bucket mode. The issue occurred because the code assumed a different data structure than what was actually provided. The fix properly handles the nested bucket structure by iterating through buckets and objects within each bucket to extract the 'Key' field correctly. diff --git a/tests/pytests/unit/fileserver/test_s3fs.py b/tests/pytests/unit/fileserver/test_s3fs.py index 3331a81ced78..0501513cf64a 100644 --- a/tests/pytests/unit/fileserver/test_s3fs.py +++ b/tests/pytests/unit/fileserver/test_s3fs.py @@ -14,6 +14,7 @@ import salt.fileserver.s3fs as s3fs import salt.utils.s3 +from tests.support.mock import patch pytestmark = [ pytest.mark.skipif(not HAS_BOTO, reason="Missing library moto or boto3"), @@ -182,119 +183,93 @@ def test_ignore_pickle_load_exceptions(): @pytest.mark.skip_on_fips_enabled_platform -def test_prune_deleted_files_multiple_envs_per_bucket(tmp_path, bucket, s3): - """Test that _prune_deleted_files works correctly with multiple environments per bucket.""" - - # Create test files in S3 +def test_prune_deleted_files_multiple_envs_per_bucket(bucket, s3): + """ + Test that _prune_deleted_files does not raise KeyError in + multi-environment-per-bucket mode (issue #68335). + + Prior to the fix the function read meta["Key"] off the + {bucket: [...]} dict it received as `meta`, raising + KeyError: 'Key'. The fix descends one more level + (meta.values() -> obj["Key"]) so the cached_files set is + populated correctly. + """ + + # Create test files in S3 with the env as the first path component + # (this is how multi-env-per-bucket mode discovers environments). keys = { "base/test1.sls": {"content": "test1 content"}, "base/test2.sls": {"content": "test2 content"}, "dev/test3.sls": {"content": "test3 content"}, } make_keys(bucket, s3, keys) - - # Configure for multiple environments per bucket mode - opts = { - "cachedir": tmp_path, - "s3.buckets": [bucket], # List mode = multiple environments per bucket - "s3.location": "us-east-1", - "s3.s3_cache_expire": -1, - } - utils = {"s3.query": salt.utils.s3.query} - - # Update the module configuration - s3fs.__opts__ = opts - s3fs.__utils__ = utils - - # Initial update to populate cache - s3fs.update() - - # Verify files are cached - for key in keys: - env, filename = key.split("/", 1) - cache_file = s3fs._get_cached_file_name(bucket, env, filename) - assert os.path.exists(cache_file) - - # Delete one file from S3 - s3.delete_object(Bucket=bucket, Key="base/test1.sls") - del keys["base/test1.sls"] - - # Update metadata to reflect the deletion - # This simulates what would happen after S3 metadata refresh - metadata = { - "base": [ - {bucket: [{"Key": "base/test2.sls"}]} - ], - "dev": [ - {bucket: [{"Key": "dev/test3.sls"}]} - ] - } - - # Call _prune_deleted_files directly - s3fs._prune_deleted_files(metadata) - - # Verify that deleted file was removed from cache - deleted_cache_file = s3fs._get_cached_file_name(bucket, "base", "test1.sls") - assert not os.path.exists(deleted_cache_file) - - # Verify that remaining files still exist - remaining_cache_file = s3fs._get_cached_file_name(bucket, "base", "test2.sls") - assert os.path.exists(remaining_cache_file) - - dev_cache_file = s3fs._get_cached_file_name(bucket, "dev", "test3.sls") - assert os.path.exists(dev_cache_file) + + # Override s3.buckets to a list, which switches s3fs into + # multi-env-per-bucket mode (vs. the dict form set by the fixture). + with patch.dict(s3fs.__opts__, {"s3.buckets": [bucket]}): + # Populate the cache so update() lays down real files. + s3fs.update() + + # In multi-env mode the env prefix is part of the S3 key, so the + # cached path is ////. + for key in keys: + env = key.split("/", 1)[0] + cache_file = s3fs._get_cached_file_name(bucket, env, key) + assert os.path.exists(cache_file) + + # Hand-built metadata in the shape s3fs._init() produces for + # multi-env-per-bucket mode: {saltenv: [{bucket_name: [file_meta]}]}. + # Prior to the fix this exact structure caused KeyError: 'Key'. + metadata = { + "base": [ + { + bucket: [ + {"Key": "base/test1.sls"}, + {"Key": "base/test2.sls"}, + ] + } + ], + "dev": [{bucket: [{"Key": "dev/test3.sls"}]}], + } + + # The fix is verified by this call returning without raising. + s3fs._prune_deleted_files(metadata) @pytest.mark.skip_on_fips_enabled_platform -def test_prune_deleted_files_single_env_per_bucket(tmp_path, bucket, s3): +def test_prune_deleted_files_single_env_per_bucket(bucket, s3): """Test that _prune_deleted_files works correctly with single environment per bucket.""" - - # Create test files in S3 + + # The configure_loader_modules fixture already sets s3.buckets to + # {"base": [bucket]} (dict form = env-per-bucket mode). keys = { "test1.sls": {"content": "test1 content"}, "test2.sls": {"content": "test2 content"}, } make_keys(bucket, s3, keys) - - # Configure for single environment per bucket mode - opts = { - "cachedir": tmp_path, - "s3.buckets": {"base": [bucket]}, # Dict mode = single environment per bucket - "s3.location": "us-east-1", - "s3.s3_cache_expire": -1, - } - utils = {"s3.query": salt.utils.s3.query} - - # Update the module configuration - s3fs.__opts__ = opts - s3fs.__utils__ = utils - + # Initial update to populate cache s3fs.update() - + # Verify files are cached for key in keys: cache_file = s3fs._get_cached_file_name(bucket, "base", key) assert os.path.exists(cache_file) - + # Delete one file from S3 s3.delete_object(Bucket=bucket, Key="test1.sls") del keys["test1.sls"] - - # Update metadata to reflect the deletion - metadata = { - "base": [ - {bucket: [{"Key": "test2.sls"}]} - ] - } - + + # Metadata in the shape s3fs._init() produces for env-per-bucket mode. + metadata = {"base": [{bucket: [{"Key": "test2.sls"}]}]} + # Call _prune_deleted_files directly s3fs._prune_deleted_files(metadata) - + # Verify that deleted file was removed from cache deleted_cache_file = s3fs._get_cached_file_name(bucket, "base", "test1.sls") assert not os.path.exists(deleted_cache_file) - + # Verify that remaining file still exists remaining_cache_file = s3fs._get_cached_file_name(bucket, "base", "test2.sls") assert os.path.exists(remaining_cache_file)