Fixes #25886: Migrate Environment variable from lift-json to zio-json #6022
Conversation
fanf
force-pushed
the
arch_25886/migrate_environment_variable_from_lift_json_to_zio_json
branch
from
d24a211 to
3557fd4
Compare
| |inventoryDate: 20180717000031.000Z | ||
| |receiveDate: 20180717000527.050Z | ||
| |lastLoggedUserTime: 20000714084300.000Z | ||
| |softwareUpdate: {"name":"rudder-agent","version":"7.0.0-realease","from":"yum","arch":"x86_64","kind":"none","description":"Local privilege escalation in pkexec","severity":"low","date":"2022-01-26T00:00:00Z","ids":["RHSA-2020-4566","CVE-2021-4034"]} | ||
| |softwareUpdate: {"name":"rudder-agent","version":"7.0.0-realease","arch":"x86_64","from":"yum","kind":"none","description":"Local privilege escalation in pkexec","severity":"low","date":"2022-01-26T00:00:00Z","ids":["RHSA-2020-4566","CVE-2021-4034"]} |
There was a problem hiding this comment.
the change was needed because of the idempotency tests, but the previous line still works
| @@ -530,7 +530,7 @@ class TestCoreNodeFactInventory extends Specification with BeforeAfterAll { | |||
| .getAttributeValue("environmentVariable") must beEqualTo("""{"name":"envVAR","value":"envVALUE"}""")) and | |||
| (mockLdapFactStorage.testServer | |||
| .getEntry("nodeId=node7,ou=Nodes,ou=Accepted Inventories,ou=Inventories,cn=rudder-configuration") | |||
| .getAttributeValue("process") must beEqualTo("""{"pid":4242,"commandName":"process 4242 command line"}""")) and | |||
| .getAttributeValue("process") must beEqualTo("""{"pid":4242,"name":"process 4242 command line"}""")) and | |||
There was a problem hiding this comment.
we're still able to read commandName, but we write name now.
|
PR updated with a new commit |
fanf
force-pushed
the
arch_25886/migrate_environment_variable_from_lift_json_to_zio_json
branch
from
f80d204 to
2624c12
Compare
| // convert the processes | ||
| node.processes.map(x => Serialization.write(x)) | ||
| node.processes.map(_.toJson) |
There was a problem hiding this comment.
yes ! this could have already been changed since a while now 😅
| ev = entry.valuesFor(A_EV).toSeq.map(Serialization.read[EnvironmentVariable](_)) | ||
| process = entry.valuesFor(A_PROCESS).toSeq.map(Serialization.read[Process](_)) | ||
| ev = entry.valuesFor(A_EV).toSeq.map(_.fromJson[EnvironmentVariable]).collect { case Right(ev) => ev } | ||
| process = entry.valuesFor(A_PROCESS).toSeq.map(_.fromJson[Process]).collect { case Right(p) => p } |
There was a problem hiding this comment.
Since the Serialization.read method throws, the equivalent would instead be a traverse/ZIO.foreach here instead of failing silently ?
There was a problem hiding this comment.
the previous case was just a bug I think.
So what do we want ? Not a full error and an unreadable inventory because a mear process / env var was uncorectly written in base and can't be read back, because it's completely unactionnable by the user. It's even possible that just sending an other inventory doesn't solve the problem by overriding datas, since we read existing one before writting (which means, we may have other problems). Ok, let's write a test on that.
I can log before ignoring, but I'm pretty sure that log either won't be read ever, or will cause major error like filling disks.
There was a problem hiding this comment.
Added logs and a test to ensure we can deserialize inventories in presence of incorrect data.
Alos, did the same for custom properties which were ignored too.
| @@ -51,7 +51,6 @@ import com.normation.ldap.sdk.schema.LDAPObjectClass | |||
| import com.softwaremill.quicklens.* | |||
| import com.unboundid.ldap.sdk.{Version as _, *} | |||
| import java.net.InetAddress | |||
| import net.liftweb.json.* | |||
fanf
force-pushed
the
arch_25886/migrate_environment_variable_from_lift_json_to_zio_json
branch
from
4ca371a to
25a44da
Compare
| diff.toArray() must beEmpty | ||
| } | ||
|
|
||
| "correctly unserialize a node even with malformed env var, process, soft update" in { |
There was a problem hiding this comment.
okay, I guess if someone is having this problem we haven't heard about it yet 😇
|
OK, merging this PR |
Normation-Quality-Assistant
merged commit 782eba3
into
Normation:master
https://issues.rudder.io/issues/25886