Fixes #25979: Migrate custom properties along with node properties for consistency

webapp/sources/ldap-inventory/inventory-repository/src/main/scala/com/normation/inventory/ldap/core/InventoryMapper.scala

@@ -823,7 +823,8 @@ class InventoryMapper(
       root.resetValuesTo(A_TIMEZONE_NAME, timezone.name)
       root.resetValuesTo(A_TIMEZONE_OFFSET, timezone.offset)
     }
-    server.customProperties.foreach(cp => root.addValues(A_CUSTOM_PROPERTY, cp.toJson))
+    // custom properties are not saved here anymore
+    root.deleteAttribute(A_CUSTOM_PROPERTY)
     server.softwareUpdates.foreach { s =>
       import JsonSerializers.implicits.*
       root.addValues(A_SOFTWARE_UPDATE, s.toJson)

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/facts/nodes/NodeFactRepository.scala

@@ -687,7 +687,7 @@ class CoreNodeFactRepository(
                         // first we persist on cold storage, which is more likely to fail. Plus, for history reason, some
                         // mapping are not exactly isomorphic, and some normalization can happen - for ex, for missing machine.
                         s <- storage.save(updated)
-                        // then, we get the actual thing that was saved from the save even
+                        // then, we get the actual thing that was saved from the save event
                         up = s match {
                                case StorageChangeEventSave.Created(node, attrs)             =>
                                  node

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/facts/nodes/NodeFactStorage.scala

@@ -685,9 +685,6 @@ class LdapNodeFactStorage(
   override def save(nodeFact: NodeFact)(implicit attrs: SelectFacts): IOResult[StorageChangeEventSave] = {
     nodeLibMutex.writeLock(for {
       con        <- ldap
-      _          <- con
-                      .save(nodeMapper.nodeToEntry(nodeFact.toNode))
-                      .chainError(s"Cannot save node with id '${nodeFact.id.value}' in LDAP")
       mergedSoft <- if (LdapNodeFactStorage.needsSoftware(attrs)) {
                       softwareSave.tryWith(nodeFact.software.map(_.toSoftware).toSet).map(Some(_))
                     } else None.succeed
@@ -710,6 +707,9 @@ class LdapNodeFactStorage(
                         nf.modify(_.software).setToIfDefined(optSoft)
                       }
                     )
+      _          <- con
+                      .save(nodeMapper.nodeToEntry(nodeFact.toNode))
+                      .chainError(s"Cannot save node with id '${nodeFact.id.value}' in LDAP")
       inv         = SelectFacts
                       .merge(nodeFact, optOld)(attrs)
                       .toFullInventory
@@ -849,8 +849,10 @@ class LdapNodeFactStorage(
         needSoftware: Boolean
     ): IOResult[Option[NodeFact]] = {
       // mostly copied from com.normation.rudder.services.nodes.NodeInfoServiceCachedImpl # getBackendLdapNodeInfo
-      val ldapAttrs =
-        (if (needSoftware) Seq(A_SOFTWARE_DN) else Seq()) ++ NodeInfoService.nodeInfoAttributes :+ LDAPConstants.A_SOFTWARE_UPDATE
+      val ldapAttrs = {
+        (if (needSoftware) Seq(A_SOFTWARE_DN)
+         else Seq()) ++ NodeInfoService.nodeInfoAttributes :+ LDAPConstants.A_SOFTWARE_UPDATE :+ LDAPConstants.A_CUSTOM_PROPERTY
+      }
 
       con.get(inventoryDitService.getDit(status).NODES.NODE.dn(nodeId.value), ldapAttrs*).flatMap {
         case None      => // end of game, no node here

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala

@@ -135,11 +135,9 @@ class LDAPEntityMapper(
       case _              =>
     }
 
-    // for node properties, we ALWAYS filter-out properties coming from inventory,
-    // because we don't want to store them there.
     entry.resetValuesTo(
       A_NODE_PROPERTY,
-      node.properties.collect { case p if (p.provider != Some(NodeProperty.customPropertyProvider)) => p.toData }*
+      node.properties.map(_.toData)*
     )
 
     node.nodeReportingConfiguration.heartbeatConfiguration match {

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/tenants/TenantService.scala

@@ -118,7 +118,7 @@ object DefaultTenantService {
 
 /*
  *  _tenantsEnabled is accessed in a lot of hot path, we prefer not to encapsulate it into a Ref.
- * We still put its modification behind a eval.
+ * We still put its modification behind an eval.
  */
 class DefaultTenantService(private var _tenantsEnabled: Boolean, val tenantIds: Ref[Set[TenantId]]) extends TenantService {
 

webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/facts/nodes/TestCoreNodeFactInventory.scala

@@ -48,6 +48,8 @@ import com.normation.rudder.domain.Constants
 import com.normation.rudder.domain.nodes.MachineInfo
 import com.normation.rudder.domain.nodes.NodeState
 import com.normation.rudder.domain.policies.PolicyMode
+import com.normation.rudder.domain.properties.GenericProperty.*
+import com.normation.rudder.domain.properties.NodeProperty
 import com.normation.rudder.tenants.DefaultTenantService
 import com.normation.rudder.tenants.TenantId
 import com.normation.utils.DateFormaterService
@@ -217,10 +219,10 @@ class TestCoreNodeFactInventory extends Specification with BeforeAfterAll {
 //    .asInstanceOf[ch.qos.logback.classic.Logger]
 //    .setLevel(ch.qos.logback.classic.Level.TRACE)
 
-  org.slf4j.LoggerFactory
-    .getLogger("nodes.details.write")
-    .asInstanceOf[ch.qos.logback.classic.Logger]
-    .setLevel(ch.qos.logback.classic.Level.TRACE)
+//  org.slf4j.LoggerFactory
+//    .getLogger("nodes.details.write")
+//    .asInstanceOf[ch.qos.logback.classic.Logger]
+//    .setLevel(ch.qos.logback.classic.Level.TRACE)
 
   sequential
 
@@ -593,10 +595,7 @@ class TestCoreNodeFactInventory extends Specification with BeforeAfterAll {
     "root status can not be modified" >> {
       val res = (for {
         r <- factRepo.get(Constants.ROOT_POLICY_SERVER_ID).notOptional("root must be here")
-        _ <- factRepo.save(NodeFact.fromMinimal(r.modify(_.rudderSettings.status).setTo(PendingInventory)))(
-               testChangeContext,
-               SelectFacts.none
-             )
+        _ <- factRepo.save(r.modify(_.rudderSettings.status).setTo(PendingInventory))(testChangeContext)
       } yield ()).either.runNow
 
       res must beLeft
@@ -605,18 +604,37 @@ class TestCoreNodeFactInventory extends Specification with BeforeAfterAll {
     "Update of policy mode to default mode after it was set to audit/enforce should be default (#25866)" >> {
       val res = (for {
         node        <- factRepo.get(node7id).notOptional("node7 must be here")
-        _           <- factRepo.save(NodeFact.fromMinimal(node.modify(_.rudderSettings.policyMode).setTo(Some(PolicyMode.Audit))))(
-                         testChangeContext,
-                         SelectFacts.none
-                       )
-        _           <- factRepo.save(NodeFact.fromMinimal(node.modify(_.rudderSettings.policyMode).setTo(None)))(
-                         testChangeContext,
-                         SelectFacts.none
-                       )
+        _           <- factRepo.save(node.modify(_.rudderSettings.policyMode).setTo(Some(PolicyMode.Audit)))(testChangeContext)
+        _           <- factRepo.save(node.modify(_.rudderSettings.policyMode).setTo(None))(testChangeContext)
         updatedNode <- factRepo.get(node7id).notOptional("node7 must be here")
       } yield updatedNode.rudderSettings.policyMode).either.runNow
 
-      res must beRight(beNone)
+      (mockLdapFactStorage.testServer
+        .getEntry("nodeId=node7,ou=Nodes,cn=rudder-configuration")
+        .getAttribute("policyMode")
+        .getValue === """default""") and
+      (res must beRight(beNone))
+    }
+  }
+
+  "We must see change in state in the diff (#25704)" >> {
+    // node7 is "initializing" in ldap sample data
+    val res = (for {
+      node <- factRepo.get(node7id).notOptional("node7 must be here")
+      diff <- factRepo.save(node.modify(_.rudderSettings.state).setTo(NodeState.PreparingEOL))(testChangeContext)
+    } yield diff).either.runNow
+
+    (mockLdapFactStorage.testServer
+      .getEntry("nodeId=node7,ou=Nodes,cn=rudder-configuration")
+      .getAttribute("state")
+      .getValue === """preparing-eol""") and
+    (res must beRight) and {
+      res.forceGet.event match {
+        case NodeFactChangeEvent.Updated(oldNode, newNode, _) =>
+          (oldNode.rudderSettings.state === NodeState.Initializing) and (newNode.rudderSettings.state === NodeState.PreparingEOL)
+
+        case x => ko(s"bad change event, get ${x}")
+      }
     }
 
     "the count of active nodes change if we disable one" >> {
@@ -664,4 +682,73 @@ class TestCoreNodeFactInventory extends Specification with BeforeAfterAll {
     }
 
   }
+
+  "Inventory properties must be retrieved, migrated and not seen new each time (#25704)" >> {
+    val existingProp1 =
+      NodeProperty.apply("datacenter", "Paris".toConfigValue, None, Some(NodeProperty.customPropertyProvider))
+    val existingProp2 = {
+      NodeProperty.apply(
+        "from_inv",
+        Map("key1" -> "custom prop value", "key2" -> "some more json").toConfigValue,
+        None,
+        Some(NodeProperty.customPropertyProvider)
+      )
+    }
+    val newProp       =
+      NodeProperty.apply("new_inv", "inventory value".toConfigValue, None, Some(NodeProperty.customPropertyProvider))
+
+    // no provider in the json of custom properties
+    val beforeInv = mockLdapFactStorage.testServer
+      .getEntry("nodeId=node1,ou=Nodes,ou=Accepted Inventories,ou=Inventories,cn=rudder-configuration")
+      .getAttribute("customProperty")
+      .getValues === Array(
+      """{"name":"datacenter", "value":"Paris"}""",
+      """{"name":"from_inv", "value":{ "key1":"custom prop value", "key2":"some more json"}}"""
+    )
+
+    val beforeNode = mockLdapFactStorage.testServer
+      .getEntry("nodeId=node1,ou=Nodes,cn=rudder-configuration")
+      .getAttribute("serializedNodeProperty")
+      .getValues === Array("""{"name":"foo","value":"bar"}""")
+
+    val res = (for {
+      node <- factRepo.get(NodeId("node1")).notOptional("node1 must be here")
+      props = node.properties.appended(newProp)
+      // first time: change should be here
+      d1   <- factRepo.save(node.modify(_.properties).setTo(props))(testChangeContext)
+      // second time: should be noop
+      d2   <- factRepo.save(node.modify(_.properties).setTo(props))(testChangeContext)
+    } yield (d1, d2)).either.runNow
+
+    // after a new save, even only touching core node fact, custom props are removed
+    val afterInv = mockLdapFactStorage.testServer
+      .getEntry("nodeId=node1,ou=Nodes,ou=Accepted Inventories,ou=Inventories,cn=rudder-configuration")
+      .getAttribute("customProperty") === null
+
+    // here, we now have the provider info
+    val afterNode = mockLdapFactStorage.testServer
+      .getEntry("nodeId=node1,ou=Nodes,cn=rudder-configuration")
+      .getAttribute("serializedNodeProperty")
+      .getValues === Array(
+      """{"name":"foo","value":"bar"}""", // this one is a node property
+      existingProp1.toData,
+      existingProp2.toData,
+      """{"name":"new_inv","provider":"inventory","value":"inventory value"}"""
+    )
+
+    beforeInv and beforeNode and afterInv and afterNode and {
+      res.forceGet match {
+        case (
+              NodeFactChangeEventCC(NodeFactChangeEvent.Updated(oldNode, newNode, _), _),
+              secondChange
+            ) =>
+          (oldNode.properties.size === 1) and // old node is incorrectly reporting 1 because of the bug: it doesn't see custom props
+          (newNode.properties.size === 4) and
+          (newNode.properties.last === newProp) and
+          (secondChange.event must beAnInstanceOf[NodeFactChangeEvent.Noop])
+
+        case x => ko(s"bad change event, got: ${x._1.event}")
+      }
+    }
+  }
 }

webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala

@@ -3269,6 +3269,7 @@ object RudderConfigInit {
         KEEP_DELETED_NODE_FACT_DURATION
       ),
       new CheckTableReportsExecutionTz(doobie),
+      new CheckMigrateCustomProperties(rwLdap, nodeFactRepository, acceptedNodesDitImpl),
       new CheckTechniqueLibraryReload(
         techniqueRepositoryImpl,
         uuidGen

webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/checks/migration/CheckMigrateCustomProperties.scala

@@ -0,0 +1,123 @@
+/*
+ *************************************************************************************
+ * Copyright 2024 Normation SAS
+ *************************************************************************************
+ *
+ * This file is part of Rudder.
+ *
+ * Rudder is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * In accordance with the terms of section 7 (7. Additional Terms.) of
+ * the GNU General Public License version 3, the copyright holders add
+ * the following Additional permissions:
+ * Notwithstanding to the terms of section 5 (5. Conveying Modified Source
+ * Versions) and 6 (6. Conveying Non-Source Forms.) of the GNU General
+ * Public License version 3, when you create a Related Module, this
+ * Related Module is not considered as a part of the work and may be
+ * distributed under the license agreement of your choice.
+ * A "Related Module" means a set of sources files including their
+ * documentation that, without modification of the Source Code, enables
+ * supplementary functions or services in addition to those offered by
+ * the Software.
+ *
+ * Rudder is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Rudder.  If not, see <http://www.gnu.org/licenses/>.
+
+ *
+ *************************************************************************************
+ */
+
+package bootstrap.liftweb.checks.migration
+
+import bootstrap.liftweb.BootstrapChecks
+import com.normation.errors.*
+import com.normation.inventory.domain.NodeId
+import com.normation.inventory.ldap.core.InventoryDit
+import com.normation.inventory.ldap.core.LDAPConstants.*
+import com.normation.ldap.sdk.BuildFilter
+import com.normation.ldap.sdk.LDAPConnectionProvider
+import com.normation.ldap.sdk.RwLDAPConnection
+import com.normation.rudder.domain.logger.MigrationLoggerPure
+import com.normation.rudder.facts.nodes.ChangeContext
+import com.normation.rudder.facts.nodes.NodeFactRepository
+import com.normation.zio.*
+import zio.*
+
+/*
+ * This migration check looks if there is still nodes with custom properties
+ * in the ou=Accepted Inventories branch, and if so, it save back them so that
+ * they are moved along with other properties.
+ * Added in rudder 8.3 (https://issues.rudder.io/issues/25979)
+ * Can be removed in Rudder 9.2.
+ */
+
+class CheckMigrateCustomProperties(
+    ldap:         LDAPConnectionProvider[RwLDAPConnection],
+    factRepo:     NodeFactRepository,
+    inventoryDit: InventoryDit
+) extends BootstrapChecks {
+
+  override def description: String =
+    "Check if some nodes still have custom properties store in LDAP inventory and migrate them to LDAP node is so"
+
+  override def checks(): Unit = {
+    migrateAll()
+      .catchAll(err => {
+        MigrationLoggerPure.error(s"Error when trying to migrate nodes' custom properties in LDAP: ${err.fullMsg}")
+      })
+      .forkDaemon // make it async to avoid blocking startup, there can be a lot of nodes to migrate
+      .runNow
+  }
+
+  /*
+   * The whole process
+   */
+  def migrateAll(): IOResult[Unit] = {
+    for {
+      ids <- findNodeNeedingMigration()
+      _   <- migrateProperties(ids)
+    } yield ()
+  }
+
+  /*
+   * Look for nodes with customProperty attribute
+   */
+  def findNodeNeedingMigration(): IOResult[Seq[NodeId]] = {
+    for {
+      con           <- ldap
+      needMigration <- con
+                         .searchOne(inventoryDit.NODES.dn, BuildFilter.HAS(A_CUSTOM_PROPERTY), A_NODE_UUID)
+                         .chainError(s"Error when trying to get node entries with existing ${A_CUSTOM_PROPERTY} attributes.")
+      ids           <- ZIO.foreach(needMigration)(e => inventoryDit.NODES.NODE.idFromDN(e.dn).toIO)
+    } yield ids
+  }
+
+  /*
+   * Migrate node after node. We don't want that one failure stop the process
+   */
+  def migrateProperties(nodeIds: Seq[NodeId]): UIO[Unit] = {
+    implicit val cc = ChangeContext.newForRudder(Some("Migrating custom properties LDAP storage"))
+    ZIO
+      .foreach(nodeIds) { id =>
+        (for {
+          opt <- factRepo.get(id)(cc.toQuery)
+          _   <- opt match {
+                   case Some(node) => factRepo.save(node)
+                   case None       => ZIO.unit
+                 }
+        } yield ()).catchAll {
+          case e => MigrationLoggerPure.error(s"Error when migrating custom properties for node '${id.value}': ${e.fullMsg}")
+        }
+      }
+      .unit
+  }
+
+}