diff --git a/changelog/68335.fixed.md b/changelog/68335.fixed.md new file mode 100644 index 000000000000..1a7b895d6d3f --- /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. diff --git a/salt/fileserver/s3fs.py b/salt/fileserver/s3fs.py index d3c3d9cd78f0..5c7b909b0967 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 objects in meta.values(): + for obj in objects: + cached_files.add(obj["Key"]) if log.isEnabledFor(logging.DEBUG): import pprint diff --git a/tests/pytests/unit/fileserver/test_s3fs.py b/tests/pytests/unit/fileserver/test_s3fs.py index 165eef4711f7..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"), @@ -179,3 +180,96 @@ 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(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) + + # 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(bucket, s3): + """Test that _prune_deleted_files works correctly with single environment per bucket.""" + + # 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) + + # 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"] + + # 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)