From 3ecda3bb62bf634c518ab5ebb5ee02756474b7c1 Mon Sep 17 00:00:00 2001 From: pablomendezroyo <41727368+pablomendezroyo@users.noreply.github.com> Date: Mon, 11 Nov 2024 15:54:25 +0100 Subject: [PATCH] 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 * replace probkematic tag --------- Co-authored-by: Marc Font <36164126+Marketen@users.noreply.github.com> --- packages/dockerCompose/src/editor.ts | 14 +- .../test/unit/composeDeleteDns.test.ts | 47 ++++ packages/migrations/src/index.ts | 202 ++++++++---------- ...gContainers.ts => removeDnsAndAddAlias.ts} | 35 ++- .../src/removeDnsFromComposeFiles.ts | 33 --- 5 files changed, 177 insertions(+), 154 deletions(-) create mode 100644 packages/dockerCompose/test/unit/composeDeleteDns.test.ts rename packages/migrations/src/{addAliasToRunningContainers.ts => removeDnsAndAddAlias.ts} (85%) delete mode 100644 packages/migrations/src/removeDnsFromComposeFiles.ts diff --git a/packages/dockerCompose/src/editor.ts b/packages/dockerCompose/src/editor.ts index db0c8a04a..d28e7f0b5 100644 --- 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(yamlString); + // Replace problematic tag with a placeholder value such as null + const yamlStringParsed = yamlString.replace(/!/g, "null"); + return yamlParse(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 new file mode 100644 index 000000000..fd92c73e4 --- /dev/null +++ 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 index 78c50cf6e..7f1b7a592 100644 --- 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; 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 { - 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/packages/migrations/src/addAliasToRunningContainers.ts b/packages/migrations/src/removeDnsAndAddAlias.ts similarity index 85% rename from packages/migrations/src/addAliasToRunningContainers.ts rename to packages/migrations/src/removeDnsAndAddAlias.ts index d4671925d..04207dd1a 100644 --- a/packages/migrations/src/addAliasToRunningContainers.ts +++ b/packages/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 { +export async function removeDnsAndAddAlias(): Promise { + 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 !` + composeServiceEditor.removeDns(); + } + } catch (e) { + logs.error(`Error removing DNS from ${serviceName} in ${dnpName} compose file`, e); + } + } +} + export async function addAliasToGivenContainers(containers: PackageContainer[]): Promise { for (const container of containers) { try { diff --git a/packages/migrations/src/removeDnsFromComposeFiles.ts b/packages/migrations/src/removeDnsFromComposeFiles.ts deleted file mode 100644 index a5110473e..000000000 --- a/packages/migrations/src/removeDnsFromComposeFiles.ts +++ /dev/null @@ -1,33 +0,0 @@ -import { listPackages } from "@dappnode/dockerapi"; -import { ComposeFileEditor } from "@dappnode/dockercompose"; -import { logs } from "@dappnode/logger"; - -/** - * Migration to allow removal of current Bind functionality - * - * DNS resolution should be handled by the Docker DNS, not by Bind package - * - * For every service in every compose file, we must make sure it does not include - * the dns configuration - */ -export async function removeDnsFromComposeFiles(): Promise { - const packages = await listPackages(); - - for (const pkg of packages) { - removeDnsFromPackageComposeFile(pkg.dnpName, pkg.isCore); - } -} - -export function removeDnsFromPackageComposeFile(dnpName: string, isCore: boolean): void { - const compose = new ComposeFileEditor(dnpName, isCore); - const services = compose.services(); - - for (const serviceName of Object.keys(services)) { - const composeService = services[serviceName].get(); - if (composeService.dns) { - logs.info(`Removing DNS from ${serviceName} in ${dnpName} compose file`); - composeService.dns = undefined; - compose.write(); - } - } -}