mk-oracle: warn about SQLS_ITEM_SID in the migration

CMK-37379

Change-Id: Ie5f5bb6c457278c319f8fbd514dcdb228db7cae5

Author: AnvarU

packages/mk-oracle/README.md

@@ -1100,6 +1100,40 @@ The alias then forms the entry alone and the dropped restriction is reported:
 In both cases, check that the alias resolves to the database the section was meant to
 query — the connection now depends on your `tnsnames.ora` alone.
 
+##### `SQLS_ITEM_SID`
+
+The legacy plugin builds the item of the `oracle_sql` output as
+`[[[<SQLS_ITEM_SID>|<SQLS_ITEM_NAME>]]]`, which lets the displayed SID differ from the
+internal name of the monitored instance. It is mainly used for remote instances, whose
+internal name is the `REMOTE_INSTANCE_<ID>` variable:
+
+```bash
+foo_views_chk1 () {
+    SQLS_SIDS="REMOTE_INSTANCE_PRODPDB1"
+    SQLS_SQL=foo_view_check1.sql
+    SQLS_ITEM_NAME="foo_views_kim1"
+    SQLS_ITEM_SID="PRODPDB1"
+}
+```
+
+**`mk-oracle` has no equivalent field**: the item is always built from the name of the
+instance the section runs on (`sid:`, or the discovered SID). `SQLS_ITEM_SID` is
+therefore not migrated, and the migration warns about every section where the item
+would change:
+
+```
+# WARNING: foo_views_chk1: SQLS_ITEM_SID 'PRODPDB1' is not supported and is not migrated; the item of the oracle_sql section is built from the name of the instance the section runs on, so the name of the service changes and it is rediscovered
+```
+
+The item is part of the Checkmk service name, so an affected service disappears and is
+rediscovered under the new name — together with the rules, downtimes and history bound
+to the old one. Compare the old and the new item before rediscovering, and rename the
+instance (`sid:`) if you need to keep the previous service name.
+
+No warning is emitted when the value cannot change anything: the section runs on that
+one SID anyway, or it uses a custom `SQLS_SECTION_NAME`, for which the legacy plugin
+emits no item at all.
+
 ### What Is Not Migrated
 
 The following variables are recognized but only preserved as comments in the output;
@@ -1109,6 +1143,7 @@ port them manually if you still need them:
 | ----------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ |
 | `SQLS_DBUSER`, `SQLS_DBPASSWORD`, `SQLS_DBSYSCONNECT` | Per-custom-SQL credentials; use per-instance `authentication:` overrides instead                                         |
 | `SQLS_PARAMETERS`                                     | SQL\*Plus parameter passing is not supported                                                                             |
+| `SQLS_ITEM_SID`                                       | The item always carries the name of the instance the section runs on (see [above](#sqls_item_sid))                       |
 | `EXCLUDE_<SID>="<section> ..."`                       | Per-SID exclusion of individual sections; only `EXCLUDE_<SID>="ALL"` is converted                                        |
 | `ORACLE_HOME`, `REMOTE_ORACLE_HOME`                   | The OCI runtime is located as described in [Options](#options) (`use_host_client`)                                       |
 | `ID_BY`                                               | Selects `SID=` vs `SERVICE_NAME=` in the legacy connect string; use the `sid:` / `service_name:` instance fields instead |

packages/mk-oracle/src/config/migration.rs

@@ -90,6 +90,9 @@ struct LegacyCustomSql {
     tns_alias: Option<String>,
     header_name: Option<String>,
     header_sep: Option<char>,
+    /// `SQLS_ITEM_SID`: the SID the legacy plugin puts into the output item.
+    /// Not supported by the new plugin, only used to warn about it.
+    item_sid: Option<String>,
 }
 
 impl LegacyCustomSql {
@@ -256,6 +259,9 @@ fn parse_custom_sqls(legacy: &str, variables: &HashMap<String, String>) -> Vec<L
                 header_sep: section_var("SQLS_SECTION_SEP")
                     .or_else(|| variables.get("SQLS_SECTION_SEP"))
                     .and_then(|v| parse_header_sep(name, v)),
+                // no global fallback: the legacy plugin unsets SQLS_ITEM_SID
+                // before each section
+                item_sid: section_var("SQLS_ITEM_SID").cloned(),
             })
         })
         .collect()
@@ -490,6 +496,28 @@ fn warn_custom_sql_alias_sids(
         .collect()
 }
 
+/// Warn about the custom SQL sections that set `SQLS_ITEM_SID`.
+fn warn_custom_sql_item_sid(custom_sqls: &[LegacyCustomSql]) -> Vec<String> {
+    custom_sqls
+        .iter()
+        .filter(|custom| custom.header_name.is_none())
+        .filter_map(|custom| {
+            let item_sid = custom.item_sid.as_ref()?;
+            // the item is unchanged when the section already runs on exactly
+            // that instance
+            if custom.tns_alias.is_none() && custom.reference_sid().as_ref() == Some(item_sid) {
+                return None;
+            }
+            Some(format!(
+                "{}: SQLS_ITEM_SID '{item_sid}' is not supported and is not migrated; the item of \
+                 the oracle_sql section is built from the name of the instance the section runs \
+                 on, so the name of the service changes and it is rediscovered",
+                custom.name
+            ))
+        })
+        .collect()
+}
+
 /// Collect raw `SQLS_SIDS=` assignments from the legacy config text, keyed by
 /// the enclosing function name (None = top level).
 fn collect_raw_sqls_sids(legacy: &str) -> HashMap<Option<String>, String> {
@@ -580,6 +608,7 @@ pub fn convert(
         &custom_sqls,
         &known_aliases(&dbuser, &dbuser_extras),
     ));
