Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove bind as DNS server from compose files #1646

Merged
merged 11 commits into from
Oct 24, 2023
4 changes: 2 additions & 2 deletions packages/dappmanager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
"watch-ts": "tsc -w",
"file": "node",
"test": "npm run test:file \"./{,!(node_modules)/**}/*.test.ts\" ",
"test:file": "mocha",
"test:file": "TEST=true mocha",
"test:int": "npm run test:int:file \"./{,!(node_modules)/**}/*.test.int.ts\" ",
"test:int:file": "mocha --config ./test/integration/.mocharc.yaml --timeout 180000",
"test:int:file": "TEST=true mocha --config ./test/integration/.mocharc.yaml --timeout 180000",
"test:all": "npm run test && npm run test:int",
"lint": "eslint . --ext .ts --fix",
"prettier": "prettier --write 'src/**/*.*' 'test/**/*.*'",
Expand Down
16 changes: 13 additions & 3 deletions packages/dappmanager/src/modules/migrations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { addAliasToRunningContainers } from "./addAliasToRunningContainers.js";
import { switchEthClientIfOpenethereumOrGethLight } from "./switchEthClientIfOpenethereumOrGethLight.js";
import { pruneUserActionLogs } from "./pruneUserActionLogs.js";
import { setDefaultEthicalMetricsEmail } from "./setDefaultEthicalMetricsEmail.js";
import { removeDnsFromComposeFiles } from "./removeDnsFromComposeFiles.js";

export class MigrationError extends Error {
migration: string;
Expand All @@ -12,9 +13,8 @@ export class MigrationError extends Error {
super();
this.migration = migration;
this.coreVersion = coreVersion;
super.message = `Migration ${migration} ${coreVersion} failed: ${
super.message
}`;
super.message = `Migration ${migration} ${coreVersion} failed: ${super.message
}`;
}
}

Expand Down Expand Up @@ -86,5 +86,15 @@ export async function executeMigrations(): Promise<void> {
})
);

removeDnsFromComposeFiles().catch(e =>
migrationErrors.push({
migration: "remove bind DNS from docker compose files",
coreVersion: "0.2.82",
name: "MIGRATION_ERROR",
message: e.message,
stack: e.stack
})
);

