Remove dns property from compose file (#2055)

* Remove dns property from compose file * check has own property * Force dns removal action before adding aliases * add logging for debugging * use dns in * use internal funcion * Use aggregate error * use promise settled * use custom error class * add deletedns unit test (#2057) * add deletedns unit test * fix removeDns() * use double quotes * fix editor with lint --------- Co-authored-by: pablomendezroyo <[email protected]> * replace probkematic tag --------- Co-authored-by: Marc Font <[email protected]>
dappnode · Nov 11, 2024 · 3ecda3b · 3ecda3b
1 parent 45d501b
commit 3ecda3b
Show file tree

Hide file tree

Showing 5 changed files with 177 additions and 154 deletions.
diff --git a/packages/dockerCompose/src/editor.ts b/packages/dockerCompose/src/editor.ts
@@ -76,6 +76,16 @@ export class ComposeServiceEditor {
     }));
   }
 
+  /**
+   * Remove the property directly from the service.
+   */
+  removeDns(): void {
+    const service = this.get();
+    if ("dns" in service) {
+      delete service.dns;
+    }
+  }
+
   removeNetworkAliases(networkName: string, aliasesToRemove: string[], serviceNetwork: ComposeServiceNetwork): void {
     this.edit((service) => {
       const networks = parseServiceNetworks(service.networks || {});
@@ -233,7 +243,9 @@ export class ComposeEditor {
 
   static readFrom(composePath: string): Compose {
     const yamlString = fs.readFileSync(composePath, "utf8");
-    return yamlParse<Compose>(yamlString);
+    // Replace problematic tag with a placeholder value such as null
+    const yamlStringParsed = yamlString.replace(/!<tag:yaml\.org,2002:js\/undefined>/g, "null");
+    return yamlParse<Compose>(yamlStringParsed);
   }
 
   static getComposePath(dnpName: string, isCore: boolean): string {

diff --git a/packages/dockerCompose/test/unit/composeDeleteDns.test.ts b/packages/dockerCompose/test/unit/composeDeleteDns.test.ts
@@ -0,0 +1,47 @@
+import "mocha";
+import { expect } from "chai";
+import { ComposeEditor } from "../../src/index.js";
+import { Compose } from "@dappnode/types";
+
+describe("ComposeServiceEditor", function () {
+  describe("removeDns()", function () {
+    it("should remove the dns field from the service", function () {
+      // Create a mock compose object
+      const mockCompose: Compose = {
+        version: "3",
+        services: {
+          myservice: {
+            image: "myimage",
+            dns: "8.8.8.8",
+            environment: []
+          }
+        }
+      };
+
+      // Create a ComposeEditor instance with the mock compose
+      const composeEditor = new ComposeEditor(mockCompose);
+
+      // Get the service editor for 'myservice'
+      const serviceEditor = composeEditor.services()["myservice"];
+
+      // Ensure dns field is present before removal
+      expect(serviceEditor.get().dns).to.deep.equal("8.8.8.8");
+
+      // Call removeDns()
+      serviceEditor.removeDns();
+
+      // Get the updated service
+      const updatedService = serviceEditor.get();
+
+      // Verify that the dns field is removed
+      expect(updatedService.dns).to.be.undefined;
+
+      // Output the compose and check that dns is not present
+      const outputCompose = composeEditor.output();
+      expect(outputCompose.services["myservice"].dns).to.be.undefined;
+
+      // Ensure other fields remain unchanged
+      expect(outputCompose.services["myservice"].image).to.equal("myimage");
+    });
+  });
+});
diff --git a/packages/migrations/src/index.ts b/packages/migrations/src/index.ts
@@ -1,8 +1,7 @@
 import { migrateUserActionLogs } from "./migrateUserActionLogs.js";
 import { removeLegacyDockerAssets } from "./removeLegacyDockerAssets.js";
-import { addAliasToRunningContainers } from "./addAliasToRunningContainers.js";
+import { removeDnsAndAddAlias } from "./removeDnsAndAddAlias.js";
 import { pruneUserActionLogs } from "./pruneUserActionLogs.js";
-import { removeDnsFromComposeFiles } from "./removeDnsFromComposeFiles.js";
 import { migrateDockerNetworkIpRange } from "./migrateDockerNetworkIpRange/index.js";
 import { recreateContainersIfLegacyDns } from "./recreateContainersIfLegacyDns.js";
 import { ensureCoreComposesHardcodedIpsRange } from "./ensureCoreComposesHardcodedIpsRange.js";
@@ -13,143 +12,112 @@ import { createStakerNetworkAndConnectStakerPkgs } from "./createStakerNetworkAn
 import { determineIsDappnodeAws } from "./determineIsDappnodeAws.js";
 import { Consensus, Execution, MevBoost, Signer } from "@dappnode/stakers";
 
-export class MigrationError extends Error {
+class MigrationError extends Error {
+  errors: Error[];
+
+  constructor(errors: Error[]) {
+    super("One or more migrations failed");
+    this.name = "MigrationError";
+    this.errors = errors; // Retain the original error details
+  }
+
+  toString(): string {
+    return `${this.name}: ${this.message}\n` + this.errors.map((err) => err.message).join("\n");
+  }
+}
+
+interface Migration {
+  fn: () => Promise<void>;
   migration: string;
   coreVersion: string;
-  constructor(migration: string, coreVersion: string) {
-    super();
-    this.migration = migration;
-    this.coreVersion = coreVersion;
-    super.message = `Migration ${migration} ${coreVersion} failed: ${super.message}`;
-  }
 }
 
-/**
- * Executes migrations required for the current DAppNode core version.
- */
 export async function executeMigrations(
   execution: Execution,
   consensus: Consensus,
   signer: Signer,
   mevBoost: MevBoost
 ): Promise<void> {
-  const migrationErrors: MigrationError[] = [];
-
-  await removeLegacyDockerAssets().catch((e) =>
-    migrationErrors.push({
+  const migrations: Migration[] = [
+    {
+      fn: removeLegacyDockerAssets,
       migration: "bundle legacy ops to prevent spamming the docker API",
-      coreVersion: "0.2.30",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await migrateUserActionLogs().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.30"
+    },
+    {
+      fn: migrateUserActionLogs,
       migration: "migrate winston .log JSON file to a lowdb",
-      coreVersion: "0.2.30",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await pruneUserActionLogs().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.30"
+    },
+    {
+      fn: pruneUserActionLogs,
       migration: "prune user action logs if the size is greater than 4 MB",
-      coreVersion: "0.2.59",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await removeDnsFromComposeFiles().catch((e) =>
-    migrationErrors.push({
-      migration: "remove bind DNS from docker compose files",
-      coreVersion: "0.2.82",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await ensureCoreComposesHardcodedIpsRange().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.59"
+    },
+    {
+      fn: ensureCoreComposesHardcodedIpsRange,
       migration: "ensure core composes files has correct hardcoded IPs in range",
-      coreVersion: "0.2.85",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await recreateContainersIfLegacyDns().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.85"
+    },
+    {
+      fn: recreateContainersIfLegacyDns,
       migration: "remove legacy dns from running containers",
-      coreVersion: "0.2.85",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await migrateDockerNetworkIpRange({
-    dockerNetworkName: params.DOCKER_PRIVATE_NETWORK_NAME,
-    dockerNetworkSubnet: params.DOCKER_NETWORK_SUBNET,
-    dappmanagerContainer: {
-      name: params.dappmanagerContainerName,
-      ip: params.DAPPMANAGER_IP
+      coreVersion: "0.2.85"
     },
-    bindContainer: { name: params.bindContainerName, ip: params.BIND_IP }
-  }).catch((e) =>
-    migrationErrors.push({
+    {
+      fn: () =>
+        migrateDockerNetworkIpRange({
+          dockerNetworkName: params.DOCKER_PRIVATE_NETWORK_NAME,
+          dockerNetworkSubnet: params.DOCKER_NETWORK_SUBNET,
+          dappmanagerContainer: {
+            name: params.dappmanagerContainerName,
+            ip: params.DAPPMANAGER_IP
+          },
+          bindContainer: { name: params.bindContainerName, ip: params.BIND_IP }
+        }),
       migration: "ensure docker network configuration",
-      coreVersion: "0.2.85",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await addAliasToRunningContainers().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.85"
+    },
+    {
+      fn: removeDnsAndAddAlias,
       migration: "add docker alias to running containers",
-      coreVersion: "0.2.80",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await addDappnodePeerToLocalIpfsNode().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.80"
+    },
+    {
+      fn: addDappnodePeerToLocalIpfsNode,
       migration: "add Dappnode peer to local IPFS node",
-      coreVersion: "0.2.88",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await changeEthicalMetricsDbFormat().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.88"
+    },
+    {
+      fn: changeEthicalMetricsDbFormat,
       migration: "change ethical metrics db format",
-      coreVersion: "0.2.92",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await determineIsDappnodeAws().catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.92"
+    },
+    {
+      fn: determineIsDappnodeAws,
       migration: "determine if the dappnode is running in Dappnode AWS",
-      coreVersion: "0.2.94",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
-  );
-
-  await createStakerNetworkAndConnectStakerPkgs(execution, consensus, signer, mevBoost).catch((e) =>
-    migrationErrors.push({
+      coreVersion: "0.2.94"
+    },
+    {
+      fn: () => createStakerNetworkAndConnectStakerPkgs(execution, consensus, signer, mevBoost),
       migration: "create docker staker network and persist selected staker pkgs per network",
-      coreVersion: "0.2.95",
-      name: "MIGRATION_ERROR",
-      message: e
-    })
+      coreVersion: "0.2.95"
+    }
+  ];
+
+  const migrationPromises = migrations.map(({ fn, migration, coreVersion }) =>
+    fn().catch((e) => new Error(`Migration ${migration} (${coreVersion}) failed: ${e.message}`))
   );
 
-  if (migrationErrors.length > 0) throw migrationErrors;
+  // Run all migrations concurrently and wait for all to settle
+  const results = await Promise.allSettled(migrationPromises);
+
+  // Collect any errors
+  const migrationErrors = results
+    .filter((result): result is PromiseRejectedResult => result.status === "rejected")
+    .map((result) => result.reason);
+
+  if (migrationErrors.length > 0) {
+    throw new MigrationError(migrationErrors);
+  }
 }
diff --git a/...ations/src/addAliasToRunningContainers.ts → ...es/migrations/src/removeDnsAndAddAlias.ts b/...ations/src/addAliasToRunningContainers.ts → ...es/migrations/src/removeDnsAndAddAlias.ts
@@ -3,12 +3,13 @@ import { uniq } from "lodash-es";
 import { ComposeNetwork, ComposeServiceNetwork, PackageContainer } from "@dappnode/types";
 import { logs } from "@dappnode/logger";
 import { params } from "@dappnode/params";
-import { ComposeFileEditor, parseServiceNetworks } from "@dappnode/dockercompose";
+import { ComposeFileEditor, ComposeServiceEditor, parseServiceNetworks } from "@dappnode/dockercompose";
 import {
   dockerComposeUp,
   dockerNetworkReconnect,
   listPackageContainers,
-  getNetworkContainerConfig
+  getNetworkContainerConfig,
+  listPackages
 } from "@dappnode/dockerapi";
 import { gte, lt, clean } from "semver";
 import { getDockerComposePath, getIsMonoService, getPrivateNetworkAliases, shell } from "@dappnode/utils";
@@ -26,11 +27,39 @@ const dncoreNetworkName = params.DOCKER_PRIVATE_NETWORK_NAME;
  * "service1.example.dappnode" if the package is multiservice
  * "service1.example.dappnode" and "example.dappnode" if the package is multiservice and has in manifest mainservice
  */
-export async function addAliasToRunningContainers(): Promise<void> {
+export async function removeDnsAndAddAlias(): Promise<void> {
+  const packages = await listPackages();
+  for (const pkg of packages) removeDnsFromPackageComposeFile(pkg.dnpName, pkg.isCore);
+
   const containers = await listPackageContainers();
   await addAliasToGivenContainers(containers);
 }
 
+export function removeDnsFromPackageComposeFile(dnpName: string, isCore: boolean): void {
+  logs.info(`Checking DNS from ${dnpName} compose file`);
+
+  const compose = new ComposeFileEditor(dnpName, isCore);
+  const services = compose.services();
+
+  for (const serviceName of Object.keys(services)) {
+    logs.info(`Checking DNS from ${serviceName} in ${dnpName} compose file`);
+    try {
+      const composeServiceEditor = new ComposeServiceEditor(compose, serviceName);
+      const composeService = composeServiceEditor.get();
+
+      // check composeService has the key dns
+      if ("dns" in composeService) {
+        logs.info(`Removing DNS from ${serviceName} in ${dnpName} compose file`);
+        // setting undefined a yaml property might result into an error afterwards making js-yaml
+        // adding the following value to the undefined `Error parsing YAML: unknown tag !<tag:yaml.org,2002:js/undefined>`
+        composeServiceEditor.removeDns();
+      }
+    } catch (e) {
+      logs.error(`Error removing DNS from ${serviceName} in ${dnpName} compose file`, e);
+    }
+  }
+}
+
 export async function addAliasToGivenContainers(containers: PackageContainer[]): Promise<void> {
   for (const container of containers) {
     try {