+    warnings.extend(warn_custom_sql_item_sid(&custom_sqls));
     warnings.extend(custom_sql_warnings(&custom_sqls));
     for warning in warnings {
         let warning = format!("# WARNING: {warning}\n");
@@ -1349,6 +1378,7 @@ mod tests {
                 tns_alias: None,
                 header_name: None,
                 header_sep: None,
+                item_sid: None,
             }]
         );
     }
@@ -1372,6 +1402,7 @@ mod tests {
                 tns_alias: None,
                 header_name: None,
                 header_sep: None,
+                item_sid: None,
             }]
         );
     }
@@ -1732,6 +1763,36 @@ sec2 () {
         );
     }
 
+    #[cfg(not(windows))]
+    #[test]
+    fn test_convert_warns_on_custom_sql_item_sid() {
+        let legacy = "sec1 () {\n    SQLS_SIDS=\"REMOTE_INSTANCE_PRODPDB1\"\n    \
+                      SQLS_SQL=\"a.sql\"\n    SQLS_ITEM_SID=\"PRODPDB1\"\n}\n";
+        let vars = HashMap::from([
+            ("DBUSER".into(), "checkmk:secret::::".into()),
+            (
+                "REMOTE_INSTANCE_PRODPDB1".into(),
+                "user:pass::remotehost:1521::PRODCDB:11.2".into(),
+            ),
+            ("SQLS_SECTIONS".into(), "sec1".into()),
+            (
+                "SQLS.sec1.SQLS_SIDS".into(),
+                "REMOTE_INSTANCE_PRODPDB1".into(),
+            ),
+            ("SQLS.sec1.SQLS_SQL".into(), "a.sql".into()),
+            ("SQLS.sec1.SQLS_ITEM_SID".into(), "PRODPDB1".into()),
+        ]);
+        let result = convert(legacy, "/test/cfg", &vars, TS).unwrap();
+        assert!(
+            result.contains("# WARNING: sec1: SQLS_ITEM_SID 'PRODPDB1' is not supported"),
+            "got: {result}"
+        );
+        assert!(
+            !result.contains("item_sid"),
+            "there is no field to migrate it to, got: {result}"
+        );
+    }
+
     #[test]
     fn test_parse_custom_sqls_header_name() {
         let vars = HashMap::from([
@@ -1760,6 +1821,70 @@ sec2 () {
         assert_eq!(result[1].name, "sec2", "default is the section name");
     }
 
+    #[test]
+    fn test_parse_custom_sqls_item_sid() {
+        let vars = HashMap::from([
+            ("SQLS_SECTIONS".into(), "sec1 sec2".into()),
+            ("SQLS_SQL".into(), "a.sql".into()),
+            ("SQLS_ITEM_SID".into(), "GLOBAL".into()),
+            ("SQLS.sec1.SQLS_ITEM_SID".into(), "PRODPDB1".into()),
+        ]);
+        let result = parse_custom_sqls("", &vars);
+        assert_eq!(result[0].item_sid.as_deref(), Some("PRODPDB1"));
+        assert!(
+            result[1].item_sid.is_none(),
+            "SQLS_ITEM_SID has no global fallback"
+        );
+    }
+
+    #[test]
+    fn test_warn_custom_sql_item_sid() {
+        let mut custom = make_custom_sql("sec1", None, "a.sql", &["REMOTE_PROD"]);
+        custom.item_sid = Some("PRODPDB1".into());
+        assert_eq!(
+            warn_custom_sql_item_sid(&[custom]),
+            vec![
+                "sec1: SQLS_ITEM_SID 'PRODPDB1' is not supported and is not migrated; the item of \
+                 the oracle_sql section is built from the name of the instance the section runs \
+                 on, so the name of the service changes and it is rediscovered"
+            ]
+        );
+    }
+
+    #[test]
+    fn test_warn_custom_sql_item_sid_same_as_only_sid() {
+        let mut custom = make_custom_sql("sec1", None, "a.sql", &["PRODPDB1"]);
+        custom.item_sid = Some("PRODPDB1".into());
+        assert!(
+            warn_custom_sql_item_sid(&[custom]).is_empty(),
+            "item is unchanged when the section runs on that instance only"
+        );
+    }
+
+    #[test]
+    fn test_warn_custom_sql_item_sid_of_aliased_section() {
+        let mut custom = make_custom_sql("sec1", None, "a.sql", &["PRODPDB1"]);
+        custom.item_sid = Some("PRODPDB1".into());
+        custom.tns_alias = Some("PROD_ALIAS".into());
+        assert_eq!(
+            warn_custom_sql_item_sid(&[custom]).len(),
+            1,
+            "an aliased section may connect to any instance"
+        );
+    }
+
+    #[test]
+    fn test_warn_custom_sql_item_sid_ignores_unaffected_sections() {
+        let plain = make_custom_sql("plain", None, "a.sql", &["PRODPDB1"]);
+        let mut custom_section = make_custom_sql("custom_section", None, "a.sql", &["PRODPDB1"]);
+        custom_section.item_sid = Some("OTHER".into());
+        custom_section.header_name = Some("my_section".into());
+        assert!(
+            warn_custom_sql_item_sid(&[plain, custom_section]).is_empty(),
+            "no SQLS_ITEM_SID, or a section the legacy plugin emits no item for"
+        );
+    }
+
     #[test]
     fn test_parse_custom_sqls_section_sep() {
         let vars = HashMap::from([
@@ -1864,6 +1989,7 @@ sec3 () {
             tns_alias: None,
             header_name: None,
             header_sep: None,
+            item_sid: None,
         }
     }