if (migrationErrors.length > 0) throw migrationErrors;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
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<void> {
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this instructions completely removes the dns field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This will remove the dns field from every docker-compose.yml file that still includes it

compose.write();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,103 +2,127 @@ import { expect } from "chai";
import fs from "fs";
import { PackageContainer } from "@dappnode/common";
import { migrateCoreNetworkAndAliasInCompose } from "../../../src/modules/migrations/addAliasToRunningContainers.js";
import { removeDnsFromPackageComposeFile } from "../../../src/modules/migrations/removeDnsFromComposeFiles.js";
import { params } from "@dappnode/params";
import { mockContainer, shellSafe } from "../../testUtils.js";

describe("Migration", () => {
const dncoreNetwork = params.DNP_PRIVATE_NETWORK_NAME;
const serviceName = "test";
const containerName = "DAppNodePackage-test.dnp.dappnode.eth";
const randomImage = "chentex/random-logger";
const dnpName = "test-migration.dnp.dappnode.eth";
const testPackagePath = `${params.REPO_DIR}/test-migration.dnp.dappnode.eth`;

const container: PackageContainer = {
...mockContainer,
containerName: "DAppNodeCore-dappmanager.dnp.dappnode.eth",
dnpName: "test-migration",
serviceName: "dappmanager.dnp.dappnode.eth",
containerName,
dnpName,
serviceName,
networks: [
{ name: "random", ip: "10.0.1.1" },
{ name: "dncore_network", ip: "172.33.1.7" }
]
};

const composeNoDns = `
version: '3.5'
networks:
${dncoreNetwork}:
name: ${dncoreNetwork}
external: true
services:
${serviceName}:
image: ${randomImage}
container_name: ${containerName}
restart: always
networks:
${dncoreNetwork}:
ipv4_address: 172.33.1.7
aliases:
- ${serviceName}.test-migration.dappnode
- ${serviceName}.dappnode`;

const composeAlreadyMigrated = `
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this file has alias migration and dns migration tests, I would rename:
composeAlreadyMigrated to composeAliasesToBeMigrated
composeToBeMigratedBefore to composeAliasesAlreadyMigrated

Or something like that, so we know that these are used for alias migration and not DNS

version: '3.5'
networks:
dncore_network:
name: dncore_network
${dncoreNetwork}:
name: ${dncoreNetwork}
external: true
services:
dappmanager.dnp.dappnode.eth:
image: chentex/random-logger
container_name: DAppNodeCore-dappmanager.dnp.dappnode.eth
${serviceName}:
image: ${randomImage}
container_name: ${containerName}
restart: always
dns: 172.33.1.2
networks:
dncore_network:
${dncoreNetwork}:
ipv4_address: 172.33.1.7
aliases:
- dappmanager.dnp.dappnode.eth.test-migration.dappnode
- dappmanager.dappnode`;
- ${serviceName}.test-migration.dappnode
- ${serviceName}.dappnode`;

const composeToBeMigratedBefore = `
version: '3.4'
networks:
dncore_network:
name: dncore_network
${dncoreNetwork}:
name: ${dncoreNetwork}
external: true
services:
dappmanager.dnp.dappnode.eth:
image: "chentex/random-logger"
container_name: DAppNodeCore-dappmanager.dnp.dappnode.eth
${serviceName}:
image: ${randomImage}
container_name: ${containerName}
restart: always
dns: 172.33.1.2
networks:
dncore_network:
${dncoreNetwork}:
ipv4_address: 172.33.1.7`;

const dncoreNetwork = params.DNP_PRIVATE_NETWORK_NAME;
const containerName = "DAppNodeCore-dappmanager.dnp.dappnode.eth";
const randomImage = "chentex/random-logger";
const testMigrationPath = process.cwd() + "/test/integration/migrations";

before("Run random container", async () => {
// Create compose
await shellSafe(`mkdir ${testMigrationPath}/test-migration`);
await shellSafe(`mkdir -p ${testPackagePath}`);
// Compose to be migrated
fs.writeFileSync(
`${testMigrationPath}/test-migration/docker-compose.yml`,
`${testPackagePath}/docker-compose.yml`,
composeToBeMigratedBefore
);
// Compose already migrated
fs.writeFileSync(
`${testMigrationPath}/test-migration/docker-compose-migrated.yml`,
composeAlreadyMigrated
);
// Redeclare global variables
params.DNCORE_DIR = testMigrationPath;
params.REPO_DIR = testMigrationPath;

// Startup container
await shellSafe(
`docker-compose -f ${testMigrationPath}/test-migration/docker-compose.yml -p DNCORE up -d`
`docker-compose -f ${testPackagePath}/docker-compose.yml -p DNCORE up -d`
);
const containerkExists = await shellSafe(
const containerExists = await shellSafe(
`docker container ls --filter name=${containerName}`
);

const networkExists = await shellSafe(
`docker network ls --filter name=${dncoreNetwork}`
);

if (!containerkExists || !networkExists)
if (!containerExists || !networkExists)
throw Error("Error creating container or/and dncore_network");
});

it("Should do alias migration in compose", async () => {
const aliases = ["dappmanager.dnp.dappnode.eth.test-migration.dappnode", "dappmanager.dappnode"];
const aliases = ["test.test-migration.dappnode", "test.dappnode"];
migrateCoreNetworkAndAliasInCompose(container, aliases);

const composeAfter = fs.readFileSync(
`${testMigrationPath}/test-migration/docker-compose.yml`,
`${testPackagePath}/docker-compose.yml`,
{ encoding: "utf8" }
);
expect(composeAfter.trim()).to.equal(composeAlreadyMigrated.trim());
});
});

it("Should remove DNS from compose file", async () => {
removeDnsFromPackageComposeFile(container.dnpName, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we test also the removeDnsFromPackageComposeFile for core packages or is not worth it?


const composeAfter = fs.readFileSync(
`${testPackagePath}/docker-compose.yml`,
{ encoding: "utf8" }
);
expect(composeAfter.trim()).to.equal(composeNoDns.trim());
});

after("Remove test setup", async () => {
// Disconnect from network
Expand All @@ -112,10 +136,6 @@ services:
// Remove image
await shellSafe(`docker image rm ${randomImage}`);
// Remove dir
await shellSafe(`rm -rf ${testMigrationPath}/test-migration`);

// Return global vars to tests normal values
params.DNCORE_DIR = "./DNCORE";
params.REPO_DIR = "./dnp_repo";
await shellSafe(`rm -rf ${testPackagePath}`);
});
});
2 changes: 1 addition & 1 deletion packages/dockerCompose/src/setDappnodeComposeDefaults.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export function setDappnodeComposeDefaults(
container_name: getContainerName({ dnpName, serviceName, isCore }),
image: getImageTag({ serviceName, dnpName, version }),
environment: parseEnvironment(serviceUnsafe.environment || {}),
dns: params.DNS_SERVICE, // Common DAppNode ENS
dns: undefined,
networks: setServiceNetworksWithAliases(serviceUnsafe.networks, {
serviceName,
dnpName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ version: "3.5"
services:
dappmanager.dnp.dappnode.eth:
container_name: DAppNodeCore-dappmanager.dnp.dappnode.eth
dns: 172.33.1.2
environment:
DISABLE_UPNP: ""
ETH_MAINNET_RPC_URL_OVERRIDE: ""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ services:
HTTP_WEB3PROVIDER: "http://geth.dappnode:8545"
CORSDOMAIN: "http://prysm.dappnode"
EXTRA_OPTS: ""
dns: 172.33.1.2
networks:
dncore_network:
aliases:
Expand All @@ -34,7 +33,6 @@ services:
BEACON_RPC_GATEWAY_PROVIDER: "beacon-chain.prysm.dappnode:3500"
GRAFFITI: validating_from_DAppNode
EXTRA_OPTS: ""
dns: 172.33.1.2
networks:
dncore_network:
aliases:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ version: "3.5"
services:
s1:
container_name: DAppNodeCore-s1.sample-core.dnp.dappnode.eth
dns: 172.33.1.2
image: "s1.sample-core.dnp.dappnode.eth:0.2.33"
logging:
driver: json-file
Expand All @@ -19,7 +18,6 @@ services:
- "data:/data"
s2:
container_name: DAppNodeCore-s2.sample-core.dnp.dappnode.eth
dns: 172.33.1.2
image: "s2.sample-core.dnp.dappnode.eth:0.2.33"
logging:
driver: json-file
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ version: "3.5"
services:
vpn.dnp.dappnode.eth:
container_name: DAppNodeCore-vpn.dnp.dappnode.eth
dns: 172.33.1.2
image: "vpn.dnp.dappnode.eth:0.2.5"
logging:
driver: journald
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ describe("setDappnodeComposeDefaults", () => {
"beacon-chain": {
container_name:
"DAppNodePackage-beacon-chain.teku-gnosis.dnp.dappnode.eth",
dns: "172.33.1.2",
environment: {
LOG_TYPE: "INFO",
BEACON_API_PORT: "3500",
Expand All @@ -129,7 +128,6 @@ describe("setDappnodeComposeDefaults", () => {
validator: {
container_name:
"DAppNodePackage-validator.teku-gnosis.dnp.dappnode.eth",
dns: "172.33.1.2",
environment: {
LOG_TYPE: "INFO",
BEACON_NODE_ADDR: "http://beacon-chain.teku-gnosis.dappnode:3500",
Expand Down Expand Up @@ -232,7 +230,6 @@ describe("setDappnodeComposeDefaults", () => {
services: {
"dappmanager.dnp.dappnode.eth": {
container_name: "DAppNodeCore-dappmanager.dnp.dappnode.eth",
dns: "172.33.1.2",
environment: {
LOG_LEVEL: "info",
ETH_MAINNET_RPC_URL_OVERRIDE: "",
Expand Down
1 change: 0 additions & 1 deletion packages/params/src/params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ export const params = {
WIREGUARD_DEVICES_ENVNAME: "PEERS",

// Docker compose parameters
DNS_SERVICE: "172.33.1.2",
DNP_PRIVATE_NETWORK_SUBNET: "172.33.0.0/16",
DNP_PRIVATE_NETWORK_NAME: "dncore_network",
DNP_PRIVATE_NETWORK_NAME_FROM_CORE: "network",
Expand Down
Loading