From 16791a63c950d5832f211f3189f0ace89ee81943 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Botond=20D=C3=A9nes?= <bdenes@scylladb.com>
Date: Fri, 5 Jan 2024 06:01:52 -0500
Subject: [PATCH 1/2] tools/scylla-sstable: allow always passing
 --scylla-yaml-file option

Currently, if multiple schema sources are provided, the tool complains
about ambiguity, over which to consider. One of these option is
--scylla-yaml-file. However, we want to allow passing this option any
time, otherwise encrypted sstables cannot be read. So relax the multiple
schema source check to also allow this option to be used even when e.g.
--schema-file was used as the schema source.
---
 tools/scylla-sstable.cc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/scylla-sstable.cc b/tools/scylla-sstable.cc
index cc7fa055d0..fa45204357 100644
--- a/tools/scylla-sstable.cc
+++ b/tools/scylla-sstable.cc
@@ -3013,7 +3013,9 @@ $ scylla sstable validate /path/to/md-123456-big-Data.db /path/to/md-123457-big-
             if (!schema_sources) {
                 sst_log.debug("No user-provided schema source, attempting to auto-detect it");
                 schema_with_source = try_load_schema_autodetect(app_config, dbcfg);
-            } else if (schema_sources == 1) {
+            } else if (schema_sources == 1 || (schema_sources == 2 && app_config.contains("scylla-yaml-file"))) {
+                // We make an exception for the case where 2 schema sources are provided, but one of them is scylla-yaml file.
+                // We want to always accept the --scylla-yaml-file option.
                 sst_log.debug("Single schema source provided");
                 schema_with_source = try_load_schema_from_user_provided_source(app_config, dbcfg);
             } else {

From 9119bcbd675b6dce89daf1a0086580ada93d7b16 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Botond=20D=C3=A9nes?= <bdenes@scylladb.com>
Date: Fri, 5 Jan 2024 02:28:50 -0500
Subject: [PATCH 2/2] tools/scylla-sstable: pass error handler to
 utils::config_file::read_from_file()

The default error handler throws an exception, which means
scylla-sstable will exit with exception if there is any problem in the
configuration. Not even ScyllaDB itself is this harsh -- it will just
log a warning for most errors. A tool should be much more lenient. So
this patch passes an error handler which just logs all errors with debug
level.
If reading an sstable fails, the user is expected to investigate turning
debug-level logging on. When they do so, they will see any problems
while reading the configuration (if it is relevant, e.g. when using EAR).

Fixes: #16538
---
 test/cql-pytest/test_tools.py | 26 ++++++++++++++++++++++++++
 tools/scylla-sstable.cc       |  4 +++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/test/cql-pytest/test_tools.py b/test/cql-pytest/test_tools.py
index 6ed1f67217..b8c75450be 100644
--- a/test/cql-pytest/test_tools.py
+++ b/test/cql-pytest/test_tools.py
@@ -993,3 +993,29 @@ def test_scylla_sstable_no_args(scylla_path):
 Usage: scylla sstable OPERATION [OPTIONS] ...
 Try `scylla sstable --help` for more information.
 """
+
+
+def test_scylla_sstable_bad_scylla_yaml(cql, test_keyspace, scylla_path, scylla_data_dir):
+    """ scylla-sstable should not choke on deprecated/unrecognized/etc options in scylla.yaml
+    It should just log a debug-level log and proceed with reading it.
+    This test checks that the config is successfully read, even if there are errors.
+    Reproduces: https://github.com/scylladb/scylladb/issues/16538
+    """
+    with scylla_sstable(simple_clustering_table, cql, test_keyspace, scylla_data_dir) as (schema_file, sstables):
+        with tempfile.NamedTemporaryFile("w+t") as scylla_yaml:
+            scylla_yaml.write("foo: bar")
+            scylla_yaml.flush()
+            res = subprocess.run([scylla_path,
+                                  "sstable", "dump-data",
+                                  "--scylla-yaml", scylla_yaml.name,
+                                  "--schema-file", schema_file,
+                                  "--logger-log-level", "scylla-sstable=debug"]
+                                 + sstables,
+                                 text=True, stderr=subprocess.PIPE)
+            assert res.returncode == 0
+            print(res.stderr)  # when the test fails, it helps to see what the actual output is
+            stderr_lines = res.stderr.split('\n')
+            for expected_msg in (
+                    "error processing configuration item: Unknown option : foo",
+                    "Successfully read scylla.yaml from"):
+                assert any(map(lambda stderr_line: expected_msg in stderr_line, stderr_lines))
diff --git a/tools/scylla-sstable.cc b/tools/scylla-sstable.cc
index fa45204357..65fa7fee68 100644
--- a/tools/scylla-sstable.cc
+++ b/tools/scylla-sstable.cc
@@ -2992,7 +2992,9 @@ $ scylla sstable validate /path/to/md-123456-big-Data.db /path/to/md-123457-big-
         }
 
         if (file_exists(scylla_yaml_path).get()) {
-            dbcfg.read_from_file(scylla_yaml_path).get();
+            dbcfg.read_from_file(scylla_yaml_path, [] (const sstring& opt, const sstring& msg, std::optional<::utils::config_file::value_status> status) {
+                sst_log.debug("error processing configuration item: {} : {}", msg, opt);
+            }).get();
             dbcfg.setup_directories();
             sst_log.debug("Successfully read scylla.yaml from {} location of {}", scylla_yaml_path_source, scylla_yaml_path);
         } else {